feat: App navigational flow for deep links/push notifications #329

eyatsenkoperpetio · 2024-03-07T13:43:08Z

[iOS] App navigational flow for deep links/push notifications #248

We need to implement the internal paths in the app for implementing deep links and push notifications. The Following wiki explains the implementation required.

Push Notifications And Deep Linking On Mobile

APNS Files:
edx apns.zip

How to use?
https://github.com/openedx/openedx-app-ios/assets/76485998/3e91c934-26ab-4a5f-b513-d4b8cfc38968

# Conflicts: # config_script/process_config.py

rnr

LGTM
just minor code-style fixes requested

rnr · 2024-03-07T17:34:42Z

Core/Core/View/Base/AlertView.swift

+                )
+            case .viewDeepLink:
+                configure(
+                    primaryButtonTitle: "View",


String should be localised

rnr · 2024-03-07T17:47:34Z

Core/Core/View/Base/AlertView.swift

please remove extra vertical spaces

rnr · 2024-03-07T17:49:52Z

Discussion/Discussion/Presentation/DiscussionRouter.swift

-    public func showThread(thread: UserThread, postStateSubject: CurrentValueSubject<PostState?, Never>) {}
+    public func showThreads(courseID: String, topics: Topics, title: String, type: ThreadType, animated: Bool) {}

+    public func showThread(thread: UserThread, postStateSubject: CurrentValueSubject<PostState?, Never>, animated: Bool) {}


please fix line length warning

rnr · 2024-03-07T17:55:55Z

OpenEdX/Router.swift

-                     DiscussionRouter {
-
+                     DiscussionRouter
+{


please fix opening brace spacing violation

rnr · 2024-03-07T17:58:22Z

Podfile.lock

 PODFILE CHECKSUM: 881176d00eabfe8f78d6022c56c277cf61aad22b

-COCOAPODS: 1.15.2
+COCOAPODS: 1.15.0


I don't think we need to downgrade cocoapod version

rnr · 2024-03-07T17:59:03Z

default_config/config_settings.yaml

@@ -1,4 +1,4 @@
-config_directory: './default_config'
+config_directory: '../edx-mobile-config'


please don't push changes in default config values

rnr · 2024-03-07T18:01:37Z

Discussion/Discussion/Data/Model/Data_TopicsResponse.swift

+    }
+}
+
+


please remove extra vertical space

rnr · 2024-03-11T13:51:09Z

@eyatsenkoperpetio please fix merge conflicts

# Conflicts: # Core/Core/View/Base/AlertView.swift # OpenEdX/View/MainScreenView.swift

saeedbashir

How about going to user edit profile for user profile instead of opening read only user info? Thoughts?

saeedbashir · 2024-03-12T04:36:21Z

Core/Core/View/Base/AlertView.swift

-                    CoreAssets.warning.swiftUIImage.renderingMode(.template)
-                        .foregroundColor(Theme.Colors.textPrimary)
+                switch type {
+                case .deleteVideo:


How about merging this case with deleteVideo because both are using the same icon?

saeedbashir · 2024-03-12T09:53:21Z

OpenEdX/Managers/DeepLinkManager/DeepLinkManager.swift

@@ -26,9 +31,31 @@ public protocol DeepLinkService {
 public class DeepLinkManager {


Need to disabled warning, Type Body Length Violation: Type body should span 300 lines or less excluding comments and whitespace: currently spans 308 lines (type_body_length)

saeedbashir · 2024-03-12T09:55:47Z

OpenEdX/Managers/DeepLinkManager/DeepLinkManager.swift

+        Task {
+            await navigateToScreen(with: link.type, link: link)
+        }


How about moving this into else of if UIApplication.shared.applicationState == .active {

saeedbashir · 2024-03-12T10:20:10Z

Core/Core/View/Base/AlertView.swift

    case logOut
    case leaveProfile
    case deleteVideo
+    case viewDeepLink


This can be simply deepLink.

saeedbashir · 2024-03-12T10:21:26Z

Core/Core/en.lproj/Localizable.strings

 "ALERT.LEAVE" = "Leave";
 "ALERT.KEEP_EDITING" = "Keep editing";
 "ALERT.DELETE" = "DELETE";
+"ALERT.WAITING" = "Waiting...";


I've discussed this with @moiz994 and he's saying lets only show full screen loader without text waiting...

saeedbashir · 2024-03-12T10:25:57Z

OpenEdX/Managers/DeepLinkManager/DeepLinkRouter/DeepLinkRouter.swift

+        DispatchQueue.main.asyncAfter(deadline: .now() + (isCourseOpened ? 0 : 1)) {
+            switch link.type {
+            case .courseDashboard:
+                self.hostCourseContainerView?.rootView.viewModel.selection = CourseTab.course.rawValue


It would be nice if you can use weak reference of self in block.

we don't need use weak self in this case, we don't have retain cycle

saeedbashir · 2024-03-12T13:36:39Z

OpenEdX/Router.swift

+    func getNavigationController() -> UINavigationController {
+        navigationController
+    }


How about changing the access modifier of navigationController to public instead of defining a method. It's a read-only property so its not possible to change it from outside. Thoughts?

I think we don't need have public navigationController, I use only for deep link

saeedbashir · 2024-03-12T13:43:56Z

OpenEdX/Managers/DeepLinkManager/Link/PushLink.swift

-        let alert = aps?[DataKeys.alert] as? [String: Any]
-        title = alert?[DataKeys.title] as? String
-        body = alert?[DataKeys.body] as? String
+        let aps = dictionary[DataKeys.aps.rawValue] as? [String: Any]


Because of Anyhashable RawStringExtractable is not useable so please remove it from DataKeys.

volodymyr-chekyrta

LGTM 👍
Please address @saeedbashir's feedback

…x#329) * chore: add router to deep link manager * chore: add deep link router * chore: add discussion threads * chore: program, discovery details, user profile * chore: disable animation for progress and refactor * chore: move deep link router to folder * chore: show user profile as sheet * chore: fix close presented screen * chore: not open again programs or discovery content * chore: improve deep link navigation * chore: improve deep link navigation * chore: improve deep link navigation * chore: check if ids isEmpty * chore: add open announcement * chore: improve deep link discussion * chore: remove empty line * chore: generate test files * chore: remove extra codes and add strings * chore: resolve PR commnets * chore: resolve PR comments * chore: avatar can be changed in "limited profile" mode * chore: make dashboard as default tab

eyatsenkoperpetio added 18 commits February 26, 2024 12:58

chore: add router to deep link manager

9d77b01

Merge branch 'develop' into feat/deep-link-routing

479f0a2

chore: add deep link router

1287279

chore: add discussion threads

c2cf4d9

chore: program, discovery details, user profile

35c0a32

chore: disable animation for progress and refactor

e891476

chore: move deep link router to folder

9c93182

Merge branch 'develop' into feat/deep-link-routing

0a925e8

# Conflicts: # config_script/process_config.py

chore: show user profile as sheet

d39ba78

chore: fix close presented screen

8a05b68

chore: not open again programs or discovery content

ce35a8d

chore: improve deep link navigation

71a6afd

chore: improve deep link navigation

1744f11

chore: improve deep link navigation

ef8bdb1

chore: check if ids isEmpty

1b2ed05

chore: add open announcement

cc9c94f

chore: improve deep link discussion

56315a6

chore: remove empty line

7e877a7

eyatsenkoperpetio requested review from rnr, saeedbashir and volodymyr-chekyrta March 7, 2024 13:43

eyatsenkoperpetio linked an issue Mar 7, 2024 that may be closed by this pull request

[iOS] App navigational flow for deep links/push notifications #248

Closed

eyatsenkoperpetio added 2 commits March 7, 2024 14:46

chore: generate test files

1d89923

chore: remove extra codes and add strings

8bdd5be

rnr previously approved these changes Mar 7, 2024

View reviewed changes

chore: resolve PR commnets

a929ca0

eyatsenkoperpetio dismissed rnr’s stale review via a929ca0 March 11, 2024 11:21

rnr previously approved these changes Mar 11, 2024

View reviewed changes

Merge branch 'develop' into feat/deep-link-routing

b475484

# Conflicts: # Core/Core/View/Base/AlertView.swift # OpenEdX/View/MainScreenView.swift

eyatsenkoperpetio dismissed rnr’s stale review via b475484 March 11, 2024 14:50

Merge branch 'develop' into feat/deep-link-routing

bef84ba

saeedbashir suggested changes Mar 12, 2024

View reviewed changes

saeedbashir reviewed Mar 12, 2024

View reviewed changes

eyatsenkoperpetio added 2 commits March 13, 2024 15:18

chore: resolve PR comments

2ddad6c

chore: avatar can be changed in "limited profile" mode

aa49bdf

eyatsenkoperpetio linked an issue Mar 14, 2024 that may be closed by this pull request

[iOS] User's logo can't be changed in "limited profile" mode #278

Closed

chore: make dashboard as default tab

f8ce0e1

eyatsenkoperpetio linked an issue Mar 14, 2024 that may be closed by this pull request

iOS: Change the default tab to Learn tab instead of Discover tab #256

Closed

volodymyr-chekyrta approved these changes Mar 15, 2024

View reviewed changes

saeedbashir approved these changes Mar 15, 2024

View reviewed changes

volodymyr-chekyrta merged commit 016cf09 into openedx:develop Mar 15, 2024

volodymyr-chekyrta mentioned this pull request Mar 18, 2024

feat: Dates Tab integration with Calendar #347

Merged

rnr deleted the feat/deep-link-routing branch March 19, 2024 14:49

HamzaIsrar12 mentioned this pull request May 23, 2024

feat: added the external router for deep links openedx/openedx-app-android#320

Merged

		@@ -1,4 +1,4 @@
		config_directory: './default_config'
		config_directory: '../edx-mobile-config'

		@@ -26,9 +31,31 @@ public protocol DeepLinkService {
		public class DeepLinkManager {

feat: App navigational flow for deep links/push notifications #329

feat: App navigational flow for deep links/push notifications #329

Uh oh!

Conversation

eyatsenkoperpetio commented Mar 7, 2024

Uh oh!

rnr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rnr commented Mar 11, 2024

Uh oh!

saeedbashir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

volodymyr-chekyrta left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants