Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DTSRD-2132.Update contactinformation for an organisation #1576

Open
wants to merge 56 commits into
base: master
Choose a base branch
from
Open

DTSRD-2132.Update contactinformation for an organisation #1576

wants to merge 56 commits into from

Conversation

SabinaHMCTS
Copy link
Contributor

Jira link (if applicable)

Change description

Checklist

  • commit messages are meaningful and follow good commit message guidelines
  • README and other documentation has been updated / added (if needed)
  • tests have been updated / new tests has been added (if needed)
  • Does this PR introduce a breaking change

lukasz-wolski
lukasz-wolski previously approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@lukasz-wolski lukasz-wolski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inlined comments

@hmcts-jenkins-j-to-z
Copy link
Contributor

Plan Result (aat)

⚠️ Resource Deletion will happen ⚠️

This plan contains resource delete operation. Please check the plan result very carefully!

Plan: 1 to add, 1 to change, 1 to destroy.
  • Update
    • azurerm_resource_group.rg
  • Replace
    • module.db-professional-ref-data-v16.null_resource.set-user-permissions-additionaldbs["dbrefdata"]
Change Result (Click me) 
  # azurerm_resource_group.rg will be updated in-place
  ~ resource "azurerm_resource_group" "rg" {
        id       = "/subscriptions/1c4f0704-a29e-403d-b719-b90c34ef14c9/resourceGroups/rd-professional-api-aat"
        name     = "rd-professional-api-aat"
      ~ tags     = {
          - "Deployment Environment" = "aat"
          - "Team Name"              = "RD"
          - "lastUpdated"            = "2024-05-03T02:32:04Z"
        } -> (known after apply)
        # (1 unchanged attribute hidden)
    }

  # module.db-professional-ref-data-v16.null_resource.set-user-permissions-additionaldbs["dbrefdata"] must be replaced
-/+ resource "null_resource" "set-user-permissions-additionaldbs" {
      ~ id       = "1644414966092894413" -> (known after apply)
      ~ triggers = { # forces replacement
          ~ "force_trigger"  = "3" -> "1"
            # (3 unchanged elements hidden)
        }
    }

Plan: 1 to add, 1 to change, 1 to destroy.

@hmcts-jenkins-j-to-z
Copy link
Contributor

Plan Result (prod)

⚠️ Resource Deletion will happen ⚠️

This plan contains resource delete operation. Please check the plan result very carefully!

Plan: 2 to add, 1 to change, 2 to destroy.
  • Update
    • azurerm_resource_group.rg
  • Replace
    • module.db-professional-ref-data-v16.null_resource.set-schema-ownership["dbrefdata"]
    • module.db-professional-ref-data-v16.null_resource.set-user-permissions-additionaldbs["dbrefdata"]
Change Result (Click me) 
  # azurerm_resource_group.rg will be updated in-place
  ~ resource "azurerm_resource_group" "rg" {
        id       = "/subscriptions/8999dec3-0104-4a27-94ee-6588559729d1/resourceGroups/rd-professional-api-prod"
        name     = "rd-professional-api-prod"
      ~ tags     = {
          - "Deployment Environment" = "prod"
          - "Team Name"              = "RD"
          - "lastUpdated"            = "2024-05-03T02:40:38Z"
        } -> (known after apply)
        # (1 unchanged attribute hidden)

        # (1 unchanged block hidden)
    }

  # module.db-professional-ref-data-v16.null_resource.set-schema-ownership["dbrefdata"] must be replaced
-/+ resource "null_resource" "set-schema-ownership" {
      ~ id       = "878889212208535071" -> (known after apply)
      ~ triggers = { # forces replacement
          ~ "force_trigger" = "3" -> "1"
            # (2 unchanged elements hidden)
        }
    }

  # module.db-professional-ref-data-v16.null_resource.set-user-permissions-additionaldbs["dbrefdata"] must be replaced
-/+ resource "null_resource" "set-user-permissions-additionaldbs" {
      ~ id       = "7832600243641400216" -> (known after apply)
      ~ triggers = { # forces replacement
          ~ "db_reader_user" = "DTS JIT Access rd DB Reader SC" -> "DTS JIT Access rd-professional-api DB Reader SC"
          ~ "force_trigger"  = "3" -> "1"
            # (2 unchanged elements hidden)
        }
    }

