Conversation
Choose between EVER and LL app for event visualization items by version. 2.43+ uses EVER, 2.42 and lower uses LL.
This means: - do not fetch the visualization (the plugin does it) - always pass all filters (the plugin knows what to do with them) - skip code that requires a visualization object (ie. errors)
Similarly to VISUALIZATION items this is to reflect the visualization type via the icon in the list of visualizations. Needed now that EVENT_VISUALIZATION can be both LL and PT.
|
🚀 Deployed on https://pr-3344.dashboard.netlify.dhis2.org |
HendrikThePendric
left a comment
HendrikThePendric
left a comment
There was a problem hiding this comment.
The quality of the code and test coverage is looking great. I left one short comment in the code but it is actually quite a fundamental point. Do we need to concern ourselves with API versions etc? I can imagine that for some older apps/plugins it is relevant to concern ourselves with app-versions and/or backend versions. But for this new app/plugin I was hoping we could just assume all is well if the app was allowed to get installed....
| @@ -1,4 +1,9 @@ | |||
| // Minimum DHIS2 api version that returns PIVOT_TABLE event visualizations and bundles the EVER app | |||
| export const MIN_API_VERSION_FOR_EVER = 43 | |||
There was a problem hiding this comment.
Not sure about this part. I was hoping we could "outsource this part to the app-hub / d2.config file". My thoughts were:
- The EVER app declares its own min version and this prevents it from being installed onto an unsuitable backend
- So here in the dashboard-app we can assume that when the app is found, the backend version must be OK
There was a problem hiding this comment.
🤔 The only exception I just came up with is when neither LL nor EVER is installed and we prompt the user to install an app (or get the sys admin to do so I guess). In this case the backend version is the only way to figure out which app is the best fit for the current instance.
But actually the best you could do is:
- If backend is below the EVER release version, the prompt for LL
- If backend supports EVER, then it's actually possible to install LL or EVER, so the prompt should maybe suggest both
There was a problem hiding this comment.
And if both LL and EVER are installed on an instance we agreed we'd use EVER in the dashboard-app... But still you wouldn't need a version for it....
There was a problem hiding this comment.
The MIN_API_VERSION_FOR_EVER is still needed for the parts where we want to verify the current version to decide what to show, for example in the search results list where we want to show "Event visualizations" instead of "Line lists".
This doesn't depend on the fact that the EVER app is installed, but solely on the backend version that returns PT visualizations (43+).
This actually made me think that the constant's name is confusing and perhaps is what triggered you comment.
I now dropped the constant and use the plain api version directly, since it's also unlikely we need to update the constant with a different version.
About point 2: relying on installed app to choose what to do, we don't do that for LL either.
It's always been based on api version and now is not different.
If we are on a 43+ instance, the list will contain PT visualizations regardless of wether the EVER app is installed.
There is already a mechanism in place that tells the user to install a certain version of an app to open a certain visualization.
Happy to discuss this further.
The constant is confusing because it mentions EVER and seem to indicate you need the EVER app present. The places where the constant is used do not care about the presence of the EVER app, they only check against the api version.
|
3 participants
Implements DHIS2-21962
Description
The new EVER app replaces Line Listing as the editor and dashboard plugin for event visualizations, and it handles both
LINE_LISTandPIVOT_TABLEtypes.The EVER app is only available from v43, which means we need to still use the LL app in supported previous versions.
This is done using version toggles around the code, as setting a minDHIS2Version to 2.43 in the dashboard app means we loose the ability to ship fixes for older versions.
Key differences:
For this to work, many parts needed to access
apiVersion.New specs have been added for parts previously untested.
Notes on related fixes required
dashboards/searchcurrently only returns event visualization of typeLINE_LIST, it should returnPIVOT_TABLEones too in 43+ (see )individual-data-visualizeras app key; this needs to be finalised and changed in EVER'sd2.config.jsvisualizationIdinstead of avisualizationobject, so the EVER plugin props needs to be changed accordinglyQuality checklist
Add N/A to items that are not applicable.
Screenshots
Search dropdown now shows a "Event visualizations" section in place of "Line lists":
LL visualization rendered with the EVER plugin:
