feat: Dates Tab integration with Calendar by shafqat-muneer · Pull Request #347 · openedx/openedx-app-ios

shafqat-muneer · 2024-03-17T20:58:27Z

Jira Tickets:

LEARNER-9665: iOS - Dates Tab integration with Calendar
LEARNER-9855: iOS - Calendar Integration deep links implementation in local calendar events

Github Issues:

Light Mode	Dark Mode

Demos

Light Mode: Sync and un-sync events in calendar application:

Simulator.Screen.Recording.-.iPhone.15.-.2024-03-15.at.12.42.44.mp4

Light Mode: Update out dated calendar:

Simulator.Screen.Recording.-.iPhone.15.-.2024-03-15.at.12.49.21.mp4

Light Mode: Deep Linking:

RPReplay_Final1710750802.mp4

Dark Mode: Sync and un-sync events in calendar application:

Simulator.Screen.Recording.-.iPhone.15.-.2024-03-15.at.12.43.27.mp4

Dark Mode: Update out dated calendar:

Simulator.Screen.Recording.-.iPhone.15.-.2024-03-15.at.12.50.38.mp4

volodymyr-chekyrta · 2024-03-18T09:46:45Z

Hey @shafqat-muneer, thanks, looks great! 🚀
Do you know why the deep link from the calendar app didn't take the user to the specific course page?
I thought it would work after @eyatsenkoperpetio introduced #329
Do we need to develop anything else to make it work like that?

RPReplay_Final1710750802.mp4

volodymyr-chekyrta · 2024-03-18T09:53:42Z

+	<key>NSCalendarsUsageDescription</key>
+	<string>edX would like to use your calendar list to subscribe to your personalized edX calendar for this course.</string>
+	<key>NSCalendarsFullAccessUsageDescription</key>
+	<string>edX would like to use your calendar list to subscribe to your personalized edX calendar for this course.</string>


Could you please not hardcode the name of the edX app?
Ideally, we should use platformName or something similar from the config.yaml.

shafqat-muneer · 2024-03-18T12:19:41Z

Hey @shafqat-muneer, thanks, looks great! 🚀 Do you know why the deep link from the calendar app didn't take the user to the specific course page? I thought it would work after @eyatsenkoperpetio introduced #329 Do we need to develop anything else to make it work like that?

RPReplay_Final1710750802.mp4

Hi @volodymyr-chekyrta, thanks for the review.
I've just finished implementing the generation of deep link URLs for the calendar in this pull request. That's why it hasn't been included in #329. I'll proceed to open another pull request to direct the user to the specific course page.

volodymyr-chekyrta · 2024-03-18T14:40:54Z

Hey @shafqat-muneer, thanks, looks great! 🚀 Do you know why the deep link from the calendar app didn't take the user to the specific course page? I thought it would work after @eyatsenkoperpetio introduced #329 Do we need to develop anything else to make it work like that?
RPReplay_Final1710750802.mp4

Hi @volodymyr-chekyrta, thanks for the review. I've just finished implementing the generation of deep link URLs for the calendar in this pull request. That's why it hasn't been included in #329. I'll proceed to open another pull request to direct the user to the specific course page.

Makes sense to me, thank you! 👍

rnr

LGTM, some small changes

rnr · 2024-03-19T13:10:23Z

+import Core
+import BranchSDK
+
+enum DeepLinkType: String {


@shafqat-muneer maybe we could re-use DeepLinkType and DeepLinkKeys defined in DeepLink class to avoid duplication?
Thank you

rnr · 2024-03-19T13:21:52Z

+                        courseID: courseID,
+                        title: calendarName,
+                        isOn: true,
+                        modalPresented: false)


Could you move the bracket to the next line to respect the "parameter on its own line" style?

