-
Notifications
You must be signed in to change notification settings - Fork 546
feat(mobile): add integration with gateway backend for notifications feature #4878
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
Conversation
Branch preview✅ Deploy successful! Storybook: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks very good 👍 ! I sadly could not test it yet as some of your dependencies are not 100% correct in the PR. (I guess due to a rebase or something)
apps/mobile/src/features/Notifications/Notifications.container.tsx
Outdated
Show resolved
Hide resolved
@Jonathansoufer - I built the app locally and installed on my phone, but when I click on "turn on" notificatoins and navigate to the settings I don't have any notifications to choose from? The permission is not in the list? |
apps/mobile/src/features/Notifications/Notifications.container.tsx
Outdated
Show resolved
Hide resolved
Coverage (26%)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good. and I think that we could merge it after few small changes
- remove the second shim import in _layout.tsx :)
- move the delegateKey PK to the keychain with default protection (no need for biometrics, the app needs to be able to access it in the background)
- once a user is subscribed for the notifications the bell icon on the dashboard does nothing. A quick fix would be to redirect to the upcoming "notification center" screen in the design with a text "comming soon"
…rage service abstraction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work on this! 🔥
There are few small things that we would have to still address, but we can do this in separate PR.
👍
headers: { | ||
'Content-Type': 'application/json', | ||
Accept: 'application/json', | ||
'Set-Cookie': 'HttpOnly;Secure;SameSite=None', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has no effect as it is a response header. This needs to be set by the service when setting e.g. an auth cookie.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we had this discussion before, based on a couple of findings here, here, and here, and the only workaround, that did the job, was this. maybe because its need to be set by the service indeed, but w/o the auth doesn't work on mobile.
What it solves
Tis PR implements the logic necessary to integrate (enable/disable) notifications from the backend (CGW) perspective, as well as making sure Firebase integration/flow works.
Resolves #
How this PR fixes it
How to test it
P.S: Since we're pausing this PR up the point where we register de device on the database, but unable to receive notifications from there, the only way to test this PR is triggering a Push Notification from the Firebase Dashboard.
Screenshots
Firebase.Flow.mov
Checklist