chore: notifications preferences screen implementation with edge cases #111

saeedbashir · 2025-01-01T11:42:52Z

Description

Jira: LEARNER-10361

APIs to implement
GET /api/notifications/configurations

POST /api/notifications/preferences/update-all/

Only APIs Implementation
/api/notifications/read/
/api/notifications/mark-seen/discussion/

Note: These APIs will be helpful in implementing the Inbox screen of notifications and will be used in a different PR.

Preference switch’s On/Off edge cases
Switch is ON

Notifications permission has been given to the app by the user &&

in the GET /api/notifications/configurations API’s response, following should be true:

data > discussion > enabled == true
data > discussion > notification_types > core > push == true

Switch is OFF

app hasn’t requested notifications permission from the user
app hasn’t been granted notifications permission from the user
the GET /api/notifications/configurations has failed for some reason
in the GET /api/notifications/configurations API’s response, any of the following is false:

data > discussion > enabled

data > discussion > notification_types > core > push

User’s interaction with the switch
The switch is OFF and the user tries to turn it ON, here’s what we’d need to do incase of:

Fresh start

Show an info popup to guide user into giving the notification permission and why it’s needed with 2 CTAs:

Cancel: dismiss the popup
Continue: send a system request for the OS to allow / decline notifications for the app. If the user:
Declines: the switch stays OFF
Accepts: the switch changes to ON and we hit the POST API to let our server know

Normal start
Given that user had previously declined the notifications permission, show the same info popup to guide user into giving the notification permission and why it’s needed with 2 CTAs:

Cancel: dismiss the popup

Continue: open the system’s Settings screen for our app where the user can update the notifications permission and comes back to app while turning/keeping the system’s switch to:

OFF: the switch stays OFF

ON: the switch changes to ON and we hit the POST API to let our server know

General flow
User has given the notifications permission and they try to turn the switch ON / OFF, we just hit the POST API and handle the API’s error cases as follows.

Error cases
Persist the state of the switch in shared preferences and initially load the switch according to it while hitting the GET API in the background upon whose response the state of the switch will be updated.

Incase the user tries to change the state of the switch while they’re offline or the GET API fails for some reason we need to:

return the switch back to its previous state

and show an error message in a snack bar

mta452 · 2025-01-02T08:57:50Z

Core/Core/Configuration/BaseRouter.swift

@@ -86,6 +86,9 @@ public protocol BaseRouter {

    @MainActor
    func hideRestoreProgressView()
+
+    @MainActor
+    func performNotificationRegistration()


Isn't router responsible for navigation purposes only?

PushNotificationsManager belongs to OpenedX target and Notifications is added as a framework to it. And accessing the parent from the child isn't a good idea. That's why it's here.

Maybe we should create a protocol in Core, implement it in PushNotificationsManager and access it from there?

It already move to the OEXFoundation a plugin based architecture, moving this there will make it harder to move to the plugin architecture, thats why I keep it simple.

mta452 · 2025-01-02T08:59:57Z

OpenEdX/Router.swift

@@ -773,6 +773,10 @@ public class Router: AuthorizationRouter,
        // show notifications inbox screen
    }

