feat(analytics): increase number of records exported for csv [MA-4298] #2453
Conversation
jordojordo
force-pushed
the
feat/ma-4298
branch
from
00d5c31 to
9e12935
Compare
| "exportAsCsv": "Export as CSV", | ||
| "exportTimeRange": "Time range", | ||
| "exportDescription": "Exports a CSV of the data represented in the chart.", | ||
| "maximumRecordAmount": "The amount of rows is limited to {value}", |
There was a problem hiding this comment.
| "maximumRecordAmount": "The amount of rows is limited to {value}", | |
| "maximumRecordAmount": "The number of groups is limited to {value}", |
| <CsvExportModal | ||
| v-if="exportModalVisible" | ||
| :chart-data="(chartData as ExploreResultV4)" | ||
| :chart-data="(exportExploreResult || (chartData as ExploreResultV4))" |
There was a problem hiding this comment.
I think the data model should be a bit more comprehensive:
- I don't think we should fall back to
chartData-- if the export request fails, the modal should reflect that. I think it's better to behave consistently rather than sometimes give different results if there's an error. - Since it's making a new request, the modal should have a way of displaying an error if there is one.
You might get along better with Typescript if you make a union type for loading | success | error and pass that as one property, rather than having separate loading / error /data props. Happy to discuss further.
There was a problem hiding this comment.
That's fair, I'll work on some changes to reflect this.
| } | ||
| // Build a larger dataset for CSV export | ||
| const requestExport = async (limit: number = EXPORT_RECORD_LIMIT): Promise<ExploreResultV4 | undefined> => { |
There was a problem hiding this comment.
Since this is code that's very similar to what's needed in ui-apps, I think this should be refactored out to some common place -- maybe a composable in analytics-chart; this would allow it to call onUnmounted and to inject the query bridge.
There was a problem hiding this comment.
Thanks for the feedback, I've refactored the export functionality into a utility/composable within the analytics-chart package. I'll add info of the changes to the main PR description.
jordojordo
force-pushed
the
feat/ma-4298
branch
2 times, most recently
from
c1b6189 to
55b43e7
Compare
jordojordo
force-pushed
the
feat/ma-4298
branch
3 times, most recently
from
e587ed8 to
ba75953
Compare
jordojordo
force-pushed
the
feat/ma-4298
branch
from
ba75953 to
d6b67fa
Compare
packages/analytics/analytics-chart/src/components/CsvExportModal.vue
Outdated
Show resolved
Hide resolved
| </div> | ||
| </template> | ||
|
|
||
| <script setup lang="ts"> |
There was a problem hiding this comment.
Cleaned up the imports in this file as they were all over the place.
- Update CSV export modal so it just emits `closeModal` rather than `toggleModal` with a parameter that's always false. - Remove requestExport composable because it's no longer code in common with ui-apps. - Add new composable, just for use in the dashboard renderer, that knows how to merge context and issue a query. - Allow DashboardTile to issue queries to export a CSV.
Summary
Fix MA-4298
This PR will increase the maximum number of records returned by the CSV export functionality from 50 to 250 records.
Main changes:
Technical changes:
useAnalyticsRequestExportinanalytics-utilitiesfor shared export functionalityCsvExportStatetype to handle loading, success, and error statesCsvExportModalnow acceptsexportStateinstead of directchartData