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.

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

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

Closed
wants to merge 5 commits into from

Conversation

pluckyswan
Copy link
Contributor

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

#1404

Checklist

  • I have added logging where useful (with appropriate log level)
  • I have added JavaDocs where required
  • I have updated the documentation accordingly

Note: You may remove items that are not applicable

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

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.

Copy link

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]

 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]

 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]

+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]

+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]

+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]

+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

@pluckyswan pluckyswan changed the title Rename sen/received submission/message id variables Rename sent/received submission id to inbound/outbound id variables Nov 19, 2024
@pluckyswan pluckyswan changed the title Rename sent/received submission id to inbound/outbound id variables Rename sent/received submission id to inbound/outbound message id variables Nov 19, 2024
e);
}
}

public void saveSentMessageSubmissionId(String receivedSubmissionId, String sentSubmissionId) {
if (sentSubmissionId == null || receivedSubmissionId == null) {
public void saveInboundMessageId(String outboundMessageId, String inboundMessageId) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to do both inbound and outbound message saving. @basiliskus Should I leave the old name if that's the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm I think we should remove the Sent part and make it plural. So it would be renamed as saveMessageSubmissionIds or saveSubmissionIds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll rename it to saveMessageSubmissionIds

Copy link

sonarcloud bot commented Nov 20, 2024

TestApplicationContext.injectRegisteredImplementations()

when:
SendOrderUseCase.getInstance().convertAndSend(Mock(Order), null)

then:
3 * mockLogger.logWarning(_)
0 * mockOrchestrator.updateMetadataForReceivedMessage(_, _)
0 * mockOrchestrator.updateMetadataForOutboundMessage(_, _)
}

def "convertAndSend logs error and continues when updateMetadataForReceivedOrder throws exception"() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updateMetadataForReceivedOrder doesn't appear to exist unless it was accidentally overriden.

Copy link
Contributor

Choose a reason for hiding this comment

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

It should be renamed as updateMetadataForOutboundMessage

@pluckyswan pluckyswan closed this Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants