Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: [FC-0047] Calendar synchronization #466

Conversation

IvanStepanok
Copy link
Contributor

@IvanStepanok IvanStepanok commented Jun 18, 2024

Quick Preview

calendar_axim.mp4

Dark theme

Screenshot 2024-06-18 at 14 07 11

Sync status on Course dates page

Screenshot 2024-06-18 at 14 42 27

⚠️ Pay attention. The feature "Use relative dates" will be implemented in the next round.

Design:
https://www.figma.com/design/iZ56YMjbRMShCCDxqrqRrR/Open-edX-Mobile-App-All-Screens-v2.1?node-id=0-1&t=V0qsuJDvWwg9aeBd-0

@openedx-webhooks
Copy link

openedx-webhooks commented Jun 18, 2024

Thanks for the pull request, @IvanStepanok!

What's next?

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

🔘 Get product approval

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

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

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

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

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

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

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

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

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 18, 2024
@marcotuts
Copy link

This looks great the videos are very helpful!

I did notice that after syncing it looks like Hide Inactive Courses was true / some courses were inactive but the setting toggle is off for this - Is that a toggle that will be update to work i na future PR?

@IvanStepanok
Copy link
Contributor Author

Hi @marcotuts
Sorry if it wasn't clear, I took a screen recording of this feature🙌

Screen.Recording.2024-06-19.at.11.47.48.mov

@volodymyr-chekyrta volodymyr-chekyrta marked this pull request as ready for review June 19, 2024 09:59
@IvanStepanok IvanStepanok force-pushed the feat/Setup-Calendar-Synchronization-logic branch from 68c0f4d to bd46905 Compare June 19, 2024 14:37
@miankhalid
Copy link

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

@shafqat-muneer shafqat-muneer self-requested a review June 20, 2024 10:15
@volodymyr-chekyrta volodymyr-chekyrta changed the title feat: calendar synchronization flow feat: [FC-0047] Calendar synchronization Jun 21, 2024
@shafqat-muneer
Copy link
Contributor

I am unable to sync the calendar due to receiving a 404 response from the enrollments_status API. It appears that this API has not been deployed in the production environment.
/api/mobile/v1/users/\(username)/enrollments_status/

It's blocker to review functionality.

Simulator.Screen.Recording.-.iPhone.15.-.2024-07-03.at.13.47.54.mp4

@GlugovGrGlib
Copy link
Member

Hi @shafqat-muneer, thank you so much for starting this review. The mentioned backend functionality was approved, but not yet merged into edx-platform master.
Therefore, to not block review of the mobile implementation, please use the sandbox with all the latest features implemented for FC-0047, similarly to how it was done for Dashboard Level Navigation PR.

API

Since the new APIs are not available in the master branch, please use the sandbox:

API_HOST_URL: 'https://axim-mobile.raccoongang.net'
OAUTH_CLIENT_ID: 'zP3vPz00c8fTRpYjNbVSlA1fxt9LnCxTM4JK1KQ0'

I hope this will unblock the further review process, thanks!

@shafqat-muneer
Copy link
Contributor

I am starting review today 🙌

@shafqat-muneer
Copy link
Contributor

shafqat-muneer commented Jul 9, 2024

Issue 1:

When the 'Calendar Access' popup appears, tapping outside the alert area causes the screen overlay to disappear, but the alert itself remains on the screen. Ideally, the alert should also disappear when tapping anywhere outside of it.

Simulator.Screen.Recording.-.iPhone.15.-.2024-07-09.at.17.14.55.mp4

@shafqat-muneer
Copy link
Contributor

I am unable to test the 'Hide Inactive Courses' toggle feature because there are no inactive courses available. I have enrolled in all available courses on the sandbox: https://axim-mobile.raccoongang.net. Is there a way to test this feature? Do we have any test user with inactive courses available?

@shafqat-muneer
Copy link
Contributor

shafqat-muneer commented Jul 9, 2024

Issue 2:

In the Figma designs, there is an option to use relative dates, but I do not see this feature in the current implementation.

Screenshot 2024-07-09 at 7 04 44 PM

Simulator Screenshot - iPhone 15 - 2024-07-09 at 19 06 55

@shafqat-muneer
Copy link
Contributor

shafqat-muneer commented Jul 9, 2024

Issue 3:

If the calendar is unsynced, the following issues are observed when the user accesses the course:

  • There is no indication that the user's calendar is outdated.
  • The Dates tab incorrectly shows that the calendar is synced, even though it is not.

To unsync the calendar, please follow these steps:

  • Go to the Dates tab of any course.
  • Mark any calendar event date.
  • Open your native calendar and find that specific event.
  • Delete the event from the native calendar.
  • Open the application.
  • Navigate to that specific course.
  • At this point, the course calendar will be outdated.
  • You will observe the said issues.

@shafqat-muneer
Copy link
Contributor

The review is currently ongoing, and I will continue with it tomorrow.

@IvanStepanok
Copy link
Contributor Author

IvanStepanok commented Jul 10, 2024

Hi, @shafqat-muneer. Happy to see you!

When the 'Calendar Access' popup appears, tapping outside the alert area causes the screen overlay to disappear, but the alert itself remains on the screen. Ideally, the alert should also disappear when tapping anywhere outside of it.

Done

In the Figma designs, there is an option to use relative dates, but I do not see this feature in the current implementation

This feature will be implemented in the next MR. It's out of current scope but it is ready.

I am unable to test the 'Hide Inactive Courses' toggle feature because there are no inactive courses available. I have enrolled in all available courses on the sandbox: https://axim-mobile.raccoongang.net. Is there a way to test this feature? Do we have any test user with inactive courses available?

You can use my credentials: sexclipt@gmail.com Qwerty123456!

If the calendar is unsynced, the following issues are observed when the user accesses the course.

Our team thought about this behavior for a long time. In my opinion, based on UX/UI principles, the behavior must be predictable and we need to use the user control principle. So in case where user deletes an event, it can't be a mistake. The way to deleting an event takes a few screens. It is not about random swipe and deleting. It is a sequence of specific user actions. In this case, we can be sure, it is not a mistake. And if the user decides to delete some events, who we are to ignore their desires?

Additionally, key UX principles support this approach:

Predictability: Good interaction design is predictable, meaning users should know what will happen when they perform an action. If a user deletes an event, the expected outcome is its removal, fostering confidence and control​ (Give Good UX)​.

User Control: UX principles emphasize giving users control over their interactions. By allowing the event to stay deleted, we respect their autonomy and decision-making​ (UX Institute)​​ (CareerFoundry)​.

Consistency and Logic: Ensuring the interface is logical and consistent helps users predict outcomes and feel more secure in their interactions. Ignoring a user’s deletion action could lead to confusion and frustration​ (Userpilot)​.

These principles ensure a positive user experience by making interactions clear, predictable, and user-controlled. Ignoring a user’s deliberate action undermines these core UX principles.

@shafqat-muneer
Copy link
Contributor

Our team thought about this behavior for a long time. In my opinion, based on UX/UI principles, the behavior must be predictable and we need to use the user control principle. So in case where user deletes an event, it can't be a mistake. The way to deleting an event takes a few screens. It is not about random swipe and deleting. It is a sequence of specific user actions. In this case, we can be sure, it is not a mistake. And if the user decides to delete some events, who we are to ignore their desires?

Thanks for all the design details. I completely agree with them. I created the scenario of an outdated calendar locally to demonstrate the use case. This situation can also occur if a teacher updates the course calendar from backend, potentially with important updates. The course calendar could become outdated without the user knowing and user can miss important updates. It will only sync when the user navigates to the Dates & Calendar section. Ideally auto sync should be there in this case but if we decide against implementing auto-sync functionality upon landing on the course, we should at least provide some indication to the user that the course dates are not synced. Perhaps we can define state to show outdated calendar course as we are showing synced, not synced, or offline states? 🤔

@shafqat-muneer
Copy link
Contributor

Issue 4:

On dates tab, by tapping on calendar cell/section, user should redirect to Dates & Calendar screen. Redirection behavior is implemented on android side.

@shafqat-muneer
Copy link
Contributor

Issue 5:

I removed calendar access from Settings and then navigated to the 'Dates & Calendar' section. Despite the console displaying NSLocalizedDescription=Access denied, the application erroneously indicates that the calendar synced successfully.

Calendar.Access.Video.mp4

@shafqat-muneer
Copy link
Contributor

Issue 6:

Initially, unsynced courses appear under the Not Synced tab. However, after marking them, they move to the Synced tab and will be synced the next time the screen is loaded. Ideally, these courses should sync instantly. If that’s not possible, there should be an indication that these courses will be synced in the next iteration. For instance, we could add the text (in queue) in front of each course name. The designer may have additional input, but there should be a clear distinction between courses that are synced and those that are queued for the next sync. Thoughts?

Simulator.Screen.Recording.-.iPhone.15.-.2024-07-11.at.15.58.43.mp4

@shafqat-muneer
Copy link
Contributor

Issue 7:

If no content is available, an empty screen message should ideally added.

Simulator Screenshot - iPhone 15 - 2024-07-11 at 14 40 34

@shafqat-muneer
Copy link
Contributor

Issue 8:

It would be helpful to display a toast message to the user indicating the success or failure of the calendar sync. Currently, no toast messages are being shown.

@shafqat-muneer
Copy link
Contributor

Issue 9:

On iPhone SE 3rd generation, sync status section hides under the tabs and header.

Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2024-07-11.at.16.13.50.mp4

@shafqat-muneer
Copy link
Contributor

Issue 10:

If the application is deleted and reinstalled, logging in with the same user results in duplicate events in the calendar. Is it possible to remove the duplicates or use the previously added events? Thoughts?

Simulator Screenshot - iPhone 15 - 2024-07-11 at 16 21 29

@IvanStepanok
Copy link
Contributor Author

@shafqat-muneer Thanks for the review!

Perhaps we can define state to show outdated calendar course as we are showing synced, not synced, or offline states? 🤔
In MainScreenViewModel we have a updateCalendarIfNeeded() function for handling relevance of the calendar.

Issue 4: – Done ✅
Issue 5: – Done ✅
Issue 6: – Make sence. The word "Synced" sounds like a completed action. For a quick solution, I suggest renaming the button to "To Sync". In the long term perspective, we need to conduct research on user feedback and maybe ask the designer to make a redesign of this screen. But in my opinion, renaming the button to "To Sync" is enough. @sdaitzman any thoughts?🙌
Issue 7, 8:@sdaitzman we need your deep expertise 🙏
Issue 9: – Already fixed by Vadim Kuznetsov ✅
Issue 10: – Great point💪 Fixed✅

@sdaitzman
Copy link

sdaitzman commented Jul 16, 2024

Hi @shafqat-muneer, thank you for flagging these issues for design review

Issue 6
On the Synced/Not Synced tabs, when tapping a checkbox next to a course to begin syncing it, it’s important to show that sync progress is taking place, and then that it has completed. I think we can accomplish this by using platform-provided animated loaders to show syncing will be briefly taking place, then showing a checked box for several seconds, then animating the row out. Here’s a quick prototype flow of this microinteraction (Figma link):

Screen Recording 2024-07-16 at 4 11 28 PM

Issue 7
Thanks for raising this! I added an empty state closely based on the course/course dashboard empty states (Figma link)

image

Issue 8
I think the updates under issue 6 above should resolve the usability issue without using toasts, but will follow up about this at the next mobile design weekly call for community input.

Let me know if this all makes sense, or if I can clarify/provide additional screens. Thanks!

@shafqat-muneer
Copy link
Contributor

Thank you @sdaitzman for the design feedback. Here are my further thoughts on it:

Issue 6
Based on the current implementation, when a user taps on any course from the 'Not Synced' tab, it immediately moves to the 'Synced' tab without initiating actual calendar synchronization. The synchronization for that course will only occur when the user revisits this screen at any time. If the user remains on this screen, the course will appear under the 'Synced' tab, but in reality, it has not been synchronized yet. We need to show any indication in Synced tab. Because in reality, at this point, Synced tab can have synced courses and flagged for sync courses, which will sync next revisit of the screen. I hope this further clarifies my point.

On the Synced/Not Synced tabs, when tapping a checkbox next to a course to begin syncing it, it’s important to show that sync progress is taking place, and then that it has completed. I think we can accomplish this by using platform-provided animated loaders to show syncing will be briefly taking place, then showing a checked box for several seconds, then animating the row out. Here’s a quick prototype flow of this microinteraction (Figma link):

Issue 7
LGTM 👍

Thanks for raising this! I added an empty state closely based on the course/course dashboard empty states (Figma link)

Issue 8
When the user arrives on the Dates & Calendar Sync screen and the courses calendar sync begins, I am referring to displaying a toast message after the sync process is complete (if it makes sense).

I think the updates under issue 6 above should resolve the usability issue without using toasts, but will follow up about this at the next mobile design weekly call for community input.

@IvanStepanok
Copy link
Contributor Author

@shafqat-muneer,
The UI changes have been applied, and all feedback has been addressed.

Regarding Issue 3, it appears to be a miscommunication.
According to the new behavior, at every app start, we compare the date checksums from the server and the local version, replacing all outdated events. Meanwhile, we ignore all user interactions in the system calendar, considering them intentional.
Simulator Screenshot - iPhone 15 - 2024-07-17 at 12 08 30

I hope all these changes make you happier!🙏

@shafqat-muneer
Copy link
Contributor

I really appreciate @IvanStepanok for addressing the feedback. Due to the priority of our MVP release, I may not be able to verify the changes myself. @volodymyr-chekyrta, could you please verify the addressed changes so that this PR can be considered for merging?

@volodymyr-chekyrta
Copy link
Contributor

I really appreciate @IvanStepanok for addressing the feedback. Due to the priority of our MVP release, I may not be able to verify the changes myself. @volodymyr-chekyrta, could you please verify the addressed changes so that this PR can be considered for merging?

@shafqat-muneer, sure I understand the situation with the release.
I'll double-check everything, and @IvanStepanok will prepare a demo video with fixes.
If we miss something, we will do a regression after the Relative Dates feature and add fixes.

@IvanStepanok
Copy link
Contributor Author

@volodymyr-chekyrta

Demo Videos for Issue Fixes

Issue 1 and 10

issue1_10.1.mp4

Description:

This video demonstrates the fix for the calendar access popup issue. Now, tapping outside the alert area will correctly dismiss the alert. Additionally, it shows the resolution of duplicate calendar events after reinstallation and logging in with the same user.

Issue 4

issue_4.1.mp4

Description:

This video showcases the fix for the redirection behavior on the dates tab. Tapping on the calendar cell/section now correctly redirects the user to the "Dates & Calendar" screen, matching the behavior implemented on the Android side.

Issue 5

issue_5.1.mp4

Description:

This video demonstrates the fix for the incorrect calendar sync status. After removing calendar access from settings, navigating to the "Dates & Calendar" section now correctly indicates access denied instead of falsely indicating a successful sync.

Issue 6, 7, and 8

issue_6_7_8.1.mp4

Description:

The video shows the corrected behavior for unsynced courses. Courses marked for sync now show a clear indication that they will be synced in the next iteration.
Issue 7: It includes the fix for displaying an empty screen message when no content is available.
Issue 8: The video demonstrates the implementation of toast messages to indicate the success or failure of calendar syncs.

Issue 9

issue_9.1.mp4

Description:

This video demonstrates the fix for the sync status section being hidden under tabs and headers on the iPhone SE 3rd generation. The sync status section is now correctly visible and accessible.

Copy link
Contributor

@volodymyr-chekyrta volodymyr-chekyrta left a comment

Choose a reason for hiding this comment

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

LGTM

@volodymyr-chekyrta volodymyr-chekyrta merged commit 9c18b30 into openedx:develop Jul 18, 2024
3 checks passed
@openedx-webhooks
Copy link

@IvanStepanok 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

@volodymyr-chekyrta volodymyr-chekyrta deleted the feat/Setup-Calendar-Synchronization-logic branch July 18, 2024 07:25
@mphilbrick211 mphilbrick211 added the FC Relates to an Axim Funded Contribution project label Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FC Relates to an Axim Funded Contribution project open-source-contribution PR author is not from Axim or 2U
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

9 participants