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

Transition data exporting #18062

Merged
merged 7 commits into from Oct 6, 2021
Merged

Transition data exporting #18062

merged 7 commits into from Oct 6, 2021

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Sep 26, 2021

Description:

Fixes #6032

Visualisations and the standard report exporter expect a single datatable structure, so instead of wrapping the transitions in a visualisation, this change adds a new transition exporter directive which provides an export action on both the main transitions map view and on transitions shown by row actions. This export action then calls the API to return the data in the chosen export format.

Review

@bx80 bx80 added the Needs Review PRs that need a code review label Sep 27, 2021
@bx80 bx80 added this to the 4.6.0 milestone Sep 27, 2021
@bx80 bx80 added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Sep 27, 2021
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Generally seems to work.
Btw. did you try reusing the report export we already have?
See

<a class="dataTableAction activateExportSelection" piwik-report-export
report-title="{{ properties.title|e('html_attr') }}" request-params="{{ requestParams|e('html_attr') }}"
api-method="{{ properties.apiMethodToRequestDataTable }}" report-formats="{{ formats|json_encode|e('html_attr') }}"
href='javascript:;' title="{{ 'General_ExportThisReport'|translate|e('html_attr') }}"
max-filter-limit="{{ properties.max_export_filter_limit|e('html_attr') }}"
><span class="icon-export"></span></a>

Guess it would need various adjustments, as it currently depends on a datatable, which we don't have for transitions. Also there are some configuration that would need to be hidden, as they aren't relevant here. But it basically does the same and would also allow to see the export url.

Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

Nice work @bx80 worked nicely. Just left one comment to look at just to be safe re security. Otherwise looks good to merge after addressing that comment 👍 🎉

Copy link
Member

@tsteur tsteur left a comment

Choose a reason for hiding this comment

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

Looks good to merge @bx80 I would merge directly but tests are still running. Be good to double check tests are still passing (ignoring any random/unrelated test failures) and then merge 🎉

@bx80 bx80 merged commit caaefb2 into 4.x-dev Oct 6, 2021
@bx80 bx80 deleted the m-6032-export-transitions branch October 6, 2021 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transitions reports: export full data as XML/JSON
3 participants