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.

Sign up for GitHub

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

Jump to bottom

[#11236] Uninformative error message for incorrectly typed date input #11524

Merged
merged 16 commits into from
Apr 17, 2022
Merged

[#11236] Uninformative error message for incorrectly typed date input #11524

merged 16 commits into from
Apr 17, 2022

Conversation

ziqing26
Copy link
Contributor

@ziqing26 ziqing26 commented Jan 15, 2022 •
edited
Loading

Fixes #11236
FIxes #11556

Outline of Solution
Add readonly attribute to datepicker in audit log page.

Update (12 Apr 2022):
This branch is now rebased from the branch in #11562 to reuse code related to e2e. After that, new commits fix failing e2e tests for:

  • admin notification page (a newly affected page)
  • instructor session edit page,
  • audit log page

@teammates-bot
Copy link

Hi @ziqing26, these parts of your pull request do not appear to follow our contributing guidelines:

  1. PR Title

@ziqing26 ziqing26 changed the title [#11236]Uninformative error message for incorrectly typed date input [#11236] Uninformative error message for incorrectly typed date input Jan 15, 2022
@madanalogy madanalogy added the s.ToReview The PR is waiting for review(s) label Jan 17, 2022
jianhandev
jianhandev approved these changes Jan 17, 2022
View reviewed changes
Copy link
Contributor

@jianhandev jianhandev left a comment

Choose a reason for hiding this comment

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

LGTM!
Screenshot 2022-01-17 at 8 59 49 PM
Screenshot 2022-01-17 at 9 00 26 PM

@madanalogy madanalogy added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Jan 18, 2022
@ziqing26 ziqing26 force-pushed the 11236-fix-uninformative-error-message branch from 1b9dd15 to f6f4ec0 Compare January 18, 2022 13:23
@jianhandev jianhandev mentioned this pull request Jan 22, 2022
Closed
halfwhole
halfwhole reviewed Jan 23, 2022
View reviewed changes
Copy link
Contributor

@halfwhole halfwhole left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This backend approach certainly works (at least for the feedback session pages, probably not for audit logs or other pages?), but I'm wondering if there might be a way to deal with the errors in the frontend instead?

If the date is invalid, the resolveLocalDateTime method returns NaN instead of a proper number, which appears to cause the issue. This NaN is then passed to the backend as a null or some other invalid value, and so the error is eventually thrown by the backend.

My feeling though is that it might be better to raise an error and deal with it purely on the frontend, rather than pass it on to the backend -- plus these error messages (should be more than zero/should not be null) might strictly speaking be more appropriate for backend developers too

@ziqing26
Copy link
Contributor Author

This backend approach certainly works (at least for the feedback session pages, probably not for audit logs or other pages?), but I'm wondering if there might be a way to deal with the errors in the frontend instead?

If the date is invalid, the resolveLocalDateTime method returns NaN instead of a proper number, which appears to cause the issue. This NaN is then passed to the backend as a null or some other invalid value, and so the error is eventually thrown by the backend.

My feeling though is that it might be better to raise an error and deal with it purely on the frontend, rather than pass it on to the backend -- plus these error messages (should be more than zero/should not be null) might strictly speaking be more appropriate for backend developers too

Okay I can look into that!

@madanalogy madanalogy added s.Ongoing The PR is being worked on by the author(s) and removed s.FinalReview The PR is ready for final review labels Jan 26, 2022
@ziqing26 ziqing26 force-pushed the 11236-fix-uninformative-error-message branch 3 times, most recently from 7aa853f to b494cf0 Compare January 30, 2022 11:31
@ziqing26 ziqing26 force-pushed the 11236-fix-uninformative-error-message branch 3 times, most recently from aac346e to fe96717 Compare February 11, 2022 13:47
@nusoss-bot
Copy link

Guys, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

1 similar comment
@nusoss-bot
Copy link

Guys, This PR seems to be stalling (no activities for the past 7 days). 🐌 😢
Hope someone can get it to move forward again soon...

@hhdqirui
Copy link
Member

Hi @ziqing26, kind reminder on this issue.

@ziqing26
Copy link
Contributor Author

Hi @ziqing26, kind reminder on this issue.

Thank you! Will push my amendments soon. From the conversation of #11562, might need to look into E2E tests as well.

@ziqing26 ziqing26 force-pushed the 11236-fix-uninformative-error-message branch 2 times, most recently from 954e6f0 to ed0beb7 Compare March 14, 2022 04:46
@ziqing26 ziqing26 force-pushed the 11236-fix-uninformative-error-message branch from ee2160a to 5617838 Compare April 17, 2022 12:03
@ziqing26
Copy link
Contributor Author

@halfwhole The latest commit updates one test case for the selective deadline which is affected. Thank you again!

halfwhole
halfwhole approved these changes Apr 17, 2022
View reviewed changes
Copy link
Contributor

@halfwhole halfwhole left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks a lot for this!

FergusMok and others added 16 commits April 17, 2022 21:52
@FergusMok @ziqing26
Add readonly attribute to datepicker
d6949de
@FergusMok @ziqing26
Change datepicker to be transparent when disabled
7dbeb53
@FergusMok @ziqing26
Fix disabled datetimepicker to match color
c3fb625
@FergusMok @ziqing26
Fix lint
8fbd7c4
@FergusMok @ziqing26
Test push
48385e9
@FergusMok @ziqing26
Add date time picker E2E
04cf4e6
@ziqing26
Fix uninformative timestamp error message
fdb763d
@ziqing26
Add frontend error handling for invalid time ranges
a7f93dd 
If the date is invalid, the `resolveLocalDateTime` method returns NaN instead of a proper number.
This NaN is then passed to the backend as an invalid value,
and so the error is eventually thrown by the backend with unclear error messages.

Let's
* raise the error in frontend for invalid date range before it is passed
  to backend.
* handle the error using try-catch blocks in frontend as well.
@ziqing26
Refactor invalid time range error handling
05c66b5 
Let's
* Keep only the relevant time range code in try catch loop.
* Remove instance checks of error.
@ziqing26
Remove try catch loop
609e693
@ziqing26
Add readonly to instructor audit logs page
dd83c25
@ziqing26
Fix datepicker for admin notif, instructor edit and audit log
81275a3
@ziqing26
Refactor ids for datetimepicker and update snapshot
601dd4d
@ziqing26
Use title instead of aria-label for fillDatePicker
3cdea09
@ziqing26
Use relative date instead of absolute date
f678f89
@ziqing26
Change datepicker e2e for selective deadline
8c74d25
@ziqing26 ziqing26 added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Apr 17, 2022
@ziqing26 ziqing26 requested a review from FergusMok April 17, 2022 14:00
@ziqing26 ziqing26 force-pushed the 11236-fix-uninformative-error-message branch from 5617838 to 8c74d25 Compare April 17, 2022 14:02
@FergusMok FergusMok mentioned this pull request Apr 17, 2022
Closed
FergusMok
FergusMok approved these changes Apr 17, 2022
View reviewed changes
Copy link
Contributor

@FergusMok FergusMok left a comment

Choose a reason for hiding this comment

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

LGTM 💯 Thanks for helping to close both issues!

@ziqing26 ziqing26 added the c.Bug Bug/defect report label Apr 17, 2022
@wkurniawan07 wkurniawan07 added 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 Apr 17, 2022
@wkurniawan07 wkurniawan07 added this to the V8.14.0 milestone Apr 17, 2022
@wkurniawan07 wkurniawan07 merged commit e4b5850 into TEAMMATES:master Apr 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t-cheepeng t-cheepeng t-cheepeng left review comments

@jianhandev jianhandev jianhandev approved these changes

@halfwhole halfwhole halfwhole approved these changes

@FergusMok FergusMok FergusMok approved these changes

Assignees
No one assigned
Labels
c.Bug Bug/defect report s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Milestone
V8.14.0
10 participants
@ziqing26 @teammates-bot @nusoss-bot @hhdqirui @FergusMok @t-cheepeng @halfwhole @jianhandev @wkurniawan07 @madanalogy