-
Notifications
You must be signed in to change notification settings - Fork 346
fix(optimiser): Add default slots to unallocated lessons #4264
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
base: master
Are you sure you want to change the base?
Conversation
|
@thejus03 is attempting to deploy a commit to the modsbot's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4264 +/- ##
==========================================
+ Coverage 54.52% 56.64% +2.11%
==========================================
Files 274 297 +23
Lines 6076 6935 +859
Branches 1455 1674 +219
==========================================
+ Hits 3313 3928 +615
- Misses 2763 3007 +244 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
The code is LGTM, but I'm not convinced that assigning a default slot is necessarily better than doing nothing. I feel that omitting unassigned modules makes it more explicit to the user that they need to tinker a bit with those modules (and/or adjust their preferences).
However, that's just my opinion so maybe a better way is to add an opt-in for this? So some sort of checkbox for "Assign default lessons if needed".
| Warning | Result Timetable |
|---|---|
![]() |
![]() |
What do you think?
|
Sure, I think adding an opt-in would make sense for some people. However, the reason why I would think most would want to have the default slot is because, let’s say, from the above example, CS2100 Lecture was unassigned. Now, if no default slots are added, he needs to go and remove the mod and re-add CS2100. Doing this, however, resets the other slots chosen for the other CS2100 lessons by the optimiser, like tutorial, lab, etc. So I thought adding a default slot was better because now users can just modify the slot for CS2100 Lec without having the need to re-add and lose the other lessons chosen by the optimiser. Hope that makes sense. I am okay with anything. Maybe what we can do is put the default state for the opt-in as 'assign default slot' ? |
|
alternatively, what if we do a flow where we show the warning, then have a button to auto-assign to default slots. i reckon this would not require a new Vercel function call, and allows for both behaviours. if we feel this is too convoluted, i'm more in favor of a warning due to the above reasons @leslieyip02 mentioned |


Context
Implementation