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

feat: use highchart offline export for downloads #3135

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

HendrikThePendric
Copy link
Contributor

@HendrikThePendric HendrikThePendric commented Jul 9, 2024

Implements DHIS2-17722 and also fixes DHIS2-12103.

Requires dhis2/analytics#1688


Key features

  1. Use HighCharts offline export method where possible
  2. Add the correct font bundle for the user's locale, this allows exporting visualizations with non-ASCII fonts

Description

Switching from server-side to client-side file exporting itself didn't require much work, but I then found that specific languages needed specific Noto font bundles. The majority of the work went into researching which bundles are needed for which scripts/languages and discussing the of "selecting the correct font". Some of these discussions are still ongoing, and may actually impact my implementation.


TODO

  • (Cypress) tests
  • Manual testing
  • Release new analytics version
  • Discuss adding script section to DB locale string with team-platform backend devs

Known issues

  • Single Value visualisations are not made with HighCharts and they still rely on the server-side SVG to PDF conversion endpoint, which does not support non-ASCII scripts.
  • The reports-app also allows users to download PDF reports. This app also still relies on the old backend conversion endpoint.
  • The current solution should support all known implementations, but there are also configurations possible which would not be supported. Specifically if we encounter a language that can be written in 2 or more non-ASCII scripts that both need to be supported. The current frontend solution would actually be able to deal with this correctly, if the locale string contained a script section. However it is currently not possible to add a script to a DB locale (see last TODO).

Screenshots

Screenshot 2024-07-16 at 15 16 14

@dhis2-bot
Copy link
Contributor

dhis2-bot commented Jul 9, 2024

@dhis2-bot dhis2-bot temporarily deployed to netlify July 9, 2024 08:44 Inactive
src/components/HighchartsChartProvider.js Outdated Show resolved Hide resolved
src/components/Visualization/Visualization.js Outdated Show resolved Hide resolved
src/components/VisualizationPlugin/ChartPlugin.js Outdated Show resolved Hide resolved
@dhis2-bot dhis2-bot temporarily deployed to netlify July 9, 2024 10:18 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 9, 2024 13:25 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 11, 2024 08:56 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 15, 2024 13:59 Inactive
@HendrikThePendric HendrikThePendric force-pushed the feat/use-highcharts-clientside-downloads-DHIS2-17722 branch from 3822611 to 41e6cf4 Compare July 16, 2024 09:06
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 09:09 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 09:15 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 09:54 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 10:26 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 10:31 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 11:04 Inactive
@dhis2-bot dhis2-bot temporarily deployed to netlify July 16, 2024 12:07 Inactive
@HendrikThePendric HendrikThePendric changed the title feat: add HighchartsChartProvider feat: use highchart offline export for downloads Jul 16, 2024
@HendrikThePendric
Copy link
Contributor Author

I have had a meeting with team-platform and we discussed the problem from two angles:

The general inability of specifying a script in a DB locale string

While considering the problem from this angle it became apparent that how we deal with locales in general needs some structural decisions. Is having multiple scripts per locale a use-case we want to support on a single instance, since it introduces a level of complexity? Do we need to keep distinguishing between UI and DB locale and that also introduces a level ambiguity? Etc, etc....

So this turned out this is a very broad topic which needs to be discussed and investigated thoroughly, and as such it will be a long time until this is resolved.

At the same the fact that our DB locales do not support script sections is also a practical problem in the field that the Uzbek instance is working around using bit of a hack.... They now use uz and uz_UZ and uses one for Latin and the other for Cyrillic DB translations. This is obviously not right, and this type of misuse of locale strings is problematic for the locale-based solution in this PR. The fact that Uzbek is only Latn (supported by standard PDF fonts) and Cyrl (supported by the base Noto font) is a happy accident which causes the current solution to actually function correctly for Uzbek.

The inability to generate PDFs correctly for non-ASCII scripts

Viet brought it to my attention that the sever-side PDF generation endpoint is not just being used by the data-visualizer-app, but also by the reports-app. I already knew that the client-side solution in this PR only partially solved the problem because the single-value visualisations would still rely on the server side PDF generation endpoint, but now I realise that the problem is more widespread.

Consequences

The points above makes me think that a fix to the server side endpoint would be more suitable than the client-side solution I have implemented in this PR:

  • A server side fix would be generic, no app-specific fixes need, no exceptions in what does or doesn't support non-ASCII characters when exported to PDF.
  • A combined Noto font exists with support for all modern scripts. We cannot use it for a client-side solution because it weighs 14MB. If we can use this font bundle on the server, then we could just use that for all locale strings.

I have some outstanding questions with Viet about the server side solution, but if what I mention about the combined Noto font is possible, then I would be in favour of closing this PR completely in favour of a server side fix.

@dhis2-bot dhis2-bot temporarily deployed to netlify July 31, 2024 08:48 Inactive
@HendrikThePendric HendrikThePendric force-pushed the feat/use-highcharts-clientside-downloads-DHIS2-17722 branch from abea112 to 0b434bf Compare August 21, 2024 14:01
@dhis2-bot dhis2-bot temporarily deployed to netlify August 21, 2024 14:09 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants