search

openedx / openedx-app-android

The mobile app for Android for the Open EdX Platform.
Apache License 2.0
17 stars 23 forks source link

feat: [FC-0047] Calendar synchronization #330

Closed PavloNetrebchuk closed 2 months ago

PavloNetrebchuk commented 4 months ago

https://www.figma.com/design/iZ56YMjbRMShCCDxqrqRrR/Open-edX-Mobile-App-All-Screens-v2.1?node-id=7368-44459&t=DjohnYrzKkt6Oipi-4

Light Theme Dark Theme
https://github.com/openedx/openedx-app-android/assets/141041606/1b21439c-6fb8-4435-9a60-1aee6320aa12 https://github.com/openedx/openedx-app-android/assets/141041606/5e0f2d42-a176-461d-a7d4-f25a45c81627
Screenshot 2024-06-12 at 11 58 10 Screenshot 2024-06-12 at 11 58 30
Screenshot 2024-06-12 at 12 00 01 Screenshot 2024-06-12 at 12 00 24
Screenshot 2024-06-12 at 12 00 55 Screenshot 2024-06-12 at 12 01 14
openedx-webhooks commented 4 months ago

Thanks for the pull request, @PavloNetrebchuk!

What's next?

Please work through the following steps to get your changes ready for engineering review:

:radio_button: Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

:radio_button: Provide context

To 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:

:radio_button: Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

:radio_button: Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/openedx-mobile-maintainers. Tag them in a comment and let them know that your changes are ready for review.

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:

:bulb: As a result it may take up to several weeks or months to complete a review and merge your PR.

sdaitzman commented 3 months ago

Thanks @PavloNetrebchuk! At a glance, this looks great to me from the design side.

volodymyr-chekyrta commented 3 months ago

@HamzaIsrar12 @k1rill, Kindly review the changes made in this pull request. Thank you 🙌

miankhalid commented 3 months ago

Since this is a complex PR, lets wait for approvals from TouchApp and Axinite both before merging this PR, thanks!

HamzaIsrar12 commented 2 months ago

Since the /api/mobile/v1/users/\(username)/enrollments_status/ API is not available on the backend master branch, I'm using the RG team's sandbox for manual testing.

API_HOST_URL: 'https://axim-mobile.raccoongang.net'
OAUTH_CLIENT_ID: 'zP3vPz00c8fTRpYjNbVSlA1fxt9LnCxTM4JK1KQ0'
HamzaIsrar12 commented 2 months ago

@volodymyr-chekyrta The newly introduced enrollment status API returns the following response:

[
  {
    "course_id": "course-v1:OpenEdX+aiforbegginers1+2024_T1",
    "course_name": "AI For Beginners",
    "is_active": true
  },
  ...
]

How is the value of is_active determined?

volodymyr-chekyrta commented 2 months ago

@volodymyr-chekyrta The newly introduced enrollment status API returns the following response:

[
  {
    "course_id": "course-v1:OpenEdX+aiforbegginers1+2024_T1",
    "course_name": "AI For Beginners",
    "is_active": true
  },
  ...
]

How is the value of is_active determined?

is_active: false means that the course (xblocks) has not been used by this user for more than 30 days. The basic idea is to exclude inactive courses from synchronization.

HamzaIsrar12 commented 2 months ago

Preferably, is_active should also be false if the course access has expired or the course hasn't started yet. In both cases, the course dates aren't available, leading to a Sync Failed call.

HamzaIsrar12 commented 2 months ago

Issue 1

The "Syncing X Courses" view does not appear during the initial sync.

HamzaIsrar12 commented 2 months ago

Issue 2 & 3

There are a couple of issues with this flow:

https://github.com/openedx/openedx-app-android/assets/71447999/84fa8f9c-d674-4f1d-832e-724662fd91d4

PavloNetrebchuk commented 2 months ago

The "Syncing X Courses" view does not appear during the initial sync.

The user will see "Syncing 0 Courses" if we show that button before initialisation. This can be confusing, so we hide the button.

HamzaIsrar12 commented 2 months ago

Issue 4

The Syncing isn't working properly once the course is unsynced externally.

https://github.com/openedx/openedx-app-android/assets/71447999/97c49d7a-d40d-4ab2-b254-24745c976525

HamzaIsrar12 commented 2 months ago

@PavloNetrebchuk That makes sense for the initial view but it should be visible once the state is changed to sync.

PavloNetrebchuk commented 2 months ago

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.

    Calendar.Syning.Issue.mp4

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.

PavloNetrebchuk commented 2 months ago

@PavloNetrebchuk That makes sense for the initial view but it should be visible once the state is changed to sync.

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.

PavloNetrebchuk commented 2 months ago

The Syncing isn't working properly once the course is unsynced externally.

Calenday.Sync.Change.Date.Issue.mp4

As far as I know, this was not part of our scope of work. @volodymyr-chekyrta Am I right?

PavloNetrebchuk commented 2 months ago

The Syncing isn't working properly once the course is unsynced externally.

Calenday.Sync.Change.Date.Issue.mp4

Actually, I think if the user made changes, they have reasons for that. As for me, it's the correct flow.

