Skip to content
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

[#12571] Instructors Edit Feedback Session: Instructor is able to edit submission opening time to an earlier timing #12580

Merged
merged 5 commits into from Sep 24, 2023

Conversation

dlimyy
Copy link
Contributor

@dlimyy dlimyy commented Sep 6, 2023

Fixes #12571

Outline of Solution

I created a new method called triggerSubmissionOpeningDateModelChange which is triggered when the instructor chooses a date in the date picker for the submission opening time. When the instructor changes the date to the earliest possible date, the submissionStartTime of the model will change accordingly to the earliest possible time for that date. Since the hours that the instructor are all in 0 minutes(with the exception of 23:59), I created another function configureSubmissionOpeningTime to configure the time properly.

I have also fixed the bug where the saved button is disabled when there is an error message and the fields are uneditable(This bug should not be able to be replicated here since I fixed the above bug but this fix is a safeguard against future bugs)

@dlimyy
Copy link
Contributor Author

dlimyy commented Sep 10, 2023

I have fixed the bug regarding the case where the save button is disabled when the user passes in an invalid submission opening time. As seen below, the fields are now editable when there is an error.

fix-disabled-button-submission-opening-time.mp4

@dlimyy dlimyy marked this pull request as ready for review September 10, 2023 14:41
@dlimyy
Copy link
Contributor Author

dlimyy commented Sep 10, 2023

This PR is also now ready for review

@cedricongjh cedricongjh self-requested a review September 12, 2023 14:38
@cedricongjh cedricongjh added the s.ToReview The PR is waiting for review(s) label Sep 12, 2023
Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR! (And great job finding this edge case)

Left a few comments that I think should be addressed.

Also, I've discovered that when switching to the min date, the time always defaults to the min time, even when the time selected is valid (e.g. 13th Sept, 2359 is selected, but switching to 12th Sept (the minDate) causes the time to change)

Do fix the issues and re-request a review!

@dlimyy
Copy link
Contributor Author

dlimyy commented Sep 14, 2023

Hi @cedricongjh, thanks for reviewing my PR and pointing out the bugs in my code! I have fixed the changes pointed out and added unit tests for all the methods requested. In addition, I have also solved the problem where switching the date to min date will always cause the time to revert to min time.

Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides that one nit, LGTM!

@dlimyy
Copy link
Contributor Author

dlimyy commented Sep 22, 2023

Hi @cedricongjh, I have changed it to triggerModelChange. Ready for review!

Copy link
Contributor

@cedricongjh cedricongjh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you for your contribution!

@cedricongjh cedricongjh added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Sep 23, 2023
Copy link
Contributor

@jasonqiu212 jasonqiu212 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for spotting this edge case and fixing it :)

@jasonqiu212 jasonqiu212 merged commit fe6a397 into TEAMMATES:master Sep 24, 2023
9 checks passed
@wkurniawan07 wkurniawan07 added c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging and removed s.FinalReview The PR is ready for final review labels Jan 21, 2024
@wkurniawan07 wkurniawan07 added this to the V8.30.0 milestone Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Instructors Edit Feedback Session: Instructor is able to edit submission opening time to an earlier timing
4 participants