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

Feature: new visualization plugin #15

Merged
merged 4 commits into from
Dec 18, 2024

Conversation

MatiasArriola
Copy link
Contributor

@MatiasArriola MatiasArriola commented Dec 13, 2024

📌 References

📝 Implementation

  • I initially removed all references to the .js plugin, but then realized that event visualizations wouldn't work with its iframe replacement, so I kept both implementations.
  • Now legacy Plugin scripts are loaded only if required
  • Created a separate PluginVisualizationRepository. Each get is performed individually instead of batching all the requests at the repository level.
  • eventCharts and eventReports are unified into eventVisualizations, as per DHIS docs.
  • I have left a placehoder for OrgUnit filtering.

🔥 Testing

  • Use any DHIS 39+ instance with some dashboards loaded to test.
  • Older versions may work, as long as they implement the eventVisualizations endpoint.
  • During testing, I encountered intermittent issues that I still need to investigate and couldn't reproduce again. These may be related to the DHIS instances used.
    • Inside the Map Visualizations, a form appeared instead of the map.
    • iframes stopped reporting installationStatus correctly
    • Timeout errors in the postRobot communication

Keep js plugins as LegacyPlugins
Load Legacy Plugins conditionally
Load Visualizations independently (new repo)
Move eventReport and eventChart to eventVisualization
Check version to use legacy only for dhis < 39
Refactor and small Type changes
Copy link
Contributor

@eperedo eperedo left a comment

Choose a reason for hiding this comment

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

Thanks @MatiasArriola! It looks good, just a few minor changes in the code.

I was able to load almost every visualization but not this dashboard with eventReport (I understand we should use the legacy plugin for this one, right?)

https://play.im.dhis2.org/stable-2-41-2/dhis-web-dashboard/#/GNlS3sXJZFE

image

image

src/domain/repositories/PluginVisualizationRepository.ts Outdated Show resolved Hide resolved
src/domain/entities/Dashboard.ts Outdated Show resolved Hide resolved
src/webapp/hooks/useVisualizationLoader.ts Outdated Show resolved Hide resolved
src/webapp/hooks/useLegacyVisualizationPlugin.ts Outdated Show resolved Hide resolved
@MatiasArriola
Copy link
Contributor Author

Thanks @eperedo !

I've implemented the corrections and I will keep them in mind for future changes.

Regarding that particular dashboard, I believe I missed it because it wasn't there past week, and also most of my testing was done against the 2-40-6 instance.
The Item is not loading because when we make the metadata GET request to build the dashboard, the eventVisualization field is not included in the response in that dashboardItem. As a result, the item is filtered out.
Here is a working example of a dashboard that uses event charts and event reports: https://play.im.dhis2.org/stable-2-41-2/dhis-web-dashboard/#/eZSIccgeq94. For these dashboardItems, the eventVisualization field is returned.

I don't understand why that dashboard doesn't return the eventVisualization field in the item, while others do.
I've stopped using the eventReports entity to align with the docs recommendations.

If needed for legacy support, we could add back the eventChart and eventReport to the metadata GET fields requested.

Copy link
Contributor

@eperedo eperedo left a comment

Choose a reason for hiding this comment

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

Thanks @MatiasArriola, looks good to me.

You're right about the eventVisualization I've tested in the test environment of the client that is working with the app and visualizations include the eventVisualization object. So let's keep your implementation.

@MatiasArriola MatiasArriola changed the base branch from master to feat/ntd-customizations December 18, 2024 14:46
@eperedo eperedo merged commit eacd33f into feat/ntd-customizations Dec 18, 2024
1 check passed
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.

2 participants