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

chore: move analytics error handling in the plugin component #3335

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

edoardo
Copy link
Member

@edoardo edoardo commented Feb 5, 2025

Key features

  1. Move analytics errors handling in the plugin component
  2. Implement new custom error screen of org unit access error (E7120)

Description

DV has a number of custom error screens where a user friendly error is shown to the user.
The errorCode returned in the analytics response is used to choose the error screen to show.

This error handling was done in the DV app components before, an onError callback was passed to the plugin component and used to pass the error back to the app.

Dashboard app also used the onError callback, but it didn't have all the custom error screen so a generic error was shown instead where the original error string from the analytics api was shown.
This error string is often obscure for the end user, and can also contain ids and other information intended more for admins or developers for debugging purposes.

When an error occured in a plugin, and the visualization was opened in the DV app, a different error was shown.

By moving the error handling in the plugin component, the same custom error screens are used in both the DV and dashboard apps.


TODO

  • Cypress tests
  • Manual testing

Screenshots

Before, no custom screen for E7120 in DV (already used in LL):
Screenshot 2025-02-05 at 15 52 36

After, new custom error for E7120 (same text as in LL):
Screenshot 2025-02-05 at 15 52 53

Same error screen used in dashboard app:
Screenshot 2025-02-05 at 15 52 59

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Feb 5, 2025

🚀 Deployed on https://pr-3335.data-visualizer.netlify.dhis2.org

@dhis2-bot dhis2-bot temporarily deployed to netlify February 5, 2025 14:58 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify February 6, 2025 09:43 Inactive
DIMENSION_ID_DATA
]?.every(
(dim) =>
response.metaData.items[dim]?.valueType ===
Copy link
Collaborator

Choose a reason for hiding this comment

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

On line 227 you check defined for both response and metadata. Any need to do that here as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was existing code moved from the Visualization component.
231 makes sure there is dim in the items object before reading valueType, it seems better to leave this check.
response and metaData are not checked again here.

@dhis2-bot dhis2-bot temporarily deployed to netlify February 6, 2025 10:13 Inactive
Now the VisualizationErrorInfo component is used also outside the
StartScreen component.
This happens when a layout error is thrown before the plugin is
rendered.
Without this, the error is shown briefly before the VisualizationPlugin
is rendered and might crash due to other uncaught errors.
@dhis2-bot dhis2-bot temporarily deployed to netlify February 6, 2025 13:35 Inactive
Copy link
Contributor

@HendrikThePendric HendrikThePendric left a comment

Choose a reason for hiding this comment

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

I think this is an excellent idea and have added a comment to perhaps improve it even further.

src/components/Visualization/Visualization.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants