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

feat: push notifications #1389

Merged
merged 7 commits into from
Sep 26, 2023
Merged

feat: push notifications #1389

merged 7 commits into from
Sep 26, 2023

Conversation

jamshale
Copy link
Contributor

@jamshale jamshale commented Aug 17, 2023

Adds firebase push notification capability when using a mediator with the configured firebase plugin.

Things of note:

  • Began adding this to bifold but was blocked by the fact that changes were needed to the android and ios build files, and that the required libraries wouldn't work without a valid firebase account and project. Could maybe figure out a way to add this as a bifold option and pull it out of the bc wallet repo at some point but I don't have a path forward with this right now.
  • Because I didn't create the firebase apps in bifold or add the react native libraries I tried to have bifold contain no code about the push notification logic. As a result I didn't follow the same patterns in some places as the bifold supported functionality. For instance I created a modal in this repo instead of bifold and didn't follow the action pattern in the Developer view.
  • Most of the development was done in android. IOS actually receiving push notifications is untested. This is because apple requires the app have APNS push notification capability to produce a firebase token, and that requires an apple developer account with signing privileges. I added the logic that I think is needed and added an attempt at a github action to load the required entitlements file. (the env variables are currently hardcoded as well).
  • Because I've been unable to test on IOS the push icon is currently the default. I added a BC icon to android.
  • The firebase account is my own personal account and they should be replaced by a bc gov managed one.

Here is some sequence diagrams of current features: https://hackmd.io/@NQjeSsXCR3Cmgs2Lr-M8ow/SJCS87BC2

Here is a demo video of current status:

Mediator doesn't support firebase:

push-notifications-no-mediator.webm

Mediator does support firebase:

push-notifications-with-mediator-plugin.webm

There are 2 new env variables:

  • MEDIATOR_USE_PUSH_NOTIFICATIONS --> if false or undefined the push notification logic will be completely ignored. Set to true if enabled
  • MEDIATOR_LABEL (Required) --> This is what is uses to discover firebase protocol and send the token. Otherwise it would need to discover on every agent. If this is a problem then could work around it.

** Currently it will ask for push notifications permission but send a blank token. It will be defaulted to off and will need to be turned on via development screen before receiving push notifications. **

I would like to write some unit tests. I had some earlier but deleted them after the modal broke the required mocks.

@jamshale jamshale force-pushed the main branch 10 times, most recently from eb039ac to 66b6081 Compare August 18, 2023 18:39
@jamshale jamshale changed the title Push Notifications Push Notifications [Do not merge] Aug 18, 2023
@jamshale jamshale marked this pull request as ready for review August 18, 2023 19:04
@jleach jleach added the do not merge This PR is on hold. label Aug 23, 2023
@jleach jleach changed the title Push Notifications [Do not merge] feat: push notifications Aug 23, 2023
@jleach jleach marked this pull request as draft August 23, 2023 23:27
@jamshale jamshale marked this pull request as ready for review August 24, 2023 16:15
@jleach
Copy link
Member

jleach commented Sep 6, 2023

@jamshale Is this one ready for review? If not are you able to move it to "Draft", otherwise we can remove the "do not merge" label and review it.

@jamshale
Copy link
Contributor Author

jamshale commented Sep 6, 2023

@jamshale Is this one ready for review? If not are you able to move it to "Draft", otherwise we can remove the "do not merge" label and review it.

Awesome! I did some work recently and want to go over it and update the description. I should have it ready for review today.
The 'do not merge' was more about the firebase config files being on my personal account. I have added some additional functionality recently after a discussion with Clecio but it should be ready soon.

@cvarjao
Copy link
Member

cvarjao commented Sep 6, 2023

can you please make it "off" by default. We will start using internally, and once we have fully tested we can change to default "on"

@jamshale
Copy link
Contributor Author

jamshale commented Sep 7, 2023

can you please make it "off" by default. We will start using internally, and once we have fully tested we can change to default "on"

Yes will do.

@jamshale jamshale marked this pull request as ready for review September 7, 2023 19:48
@jamshale
Copy link
Contributor Author

jamshale commented Sep 7, 2023

@jleach Ok, I would like to open this for review now.

Copy link
Contributor

@bryce-mcmath bryce-mcmath left a comment

Choose a reason for hiding this comment

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

Some small things, overall seems fairly sturdy. I'll check out the branch and test locally later and see if I find any other issues.

app/src/components/PushNotifications.tsx Outdated Show resolved Hide resolved
app/src/components/PushNotifications.tsx Outdated Show resolved Hide resolved
app/src/helpers/PushNotificationsHelper.ts Outdated Show resolved Hide resolved
app/src/helpers/PushNotificationsHelper.ts Outdated Show resolved Hide resolved
app/src/modals/PushNotificationsModal.tsx Outdated Show resolved Hide resolved
app/src/screens/Developer.tsx Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Sep 19, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@jamshale
Copy link
Contributor Author

I can give access to my firebase account if anyone wants to test with the mediator plugin. Or just send the service account file. It's just on free tier, so it's not much of a concern giving it out.

Copy link
Contributor

@bryce-mcmath bryce-mcmath left a comment

Choose a reason for hiding this comment

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

Looks good to me. @jleach @wadeking98 @cvarjao how do we feel about merging this?

@jleach jleach merged commit 64d72f7 into bcgov:main Sep 26, 2023
9 checks passed
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.

4 participants