chore: discussions deep link and push notifications support for new course dashboard #1752
Conversation
| @@ -103,33 +103,36 @@ import UIKit | |||
| return .none | |||
| } | |||
|
|
|||
| private func showCourseDashboardViewController(with link: DeepLink) { | |||
| private func showCourseDashboardViewController(with link: DeepLink, completion: ((UIViewController?) -> Void)? = nil) { | |||
There was a problem hiding this comment.
The completion handler should always called, but there are several cases when it is not. I would suggest reworking the logic for this function a bit, because it is rather confusing with all the if statements.
There was a problem hiding this comment.
The if statements are there to start the deep link navigation from the course dashboard if the user is already on the same course for which the deep link received.
I've removed all the cases because I guess it's not a big deal if the flow starts from the my courses screen on opening/activating the app from the push notification or deep link.
| @@ -103,33 +103,36 @@ import UIKit | |||
| return .none | |||
| } | |||
|
|
|||
| private func showCourseDashboardViewController(with link: DeepLink) { | |||
| private func showCourseDashboardViewController(with link: DeepLink, completion: ((UIViewController?) -> Void)? = nil) { | |||
| guard let topViewController = topMostViewController else { return } | |||
|
|
|||
| if environment?.config.isNewDashboardEnabled == true { | |||
There was a problem hiding this comment.
It may help to break "new" and "old" cases out into their own function. Like:
if environment?.config.isNewDashboardEnabled {
showCourseDashboardViewControllerNew(with: link, completion)
} else {
showCourseDashboardViewControllerOld(with: link, completion)
}
There was a problem hiding this comment.
This isn't needed here as the router is doing this job like
if environment.config.isNewDashboardEnabled {
showCourseNew(with: deeplink, courseID: courseID, from: controller, completion: completion)
} else {
showCourseOld(with: deeplink, courseID: courseID, from: controller, completion: completion)
}
| @@ -363,10 +370,10 @@ import UIKit | |||
| courseDashboardController.switchTab(with: link.type) | |||
| } | |||
| else { | |||
| self?.showCourseDashboardViewController(with: link) | |||
| self?.showCourseDashboardViewController(with: link) {_ in | |||
There was a problem hiding this comment.
The completion handler has a UIViewController parameter, but you don't use the parameter in this closure. I think you can get rid of it.
There was a problem hiding this comment.
It was an old piece of code and I haven't checked either the parameter is in use somewhere else or not. It seems it's not being used anywhere, I've removed it.
| return | ||
| } | ||
| } | ||
| } | ||
|
|
||
| dismiss() { [weak self] _ in | ||
| if let topController = self?.topMostViewController { | ||
| self?.environment?.router?.showCourse(with: link, courseID: link.courseId ?? "", from: topController) | ||
| self?.environment?.router?.showCourse(with: link, courseID: link.courseId ?? "", from: topController, completion: completion) |
There was a problem hiding this comment.
This function has a completion handler with a UIViewController parameter, but as far as I can tell from the rest of the code, it is never used. I think you an get rid of it.
| if let currentVisibleController = dashboardController.currentVisibleController { | ||
| completion?(currentVisibleController) | ||
| } | ||
| completion?() | ||
| } else if let enrolledTabBarController = controller.find(viewController: EnrolledTabBarViewController.self) { | ||
| if let courseDashboardController = courseDashboardController { |
There was a problem hiding this comment.
if this line is false, completion() will never get called
There was a problem hiding this comment.
This is intentionally, because if a certain flow didn't happen in a path as required, the deep link navigation will be ignored instead of a wrong navigation. If we want to call it in all cases, in that case we have to pass a boolean to see either the completion was from the desired place or not.
| } | ||
| } else if let enrolledTabBarController = controller.find(viewController: EnrolledTabBarViewController.self) { | ||
| private func showCourseNew(with deeplink: DeepLink, courseID: String, from controller: UIViewController, completion: (() -> Void)? = nil) { | ||
| if let enrolledTabBarController = controller.find(viewController: EnrolledTabBarViewController.self) { |
There was a problem hiding this comment.
if this line is false, completion() will never get called
Source/OEXRouter+Swift.swift
Outdated
| @@ -285,8 +272,8 @@ extension OEXRouter { | |||
| dashboardController = controller as? NewCourseDashboardViewController | |||
| } | |||
| dashboardController?.switchTab(with: deeplink.type, deeplink: deeplink) | |||
| if let currentVisibleController = dashboardController?.currentVisibileController { | |||
| completion?(currentVisibleController) | |||
| if (dashboardController?.currentVisibileController) != nil { | |||
There was a problem hiding this comment.
if this line is false, completion() will never get called
*chore: revert app nav changes (#1685) * chore: add indicator on tab bar (#1687) * chore: update learn tab icon (#1698) * chore: add empty state to my courses (#1697) * chore: add dashboard header view (#1705) * chore: Course Dashboard Error Screen for Non-Upgradable Courses (#1708) * chore: course tab bar flow changes in course dashboard (#1707) * chore: add error view on enrolled courses screen (#1718) * chore: course dashboard error screen for no access but upgradable courses (#1716) * chore: scrolling behaviour on course dashboard header (#1723) * feat: add new error screen for outdated version (#1725) * feat: certificate banner stylistic changes on new course dashboard (#1727) * chore: add scrolling behaviour to redesigned Learn tab (#1729) * chore: add collapsible section headers (#1726) * chore: resume course button implementation for new design #1737 (#1738) * chore: fix deeplinks with new dashboard (#1742) * fix: fix broken tests * chore: discussions deep link and push notifications support for new course dashboard (#1752) * chore: update status bar color to match course dashboard header color (#1754) * feat: show shift dates banner on new course dashboard (#1753) * feat: show shift dates banner on new course dashboard * refactor: improve value prop logic in case of embedded value prop on course dashboard fix: fix fetching of course price * fix: fix course share button on iPad (#1757) * fix: fix blank screen on fresh enrollment with app nav work (#1759) * fix: view showing behind collapsible headers (#1758) * chore: unit and component navigation (#1748) * chore: handle user interaction when user swipes quickly in page view controller (#1766) * chore: incorporate changes of course price and currency code for execute api in app_nav (#1768) * refactor: celebratory modal layout improvements (#1767) * chore: handle landscape for new dashboard and component navigation (#1769) * chore: address app nav feedback (#1772) * chore: fix title label constraint (#1775) * fix: revert of fix title label constraint (#1775)" (#1778) * fix: fix unable to dismiss an error message from course list screen (#1777) * fix: fix for subsection unwanted collapse (#1776) * fix: fix crash on new enrollment on course dashboard (#1781) * chore: expand next section after a section is marked as completed (#1782) * chore: update the edit screen title to Personal information (#1785) * chore: update shift dates banner on component screen (#1786) * chore: improve scrolling experience on discover title bar (#1788) * chore: handle header view in video block (#1798)
Description
LEARNER-9322
Test
I've created the following links for
course_id: course-v1:edX+Inf101+2022The command can be used to activate deeplink from the command line,
xcrun simctl openurl booted {}deep_link