-
Notifications
You must be signed in to change notification settings - Fork 319
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
Fix for Mobile Google Calendar Odd/Even Week Imports #3684
base: master
Are you sure you want to change the base?
Conversation
Added interval checking for numeric week calculations as Google Calendar imports on mobile do not handle exclusions well. Also fixed additional tutorials for odd/even weeks being added after Week 11/13.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@onx001 is attempting to deploy a commit to a Personal Account owned by @nusmodifications on Vercel. @nusmodifications first needs to authorize it. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3684 +/- ##
==========================================
+ Coverage 53.62% 53.71% +0.09%
==========================================
Files 272 272
Lines 5979 5991 +12
Branches 1428 1433 +5
==========================================
+ Hits 3206 3218 +12
Misses 2773 2773 ☔ View full report in Codecov by Sentry. |
website/package.json
Outdated
@@ -96,7 +96,7 @@ | |||
"icalendar": "0.7.1", | |||
"identity-obj-proxy": "3.0.0", | |||
"jest": "29.6.2", | |||
"jest-environment-jsdom": "^29.5.0", | |||
"jest-environment-jsdom": "^29.7.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I'll get to reviewing and testing the code by this Wednesday. In the meantime, can I check if bumping the dependencies here (and creating a package.json for the monorepo) is required for the change to work? Otherwise, could you help remove those diffs (package.json
, website/package.json
, and website/src/utils/yarn.lock
from this PR? Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, done! The dependencies were not required to be updated. I have not worked with yarn before so was a little unfamiliar with the process. Thank you for your help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hold on, my bad -- it looks like this new PR is creating events beyond the end date of the semester. @onx001 if you look export from this PR's deployment (https://nusmods-website-zrekiv0gl-mods-bot.vercel.app/timetable/sem-1) does the calendar work for you?
Hi, just tested out this PR as well, and there seems to be another bug: events on odd weeks only show up before recess week, while events on event weeks only show up after recess week. Here's the timetable I used and I've also attached the exact .ics file I used. |
Added interval checking for numeric week calculations as Google Calendar imports on mobile do not handle exclusions well.
Also fixed additional tutorials for odd/even weeks being added after Week 11/13.
Context
#3679
.ics iCal exports have weird handling issues with even/odd weeks, namely extra lessons after the intended one and a lack of proper exclusion handling for mobile Google Calendar users.
Implementation
A simple interval calculator is implemented for the calculateNumericWeek function, and the count and interval fields are passed to the ics generator where previously they were missing and filled through exclusions.
Other Information
Current handling of edge case intervals that do not follow the regular pattern or that skip intervals might still have the bug for extra lessons. Needs further investigation.