teammates
teammates copied to clipboard
TEAMMATES
[#11947] Improve UI of Error Toasts
Fixes #11947
Outline of Solution The back-end:
- Created
ErrorMessageFormatterto 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
Toastinterface to facilitatestringarrays. - Updated the
StatusMessageServiceto clean and parse the back-end error response. - Updated the
Toastcomponent 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.
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)
@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.
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?
@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.
Guys, This PR seems to be stalling (no activities for the past 7 days). :snail: :cry: Hope someone can get it to move forward again soon...
@fsgmhoward I believe that the proposed solution is completed and is pending review. However, I need to examine why the E2E tests are failing
@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.
Guys, This PR seems to be stalling (no activities for the past 7 days). :snail: :cry: Hope someone can get it to move forward again soon...
Guys, This PR seems to be stalling (no activities for the past 8 days). :snail: :cry: Hope someone can get it to move forward again soon...
Guys, This PR seems to be stalling (no activities for the past 7 days). :snail: :cry: Hope someone can get it to move forward again soon...
Guys, This PR seems to be stalling (no activities for the past 7 days). :snail: :cry: Hope someone can get it to move forward again soon...
@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 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.
Guys, This PR seems to be stalling (no activities for the past 7 days). :snail: :cry: Hope someone can get it to move forward again soon...
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.
@zhaojj2209 Thank you for the verifying this! I will set up GH actions on the fork and tryout the workflow you mentioned.
Guys, This PR seems to be stalling (no activities for the past 8 days). :snail: :cry: Hope someone can get it to move forward again soon...
Guys, This PR seems to be stalling (no activities for the past 14 days). :snail: :cry: Hope someone can get it to move forward again soon...
Guys, This PR seems to be stalling (no activities for the past 16 days). :snail: :cry: Hope someone can get it to move forward again soon...
Guys, This PR seems to be stalling (no activities for the past 19 days). :snail: :cry: Hope someone can get it to move forward again soon...
@thikhinab Any updates on this PR? Let me know if you need help debugging the E2E tests.
Guys, This PR seems to be stalling (no activities for the past 23 days). :snail: :cry: Hope someone can get it to move forward again soon...
@zhaojj2209 It would be great if I could get your assistance. The PR created for testing: https://github.com/thikhinab/teammates/pull/4 After removing the CSS styling, E2E tests have passed several times
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.