feat: Ability to shift courses dates if deadlines have been missed #288

shafqat-muneer · 2024-02-18T21:24:50Z

Jira Ticket: LEARNER-9781: iOS - Ability to shift courses dates if deadlines have been missed

Github Issues: #161

Light Mode	Dark Mode

saeedbashir

Getting response validation failed error for course course-v1:USMx+PGPM661.3x+2T2022 while getting course dates status.
It would be nice if dates status info view appear with animation like resume course.
Please remove all the temp files pushed like [.mocky3C3F2BD4-1ED2-461C-96E3-33CAE5082F47/.config.yml.tmp](https://github.com/openedx/openedx-app-ios/pull/288/files#diff-ae688431c3fdb1778a03a28b5d0af9281947494db8d4c254fe74bcce1cf97637)

saeedbashir · 2024-02-19T06:15:44Z

Core/Core/View/Base/SuccessViewWithButton.swift

+import Combine
+import Theme
+
+public struct SuccessViewWithButton: View {


How about renaming it to DatesShiftedSuccessView Thoughts?

saeedbashir · 2024-02-21T10:21:39Z

Core/Core/en.lproj/Localizable.strings

+"COURSE_DATES.TOAST_SUCCESS_MESSAGE" = "Your due dates have been successfully shifted to help you stay on track.";
+"COURSE_DATES.VIEW_ALL_DATES" = "View all dates";
+
+"COURSE_DATES.RESET_DATE.RESET_DATE_BANNER.BODY" = "Don't worry - shift our suggested schedule to complete past due assignments without losing any progress.";


It would be nice if they can move these to course module as they are related to course.

saeedbashir · 2024-02-21T10:38:26Z

Course/Course/Presentation/Container/CourseContainerViewModel.swift

@@ -81,6 +84,7 @@ public class CourseContainerViewModel: BaseCourseViewModel {
        super.init(manager: manager)

        addObservers()
+        addNotificationObserver()


No need to define new method, new observers can be added to addObservers.

saeedbashir · 2024-02-21T10:52:34Z

Course/Course/Presentation/Container/CourseContainerViewModel.swift

@@ -47,6 +49,7 @@ public class CourseContainerViewModel: BaseCourseViewModel {
    private let authInteractor: AuthInteractorProtocol
    private let analytics: CourseAnalytics
    private(set) var storage: CourseStorage
+    private var shiftCourseDatesObserver: NSObjectProtocol?


I guess this observer declaration isn't needed and work can be done by NotificationCenter.default.addObserver(.....

saeedbashir · 2024-02-21T10:54:00Z

Course/Course/Presentation/Container/CourseContainerViewModel.swift

+        let center = NotificationCenter.default
+        guard let shiftCourseDatesObserver = self.shiftCourseDatesObserver else { return }
+        center.removeObserver(shiftCourseDatesObserver)


No need to remove a specific observer, you can simply call NotificationCenter.default.removeObserver(self).

saeedbashir · 2024-02-21T11:17:23Z

Course/Course/Presentation/Dates/CourseDatesViewModel.swift

+                guard let strongSelf = self else {
+                    return
+                }


it can be updated to guard let self.

saeedbashir · 2024-02-21T11:18:13Z

Course/Course/Presentation/Dates/CourseDatesViewModel.swift

+                guard let strongSelf = self else {
+                    return
+                }
+                Task {


What if we can define addNotificationObserver as MainActor so we don't have to define tasks here and even we can omit DispatchQueue

If we designate it with @MainActor and eliminate the Task closure, we would have to utilize an async function here. However, this change leads to certain exceptions. To resolve this issue, we may consider employing addObserver(forName:object:queue:using:). Currently, this method is not being utilized for notifications in the application.

saeedbashir · 2024-02-21T11:21:42Z

Course/Course/Presentation/Container/CourseContainerViewModel.swift

+                Task {
+                    await strongSelf.getCourseBlocks(courseID: courseID, withProgress: true)
+                    await strongSelf.getCourseDeadlineInfo(courseID: courseID, withProgress: true)
+                    DispatchQueue.main.async {
+                        strongSelf.dueDatesShifted = true
+                    }


How about defining addNotificationObserver as MainActor so we don't have to define Task here and even can omit DispatchQueue

saeedbashir · 2024-02-21T11:23:14Z

Course/Course/Presentation/Outline/CourseOutlineView.swift


    @State private var openCertificateView: Bool = false
    private var idiom: UIUserInterfaceIdiom { UIDevice.current.userInterfaceIdiom }

    @State private var showingDownloads: Bool = false
    @State private var showingVideoDownloadQuality: Bool = false
    @State private var showingNoWifiMessage: Bool = false
+    @Binding private var selection: Int


How about renaming it to some more meaning name like selectedTabIndex or something similar.

The ScrollSlidingTabBar also employs an identical naming convention and logic for consistency purposes.

saeedbashir · 2024-02-21T11:28:22Z

Podfile.lock

@@ -182,4 +182,4 @@ SPEC CHECKSUMS:

 PODFILE CHECKSUM: 544edab2f9ecc4ac18973fb8865f1d0613ec8a28

-COCOAPODS: 1.15.0
+COCOAPODS: 1.14.3


We don't need to push files generated with a lower version.

shafqat-muneer · 2024-02-23T14:42:07Z

Getting response validation failed error for course course-v1:USMx+PGPM661.3x+2T2022 while getting course dates status.

It would be nice if dates status info view appear with animation like resume course.

Please remove all the temp files pushed like [.mocky3C3F2BD4-1ED2-461C-96E3-33CAE5082F47/.config.yml.tmp](https://github.com/openedx/openedx-app-ios/pull/288/files#diff-ae688431c3fdb1778a03a28b5d0af9281947494db8d4c254fe74bcce1cf97637)

Done with 2nd and 3rd point.
The first one pertains to error handling and should be addressed in a distinct pull request, as it is unrelated to the modifications made in this pull request.

volodymyr-chekyrta · 2024-02-26T09:29:11Z

I haven't finished the review yet, but most of the code looks good.

I have a slight concern about this view.
It doesn't look like a notification or alert from the Open edX mobile design system.
Button with no rounded corners, the X button paddings.

@marcotuts @sdaitzman, could you please take a look from a design perspective?

rnr

LGTM

rnr · 2024-02-26T09:26:42Z

Course/Course/Data/CourseRepository.swift

@@ -224,9 +237,6 @@ public class CourseRepository: CourseRepositoryProtocol {
    }
 }

-// Mark - For testing and SwiftUI preview
-// swiftlint:disable all
-#if DEBUG


@shafqat-muneer I don't think we need to remove this condition for Mock purposes

rnr · 2024-02-26T10:09:40Z

Course/Course/Views/DatesShiftedSuccessView.swift

+    }
+}
+
+struct SuccessViewWithButton_Previews: PreviewProvider {


please add #if DEBUG

…9781

saeedbashir · 2024-02-26T11:51:43Z

Course/Course/Presentation/Dates/DatesStatusInfoView.swift

+}
+
+#if DEBUG
+struct AdjustScheduleView_Previews: PreviewProvider {


It would be nice if you can rename the preview class as per the class name

Thanks for highlighting it; It was skipped during struct rename.

saeedbashir · 2024-02-26T11:53:07Z

Course/Course/Views/DatesShiftedSuccessView.swift

+}
+
+#if DEBUG
+struct SuccessViewWithButton_Previews: PreviewProvider {


It would be nice if you could rename the preview as per the class name.

saeedbashir · 2024-02-26T11:54:18Z

Course/Course/en.lproj/Localizable.strings

+"COURSE_DATES.TOMORROW" = "Tomorrow";
+"COURSE_DATES.YESTERDAY" = "Yesterday";


No need to duplicate these keys, we can use them from the Core.

moiz994 · 2024-02-26T12:25:38Z

I haven't finished the review yet, but most of the code looks good.
I have a slight concern about this view.
It doesn't look like a notification or alert from the Open edX mobile design system.
Button with no rounded corners, the X button paddings.
image
@marcotuts @sdaitzman, could you please take a look from a design perspective?

@volodymyr-chekyrta Regarding the above message, we are trying to display a message in the form of a snack bar similar to Adroid's native snack bar. Currently, it's replicating what the live application shows and I don't think such a component exists in the schema design system. Let me reach out to marco and sam on slack to finalize the approach for this.

volodymyr-chekyrta · 2024-02-26T12:30:09Z

I haven't finished the review yet, but most of the code looks good.
I have a slight concern about this view.
It doesn't look like a notification or alert from the Open edX mobile design system.
Button with no rounded corners, the X button paddings.
image
@marcotuts @sdaitzman, could you please take a look from a design perspective?

@volodymyr-chekyrta Regarding the above message, we are trying to display a message in the form of a snack bar similar to Adroid's native snack bar. Currently, it's replicating what the live application shows and I don't think such a component exists in the schema design system. Let me reach out to marco and sam on slack to finalize the approach for this.

@moiz994 We have this type of view in the profile and a few other screens.
Paddings are different, and all buttons have rounded corners.
We can discuss it tomorrow at the Mobile Design Weekly meeting.

moiz994 · 2024-02-26T12:41:26Z

I haven't finished the review yet, but most of the code looks good.
I have a slight concern about this view.
It doesn't look like a notification or alert from the Open edX mobile design system.
Button with no rounded corners, the X button paddings.
image
@marcotuts @sdaitzman, could you please take a look from a design perspective?

@volodymyr-chekyrta Regarding the above message, we are trying to display a message in the form of a snack bar similar to Adroid's native snack bar. Currently, it's replicating what the live application shows and I don't think such a component exists in the schema design system. Let me reach out to marco and sam on slack to finalize the approach for this.

@moiz994 We have this type of view in the profile and a few other screens. Paddings are different, and all buttons have rounded corners. We can discuss it tomorrow at the Mobile Design Weekly meeting.

Thanks for sharing, @volodymyr-chekyrta! I can't find these components in the figma design file but I think the team is updating the snack bar to match this styling.

sdaitzman · 2024-02-26T15:58:18Z

Thanks for flagging this for design review. I added this to our mobile design weekly agenda for tomorrow, and can add the missing snack bar component in Figma. I also think the snack bar background in dark mode should be darker to improve contrast and match the rest of the app, will make sure to include that change in Figma:

I also wonder if the upgrade suggestion messages should include an inline upgrade button to offer a clear path to upgrades?

Everything else looks good to me!

shafqat-muneer · 2024-02-26T18:15:38Z

@sdaitzman I have implemented the snackbarInfoAlert background to accommodate both darker and lighter modes of alerts. Please assess whether the changes appear to be an enhancement.
Light Mode:

Dark Mode:

Regarding second point of inline upgrade button, it's intentional because:

Mobile payments are not implemented yet and to avoid breaking appstore guidelines, upgrade button is hidden, which leads user to payments

sdaitzman · 2024-02-26T18:22:43Z

Thanks, @shafqat-muneer. Makes sense about mobile payments.

Unfortunately, using light text on the info background actually has a worse contrast ratio.

I've created an early rough draft of this bottom dialog in Figma this morning, screenshots attached below. I want to run this by the mobile design weekly group tomorrow morning, then will follow up with any design updates to get this unblocked quickly.

volodymyr-chekyrta · 2024-02-27T15:07:12Z

@shafqat-muneer, at the Mobile Design Weekly meeting, we discussed and agreed on a design for the "Due Dates Shifted" view/snackbar.
It's coming soon; @sdaitzman is working on it.

sdaitzman · 2024-02-27T18:24:41Z

@shafqat-muneer here is the proposed design, attached and viewable in Figma. When the user shifts due dates from within the Course tab, the dialog should include the button to View All Dates. When the user shifts due dates from within the CTA inside the Dates tab, the dialog should not include a View All Dates button.

Also want to make sure to note the wider dialog, which helps to distinguish this snack bar view from the content underneath.

With View All Dates, for within the Course tab

Without View All Dates, for within the Dates tab

In-context within Course tab (Figma link)	In-context within Dates tab (Figma link)

shafqat-muneer · 2024-02-28T05:41:13Z

@sdaitzman Thank you for the design review and for providing all necessary details. 👏 I will address the change and share details.

shafqat-muneer · 2024-02-28T12:10:06Z

@sdaitzman @volodymyr-chekyrta snackbar design has been updated.

Light Mode Course Tab	Dark Mode Course Tab

Light Mode Dates Tab	Dark Mode Dates Tab

sdaitzman · 2024-02-28T13:14:27Z

Thanks @shafqat-muneer, this looks great!

saeedbashir · 2024-02-29T03:17:42Z

Course/Course/Presentation/Container/CourseContainerViewModel.swift

+    func resetDueDatesShiftedFlag() {
+        dueDatesShifted = false


Do we need a method just to change one variable? Can we directly change it's value as the variable isn't private.

This variable cannot be set as private since it is a @Published property and is being displayed in various views. The reason for creating a method to set its default value is to ensure that the view model is responsible for updating the property value, maintaining a single source of truth.

saeedbashir · 2024-02-29T03:24:38Z

Course/Course/Presentation/Container/CourseContainerViewModel.swift

+    @MainActor
+    func shiftDueDates(courseID: String, withProgress: Bool = true) async {
+        isShowProgress = withProgress
+        dueDatesShifted = false


Do we really need to set it to false, as it will be false by default and even after dates shift?

I utilized it specifically for generating mock data. The initializer won't be invoked again if we remain on the same screen and trigger the shift due dates request within a time span of 5 seconds. The snackbar will not be hidden before the next API call. Indeed, you're correct, in a real-world scenario, this situation is unlikely to occur. Therefore, I am just removing it.

saeedbashir · 2024-02-29T03:27:54Z

Course/Course/Presentation/Container/CourseContainerViewModel.swift

+                        await self.getCourseDeadlineInfo(courseID: courseID, withProgress: true)
+                    }
+                    DispatchQueue.main.async {
+                        self.dueDatesShifted = true


How about using weak self here?

I would even ask you not to use Concurrency and GCD frameworks at the same time.
Please use MainActor.run or similar approach to switch to the main thread.

saeedbashir · 2024-02-29T03:30:30Z

Course/Course/Presentation/Dates/CourseDatesViewModel.swift

+    @objc private func handleShiftDueDates(_ notification: Notification) {
+        if let courseID = notification.object as? String {
+            Task {
+                await self.getCourseDates(courseID: courseID)


self is redundant.

saeedbashir · 2024-02-29T03:31:34Z

Course/Course/Presentation/Dates/CourseDatesViewModel.swift

+                    guard let self else { return }
+                    self.dueDatesShifted = true


Do we need to check wether the self exists or not, can't self?.dueDatesShifted = true be ignored if the self will be nil?

Same here, please do not combine Swift Concurrency and GCD frameworks.
Use the Main Actor instead.

volodymyr-chekyrta · 2024-02-29T13:34:20Z

Course/Course/Data/CourseRepository.swift

+        do {
+            let courseDates = try
+            await getCourseDates(courseID: courseID)
+            return CourseDateBanner(
+                datesBannerInfo: courseDates.datesBannerInfo,
+                hasEnded: courseDates.hasEnded
+            )
+        } catch {
+            throw error
+        }


Do we need to catch an error just to throw it further?
Seems like we can simplify by removing the do-catch block.

volodymyr-chekyrta · 2024-02-29T13:36:25Z

Course/Course/Data/Model/Data_CourseDates.swift

+        }
+
+        // Cases are defied according to this link
+        // https://2u-internal.atlassian.net/browse/LEARNER-7724?focusedCommentId=479226


This private link does not make sense for the Open edX repo and community.
Better to describe the entire flow here or move it to the documentation.

Cases are self explanatory and I believe we can remove this reference link.

volodymyr-chekyrta · 2024-02-29T13:38:13Z

Course/Course/Data/Network/CourseEndpoint.swift

@@ -37,6 +39,10 @@ enum CourseEndpoint: EndPointType {
            return url
        case .getCourseDates(let courseID):
            return "/api/course_home/v1/dates/\(courseID)"
+        case .getCourseDeadlineInfo(courseID: let courseID):


case .getCourseDeadlineInfo(courseID: let courseID):
can be simplified:
case .getCourseDeadlineInfo(let courseID):

volodymyr-chekyrta · 2024-02-29T13:40:42Z

Course/Course/Presentation/Container/CourseContainerViewModel.swift

+
+        NotificationCenter.default.addObserver(
+            self,
+            selector: #selector(handleShiftDueDates(_:)),


If I'm not mistaken, just #selector(handleShiftDueDates) is enough.

volodymyr-chekyrta · 2024-02-29T13:43:13Z

Course/Course/Presentation/Dates/CourseDatesViewModel.swift

+    private func addObservers() {
+        NotificationCenter.default.addObserver(
+            self,
+            selector: #selector(handleShiftDueDates(_:)),


#selector(handleShiftDueDates)

volodymyr-chekyrta · 2024-02-29T13:49:41Z

👍 🚀 🔥

shafqat-muneer · 2024-02-29T15:17:13Z

@volodymyr-chekyrta @saeedbashir Feedback addressed 🎉

volodymyr-chekyrta

LGTM 👍

saeedbashir

LGTM 👍

feat: Ability to shift courses dates if deadlines have been missed

33ca703

shafqat-muneer requested a review from saeedbashir February 18, 2024 21:24

saeedbashir linked an issue Feb 19, 2024 that may be closed by this pull request

[iOS] Option "Shift due date" #161

Open

shafqat-muneer added 2 commits February 19, 2024 16:16

refactor: address failed test cases.

aedcf55

refactor: address failed test cases.

80f6f0a

saeedbashir requested changes Feb 21, 2024

View reviewed changes

refactor: address review feedback

7e35d91

shafqat-muneer requested review from rnr and volodymyr-chekyrta February 23, 2024 14:31

shafqat-muneer added 2 commits February 23, 2024 19:32

refactor: address review feedback

ca85abb

refactor: address review feedback

b56337a

rnr previously approved these changes Feb 26, 2024

View reviewed changes

shafqat-muneer added 2 commits February 26, 2024 15:51

Merge remote-tracking branch 'upstream/develop' into Shafqat/LEARNER-…

3bc581c

…9781

refactor: address review feedback

43cd286

shafqat-muneer dismissed rnr’s stale review via 43cd286 February 26, 2024 11:06

fix: resolved failed test cases

bdefe98

saeedbashir reviewed Feb 26, 2024

View reviewed changes

refactor: address review feedback

50de3dd

shafqat-muneer added 2 commits February 28, 2024 17:00

refactor: due dates shifted snakbar design updates

65a5262

refactor: removed process_config.py file changes

0ea59d6

shafqat-muneer added 2 commits February 28, 2024 21:48

Merge remote-tracking branch 'origin/develop' into Shafqat/LEARNER-9781

871a262

Merge remote-tracking branch 'origin/develop' into Shafqat/LEARNER-9781

fcc45cc

saeedbashir reviewed Feb 29, 2024

View reviewed changes

volodymyr-chekyrta reviewed Feb 29, 2024

View reviewed changes

refactor: address review feedback

1e5c062

volodymyr-chekyrta approved these changes Mar 1, 2024

View reviewed changes

saeedbashir approved these changes Mar 1, 2024

View reviewed changes

shafqat-muneer merged commit be22a55 into openedx:develop Mar 1, 2024
3 checks passed

shafqat-muneer deleted the Shafqat/LEARNER-9781 branch March 1, 2024 11:09

		"COURSE_DATES.TOMORROW" = "Tomorrow";
		"COURSE_DATES.YESTERDAY" = "Yesterday";

feat: Ability to shift courses dates if deadlines have been missed #288

feat: Ability to shift courses dates if deadlines have been missed #288

Conversation

shafqat-muneer commented Feb 18, 2024 • edited Loading

saeedbashir left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shafqat-muneer commented Feb 23, 2024

volodymyr-chekyrta commented Feb 26, 2024

rnr left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moiz994 commented Feb 26, 2024

volodymyr-chekyrta commented Feb 26, 2024

moiz994 commented Feb 26, 2024

sdaitzman commented Feb 26, 2024 • edited Loading

shafqat-muneer commented Feb 26, 2024

sdaitzman commented Feb 26, 2024

volodymyr-chekyrta commented Feb 27, 2024

sdaitzman commented Feb 27, 2024 • edited Loading

shafqat-muneer commented Feb 28, 2024

shafqat-muneer commented Feb 28, 2024

sdaitzman commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saeedbashir Feb 29, 2024 • 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

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

Choose a reason for hiding this comment

volodymyr-chekyrta commented Feb 29, 2024

shafqat-muneer commented Feb 29, 2024

volodymyr-chekyrta left a comment

Choose a reason for hiding this comment

saeedbashir left a comment

Choose a reason for hiding this comment

shafqat-muneer commented Feb 18, 2024 •

edited

Loading

sdaitzman commented Feb 26, 2024 •

edited

Loading

sdaitzman commented Feb 27, 2024 •

edited

Loading

saeedbashir Feb 29, 2024 •

edited

Loading