fix(optimiser): Add default slots to unallocated lessons#4264
fix(optimiser): Add default slots to unallocated lessons#4264leslieyip02 merged 15 commits intonusmodifications:masterfrom
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.82% +2.30%
==========================================
Files 274 298 +24
Lines 6076 6949 +873
Branches 1455 1681 +226
==========================================
+ Hits 3313 3949 +636
- Misses 2763 3000 +237 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
thejus03
changed the title
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
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 |
| - Reduces search space by merging slots of the same lesson type happening at the same day and time and building. | ||
| */ | ||
| func GetAllModuleSlots(optimiserRequest models.OptimiserRequest) (map[string]map[string]map[string][]models.ModuleSlot, error) { | ||
| func GetAllModuleSlots(optimiserRequest models.OptimiserRequest) (map[string]map[string]map[string][]models.ModuleSlot, map[string]map[string][]models.ModuleSlot, error) { |
There was a problem hiding this comment.
Could we can add some type aliases for this? It's getting a bit hard to read.
Maybe something like
type ModuleSlotsMap = map[string]map[string][]models.ModuleSlot
…se new types for better clarity
3 participants
Context
Implementation