-
Notifications
You must be signed in to change notification settings - Fork 2
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
feature/9265-Binny-PushLoginFlowChanges #9864
base: develop
Are you sure you want to change the base?
Conversation
// 'Claims', | ||
// 'Claims history', | ||
// 'Received December 05, 2021', | ||
// 'Submit evidence', |
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 is the linter, not me. /shrug
@@ -57,13 +57,15 @@ const NotificationManager: FC = ({ children }) => { | |||
registeredNotifications.remove() | |||
failedNotifications.remove() | |||
}) | |||
Notifications.registerRemoteNotifications() | |||
if (firstTimeLogin === false && requestNotifications === true) { |
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.
I had to wait until the firstTimeLogin turns false in able to request permissions AFTER onboarding Carousel.
), | ||
screen.getByRole('header', { | ||
name: t('biometricsPreference.doYouWantToAllow', { | ||
biometricsText: t('biometric.touchID'), |
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.
Intentionally not calling the function we use on the screen here to continue checking the function logic with the test.
export const completeRequestNotifications = (): AppThunk => async () => { | ||
await AsyncStorage.setItem(NOTIFICATION_COMPLETED_KEY, FIRST_NOTIFICATION_STORAGE_VAL) | ||
} | ||
|
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 probably doesn't need to be a function, since it's just setting a value in Asnyc Storage.
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.
Removed. I needed to drop the await in order to include it in the screen itself, but that shouldn't be a problem I don't think.
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.
Yeah that shouldn't cause any issues since there's not anything that needs to wait for async storage to finish setting the value
const setNotificationsPreferenceScreenVal = await AsyncStorage.getItem(NOTIFICATION_COMPLETED_KEY) | ||
console.debug(`checkRequestNotificationPreferenceScreen: is ${!setNotificationsPreferenceScreenVal}`) | ||
|
||
const shouldShowScreen = !setNotificationsPreferenceScreenVal |
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.
I don't see any logic for resetting NOTIFICATION_COMPLETED_KEY
at logout. If a previously logged-in user had notifications enabled, it looks like a new user signing in wouldn't see the RequestNotificationScreen?
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 is a real head scratcher for me and i'll need to bug QA about this tomorrow.
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.
So, the fact that this tangles with OS specific permission granting and isn't actually tied to our server side notification manager is what is selling me that we should do nothing here. This is very similar to biometric preferences. When you logout of the app we do not reset your biometric preferences, we continue to allow you to use them and if another user signed on we wouldn't re-prompt. This being the 0.1% issue is another reason I don't want it to hold us up. Thoughts?
@@ -94,6 +98,8 @@ export const initialAuthState: AuthState = { | |||
displayBiometricsPreferenceScreen: false, | |||
showLaoGate: false, | |||
authParamsLoadingState: AuthParamsLoadingStateTypeConstants.INIT, | |||
requestNotificationsPreferenceScreen: false, |
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.
I might be missing something, but it seems like requestNotificationsPreferenceScreen
might be overlapping with NOTIFICATION_COMPLETED_KEY
in async storage. Couldn't we just use NOTIFICATION_COMPLETED_KEY
to determine whether to show the RequestNotificationScreen?
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.
So you're saying set the session state from the async somewhere? One is setting the async storage and the other is setting the state variable in the session. The state never loads from async storage so if i close the app and re-entered without the state setup I think it might be an issue?
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.
Yeah, we could just use the value from async storage since it stays in sync with requestNotificationsPreferenceScreen
anyway. To kill 2 birds with 1 stone, we could create a query for getting the NOTIFICATION_COMPLETED_KEY
value, that way it'll be cached and easier to access. And when the app launches, it'll automatically load from async storage. Kinda like how we do with useSystemNotificationsSettings.
@@ -79,6 +81,8 @@ export type AuthState = { | |||
authorizeStateParam?: string | |||
authParamsLoadingState: AuthParamsLoadingStateTypes | |||
successfulLogin?: boolean | |||
requestNotificationsPreferenceScreen?: boolean | |||
requestNotifications?: boolean |
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.
Since this state variable is related to notifications (and also since the authSlice will be going away soon) this should probably be moved to the notification context
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.
I will do this in the second part of this ticket. Adding an AC.
Description of Change
Screenshots/Video
Biometric Screen Changes (Ignore BitterBoldHeading i've rectified that to the normal)
FaceID Before/After
TouchID Before/After
Face Recognition Before/After
Fingerprint Before/After
Iris Before/After
New Notification Request Screen
Testing
Reviewer Validations
PR Checklist
Reviewer: Confirm the items below as you review
For QA
Run a build for this branch