Skip to content
This repository has been archived by the owner on May 6, 2024. It is now read-only.

chore: discussions deep link and push notifications support for new course dashboard #1752

Merged
merged 3 commits into from
May 24, 2023
Merged

chore: discussions deep link and push notifications support for new course dashboard #1752

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b1ab4c
chore: deep link and push notifications support for new course dashboard
saeedbashir May 5, 2023
f659c5b
refactor: address review feedback
saeedbashir May 10, 2023
f56351d
refactor: remove unnecessary if condition
saeedbashir May 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions Source/Deep Linking/DeepLinkManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,33 +103,36 @@ import UIKit
return .none
}

private func showCourseDashboardViewController(with link: DeepLink) {
private func showCourseDashboardViewController(with link: DeepLink, completion: ((UIViewController?) -> Void)? = nil) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

guard let topViewController = topMostViewController else { return }

if environment?.config.isNewDashboardEnabled == true {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
        }

if let courseDashboardView = topViewController as? NewCourseDashboardViewController, courseDashboardView.courseID == link.courseId {
if !controllerAlreadyDisplayed(for: link.type) {
courseDashboardView.switchTab(with: link.type)
completion?(courseDashboardView)
return
}
} else if let dashboardViewController = topViewController.navigationController?.viewControllers.first(where: { $0 is NewCourseDashboardViewController }) as? NewCourseDashboardViewController, dashboardViewController.courseID == link.courseId {
dashboardViewController.navigationController?.popToRootViewController(animated: true) {
dashboardViewController.switchTab(with: link.type)
completion?(dashboardViewController)
}
return
}
} else {
if let courseDashboardView = topViewController.parent as? CourseDashboardViewController, courseDashboardView.courseID == link.courseId {
if !controllerAlreadyDisplayed(for: link.type) {
courseDashboardView.switchTab(with: link.type, componentID: link.componentID)
completion?(courseDashboardView)
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
Expand Down Expand Up @@ -345,6 +348,10 @@ import UIKit
return postController.topicID == link.topicID
}

if isControllerAlreadyDisplayed {
return
}

func showDiscussionPosts() {
if let topController = topMostViewController {
environment?.router?.showDiscussionPosts(from: topController, courseID: courseId, topicID: topicID)
Expand All @@ -363,10 +370,10 @@ import UIKit
courseDashboardController.switchTab(with: link.type)
}
else {
self?.showCourseDashboardViewController(with: link)
self?.showCourseDashboardViewController(with: link) {_ in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

showDiscussionPosts()
}
}

showDiscussionPosts()
}
}
}
Expand All @@ -381,6 +388,10 @@ import UIKit
return discussionResponseController.threadID == link.threadID
}

if isControllerAlreadyDisplayed {
return
}

func showResponses() {
if let topController = topMostViewController {
environment?.router?.showDiscussionResponses(from: topController, courseID: courseId, threadID: threadID, isDiscussionBlackedOut: false, completion: completion)
Expand Down Expand Up @@ -423,6 +434,10 @@ import UIKit
return discussionResponseController.threadID == link.threadID
}

if isControllerAlreadyDisplayed {
return
}

func showComment() {
if let topController = topMostViewController, let discussionResponseController = topController as? DiscussionResponsesViewController {
environment?.router?.showDiscussionComments(from: discussionResponseController, courseID: courseID, commentID: commentID, threadID:threadID)
Expand Down Expand Up @@ -559,25 +574,13 @@ import UIKit
showUserProfile(with: link)
break
case .discussionTopic:
if isNewDashboardEnabled {
showCourseDashboardViewController(with: link)
} else {
showDiscussionTopic(with: link)
}
showDiscussionTopic(with: link)
break
case .discussionPost:
if isNewDashboardEnabled {
showCourseDashboardViewController(with: link)
} else {
showDiscussionResponses(with: link)
}
break
case .discussionComment:
if isNewDashboardEnabled {
showCourseDashboardViewController(with: link)
} else {
showdiscussionComments(with: link)
}
case .courseHandout:
if isNewDashboardEnabled {
showCourseDashboardViewController(with: link)
Expand Down
Loading