-
Notifications
You must be signed in to change notification settings - Fork 0
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 Bug That Prevented Editing OH Time #818
base: master
Are you sure you want to change the base?
Conversation
[diff-counting] Significant lines: 5. |
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 editing for an existing office hour looks like it's working good! I don't seem to be able to directly add a weekly recurring office hour as a professor (doesn't show up) so I can't actually test editing weekly recurring hybrid office hours though.
Some additional things I saw while testing:
- In a professor role, when this button is checked for a non-weekly recurring office hours the deletion doesn't seem to work. It works without the checkmark however, and I'm not sure if this has the same behavior for weekly recurring office hours.
- If you try to create an in person office hour that's weekly recurring without a location, you are prompted to put one. If you try to do this for a not weekly recurring office hour, it lets you. Not sure if that was intended or not?
@NIDHI2023 It was good that you noticed these other inconsistencies/issues with editing office hours, thanks for doing such thorough testing. To provide a bit of context, those are other things we plan on addressing on dev (sometimes not even a problem on prod) in the future, but unrelated to this PR. The recurring OH deletion especially is a bug we've tried to figure out for a while but haven't found a solution yet. |
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 providing a simple fix to this bug and providing great detail for the rationale behind it! This will definitely help us solve a pretty pressing issue for 2110, so good work helping us get this out in time for deployment later this week.
Summary
Test Plan
Notes
Breaking Changes
None