HamzaIsrar12 commented 2 months ago

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.

HamzaIsrar12 commented 2 months ago

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.

https://github.com/openedx/openedx-app-android/assets/71447999/8b96c469-7f7a-4d0a-99c2-08867b177044

HamzaIsrar12 commented 2 months ago

Issue 6

App Crashes on Calendar Permission Revoked.

Steps to Reproduce:

Issue 7

Some UX changes were suggested by the iOS team for the Design Team:

PavloNetrebchuk commented 2 months ago

@HamzaIsrar12 Issue 6 has been resolved.

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.

marcotuts commented 2 months ago

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.

marcotuts commented 2 months ago

Issue 1 - The "Syncing X Courses" view does not appear during the initial sync. Edit: Fixed ~~Update: The user will see "Syncing 0 Courses" if we show that button before initialization. This can be confusing, so we hide the button. Result: No change required here I think~~

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. Question 2b: Is the broader issue that syncs fail / break when we attempt to change sync settings mid-sync? Question 2c: Does the system automatically attempt resync (I think that's the csae but want to confirm. Question 2d: Is the issue of a course that is synced vs unsynced being inaccurately labeled only a problem in the sync settings view or do the course dates tab for those courses also show the wrong status. Is the wrong status shown temporarily while the resync happens or it is permanently incorrect once broken the first time?

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. https://github.com/openedx/openedx-app-android/issues/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 🐛 Question 5a: I agree we generally want to rename an existing calendar instead of creating a new calendar if possible. If this isn't a simple fix we should create a separate ticket as with issue 4 above. Will wait for development clarity on this. I know in certain cases (ex: renamed calendar outside of application) we can't detect this and would need to "resync" again, but for renames of the calendar name within the app I would expect to have full knowledge of this and be able to simply edit the existing calendar, or delete the old and replace with the new.

Result: pending Question 5a

Issue 6 - App Crashes on Calendar Permission Revoked. ~~Edit: Fixed Result: Sounds like this has been fixed in a follow-up.~~

Issue 7 - Some UX changes were suggested by the iOS team for the Design Team: It would be nice to have a Snackbar on Sync Complete In case of an empty list in Synced or UnSynced state we should show an Empty State. Just to point it out, The "Relative Dates" section is also missing.

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 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.

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.

volodymyr-chekyrta commented 2 months ago

@PavloNetrebchuk

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. The API should always return dates. Even if an error occurs during this iteration, the app will try again in 24 hours.

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. However, if the dates are changed on the backend, the application will update them since their checksum will change.

Issue 5: Needs to be fixed 🪲

Issue 6: Fixed.

Issue 7: Some UI updates need to be applied.

volodymyr-chekyrta commented 2 months ago

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.

@HamzaIsrar12, thank you for identifying this tricky case 🙌 . We have confirmed that this is a bug that interrupts the flow. It will be fixed.

PavloNetrebchuk commented 2 months ago

Hello @HamzaIsrar12, Issue 1: Fixed. Synchronisation wouldn't stop if one of the courses encountered an exception. Issue 5: Fixed. Issue 7: a) An empty state was added to the list. b) Relative dates will be added in the next PR. c) It was decided not to add a Snackbar on Sync Complete. I confirmed this with the iOS team. Let me know if there's anything else you need!

HamzaIsrar12 commented 2 months ago

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: The issues should hopefully be resolved with the solution for Issue 1. I only conducted a manual review, so @PavloNetrebchuk , please assist Marco with the followup questions.

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. Question 2b: Is the broader issue that syncs fail / break when we attempt to change sync settings mid-sync? Question 2c: Does the system automatically attempt resync (I think that's the csae but want to confirm. Question 2d: Is the issue of a ....

Issue 4: I will add details in the Ticket once free from the MVP or will close it if not needed. 👍🏻

Result: Created backlog ticket for this case, not worried about this causing issues in the short term. https://github.com/openedx/openedx-app-android/issues/365 ... 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 5 & 6: Fixed based on Pavlo message 🚀

HamzaIsrar12 commented 2 months ago

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.

volodymyr-chekyrta commented 2 months ago

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. I'll verify the changes, and we will double-check and provide a demo video for issue 2 with explanations. If we miss something, we will do a regression after the Relative Dates feature and add fixes.

PavloNetrebchuk commented 2 months ago

Explanation regarding the synchronisation of Demo Course 7 and Demo Course 8. (Issue 2 & 3)

The normal case, the course with dates Sync course that has not yet started
https://github.com/user-attachments/assets/5783bb8c-60df-4c44-8d26-eec02af64256 https://github.com/user-attachments/assets/5ef15c8a-b645-4908-a03c-f35a5c644096

We apologize for the confusion, we did not point this out earlier. At the moment there is a known issue on the backend with not being able to access course dates on a course that has not yet started. In this case, it will appear in the Synced list but will not actually be synchronized due to an API error. This problem will be solved on the API level later.

openedx-webhooks commented 2 months ago

@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.

marcotuts commented 2 months ago

I created tasks for these API Start Date issues fyi @PavloNetrebchuk , @volodymyr-chekyrta ^

Great to see this merged!