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/notification permission #1268

Merged
merged 15 commits into from
Sep 25, 2023
Merged

Feat/notification permission #1268

merged 15 commits into from
Sep 25, 2023

Conversation

bp289
Copy link
Contributor

@bp289 bp289 commented Sep 21, 2023

closes #1265

Changes made :

  • Changed around the order of when "setNotificationChannelAsync()' is called as that is required on Android before requesting permissions( for the native popup to come up).
  • reworked token hook so that we don't have to request notification permissions when we
  • Added a new popup modal that links to the settings app when permissions are not accepted.

Setup Instructions for testing

For Expo Go:

On your phone

  • If we already have the app installed we need to clear data and reinstall the expo go app, to get rid of the permissions token.

  • In the expo app Sign in with an account that is in the nearform org ( ask @simone if you are not already in the org)

  • Scan the QR code from the repo, whould be the first comment on this PR. This should work for both iPhone and android if you scan the correct codes.

Native:

For native we should clone the repo and refer to the setup guide here:

https://github.com/nearform/optic-expo

  • Once you have installed the dependencies using 'yarn'. look at the relevant section for your device.

Andriod

  • For android, we need to make sure that a real Android device is plugged in. (note USB debugging should be enabled on your device, this is usually in settings > developer options), also if you are on Samsung you should disable/ uninstall secure folder before trying to build on your phone, I was only able to get this to work after I did that.

  • We need to make sure we have Andriod Studio installed on your computer(macOS) and configured as the guide says here:
    https://docs.expo.dev/workflow/android-studio-emulator/

  • We need to also have OpenJDK 11 installed which can be found here: https://formulae.brew.sh/formula/openjdk@11

  • After the installation, the output should give instructions on the relevant PATH variables and what to do with them.

  • Once this is done we simply run yarn android in the terminal of the project with an Android device connected to your computer via USB.

iOS

Testing instructions

  • Testing notifications
    Once we open the app, if there is no previous data, we should get a popup asking to enable push notification permissions.
    We need to confirm the respective native popup appears for both Android & Ios. This will only appear once, If the notifications are declined on the first go, clicking enable should navigate us to the app settings page so we can manually enable notifications.
  • Test that tokens are added after accepting notifications.
  • If the notifications are accepted, we should be able to add a secret and a token, click the "+" Icon on the bottom right of the screen and add a secret.
  • Click 'add token' and then create a token, you should be able to successfully create a token without errors.

@github-actions
Copy link

github-actions bot commented Sep 21, 2023

Preview available at https://expo.dev/accounts/nearform/projects/optic-expo/updates/50643cac-328b-467c-8bc4-65d34a496fe9

Or scan QR Codes...

android:
QR Code

ios:
QR Code}

@bp289 bp289 marked this pull request as ready for review September 22, 2023 09:47
@bp289 bp289 marked this pull request as draft September 22, 2023 09:54
@bp289 bp289 marked this pull request as ready for review September 22, 2023 10:39
@bp289 bp289 merged commit 7d3d9ff into master Sep 25, 2023
6 checks passed
@bp289 bp289 deleted the feat/notification-permission branch September 25, 2023 14:02
Copy link
Member

@simoneb simoneb left a comment

Choose a reason for hiding this comment

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

if I understood correctly, @williamlines couldn't really test this PR. wouldn't have it been wiser to wait for a review from somebody who can test it?

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.

Notifications permission needs to be requested
3 participants