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.

Sign up for GitHub

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

Jump to bottom

Remove old Notification tokens #2173

Merged
merged 14 commits into from
Dec 5, 2024
Merged

Remove old Notification tokens #2173

merged 14 commits into from
Dec 5, 2024

Conversation

PooyaRaki
Copy link
Contributor

Summary

This PR ensures that duplicate push notifications are avoided by removing the registered push notification token from the TX service during device registration through the Client Gateway (CGW). This prevents multiple registrations from causing redundant notifications.

Changes

  • Remove the push notification token from the TX service during device registration to prevent duplicates.
  • Ensure the token is also removed from the TX service when unregisterDevice is called.
  • Remove subscriptions from the TX service when unregisterSafe is invoked to maintain data consistency.

@PooyaRaki PooyaRaki force-pushed the NotificationRemoveOldTokens branch from 85796c5 to 3870175 Compare December 4, 2024 12:24
@PooyaRaki PooyaRaki marked this pull request as ready for review December 4, 2024 12:26
@PooyaRaki PooyaRaki requested a review from a team as a code owner December 4, 2024 12:26
iamacook
iamacook requested changes Dec 4, 2024
View reviewed changes
Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

Conditionally rely on the type of areas across different services seems very brittle.

What would be the issue with calling unregister on every incoming device, catching the errors, and then continuing registration?

src/domain/notifications/v2/notifications.repository.integration.spec.ts Show resolved Hide resolved
src/domain/notifications/v2/notifications.repository.spec.ts Outdated Show resolved Hide resolved
src/routes/notifications/v1/notifications.controller.ts Outdated Show resolved Hide resolved
src/routes/notifications/v1/notifications.controller.ts Outdated Show resolved Hide resolved
@PooyaRaki
Copy link
Contributor Author

What would be the issue with calling unregister on every incoming device, catching the errors, and then continuing registration?

That’s quite risky. If we call unregister first and encounter an error during registration, it could result in token loss.

In this PR, we address this by ensuring that tokens are successfully registered in the new system before attempting to remove them from the old system.

You might argue that if a token is registered but the unregister process fails, we could end up delivering multiple notifications. However, between duplicate notifications and no notifications, I’d prefer the former.

@PooyaRaki PooyaRaki requested a review from iamacook December 4, 2024 14:15
iamacook
iamacook previously approved these changes Dec 4, 2024
View reviewed changes
Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

For visbility: the conditional error checks are confusing but unavoidable due to the compatability layer. They will, however, be removed soon as we will keep them as is.

@PooyaRaki PooyaRaki self-assigned this Dec 5, 2024
@PooyaRaki PooyaRaki force-pushed the notificationMigration branch from 5e5999d to bdf9a89 Compare December 5, 2024 10:38
Base automatically changed from notificationMigration to main December 5, 2024 11:48
@PooyaRaki PooyaRaki dismissed iamacook’s stale review December 5, 2024 11:48

The base branch was changed.

PooyaRaki and others added 13 commits December 5, 2024 12:58
@PooyaRaki
rewrite notification v2 using typeorm
fffb49d
@PooyaRaki
refactor notification hooks to use notification v2
47bcba7
@PooyaRaki
Improve typing and validation schemas
8cb79dd
@PooyaRaki
Call notification v2 from v1 using Promise.all
777f457
@PooyaRaki
Add validation to notification controller v1, refactor insertSubscrip…
154cfef 
…tionNotificationTypes
@PooyaRaki
fix typos, moves notification repository mock to __tests__ folder
c26762f
@PooyaRaki
Check whether the notification feature flag is enabled before calling…
15794ba 
… notification v2
@PooyaRaki
Remove notification token from the tx service when registered through…
3e611d8 
… cgw
@PooyaRaki
Change notification tests
5cf4169
@PooyaRaki
Refactor notification controller error handling
44064cd
@PooyaRaki @iamacook
Update src/domain/notifications/v2/notifications.repository.spec.ts
5eda725 
Co-authored-by: Aaron Cook <aaron@safe.global>
@PooyaRaki
Refactor notification controller error handling
fc1d77a
@PooyaRaki
Import getAddress in notification repository tests
ac5074b
@PooyaRaki PooyaRaki force-pushed the NotificationRemoveOldTokens branch from 293fed8 to ac5074b Compare December 5, 2024 12:10
iamacook
iamacook reviewed Dec 5, 2024
View reviewed changes
Copy link
Member

@iamacook iamacook left a comment

Choose a reason for hiding this comment

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

Looking back at this, I realised we aren't testing the new logic on the controller.

I suggest we extend the notifications.controller.spect to ensure we are handling unregistration as expected/throwing the correct error. What do you think?

src/routes/notifications/v1/notifications.controller.ts Outdated
Comment on lines 205 to 209
if (error instanceof Error) {
if ('code' in error && error.code !== 404) {
throw error;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

If the error is not instanceof Error, it is being swallowed 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.

Change in the new version
6a9aa2f

src/routes/notifications/v1/notifications.controller.ts Outdated
Comment on lines 261 to 265
if (error instanceof Error) {
if ('code' in error && error.code !== 404) {
throw error;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

If the error is not instanceof Error, it is being swallowed 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.

Change in the new version
6a9aa2f

@PooyaRaki PooyaRaki requested a review from iamacook December 5, 2024 13:35
@PooyaRaki
Copy link
Contributor Author

Looking back at this, I realised we aren't testing the new logic on the controller.

I suggest we extend the notifications.controller.spect to ensure we are handling unregistration as expected/throwing the correct error. What do you think?

Thanks for the suggestion! I gave this some thought, and while I agree that testing is important, in this specific case, the logic is quite straightforward and only covers one error scenario. I don’t believe adding a test here provides enough value to justify the extra maintenance overhead.

If there are more complex cases or future changes to this part of the controller, we can revisit and add tests as needed. Let me know what you think!

@PooyaRaki PooyaRaki merged commit 530e230 into main Dec 5, 2024
20 checks passed
@PooyaRaki PooyaRaki deleted the NotificationRemoveOldTokens branch December 5, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamacook iamacook iamacook approved these changes

Assignees

@PooyaRaki PooyaRaki

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@PooyaRaki @iamacook