CDCgov / trusted-intermediary

Bringing together healthcare providers by reducing the connection burden.
Apache License 2.0
11 stars 5 forks source link

Rename sent/received submission id to inbound/outbound message id variables #1597

Closed pluckyswan closed 5 days ago

pluckyswan commented 6 days ago

Description

Renamed sentSubmissionId and receivedSubmissionId to inboundMessageId and outboundMessageId. Renamed sentMessageId and receivedMessageId to inboundMessageId and outboundMessageId. The rename is from the perspective of TI who is receiving and sending to ReportStream.

Issue

https://github.com/CDCgov/trusted-intermediary/issues/1404

Checklist

Note: You may remove items that are not applicable

github-actions[bot] commented 6 days ago

PR Reviewer Guide ๐Ÿ”

Here are some key observations to aid the review process:

**๐ŸŽซ Ticket compliance analysis ๐Ÿ”ถ** **[1404](https://github.com/CDCgov/trusted-intermediary/issues/1404) - Partially compliant** Fully compliant requirements: - Rename sentSubmissionId to outboundMessageId. - Rename receivedSubmissionId to inboundMessageId. Not compliant requirements: - Verify consistency with naming conventions with Basilio. - Rename comments to reflect new naming. - Update the database to reflect new naming.
โฑ๏ธ Estimated effort to review: 3 ๐Ÿ”ต๐Ÿ”ต๐Ÿ”ตโšชโšช
๐Ÿงช No relevant tests
๐Ÿ”’ No security concerns identified
โšก Recommended focus areas for review

Incomplete Testing
The PR lacks tests for the updated variable names in different scenarios and edge cases.
github-actions[bot] commented 6 days ago

PR Code Suggestions โœจ

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Add validation for outboundMessageId to ensure it is not null or empty before usage ___ **Ensure that the outboundMessageId is validated for non-null and non-empty values
before using it in method calls and data operations to prevent runtime errors or
incorrect data handling.** [etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/EtorDomainRegistrationTest.groovy [174-182]](https://github.com/CDCgov/trusted-intermediary/pull/1597/files#diff-ffcc59ab37111a385fcab5330179c5e48a74b9a0b8975634fa86caffbb5f0cb6R174-R182) ```diff def outboundMessageId = "outboundMessageId" -def metadata = new PartnerMetadata("outboundMessageId", "hash", PartnerMetadataMessageType.ORDER, sendingApp, sendingFacility, receivingApp, receivingFacility, "placer_order_number") +if (!outboundMessageId) { + throw new IllegalArgumentException("outboundMessageId cannot be null or empty") +} +def metadata = new PartnerMetadata(outboundMessageId, "hash", PartnerMetadataMessageType.ORDER, sendingApp, sendingFacility, receivingApp, receivingFacility, "placer_order_number") def linkedMessageIds = Set.of(outboundMessageId, "Test1", "Test2") request.setPathParams(["id": outboundMessageId]) ```
Suggestion importance[1-10]: 7 Why: Adding validation for `outboundMessageId` enhances the robustness of the code by preventing potential runtime errors or incorrect data handling due to null or empty values. This is a practical improvement, especially in a testing environment where ensuring data integrity is crucial.
7
Add validation for null inboundMessageId to prevent potential null pointer exceptions ___ **Ensure that inboundMessageId is properly validated or handled when it is null in the
updateMetadataForSentMessage method to prevent potential issues with methods that
expect a non-null value.** [etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/metadata/partner/PartnerMetadataOrchestratorTest.groovy [136-143]](https://github.com/CDCgov/trusted-intermediary/pull/1597/files#diff-6e2ea4692146c404edbcf43d5207e75a900b3ca180a1e02fad29cdff46627fe9R136-R143) ```diff def "updateMetadataForSentMessage test case when inboundMessageId is null"() { when: def inboundMessageId = null + if (inboundMessageId == null) { + throw new IllegalArgumentException("inboundMessageId cannot be null") + } PartnerMetadataOrchestrator.getInstance().updateMetadataForSentMessage(outboundMessageId, inboundMessageId) then: 0 * mockPartnerMetadataStorage.readMetadata(outboundMessageId) } ```
Suggestion importance[1-10]: 7 Why: The suggestion to add null checks for `inboundMessageId` is valid and improves the robustness of the method by preventing potential null pointer exceptions. This is a good practice, especially in a testing environment where ensuring data integrity is crucial.
7
General
Ensure consistency in variable usage across test cases ___ **Ensure that the new variable names outboundMessageId and inboundMessageId are
correctly used in all method calls and assertions throughout the test cases to
maintain consistency with the updated variable names.** [etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/metadata/partner/PartnerMetadataTest.groovy [37-38]](https://github.com/CDCgov/trusted-intermediary/pull/1597/files#diff-9f402f9f99421814398f8f55eb23d20dea0dc25353128f07ed9e6fa9dd4c7375R37-R38) ```diff +metadata.outboundMessageId() == outboundMessageId +metadata.inboundMessageId() == inboundMessageId - ```
Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies the need for consistent use of updated variable names across all test cases, which is crucial for maintaining test accuracy and readability.
7
Confirm accurate metadata updates with new variable names ___ **Verify that the method updateMetadataForSentMessage is updated to handle the new
variable names outboundMessageId and inboundMessageId correctly, ensuring that
metadata is accurately updated in the system.** [etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/orders/SendOrderUseCaseTest.groovy [51]](https://github.com/CDCgov/trusted-intermediary/pull/1597/files#diff-56696e8718f522c7926a514339b630853370a926d4ca74c4911c87ccd7585493R51-R51) ```diff +1 * mockOrchestrator.updateMetadataForSentMessage(outboundMessageId, inboundMessageId) - ```
Suggestion importance[1-10]: 7 Why: This suggestion is relevant as it ensures that the method handling metadata updates uses the new variable names correctly, which is important for the integrity of metadata management.
7
Ensure correct method parameter usage in result sending and metadata updating ___ **Ensure that the SendResultUseCase class's convertAndSend method correctly utilizes
the new variable names outboundMessageId and inboundMessageId for sending results
and updating metadata.** [etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/results/SendResultUseCaseTest.groovy [42]](https://github.com/CDCgov/trusted-intermediary/pull/1597/files#diff-140f16ba8ce4161dc83b193a35ee178dbb5765b120760538b950068f99f8602dR42-R42) ```diff +SendResultUseCase.getInstance().convertAndSend(mockResult, outboundMessageId) - ```
Suggestion importance[1-10]: 7 Why: The suggestion is valid as it emphasizes the correct usage of new variable names in method parameters for result sending and metadata updating, ensuring system consistency.
7
Validate proper metadata updating with new inbound message ID handling ___ **Check that the withInboundMessageId method is correctly implemented to handle the
new inboundMessageId variable, ensuring that it properly updates the metadata within
the PartnerMetadata class.** [etor/src/test/groovy/gov/hhs/cdc/trustedintermediary/etor/metadata/partner/PartnerMetadataTest.groovy [84]](https://github.com/CDCgov/trusted-intermediary/pull/1597/files#diff-9f402f9f99421814398f8f55eb23d20dea0dc25353128f07ed9e6fa9dd4c7375R84-R84) ```diff +def updatedMetadata = metadata.withInboundMessageId(inboundMessageId) - ```
Suggestion importance[1-10]: 7 Why: This suggestion is pertinent as it checks the correct implementation of the method handling the new inbound message ID, which is critical for accurate metadata updates in the system.
7
sonarcloud[bot] commented 5 days ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
97.4% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud