feat: Update the enable-notifications flows and notification settings UI #22524
Conversation
seanstrom
changed the title
Jenkins BuildsClick to see older builds (93)
|
seanstrom
force-pushed
the
seanstrom/news-notifications-ui
branch
2 times, most recently
from
abbf4d0 to
6f54b9b
Compare
seanstrom
force-pushed
the
seanstrom/news-notifications-ui
branch
from
6f54b9b to
7bfbd86
Compare
src/legacy/status_im/events.cljs
Outdated
| notifications-settings? (= (get-in db [:view-id]) | ||
| :screen/settings.notifications) |
There was a problem hiding this comment.
This value is used inside the event that's called when the app returns from being in the background. For example, when we return from outside the app, we'll check if we've returned to the notification settings screen so we can refresh the permissions state.
There was a problem hiding this comment.
(= (get-in db [:view-id]) :screen/settings.notifications)->
(= (:view-id db) :screen/settings.notifications)There was a problem hiding this comment.
I wonder if instead we could add a handler in the screen mounted (by using use-effect or something related). If we can't, it's ok to keep it here.
There was a problem hiding this comment.
I wonder if instead we could add a handler in the screen mounted (by using use-effect or something related). If we can't, it's ok to keep it here.
I remember trying to use a hook for detecting when the screen is mounted, but it doesn't seem like the screen is re-mounted when returning from a background state, which lead to hooking into the event here.
However, there's a chance we could try creating a hook that uses a re-frame subscription, and we can somehow use that hook in the view to trigger changes when the app returns from the background (?) Just an idea but maybe that will make it easier to extend the app.
| {:dispatch [:biometric/authenticate {:on-fail on-biometric-auth-fail}]}) | ||
| #(when notifications-settings? | ||
| {:dispatch [:notifications/check-notifications-blocked]})))) |
There was a problem hiding this comment.
Here we're conditionally dispatching the :notifications/check-notifications-blocked event to refresh the screen UI if the user has recently unblocked or allowed notification permissions from outside the Status app and then returned to the notifications settings screen.
|
|
||
| (defn open-notification-settings | ||
| [] | ||
| (openSettings "notifications")) |
There was a problem hiding this comment.
Here we're creating a function that should open the OS system settings screen for the app, and if possible it will display the notification related settings for the app (from the OS System pov, not inside Status app).
| :on-press #(rf/dispatch [:onboarding/biometrics-setup-start | ||
| {:enable-biometrics? true | ||
| :syncing? syncing?}])} |
There was a problem hiding this comment.
Here we're using a different pattern for dispatching events from the onboarding screens. Instead of dispatching different events from the screen, we attempt to dispatch the same event with different parameters.
This pattern is an attempt to simplify how we manage the onboarding screens, for example, if we choose to expand the onboarding steps, it would be ideal that we can refactor mostly the event layer instead of needing to refactor both the event layer and view layer.
src/status_im/subs/profile.cljs
Outdated
| {:notifications-blocked? (boolean notifications-blocked?) | ||
| :notifications-enabled? (and (boolean notifications-enabled?) | ||
| (not (boolean notifications-blocked?))) |
There was a problem hiding this comment.
Here we're expanding this subscription to include whether the notifications have been blocked at the OS level.
There was a problem hiding this comment.
I suppose (not (boolean notifications-blocked? is unnecessary and could be just (not notifications-blocked? because not will already coerce values to booleans. If you really want to make sure the sub value is a boolean you could call boolean only once outside the conditionals, or better yet, when reading profile data from status-go, thus coercing on write, which leads to less type mismatch issues (but I think it's not worth it for these flags tbh).
But do we even need this external extra boolean call? notifications-blocked? and notifications-enabled? seem to be either nil or a boolean and there's no risk of it being a seq, which could be kind of a problem in sub values that should be truthy/falsy values.
There was a problem hiding this comment.
The main reason for the extra boolean expressions is to ensure we're not passing nil into the quo components for things related to boolean states. I recall there being some issues with how nil handled differently than false.
But I can revise this subscription to at least re-use the (boolean notifications-blocked?) expression
There was a problem hiding this comment.
Most likely, if there's any part of the code that can't work with nil instead of an explicit false it should be revisited because it's asking for bugs. Idiomatic Clojure embraces nullable booleans. I know of one place in our repo where a value can be a tri-state boolean, but this should be very rare.
| (fn [{:keys [fcm-token]}] | ||
| (logger/log-debug "registered fcm token for status news" | ||
| ;; NOTE(@seanstrom): temporarily avoid logging fcm-token | ||
| #_{:fcm-token fcm-token}))) | ||
| {:fcm-token fcm-token}))) |
There was a problem hiding this comment.
Here we're temporarily logging the Firebase token after successfully registering with Firebase for Status news notifications. This likely should not be logged in production, so this change has been marked as "donotmerge", but it still is required for testing, so we'll keep it in the PR for now.
| :else | ||
| [[:dispatch [:update-theme-and-init-root :screen/shell-stack]] | ||
| [:dispatch [:profile/toggle-testnet-mode-banner]]]))}))) | ||
| [[:dispatch [:shell/show-root-view]]]))}))) |
There was a problem hiding this comment.
Here we're introducing a new event for representing when we want to show root shell-stack of the app. This is being introduced because we needed a common place to check whether someone has been prompted with the screen for the notification permissions, and we needed to cover all the possible ways someone could arrive to the app including:
- login
- signup
- recovery
- syncing
| temporary-display-name? | ||
| enable-news-notifications? | ||
| enable-notifications?]} (:onboarding/profile db) | ||
| {:keys [key-uid] :as profile} (:profile/profile db) | ||
| biometric-enabled? (= auth-method constants/auth-method-biometric)] | ||
| {:db (assoc db :onboarding/generated-keys? true) | ||
| :fx [(when temporary-display-name? | ||
| [:dispatch [:profile/set-default-profile-name profile]]) | ||
| (when biometric-enabled? | ||
| [:keychain/save-password-and-auth-method | ||
| {:key-uid key-uid | ||
| :masked-password (if syncing? | ||
| password | ||
| (security/hash-masked-password password)) | ||
| :on-success (fn [] | ||
| (rf/dispatch [:onboarding/set-auth-method auth-method]) | ||
| (when syncing? | ||
| (rf/dispatch | ||
| [:onboarding/finish-onboarding false]))) | ||
| :on-error #(log/error "failed to save biometrics" | ||
| {:key-uid key-uid | ||
| :error %})}])]}))) | ||
| :fx (cond-> | ||
| [(when temporary-display-name? | ||
| [:dispatch [:profile/set-default-profile-name profile]]) | ||
| (when biometric-enabled? | ||
| [:keychain/save-password-and-auth-method | ||
| {:key-uid key-uid | ||
| :masked-password (if syncing? | ||
| password | ||
| (security/hash-masked-password password)) | ||
| :on-success (fn [] | ||
| (rf/dispatch [:onboarding/set-auth-method auth-method]) | ||
| (when syncing? | ||
| (rf/dispatch | ||
| [:onboarding/finish-onboarding]))) | ||
| :on-error #(log/error "failed to save biometrics" | ||
| {:key-uid key-uid | ||
| :error %})}])] | ||
|
|
||
| (and enable-notifications? | ||
| enable-news-notifications?) | ||
| (conj [:dispatch | ||
| [:notifications/news-notifications-switch true]]) | ||
|
|
||
| enable-notifications? | ||
| (conj [:dispatch | ||
| [:push-notifications/switch true]]))}))) |
There was a problem hiding this comment.
Here we're expanding the :onboarding/finalize-setup event to handle the activation of the push notification settings if the user has opted-in.
| {:db (assoc db :profile/profiles-overview multiaccounts) | ||
| :fx (cond-> [] | ||
| (ff/enabled? ::ff/settings.news-notifications) | ||
| (conj [:effects.profile/remove-local-profile-storage | ||
| {:key-uid key-uid}]) | ||
| (not (seq multiaccounts)) | ||
| (conj [:dispatch | ||
| [:update-theme-and-init-root :screen/onboarding.intro]]))})) |
There was a problem hiding this comment.
Here we're extending the :onboarding/on-delete-profile-success event to remove any profile related data that had been stored inside the mmkv storage via the key-uid.
seanstrom
force-pushed
the
seanstrom/news-notifications-ui
branch
from
5374b35 to
c45b399
Compare
seanstrom
changed the title
seanstrom
changed the title
seanstrom
force-pushed
the
seanstrom/news-notifications-ui
branch
2 times, most recently
from
2a9758b to
043722c
Compare
| (defn notifications-info-view | ||
| [{:keys [blur?]}] | ||
| [quo/documentation-drawers | ||
| {:title (i18n/label :t/enable-notifications) | ||
| :show-button? true | ||
| :shell? blur? | ||
| :button-label (i18n/label :t/read-more) | ||
| :button-icon :i/info} | ||
| [quo/text (i18n/label :t/enable-notifications-info-description)]]) |
There was a problem hiding this comment.
This drawer view still needs the correct verbiage from the docs team (I think ?)
Reference: https://www.figma.com/design/JlpPhJp0SMnEyBQy4nOZHo/Settings----Mobile?node-id=9395-120255&t=IZIHOX0Oys0QGyTf-4
| (when (and platform/android? | ||
| (ff/enabled? ::ff/settings.news-notifications)) | ||
| [rn/view | ||
| {:style style/news-notifications-checkbox-container} | ||
| [quo/selectors | ||
| {:type :checkbox | ||
| :blur? true | ||
| :checked? third-party-checked? | ||
| :on-change set-third-party-checked}] |
There was a problem hiding this comment.
Here we're only showing the third-party services checkbox (activation for news-notifications) on Android. On iOS the setting will be active by default (based on the designs).
| set-third-party-checked] (rn/use-state | ||
| (if (ff/enabled? ::ff/settings.news-notifications) | ||
| true | ||
| false)) |
There was a problem hiding this comment.
Here we're defaulting the third-party services option to true when we make news-notifications available. On Android, users will be able to opt-out of this feature.
There was a problem hiding this comment.
(if condition? true false)Seems a bit uncommon to me, what about just (boolean condition?), or if condition? already is a boolean (it should be because of ?) just condition?
There was a problem hiding this comment.
Yeah I intentionally made it so unappealing because I thought leaving a (ff/enabled?) expression might seem to subtle. But I agree it should be flattened, I mainly wanted to draw attention to this weird part of the code 😄
| (and biometrics? onboarding? syncing?) | ||
| (conj [:dispatch [:onboarding/finalize-setup]]) | ||
|
|
||
| (and biometrics? onboarding? (not syncing?)) | ||
| (conj [:dispatch [:onboarding/create-account-and-login]]) | ||
|
|
||
| (and (not biometrics?) onboarding? syncing?) | ||
| (conj [:dispatch [:onboarding/finish-onboarding]]) | ||
|
|
||
| (and (not biometrics?) onboarding? (not syncing?)) | ||
| (conj [:dispatch [:onboarding/create-account-and-login]]) | ||
|
|
||
| (and (not biometrics?) (not onboarding?) (not syncing?)) | ||
| (conj [:dispatch [:shell/show-root-view]]))}) |
There was a problem hiding this comment.
This logic was extracted from a few places related to the biometrics onboarding screen, onboarding events, and login events. The gist of this code is to correctly dispatch the continuation event based on whether we're onboarding with or without syncing (and with or without biometrics), or if it's a login.
| current-view-id (if (= db-view-id :screen/onboarding.syncing-biometric) | ||
| :screen/onboarding.enable-biometrics | ||
| db-view-id)] |
There was a problem hiding this comment.
Here we're handling a special case when attempting to navigate from the current view to the notification setup screen. During the onboarding flow with syncing, we've aliased to the onboarding view-id to :screen/onboarding.syncing-biometric, but that is not correct view-id for performing a navigation, so I replace it with the correct view-id
| (defn inject-local-profile-storage | ||
| [context] | ||
| (let [db (interceptor/get-coeffect context :db) | ||
| key-uid (get-in db [:profile/profile :key-uid]) | ||
| local-profile (mmkv/get-object key-uid)] | ||
| (assoc-in context | ||
| [:coeffects :local-profile-storage] | ||
| local-profile))) |
There was a problem hiding this comment.
Here's an implementation of a Re-Frame interceptor that synchronously accesses the mmkv key-value store, and it retrieves some metadata about a user profile via the key-uid.
…ding or login when displaying the enable-notifications screen
…rofile color does not exist
…reen during onboarding or login
…ews-notifications feature flag is active
# 50-character subject line # # 72-character wrapped longer description. # # * Maybe try answering these questions: * # * What did I change? * # * Why did I change it? * # * How did I change it? *
seanstrom
force-pushed
the
seanstrom/news-notifications-ui
branch
from
1ee1fd3 to
a5bdf6e
Compare
fixes #22110
fixes #22520
Related Figma Designs: https://www.figma.com/design/JlpPhJp0SMnEyBQy4nOZHo/Settings----Mobile?node-id=9076-64860&t=LcQeTqpusDFjU8Lq-4
Summary
react-mmkvdependency for storing some state for each user profile.Review notes
Here's some of the flows that I've tested to show the intended behaviour
Note, please enable the feature-flag
news-notificationsunder thesettingssection. Here's a screen capture of me shaking the device to enable the feature flag during onboarding.Feature-Flag.Activation.mp4
Create.Profile.-.New.Profile.mp4
Create.Profile.-.Recover.Profile.mp4
Create.Profile.-.Empty.Keycard.mp4
Login.Profile.-.Sync.Profile.mp4
Login.Profile.-.Existing.Profile.needing.prompt.mp4
Notification.Settings.-.Permissions.Banner.mp4
Notification.Settings.-.Permissions.Banner.iOS.mp4
Testing notes
Platforms
Areas that may be impacted
Functional
Steps to test
The steps to test the scope of this PR is mainly to confirm that
For testing the push notifications themselves, here's a document describing the process, but that might be out-of-scope of this PR. Likely we'll need to test this soon, but maybe it can wait for when we remove the feature flag (?)
Here's the doc: https://www.notion.so/How-to-manually-test-News-Push-Notifications-1ed8f96fb65c80ae84ecf27be6b61067?pvs=4
status: ready