rnr · 2024-03-19T13:24:10Z


            if viewModel.dueDatesShifted {
-                DatesShiftedSuccessView(selectedTab: .dates, courseDatesViewModel: viewModel)
+                DatesSuccessView(title: CourseLocalization.CourseDates.toastSuccessTitle,


"parameter on its own line" style please

rnr · 2024-03-19T13:24:47Z

+                CoreAssets.syncToCalendar.swiftUIImage
+                Toggle(
+                    CourseLocalization.CourseDates.syncToCalendar,
+                    isOn: $viewModel.isOn)


"parameter on its own line" style please

rnr · 2024-03-19T13:25:55Z


                if viewModel.dueDatesShifted && !isVideo {
-                    DatesShiftedSuccessView(selectedTab: .course, courseContainerViewModel: viewModel) {
+                    DatesSuccessView(title: CourseLocalization.CourseDates.toastSuccessTitle,


"parameter on its own line" style please

…9665

saeedbashir

On adding the second calendar the syncing calendar keeps on screen and never goes away.
On selecting cancel remove on remove calendar alert, the switch should turns back on.

saeedbashir · 2024-03-21T06:36:27Z

+                DatesSuccessView(
+                    title: CourseLocalization.CourseDates.calendarEvents,
+                    message: CourseLocalization.CourseDates.calendarEventsAdded,
+                    selectedTab: .dates
+                ) {
+                    Task {
+                        await MainActor.run {
+                            viewModel.calendarEventsAdded = false
+                        }
+                    }
+                }


This code is being shared between the following three conditions calendarEventsAdded, calendarEventsRemoved, and calendarEventsUpdated. How about moving it to a function and use that. Even that func can be used for dueDatesShifted with an additional param.

saeedbashir · 2024-03-21T06:38:18Z

+    @Published var calendarEventsAdded: Bool = false
+    @Published var calendarEventsRemoved: Bool = false
+    @Published var calendarEventsUpdated: Bool = false


Instead of using a boolean for each condition, how about using an enum for these cases?

saeedbashir · 2024-03-21T06:46:26Z

+            },
+            okTapped: {
+                self.router.dismiss(animated: true)
+                self.removeCourseCalendar { [weak self] success in


success is unused here, so it can be replaced with _.

saeedbashir · 2024-03-21T06:50:18Z

+            positiveAction: CourseLocalization.CourseDates.calendarShiftPromptUpdateNow,
+            onCloseTapped: {
+                // Remove course calendar
+                self.router.dismiss(animated: true)


How about using weak self in closures? if so please update it for all alert functions.

saeedbashir · 2024-03-21T06:56:28Z

+        get {
+            if let calendarEntry = calendarEntry,
+               let localCalendar = localCalendar {
+                if calendarEntry.identifier == localCalendar.calendarIdentifier {


Is it possible to move this if condition to where clause of if let?

shafqat-muneer · 2024-03-22T15:26:38Z

@saeedbashir I have made the recommended adjustments. Kindly review them.

saeedbashir · 2024-03-25T02:56:23Z

+            } else {
+                if let localCalendar = localCalendar {


I guess else and il let can be merged like else if let

saeedbashir · 2024-03-25T02:59:34Z

+            ),
+            positiveAction: CoreLocalization.Alert.accept,
+            onCloseTapped: { [weak self] in
+                guard let self else { return }


It seems strong reference if self isn't required in this type of blocks because if the self will be nill, by using the optional self will not crash the app and following code will automatically be ignored.

shafqat-muneer · 2024-03-25T08:24:46Z

@saeedbashir I have addressed requested changes.

@volodymyr-chekyrta Please also review this pull request. Thank you.

saeedbashir

LGTM 👍

volodymyr-chekyrta

LGTM 👍
Just one question

volodymyr-chekyrta · 2024-03-26T09:28:48Z

+            return DatesSuccessView(
+                title: title,
+                message: message,
+                selectedTab: .dates
+            ) {
+                Task {
+                    await MainActor.run {
+                        viewModel.eventState = CourseDatesViewModel.EventState.none
+                    }
+                }
+            }


Can it be simplified just to something like this?

return DatesSuccessView( title: title, message: message, selectedTab: .dates ) { viewModel.eventState = CourseDatesViewModel.EventState.none }

This seems a bit odd. We switch to a Task only to switch back to the MainActor.

@volodymyr-chekyrta Thank you for bringing that to my attention. I've streamlined the logic for simplification.

Explanation:
The DatesSuccessView struct was previously invoking action closures asynchronously on a background thread. Upon receiving the closure action, we had to switch back to the main thread to avoid warnings such as:

Publishing changes from background threads is not allowed; make sure to publish values from the main thread (via operators like receive(on:)) on model updates.

I've refactored this logic so that the action closures in DatesSuccessView are no longer asynchronous.

feat: Dates Tab integration with Calendar

13f53bf

shafqat-muneer requested a review from saeedbashir March 17, 2024 20:58

shafqat-muneer modified the milestone: https://github.com/openedx/openedx-app-ios/issues/160 Mar 17, 2024

shafqat-muneer requested review from rnr and volodymyr-chekyrta March 18, 2024 07:46

volodymyr-chekyrta reviewed Mar 18, 2024

View reviewed changes

refactor: address review feedback and fix failed test cases.

6d23d1a

rnr previously approved these changes Mar 19, 2024

View reviewed changes

shafqat-muneer dismissed rnr’s stale review via 23b03dc March 20, 2024 14:24

refactor: address review feedback and course link outside application

9ad0c82

shafqat-muneer force-pushed the Shafqat/LEARNER-9665 branch from 23b03dc to 9ad0c82 Compare March 20, 2024 14:29

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

9363059

…9665

saeedbashir suggested changes Mar 21, 2024

View reviewed changes

Shafqat Muneer added 3 commits March 22, 2024 18:14

refactor: address review feedback

13124e2

refactor: auto mock test cases generation

7f2b223

refactor: resolve conflicts

0843f48

saeedbashir reviewed Mar 25, 2024

View reviewed changes

refactor: address review feedback

d0a3bbc

saeedbashir previously approved these changes Mar 25, 2024

View reviewed changes

rnr previously approved these changes Mar 25, 2024

View reviewed changes

volodymyr-chekyrta previously approved these changes Mar 26, 2024

View reviewed changes

volodymyr-chekyrta reviewed Mar 26, 2024

View reviewed changes

refactor: address review feedback

8363d7f

shafqat-muneer dismissed stale reviews from volodymyr-chekyrta, rnr, and saeedbashir via 8363d7f March 26, 2024 11:47

rnr approved these changes Mar 26, 2024

View reviewed changes

shafqat-muneer merged commit d02b303 into openedx:develop Mar 26, 2024

shafqat-muneer deleted the Shafqat/LEARNER-9665 branch March 26, 2024 12:39

Uh oh!

Conversation

shafqat-muneer commented Mar 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Demos

Uh oh!

volodymyr-chekyrta commented Mar 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shafqat-muneer commented Mar 18, 2024

Uh oh!

volodymyr-chekyrta commented Mar 18, 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!

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!

shafqat-muneer commented Mar 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shafqat-muneer commented Mar 25, 2024

Uh oh!

saeedbashir left a comment

Choose a reason for hiding this comment

Uh oh!

volodymyr-chekyrta 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shafqat-muneer commented Mar 17, 2024 •

edited

Loading