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

fix: notificationDateTime has correct date #1007

Merged
merged 6 commits into from Oct 30, 2022
Merged

Conversation

sitaram-kalluri
Copy link
Member

@sitaram-kalluri sitaram-kalluri commented Oct 27, 2022

Fixes #1003

- What I did

  1. Add mutex to the the NotifyVerbHandler.processVerb to prevent race conditions as we now have long-lived state (the NotificationBuilder which we are now reusing rather than creating a new one for every notification)
  2. Move the reset method to the start of the NotifyVerbHandler.processVerb. So that it gets called before the start of every notification.
  3. Added unit test, functional test and end-to-end test to assert that the second notification has a correct date and time.

- How I did it

  1. Moved the reset method to the start of the NotifyVerbHandler.processVerb in NotifyVerbHandler to fix the issue of subsequent notifications having incorrect date-time.

- How to verify it

  1. The subsequent notification should have the correct date and time.
  2. The unit tests, functional tests and end-to-end tests assert the functionality,

- Description for the changelog

  1. fix: Subsequent notifications have the incorrect date-time.

@gkc
Copy link
Contributor

gkc commented Oct 28, 2022

TODO: Explicitly add date time to notification in the NotifyVerbBuilder. @gkc: please confirm if we need this change or if changes made would suffice

Now that you're calling reset() at the start, it should be fine

Note: The end2end tests are failing the changes are not available in the test atsign's. Will look for an approach; perhaps add a version to the tests.

Yes. I think @murali-shris is adding a server version check to tests in another branch at the moment also.

@sitaram-kalluri
Copy link
Member Author

TODO: Explicitly add date time to notification in the NotifyVerbBuilder. @gkc: please confirm if we need this change or if changes made would suffice
Now that you're calling reset() at the start, it should be fine

Note: The end2end tests are failing the changes are not available in the test atsign's. Will look for an approach; perhaps add a version to the tests.
Yes. I think @murali-shris is adding a server version check to tests in another branch at the moment also.

@gkc: Skipped the e2e test for now. Will remove the skip tag and add a version check once we have the changes merged to the trunk branch.

The reason we skipped is, at the moment the trunk branch is already on v3.0.25 and the prod server is running on v3.0.24.

If we have a check to run tests on a version greater than or equal to 3.0.25, then the test fails on atsign running on the trunk - since the changes are not merged yet. Hence, we thought to merge to the trunk branch, and then have a version check to run tests between the trunk and prod servers.

@sitaram-kalluri sitaram-kalluri marked this pull request as ready for review October 28, 2022 10:54
@gkc
Copy link
Contributor

gkc commented Oct 28, 2022

@kalluriramkumar ACK

@gkc gkc merged commit 94c30ff into trunk Oct 30, 2022
@gkc gkc deleted the fix_notification_datetime branch October 30, 2022 16:00
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.

Memory optimisations for notifications have resulted in epochMillis not being set correctly for notifications
2 participants