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

Idempotence (GSI-626) #12

Merged
merged 18 commits into from
Mar 4, 2024
Merged

Idempotence (GSI-626) #12

merged 18 commits into from
Mar 4, 2024

Conversation

TheByronHimes
Copy link
Member

New Model

There is a new model, NotificationRecord. It has two fields: hash_sum (str) and sent (bool).

No changes were made to the event subscriber. It still passes the notification event as it did before.
Inside the Notifier instance, the notification event is used to generate a NotificationRecord.
The hash sum from the record is used to determine if the notification has been encountered before, and
whether or not it was successfully sent.
If the notification has been seen before, the operation is complete.
If not, the record is added to the database.
The notification is then sent out, and afterwards the record is updated with sent set to True.

New methods for Notifier

A few private methods were added to the Notifier class:

  • _ensure_not_sent(hash_sum) -> bool
  • _create_notification_record(notification) -> NotificationRecord
  • _register_new_notification(NotificationRecord) -> None

Tests

Unit tests were created to verify that the above new methods work as expected. There is a single tests that covers the three methods above, and then a general idempotence test. The latter naturally tests the transmission capability, so the old test_transmission test case was removed.

Copy link
Member

@mephenor mephenor left a comment

Choose a reason for hiding this comment

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

There are some minor issues that should be addressed, but the rest looks good.

src/ns/core/notifier.py Outdated Show resolved Hide resolved
src/ns/core/notifier.py Outdated Show resolved Hide resolved
tests/test_basic.py Outdated Show resolved Hide resolved
tests/test_basic.py Outdated Show resolved Hide resolved
tests/test_basic.py Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Mar 4, 2024

Pull Request Test Coverage Report for Build 8142443883

Details

  • 47 of 47 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+4.0%) to 80.159%

Totals Coverage Status
Change from base Build 7558586254: 4.0%
Covered Lines: 202
Relevant Lines: 252

💛 - Coveralls

Copy link
Member

@mephenor mephenor left a comment

Choose a reason for hiding this comment

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

One small thing can still be improved

src/ns/core/notifier.py Outdated Show resolved Hide resolved
@TheByronHimes TheByronHimes merged commit 33f66ed into main Mar 4, 2024
8 checks passed
@TheByronHimes TheByronHimes deleted the feature/idempotence_GSI-626 branch March 4, 2024 16:58
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.

3 participants