[#11947] Improve UI of Error Toasts #11948

thikhinab · 2022-08-24T13:16:56Z

Fixes #11947

Outline of Solution
The back-end:

Created ErrorMessageFormatter to format list of error messages that could be parsed from the front-end.
Updated three classes (CreateFeedbackQuestionAction, SubmitFeedbackResponseAction, UpdateFeedbackResponseAction) where the Lists of messages are used to create error responses.

The front-end:

Updated the Toast interface to facilitate string arrays.
Updated the StatusMessageService to clean and parse the back-end error response.
Updated the Toast component to render multiple error messages as bullet points.

UI

Shortcomings & Alternatives
Future developers must be aware to use ErrorMessageFormatter instead of calling List::toString(). An alternative is to ensure that there is a class such as ErrorMessageList embedded at points where error messages outputted. This would ensure that all error messages will follow the desired format. However this alternative will require to refactor the entire validation process and exception handling.

damithc · 2022-08-24T13:34:49Z

I'm OK with the proposed UI. 👍

samuelfangjw · 2022-08-30T16:21:54Z

Future developers must be aware to use ErrorMessageFormatter instead of calling List::toString()

If I may ask, what is the behaviour if this is not followed? (Using List::toString() instead)

thikhinab · 2022-08-30T16:41:20Z

@samuelfangjw List::toString will output the error message as follows: [ error_msg1, error_msg2, ... ] since the delimiter is a , it makes it harder to parse in the front end (as error messages itself can contain the delimiter.). Moreover, the the back-end is currently inconsistent on whether it sends the error with brackets [...] or without. Instances where there is only a single error message, simply the string passed as param to the Error thrown.

A solution to deal with it (where the developer is not responsible for usingErrorMessageFormatter) would need to refactor the back-end such that Error messages are abstracted out into a different class as suggested under alternatives.

samuelfangjw · 2022-08-30T17:41:05Z

I'm not comfortable with the proposed solution at the moment.

I'll preface this by saying that you are right when you say that the current error messages could be improved. The backend should be providing reasonable error messages, i.e. not array::toString().

However, the backend really should not be specifically formatting error messages for a specific consumer, the frontend in this case. Think about it this way, there shouldn't be a way for an error message sent on the backend to break/affect the formatting on the frontend, however improbable. If we do it this way, the developers working on the backend also have to be conscious of how any changes will affect the frontend as well. I'm not saying this cannot be done or there's no examples of strong coupling in the current codebase, I'm saying this is something we should try to avoid unless absolutely necessary.

There's also an issue of consistency. The fact that the format of the error message in the response and the way it should be parsed differs depending on the endpoint doesn't sit well with me. In my opinion, this is a short sighted workaround to this issue and the tech debt from this will cause more issues in the future.

The alternative suggested is definitely a cleaner solution, and one that I have been considering for awhile now. However, I'm not keen on rushing this out at the moment. Any solution that affects error handling has to be well thought out and designed in a way that is consistent yet extensible enough to handle all the different use cases in our application.

A simple compromise I would like to explore: We can try formatting the error messages by joining the array using newlines instead. This is more reasonable than the current toString implementation and should display nicely enough on the frontend within the toast. It's not as pretty as a bulleted list, but i suspect it might be enough for our use case.

Thoughts?

thikhinab · 2022-08-31T17:36:20Z

@samuelfangjw I understand your concerns. I agree with you. My intentions were to avoid large scale refactor of the Error Messaging in the back-end, which should preferably be solved under a different issue since it is more general and have direct impact on multiple points of the back-end. Thank you for pointing this out. I am interested in working on you idea for the compromise. I think it is an elegant way to solve the issue without major refactoring. Furthermore, this would also solve the issue of inconsistency on the manner which the front-end deals with error messages.

thikhinab · 2022-08-31T19:15:37Z

Updated UI:

nusoss-bot · 2022-09-08T14:42:52Z

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

fsgmhoward · 2022-09-12T05:34:27Z

@thikhinab Any updates for this PR?

thikhinab · 2022-09-12T05:37:39Z

@fsgmhoward I believe that the proposed solution is completed and is pending review. However, I need to examine why the E2E tests are failing

fsgmhoward · 2022-09-12T05:49:40Z

@thikhinab It can fail randomly for many reasons. You can check whether it is due to your modification. I have updated this PR as to be reviewed.

nusoss-bot · 2022-09-19T14:52:44Z

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

nusoss-bot · 2022-09-27T15:32:41Z

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

nusoss-bot · 2022-10-05T15:09:43Z

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

nusoss-bot · 2022-10-13T14:56:00Z

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

thikhinab · 2022-10-17T18:02:47Z

I will try to take a look at this again this week

thikhinab · 2022-10-25T12:01:09Z

@fsgmhoward I verified that toast with toast-body class is being displayed, however the tests E2E test still fail. Any advice on how to proceed? However, the tests needs to updated to verify those that get printed in multiple lines.
@samuelfangjw Is there any other suggestions/improvements that I need to make.

fsgmhoward · 2022-10-25T14:53:18Z

@thikhinab Could you run a local check and see if it fails or not?

thikhinab · 2022-10-25T15:39:02Z

@fsgmhoward It does not run locally. To check if it is isolated to my working branch, I tried running the tests in master branch, but it still fails locally. Under the developer tools for Google Chrome, it is visible that the toast appears with the toast-body class.

thikhinab · 2022-10-29T17:49:32Z

@fsgmhoward I tried running again locally, this time the test worked:

nusoss-bot · 2022-11-06T15:03:44Z

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

zhaojj2209 · 2022-11-08T10:01:39Z

E2E tests are passing on my local machine as well, which makes this difficult to debug. One way to debug this without spamming the PR itself would be to enable GH actions on your own forked repo, duplicating this branch, and making a PR to your own repo's master branch. Once the PR is up, you can make edits to your branch (e.g. removing changes incrementally to see which change caused the E2E tests to fail) and wait for the E2E test results on GH actions to see if they pass.

Maybe you can start by removing the change to toast.component.scss and checking if the E2E tests pass on GH actions after removing the change.

thikhinab · 2022-11-08T10:24:33Z

@zhaojj2209 Thank you for the verifying this! I will set up GH actions on the fork and tryout the workflow you mentioned.

nusoss-bot · 2022-11-16T14:05:14Z

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

nusoss-bot · 2022-11-30T15:14:26Z

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

nusoss-bot · 2022-12-02T17:11:07Z

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

nusoss-bot · 2022-12-05T14:52:33Z

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

zhaojj2209 · 2022-12-06T09:23:53Z

@thikhinab Any updates on this PR? Let me know if you need help debugging the E2E tests.

nusoss-bot · 2022-12-09T16:12:59Z

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

thikhinab · 2022-12-09T18:05:51Z

@zhaojj2209 It would be great if I could get your assistance.
The PR created for testing: thikhinab#4
After removing the CSS styling, E2E tests have passed several times

zhaojj2209 · 2022-12-13T13:39:33Z

After removing the CSS styling, E2E tests have passed several times

Then I guess my suspicions were correct; it seems that white-space: pre-line causes the E2E tests to fail for reasons that are beyond me. Can you try changing the CSS to white-space: pre-wrap instead? If pre-wrap also causes E2E tests to fail, then I think removing the white-space property can also be an acceptable compromise.

nusoss-bot · 2022-12-20T16:33:38Z

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

thikhinab · 2022-12-20T19:20:10Z

@zhaojj2209 Understood, will update.

nusoss-bot · 2022-12-23T15:56:46Z

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

thikhinab · 2022-12-28T19:06:15Z

Since PR failing E2E tests even with the new changes, I would close the issue until the issue is resolved or alternative is found.

nusoss-bot · 2023-01-05T14:00:02Z

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

nusoss-bot · 2023-01-11T15:31:50Z

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

Update error toast

73cb9b3

Concatenate error messages with newline

e2d76aa

hhdqirui added the s.Ongoing The PR is being worked on by the author(s) label Sep 10, 2022

hhdqirui requested a review from samuelfangjw September 10, 2022 15:02

fsgmhoward added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Sep 12, 2022

Merge branch 'master' into 11947-update-error-toast

4262f4d

Merge branch 'master' into 11947-update-error-toast

af6819a

Merge branch 'master' into 11947-update-error-toast

50e5dc3

Merge branch 'master' into 11947-update-error-toast

bbe73a3

Merge branch 'master' into 11947-update-error-toast

34d5e77

Change css

c29f1bd

Merge branch 'master' into 11947-update-error-toast

aa43f53

thikhinab closed this Dec 28, 2022

domoberzin mentioned this pull request Jul 12, 2023

Improve UI of Error Toasts #11947

Closed

github-actions bot mentioned this pull request Jul 12, 2023

Issues Digest damithc/foo#117

Open

[#11947] Improve UI of Error Toasts #11948

[#11947] Improve UI of Error Toasts #11948

Conversation

thikhinab commented Aug 24, 2022

damithc commented Aug 24, 2022

samuelfangjw commented Aug 30, 2022

thikhinab commented Aug 30, 2022

samuelfangjw commented Aug 30, 2022

thikhinab commented Aug 31, 2022

thikhinab commented Aug 31, 2022

nusoss-bot commented Sep 8, 2022

fsgmhoward commented Sep 12, 2022

thikhinab commented Sep 12, 2022

fsgmhoward commented Sep 12, 2022

nusoss-bot commented Sep 19, 2022

nusoss-bot commented Sep 27, 2022

nusoss-bot commented Oct 5, 2022

nusoss-bot commented Oct 13, 2022

thikhinab commented Oct 17, 2022

thikhinab commented Oct 25, 2022

fsgmhoward commented Oct 25, 2022

thikhinab commented Oct 25, 2022

thikhinab commented Oct 29, 2022

nusoss-bot commented Nov 6, 2022

zhaojj2209 commented Nov 8, 2022

thikhinab commented Nov 8, 2022

nusoss-bot commented Nov 16, 2022

nusoss-bot commented Nov 30, 2022

nusoss-bot commented Dec 2, 2022

nusoss-bot commented Dec 5, 2022

zhaojj2209 commented Dec 6, 2022

nusoss-bot commented Dec 9, 2022

thikhinab commented Dec 9, 2022 • edited Loading

zhaojj2209 commented Dec 13, 2022 • edited Loading

nusoss-bot commented Dec 20, 2022

thikhinab commented Dec 20, 2022

nusoss-bot commented Dec 23, 2022

thikhinab commented Dec 28, 2022

nusoss-bot commented Jan 5, 2023

nusoss-bot commented Jan 11, 2023

thikhinab commented Dec 9, 2022 •

edited

Loading

zhaojj2209 commented Dec 13, 2022 •

edited

Loading