-
Notifications
You must be signed in to change notification settings - Fork 347
Fix last TA module of a lesson type cannot be removed #4279
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 last TA module of a lesson type cannot be removed #4279
Conversation
- Fixed TA modules cannot be deselected if they are the only one or the last one of their type
|
@zehata 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 #4279 +/- ##
==========================================
+ Coverage 54.52% 57.00% +2.47%
==========================================
Files 274 297 +23
Lines 6076 6933 +857
Branches 1455 1674 +219
==========================================
+ Hits 3313 3952 +639
- Misses 2763 2981 +218 ☔ 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.
|
|
@leslieyip02 Btw, while writing this patch, I realized that there is some refactoring that should be done, but it might be better to do it together with changing TimetableContent to a functional component. Specifically, currently I am passing a lot of parameters to the Something I would like to ask for help for is to check if the test cases I wrote for |
leslieyip02
left a comment
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 deployment preview seems to work fine. Should be good once we add a few more test cases.
Regarding the refactor, I'll leave it up to you but I think it's not too urgent right now.
|
@leslieyip02 Thanks! I will work on fixing the regression and test cases so we can push the fix out to users first. The refactor will come later. |
zehata
left a comment
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.
While reading your comment
#4279 (comment) I realized that I really should be writing these test in the conditional style instead, so I basically rewrote the entire test. There will be quite a lot of new things to review, sorry about that.
|
@leslieyip02 I forgot to mention, while writing the tests, I realized we don't have a sample module similar to CG1111A where there are modules with multiple I don't think it's urgent, so I think will add that on once this patch is out. |
leslieyip02
left a comment
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.
Context
Resolves #4280 . As reported by quite a number of users starting at https://t.me/NUSMods/13309 , my implementation in #4225 causes the behavior to be different from the original specification: the last lesson of a lesson type cannot be removed for a TA module.
Implementation
Lessons that are the only one of their lesson type can now be deselected
Lessons that are the last one of their module cannot be deselected
Lessons in non-TA modules are not affected by this change
Other Information