Plan: 2 to add, 1 to change, 2 to destroy.

rajkatla-hmcts
rajkatla-hmcts requested changes Sep 3, 2024
View reviewed changes
...nalTest/java/uk/gov/hmcts/reform/professionalapi/ProfessionalInternalUserFunctionalTest.java
.hasSizeGreaterThan(0);

List<ContactInformationCreationRequest> contactInformationCreationRequestList =
professionalApiClient.createContactInformationCreationRequests();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still new and existing contact addresses and DX are same

...nalTest/java/uk/gov/hmcts/reform/professionalapi/ProfessionalInternalUserFunctionalTest.java Outdated
existingContactInfo.get("addressId").toString());

assertNotNull(result);
assertThat(result.getBody()).isNotNull();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add assertions to verify the new DX and contact address details

...nalTest/java/uk/gov/hmcts/reform/professionalapi/ProfessionalInternalUserFunctionalTest.java Outdated
HashMap existingContactInfo = existingContactInformationList.get(0);

Response result = professionalApiClient.updateContactInformationsToOrganisation(
aContactInformationCreationRequest().addressLine1("addressLine1")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to update dx address alone , do you still need to pass addressLine1 ?

...nalTest/java/uk/gov/hmcts/reform/professionalapi/ProfessionalInternalUserFunctionalTest.java Outdated
existingContactInfo.get("addressId").toString());

assertNotNull(result);
assertThat(result.getBody()).isNotNull();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add assertions to verify the new DX address details

...nalTest/java/uk/gov/hmcts/reform/professionalapi/ProfessionalInternalUserFunctionalTest.java Outdated
@@ -1231,4 +1233,267 @@ private static List<Map<String, Object>> sortByValue(final List<Map<String, Obje
.sorted(Comparator.comparing(map -> (String) map.get(key)))
.collect(Collectors.toList());
}

@Test
@ExtendWith(FeatureToggleConditionExtension.class)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is not required, remove it : @ExtendWith(FeatureToggleConditionExtension.class)

...est/java/uk/gov/hmcts/reform/professionalapi/UpdateOrgContactInformationIntegrationTest.java Outdated
}

@Test
void shouldReturn400InvalidDatainRequest() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change method name to shouldReturn400WhenAddressLine1IsNull

...est/java/uk/gov/hmcts/reform/professionalapi/UpdateOrgContactInformationIntegrationTest.java Outdated

assertThat(updateResponse).containsEntry("http_status", 200);

assertThat(existingUpdated.get("addressLine1").toString())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Old and new data are same, add prefix "updated" to each new field so that it is easy to identify what is updated data.

...est/java/uk/gov/hmcts/reform/professionalapi/UpdateOrgContactInformationIntegrationTest.java Outdated
Map<String, Object> updateResponse = professionalReferenceDataClient
.updateOrgContactInformation(contactInformationCreationRequest,
hmctsAdmin,orgIdentifier,false,
true,"");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont need to pass empty addressId

...est/java/uk/gov/hmcts/reform/professionalapi/UpdateOrgContactInformationIntegrationTest.java Outdated
.updateOrgContactInformation(contactInformationCreationRequest, hmctsAdmin,organisationIdentifier,
true,true,"");

assertThat(updateResponse).containsEntry("http_status", "500");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your test method says should return 400 but you are asserting 500, api should return 400 with error "DX Address details are missing"

...est/java/uk/gov/hmcts/reform/professionalapi/UpdateOrgContactInformationIntegrationTest.java
deleteOrganisation(organisationIdentifier);
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add below test cases for dx address update, sometimes we may have to update only one field:

  1. to update DX Exchange field alone
  2. to update DX address number alone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajkatla-hmcts rajkatla-hmcts rajkatla-hmcts requested changes

@kiran-yenigala-hmcts kiran-yenigala-hmcts kiran-yenigala-hmcts left review comments

@lukasz-wolski lukasz-wolski lukasz-wolski left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
aat/destroy ns:rd prd:rd prod/destroy rel:rd-professional-api-pr-1576
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SabinaHMCTS @lukasz-wolski @kiran-yenigala-hmcts @rajkatla-hmcts