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

[Fix] Improve Error Toast Display Content #5345

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

willie-hung
Copy link
Contributor

@willie-hung willie-hung commented Oct 20, 2023

Description

  • Remove the "See the full error" button, as error logs should be hidden from users in the frontend.
  • Update the error message shown in the error toast.

Issues Resolved

closes #5326

Screenshot

Before

Error Toast

Screenshot 2023-10-20 at 2 17 55 PM

Error Modal

Screenshot 2023-10-20 at 8 18 23 PM

After

Screenshot 2023-10-20 at 2 23 15 PM

Testing the changes

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@codecov
Copy link

codecov bot commented Oct 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (3c0211b) 66.82% compared to head (7925cbc) 66.97%.
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5345      +/-   ##
==========================================
+ Coverage   66.82%   66.97%   +0.15%     
==========================================
  Files        3291     3291              
  Lines       63174    63223      +49     
  Branches    10055    10052       -3     
==========================================
+ Hits        42213    42342     +129     
+ Misses      18479    18443      -36     
+ Partials     2482     2438      -44     
Flag Coverage Δ
Linux_1 35.26% <0.00%> (+0.01%) ⬆️
Linux_2 55.16% <100.00%> (-0.13%) ⬇️
Linux_3 43.84% <50.00%> (+0.03%) ⬆️
Linux_4 35.37% <0.00%> (+0.03%) ⬆️
Windows_1 35.28% <0.00%> (+0.02%) ⬆️
Windows_2 55.13% <100.00%> (-0.13%) ⬇️
Windows_3 43.85% <50.00%> (+0.03%) ⬆️
Windows_4 35.37% <0.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@willie-hung willie-hung changed the title [Fix] Remove search error button [Fix] Improve Error Toast Display Content Oct 20, 2023
Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
Signed-off-by: Willie Hung <[email protected]>
Comment on lines -84 to -86
const fullMessage = [message, expression, repeat('-', error.location.start.offset) + '^'].join(
'\n'
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my view, this line is intended to facilitate easier debugging. It is typically output to the console or saved in a log file and should not be displayed on the frontend. Any ideas for improvement would be appreciated!

@joshuarrrr
Copy link
Member

@Willie-The-Lord Can you add a Changelog entry under the bug section?

willie-hung and others added 2 commits October 30, 2023 15:15
Signed-off-by: Willie Hung <[email protected]>
@bandinib-amzn
Copy link
Member

Looks like this change is modifying toast across the entire app (not just Visualization or one plugin). I think we need to evaluate few things as this change affects entire Dashboards.

  1. Did we do research on history why See the Full error button has been added in toast? What are the use cases?
  2. I agree that it is not right way to see the error but in some extent it helps to debug the issue. If we are removing it, we need to make sure we have enough logs which can help to debug.

Also I would like to hear from @opensearch-project/opensearch-ux how they feel about this change?

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Willie Hung <[email protected]>
@manasvinibs
Copy link
Member

Hi @wooonka, Did you get a chance to give a thought to @bandinib-amzn comment here? Also, please resolve the merge conflict caused by changelog file.

@@ -75,6 +75,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [BUG][Fuctional Test] Make setDefaultAbsoluteRange more robust and update doc views tests ([#5242](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5242))
- [BUG] Add platform "darwin-arm64" to unit test ([#5290](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5290))
- [BUG][Dev Tool] Add dev tool documentation link to dev tool's help menu [#5166](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5166)
- [BUG] Improve the display content of the error toasts ([#5345](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5345))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an enhancement; isn't it? Also, we don't need to add [BUG] for bug fixes because they are listed in a section meant for bug fixes.

@ashwin-pc
Copy link
Member

I dont think that hiding the "see the full error" button is a good idea :/ As a user im always frustrated when the software tries to hide information from me. We already know that many of OSD's users are technically competent. The See the full error button is a nice way to surface this information without unnecessarily polluting the toast notification

@kgcreative
Copy link
Member

I would be open to making "See the full error" a little less prominent, but hiding it all together is not a great experience.

@kavilla
Copy link
Member

kavilla commented Apr 2, 2024

@bandinib-amzn @kgcreative what is the status on this?

@ananzh
Copy link
Member

ananzh commented Oct 30, 2024

Convert to draft due to no progress.

@ananzh ananzh marked this pull request as draft October 30, 2024 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Improper Error Handling
9 participants