+    public func performNotificationRegistration() {
+        Container.shared.resolve(PushNotificationsManager.self)?.performRegistration()


This seems like business logic. Please move this function to relevant screen.

If we move it to the Notifications framework, then we have to do some work that needs to be performed after requesting the permissions. PushNotificationsManager is responsible for doing all the notification related stuff.

Core/Core/Data/CoreStorage.swift

Dashboard/Dashboard/Presentation/PrimaryCourseDashboardView.swift

Notifications/Notifications/Presentation/Settings/NotificationsSettingsViewModel.swift

mta452 · 2025-01-02T10:32:13Z

Notifications/Notifications/Presentation/Settings/NotificationsSettingsViewModel.swift

+    }
+
+    @MainActor
+    public func toggleNotificationsPermissionAction() async {


A better name of this function would be syncDiscussionsActivitySetting if I'm understanding its functionality correctly. Also, the async nature of this function requires calling it from a Task; would be better to keep it a regular function.

API calls cannot be made from a normal function.

Yeah, the API part can be handled in a separate function.

I guess there isn't a need of a new function because that function will be called with await.

shafqat-muneer · 2025-01-03T11:33:30Z

Notifications/Notifications/Presentation/Settings/NotificationsSettingsView.swift

+                                .simultaneousGesture(
+                                    TapGesture().onEnded {
+                                        Task {
+                                            await viewModel.toggleNotificationsPermissionAction()
+                                        }
+                                    }
+                                )
+                                .simultaneousGesture(
+                                    DragGesture(minimumDistance: 20, coordinateSpace: .local).onEnded { _ in
+                                        Task {
+                                            await viewModel.toggleNotificationsPermissionAction()
+                                        }
+                                    })


It appears there is an issue with the toggles; if I long-press the switch, it turns on or off without making any API calls.

OpenEdX/Data/AppStorage.swift

shafqat-muneer · 2025-01-03T11:53:22Z

Suggestion: Maybe we should add a network indicator or some kind of progress during notification preference toggle API call. This would let the user know something is happening in the background and also prevent multiple interactions with the toggle switch.

saeedbashir · 2025-01-03T12:09:33Z

It was added and then removed as we don't want to show loader on this screen.

rnr · 2025-01-03T13:54:27Z

Screen.Recording.2025-01-03.at.14.43.43.mov

First run, user goes to Settings and tap on some preference Toggle

Do we need to show alert which says "You need to allow notifications in settings" (which means you will redirect to the Settings app) and second immediately after this with request permissions? Why just not request permissions?
Why after user allowed notifications Toggle is still opted off? If user tapped this and gave permissions the user expects it will be opted on, right?

@saeedbashir WDYT?

saeedbashir · 2025-01-03T13:58:34Z

Yes after giving the permission, the switch should turn on automatically. Let me check why it's not turning on.

rnr · 2025-01-03T14:03:28Z

@saeedbashir The second question is why preferences showing state is depend on device permissions status? Preferences are something stored on backend and user should have ability to see and change this independent on permissions on particular device, no? For example user has two devices: first one has push notifications allowed and second one doesn't have. So user will see different values on different devices (actual state for first and all disabled on second one)?

rnr · 2025-01-03T14:16:19Z

For example, one day we got the ability to change push settings on a website (as other systems do). This does not depend on whether push notifications are allowed or not on the user's device. Why do we limit this capability to the device?
Maybe it would be better to show some indicator in the push notification settings, for example: Push notifications are not allowed in the Settings app - please allow to use this feature.

saeedbashir · 2025-01-03T14:20:05Z

The user will not be able to receive the notifications regardless of the server saved value if the user denied / not given the notification permissions to the app. That's why it's dependent on the device settings / permissions.

saeedbashir · 2025-01-03T14:20:29Z

@rnr Change is pushed and ready to review.

rnr · 2025-01-03T14:34:23Z

Yes, I realise that the user will not receive push notifications without permissions.
I mean it's weird to show all preferences disabled, but actually have them enabled. Is this a product/design decision @moiz994 @touchapp ?
If we want to restrict the user from viewing/editing push notification preferences on a device without push enabled, perhaps we should check permissions one level higher and not allow the user to open the push notification settings View without permissions at all?

saeedbashir · 2025-01-03T14:37:12Z

We don't have such a design at the moment; we might cater / consider it in the 2nd phase of push notifications.

rnr · 2025-01-03T14:45:28Z

I don't think we need to wait for 2nd phase to fix showing preferences with the wrong state, what do you think? Do we need to open discussion about this (in Slack for example)?

saeedbashir · 2025-01-03T14:47:02Z

Yes a slack thread would be nice.

Notifications/Notifications/Data/Network/NotificationsEndpoint.swift

Notifications/Notifications/Data/NotificationsRepository.swift

Notifications/Notifications/Domain/NotificationsInteractor.swift

Notifications/Notifications/Presentation/Settings/NotificationsSettingsViewModel.swift

rnr · 2025-01-06T16:35:18Z

On the iPad, the notification icon in the Learn view looks displaced

Learn	Profile

rnr · 2025-01-06T16:36:43Z

UI bug for Notifications Settings View on iPad

mta452 · 2025-01-10T14:10:13Z

This needs to handled as a separate task.

CC: @moiz994 @miankhalid

rnr · 2025-01-13T12:03:18Z

@mta452
Do we have Figma design for Settings page? Asking because I see the difference between these Push Settings and Video Settings for example

moiz994 · 2025-01-13T12:29:57Z

Hey Anton, the UX team did not create the design I did really since we were short on UX bandwidth. I created the design just to give an idea of what it will look like. We should definitely follow the same pattern as the video settings example you have quoted.

mta452 · 2025-01-14T08:57:33Z

I have updated the design.

iPhone	iPad

rnr

LGTM!

shafqat-muneer

LGTM 👍

chore: notifications preferences screen implementation with edge cases

393607e

saeedbashir requested review from rnr and shafqat-muneer January 1, 2025 11:42

mta452 reviewed Jan 2, 2025

View reviewed changes

refactor: address review feedback

8716ec3

saeedbashir requested a review from mta452 January 3, 2025 04:44

saeedbashir added 2 commits January 3, 2025 09:50

refactor: address review feedback

c60e49c

chore: implement seen and read APIs

118c68c

shafqat-muneer requested changes Jan 3, 2025

View reviewed changes

saeedbashir added 2 commits January 3, 2025 17:53

refactor: address review feedback

f18d521

refactor: address review feedback

97949a1

shafqat-muneer approved these changes Jan 3, 2025

View reviewed changes

fix: address review feedback

90ef35b

rnr requested changes Jan 6, 2025

View reviewed changes

shafqat-muneer mentioned this pull request Jan 10, 2025

chore: Notifications Inbox Screen Implementation #113

Open

chore: fix spelling mistakes

a310d6b

mta452 added 3 commits January 10, 2025 18:27

chore: fix indentation

1b48556

chore: present system alert for push notifications

74ed02f

chore: set optional handler parameter to nil

1a20310

mta452 added 2 commits January 13, 2025 12:45

chore: fix ui issues on ipad

8d1ca84

chore: minor refactoring

7cf97d8

mta452 requested review from rnr and shafqat-muneer and removed request for mta452 January 13, 2025 09:00

chore: make ui consistent with other settings screens

15fe1fc

rnr approved these changes Jan 14, 2025

View reviewed changes

shafqat-muneer approved these changes Jan 14, 2025

View reviewed changes

mta452 merged commit 57bf369 into 2U/develop Jan 14, 2025
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: notifications preferences screen implementation with edge cases #111

chore: notifications preferences screen implementation with edge cases #111

saeedbashir commented Jan 1, 2025 •

edited

Loading

mta452 Jan 2, 2025

saeedbashir Jan 2, 2025

mta452 Jan 2, 2025

saeedbashir Jan 3, 2025

mta452 Jan 2, 2025

saeedbashir Jan 2, 2025 •

edited

Loading

mta452 Jan 2, 2025

saeedbashir Jan 2, 2025

mta452 Jan 2, 2025

saeedbashir Jan 3, 2025

shafqat-muneer Jan 3, 2025

shafqat-muneer commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

rnr commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

rnr commented Jan 3, 2025

rnr commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

rnr commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

rnr commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

rnr commented Jan 6, 2025

rnr commented Jan 6, 2025 •

edited

Loading

mta452 commented Jan 10, 2025

rnr commented Jan 13, 2025 •

edited

Loading

moiz994 commented Jan 13, 2025

mta452 commented Jan 14, 2025

rnr left a comment

shafqat-muneer left a comment

chore: notifications preferences screen implementation with edge cases #111

chore: notifications preferences screen implementation with edge cases #111

Conversation

saeedbashir commented Jan 1, 2025 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saeedbashir Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shafqat-muneer commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

rnr commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

rnr commented Jan 3, 2025

rnr commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

rnr commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

rnr commented Jan 3, 2025

saeedbashir commented Jan 3, 2025

rnr commented Jan 6, 2025

rnr commented Jan 6, 2025 • edited Loading

mta452 commented Jan 10, 2025

rnr commented Jan 13, 2025 • edited Loading

moiz994 commented Jan 13, 2025

mta452 commented Jan 14, 2025

rnr left a comment

Choose a reason for hiding this comment

shafqat-muneer left a comment

Choose a reason for hiding this comment

saeedbashir commented Jan 1, 2025 •

edited

Loading

saeedbashir Jan 2, 2025 •

edited

Loading

rnr commented Jan 6, 2025 •

edited

Loading

rnr commented Jan 13, 2025 •

edited

Loading