Skip to content
This repository has been archived by the owner on Feb 7, 2025. It is now read-only.

addidng sender in place of history api #1017

Closed
wants to merge 18 commits into from

Conversation

rajeev-flexion
Copy link
Contributor

Add a PR title

Describe what changed in this PR at a high level.

Issue

Add a link to the issue here. Consider using
closing keywords
if the this PR isn't for a story (stories will be closed through different means).

Checklist

  • I have added tests to cover my changes
  • 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
Contributor

@basiliskus basiliskus left a comment

Choose a reason for hiding this comment

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

Left a few comments

@@ -191,7 +190,6 @@ public Optional<PartnerMetadata> getMetadata(String receivedSubmissionId)
var ourStatus = ourStatusFromReportStreamStatus(rsStatus);

logger.logInfo("Updating metadata with receiver {} and status {}", receiver, ourStatus);
partnerMetadata = partnerMetadata.withReceiver(receiver).withDeliveryStatus(ourStatus);
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you mean to remove .withDeliveryStatus(ourStatus) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, added it back

@@ -375,7 +373,7 @@ PartnerMetadataStatus ourStatusFromReportStreamStatus(String rsStatus) {
}

private boolean metadataIsStale(PartnerMetadata partnerMetadata) {
return partnerMetadata.receiver() == null
return partnerMetadata.receivingFacilityDetails().namespace() == null
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to check for universalId instead of namespace, as the id is always required

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

new DbColumn("receiver", metadata.receiver(), true, Types.VARCHAR),
new DbColumn(
"sender",
metadata.sendingFacilityDetails().namespace(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the namespace is the right thing to store here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

.add(createInformationIssueComponent("receiver name", metadata.receiver()));
.add(
createInformationIssueComponent(
"sender name", metadata.sendingFacilityDetails().namespace()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here. We should probably either have all properties in MessageHdDataType, or use universalId. The name should also probably change to either sender or sender id

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

.getIssue()
.add(
createInformationIssueComponent(
"receiver name", metadata.receivingFacilityDetails().namespace()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as with sender name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

.filter(metadata -> metadata.sender().equals(sender))
.filter(
metadata ->
metadata.sendingFacilityDetails().namespace().equals(sender))
Copy link
Contributor

Choose a reason for hiding this comment

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

The comparison should probably be with universalId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@rajeev-flexion rajeev-flexion marked this pull request as ready for review April 25, 2024 19:50
Copy link

@jorg3lopez
Copy link
Contributor

Closing this PR as the code has been merged using this other PR.

@jorg3lopez jorg3lopez closed this May 3, 2024
@basiliskus basiliskus deleted the story-990/sender-receiver-metadata branch December 26, 2024 17:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants