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

SDKS-3533 Prevent duplicated notification on iOS SDK #314

Merged
merged 2 commits into from
Dec 23, 2024
Merged

Conversation

rodrigoareis
Copy link
Contributor

JIRA Ticket

SDKS-3533 [iOS][Authenticator] Prevent duplicated notification on iOS SDK

Description

The iOS SDK does not verify whether a push notification with an identical identifier has already been stored, delegating this responsibility to the application developer. This PR include the following changes:

  • Added a new getNotificationByMessageId method to the StorageClient and FRAClient
  • Updated the FRAPushHandler class to check whether a payload was already persisted
  • Update unit tests

Definition of Done Checklist:

  • Acceptance criteria is met.
  • All tasks listed in the user story have been completed.
  • Coded to standards.
  • Ensure backward compatibility.
  • API reference docs is updated.
  • Unit tests are written.
  • Integration tests are written.
  • e2e tests are written.
  • Functional spec is written/updated.
  • Example code snippets have been added.
  • Change log updated.
  • Documentation story is created and tracked.
  • Tech debts and remaining tasks are tracked in separated ticket(s).

FRALog.v("PushNotification object is created and saved into StorageClient")
}
else {
FRALog.w("PushNotification object failed to be stored into StorageClient")

Choose a reason for hiding this comment

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

Should we not treat that as an error and return nil? If we end up on this situation, is the Push actionable or would the app be stuck?

Choose a reason for hiding this comment

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

Is there a valid reason that the notification will fail the storage step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! we should return nil here. It's unlikely we will have an exception while persisting it because the default storage is the Keychain. However, if a developer decide to use SQLite or any other storage, it may happen, so we need to confirm

Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@vahancouver vahancouver left a comment

Choose a reason for hiding this comment

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

Changes look good to me

@rodrigoareis rodrigoareis merged commit 1b369d9 into develop Dec 23, 2024
8 of 9 checks passed
@rodrigoareis rodrigoareis deleted the SDKS-3533 branch December 23, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants