nusmodifications / nusmods

🏫 Official course planning platform for National University of Singapore.
https://nusmods.com
MIT License
558 stars 270 forks source link

Add well-formed timetable validation check #3700

Closed leslieyip02 closed 2 months ago

leslieyip02 commented 3 months ago

Context

Resolves #3673

Implementation

Added logic to check if a given timetable is well-formed:

Other Information

One thing I wasn't sure about is this part in export.ts where the URL is hard coded:

https://github.com/nusmodifications/nusmods/blob/7334b074fc4fee253592418b28528784129c0f3d/website/src/apis/export.ts#L12

For local testing, I changed this to a local host manually, but I think this should be put into an environment variable?

Also, let me know if I should add any additional checks. I'm new to this so let me know if there's anything I missed!

vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nusmods-export ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2024 8:37am
nusmods-website ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 15, 2024 8:37am
vercel[bot] commented 3 months ago

@leslieyip02 is attempting to deploy a commit to a Personal Account owned by @nusmodifications on Vercel.

@nusmodifications first needs to authorize it.

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 53.59%. Comparing base (2f723c6) to head (9e06b93). Report is 7 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3700 +/- ## ========================================== - Coverage 53.61% 53.59% -0.02% ========================================== Files 272 272 Lines 5974 5974 Branches 1426 1426 ========================================== - Hits 3203 3202 -1 - Misses 2771 2772 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kokrui commented 2 months ago

Hey @leslieyip02, thanks for the PR -- it's cool to see that you managed to identify the relevant code to look for timetable / exam clashes and stuff. However, I think I provided too little detail in my original issue about what "well-formed" entails -- I meant that we should write a basic check that the implicit conversion of data to PageData worked (and something like the fact that timetable is indeed a map from moduleCode to ModuleLessonConfig).

I'm not super sure about whether we should block the exporting of a timetable of there are lesson/exam clashes right now, especially since there isn't an option to customise timetable slots yet. We could discuss this in a new issue though (maybe a warning when exporting might be useful? idk)

leslieyip02 commented 2 months ago

Oh I see. I think I will create a new PR in that case.