feat: [FC-0047] Calendar synchronization #330
Conversation
# Conflicts: # core/src/main/res/values/strings.xml # course/src/main/java/org/openedx/course/presentation/container/CourseContainerFragment.kt # course/src/main/java/org/openedx/course/presentation/container/CourseContainerViewModel.kt # course/src/main/java/org/openedx/course/presentation/dates/CourseDatesScreen.kt # course/src/main/java/org/openedx/course/presentation/dates/CourseDatesViewModel.kt # course/src/main/java/org/openedx/course/presentation/videos/CourseVideosFragment.kt # course/src/test/java/org/openedx/course/presentation/container/CourseContainerViewModelTest.kt
…r_sync # Conflicts: # profile/src/main/java/org/openedx/profile/presentation/calendar/NewCalendarDialogFragment.kt
# Conflicts: # app/src/main/java/org/openedx/app/di/ScreenModule.kt # core/src/main/java/org/openedx/core/ui/ComposeCommon.kt # core/src/main/res/values/strings.xml # course/src/main/java/org/openedx/course/presentation/container/CourseContainerViewModel.kt # course/src/main/java/org/openedx/course/presentation/dates/CourseDatesScreen.kt # course/src/main/java/org/openedx/course/presentation/dates/CourseDatesViewModel.kt # course/src/main/java/org/openedx/course/presentation/outline/CourseOutlineViewModel.kt # course/src/main/res/values/strings.xml # course/src/test/java/org/openedx/course/presentation/dates/CourseDatesViewModelTest.kt # profile/src/main/java/org/openedx/profile/presentation/calendar/CalendarAccessDialogFragment.kt # profile/src/main/java/org/openedx/profile/presentation/calendar/NewCalendarDialogFragment.kt
…start CalendarSyncService
# Conflicts: # app/src/main/java/org/openedx/app/data/storage/PreferencesManager.kt # core/src/main/java/org/openedx/core/data/storage/CorePreferences.kt
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @PavloNetrebchuk! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
PavloNetrebchuk
force-pushed
the
feat/calendar_sync
branch
from
a068578
to
fe06870
Compare
PavloNetrebchuk
force-pushed
the
feat/calendar_sync
branch
from
f368fcd
to
5a4a2e9
Compare
PavloNetrebchuk
force-pushed
the
feat/calendar_sync
branch
from
5a4a2e9
to
0fec040
Compare
|
@PavloNetrebchuk That makes sense for the initial view but it should be visible once the state is changed to sync. |
We synchronize all courses that can be synced. The situation in the video looks as if "Demo Course 8" did not synchronize due to an error during the initial sync. We can see this from the synchronization indicator. Therefore, the application synchronized on the second attempt. |
We show the button as soon as it becomes possible. Before showing it, we need to check the state of all enrolled courses one by one. |
As far as I know, this was not part of our scope of work. @volodymyr-chekyrta Am I right? |
Actually, I think if the user made changes, they have reasons for that. As for me, it's the correct flow. |
|
I have marked the issues for easy reference. 😄 Issue 1: When an API error occurs during sync, the view is not visible. However, this should not happen. We should still display the synced courses and move the others to the unsynced state. Issue 2: A course should not be in the Synced state if we are not receiving the course dates Or if we encounter an error on it. Issue 3: From the logs, it appears there was never a call for Demo Course 8, meaning the execution stopped after encountering an error on Demo Course 7. We should ensure the process continues to pass all syncable courses. Issue 4: I apologize if my hardcoded example was confusing. I was referring to the mobile option "Reset Course Dates," which updates the course dates (also available on the web). In this case, the calendar and course dates will be in an unsynced state and will never be synced with the current logic. |
Issue 5The "Change Sync Options" feature creates a new calendar instance with every change. I think the intended implementation was to replace the existing instance instead. Change.Calendar.mp4 |
Issue 6App Crashes on Calendar Permission Revoked. Steps to Reproduce:
Issue 7Some UX changes were suggested by the iOS team for the Design Team:
|
|
@HamzaIsrar12 Regarding the other issues mentioned, we understand these concerns. While they are not critical, we are open to considering improvements and will incorporate them into the next pull requests as necessary. |
|
Can we find a way to split out suggestions for improvements from gaps in functionality or bugs? It is a bit difficult to follow here what needs to change versus what can be a future enhancement. I will try to re-read this from the top to gather the list of open issues as identified, but if anyone else has a clearer sense of the recap / the latest let me know. |
|
Issue 1 - The "Syncing X Courses" view does not appear during the initial sync. Issue 2 & 3 - There are a couple of issues with this flow: "Demo Course 7" was never synced, but it was placed on the Synced side. "Demo Course 8" wasn't synced initially but appeared after unsyncing other courses. Question 2a: I had a hard time understanding exactly what was happening in the shared video, I didn't catch / see when we triggered the unsync option. Can this be clarified? I'm curious to understand how often we expect this will happen / what triggers this. Result: Not clear what the impact / scope of this and whether we need to fix ASAP. Questions included above for next steps. Issue 4 - The Syncing isn't working properly once the course is unsynced externally. It sounds like we don't properly override any edits you may have made to calendar events edited from the calendar app when you rejoin the app / resync dates. Is that a fair assessment of the impact of this? If so, we can make a ticket for future cleanup of this case, but I dont expect it to be common for someone to edit these calendar events. Result: Created backlog ticket for this case, not worried about this causing issues in the short term. #365 Issue 5 - The "Change Sync Options" feature creates a new calendar instance with every change. I think the intended implementation was to replace the existing instance instead. Edit: Confirmed 🐛
Issue 6 - App Crashes on Calendar Permission Revoked. Issue 7 - Some UX changes were suggested by the iOS team for the Design Team: Result: I will flag these items to @sdaitzman for future review / updates but I think the first two items above we should consider out of scope of this (snackbar + empty state for synced / unsynced). I think the Relative Dates work might be in a separate PR. Second list of issues included below as well Question: Which view isn't visible when an API error occurs? Trying to understand where the user impact is here. Is this only within the sync options view or within the dates tab for a given course as well? Issue 2: A course should not be in the Synced state if we are not receiving the course dates Or if we encounter an error on it. Question: A course that isn't synced can be marked unsynced, but is this temporary between attempted resyncs or a permanent issue? This is similar to my question in "issue 2+3" above. Issue 3: From the logs, it appears there was never a call for Demo Course 8, meaning the execution stopped after encountering an error on Demo Course 7. We should ensure the process continues to pass all syncable courses. Question: Similar question about resync to other issues above. Issue 4: I apologize if my hardcoded example was confusing. I was referring to the mobile option "Reset Course Dates," which updates the course dates (also available on the web). In this case, the calendar and course dates will be in an unsynced state and will never be synced with the current logic. Question: I'm not sure I follow this - Does this mean that when you reset / update course dates we don't automatically trigger an update to the course dates calendar? If not we should create a ticket for this. |
|
Issue 1: To double-check. It’s okay that we don’t show the count while syncing, but an API error shouldn’t interrupt the flow. Issue 2: The course should be in the "Synced" state if the user selected it from the list or enrolled in it. Issue 3: The problem is unclear; @HamzaIsrar12 please provide us with more information. Issue 4: We ignore manual date changes as we consider them intentional. Issue 5: Needs to be fixed 🪲
Issue 7: Some UI updates need to be applied. |
@HamzaIsrar12, thank you for identifying this tricky case 🙌 . We have confirmed that this is a bug that interrupts the flow. |
|
Hello @HamzaIsrar12, |
|
Hey @marcotuts, Thank you for looking into this. To simplify, Issues 1 to 6 were bugs, and Issue 7 was a UX suggestion. Let's address your questions. To keep it simple, I will skip over the items that have been fixed based on recent comments. 😄 Issue 1: Fixed based on Volodymyr message 🚀 Issue 2 & 3:
Issue 4:
Issue 5 & 6: Fixed based on Pavlo message 🚀 |
|
Thanks you @PavloNetrebchuk for addressing the feedback. ✨ Due to the high priority of our MVP release, I won't be able to verify the changes myself at the moment. 🏎️ @volodymyr-chekyrta , could you please verify the addressed changes so we can proceed with merging this PR? To answer some of your questions: I compared the Android build with iOS for Issue 2, and it appears they were moving the courses to the unsynced state. You might want to compare both apps on a similar user to see the results. |
@HamzaIsrar12, sure, I understand the situation with the release. |
|
Explanation regarding the synchronisation of Demo Course 7 and Demo Course 8. (Issue 2 & 3)
We apologize for the confusion, we did not point this out earlier. |
|
@PavloNetrebchuk 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
I created tasks for these API Start Date issues fyi @PavloNetrebchuk , @volodymyr-chekyrta ^ Great to see this merged! |
https://www.figma.com/design/iZ56YMjbRMShCCDxqrqRrR/Open-edX-Mobile-App-All-Screens-v2.1?node-id=7368-44459&t=DjohnYrzKkt6Oipi-4