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

RN-484: Add ability for defining 'custom reports' in the report-server #4090

Merged
merged 4 commits into from
Sep 6, 2022

Conversation

rohan-bes
Copy link
Collaborator

@rohan-bes rohan-bes commented Aug 16, 2022

Issue RN-484:

There often come times where we need to rush out a viz, and the demands to get it to work are too tricky for the viz builder at the moment.

The 'Custom Report' is an option to just handcraft a single JS function that pulls and transforms all the data we need. I prefer this option over using the legacy databuilders for a few reason:

  1. We will probably always need some capacity for this, so it's worthwhile to manage it in a non-legacy way
  2. The legacy pattern really blurs the line between what should be config, and what should be code. This however is very clear. Either use the viz-builder transform configs, or just code. I think that makes it simpler to understand and reverse engineer these custom reports when we need to
  3. They're in typescript which makes them a lot easier to work with, and the have access to the @tupaia/api-client

But happy to hear push back from the reviewer if they feel this really muddies the report-server code?

Thoughts on the name 'Custom Reports' btw? It feel a bit weak and non-descriptive to me... HardCodedReport? idk...

- This meets a long time need of a way for viz builders to create ad hoc js functions that pull whatever data they need
- We want these to be uncommonly used, and we should endeavour to consolidate common needs into clean ways of doing things via the viz-builder
- Added test to check we can extract a customReport
@@ -24,20 +18,22 @@ const getPresentation = (dashboardItem: DashboardItem, report: Report | LegacyRe
const { type, name, ...config } = dashboardItemConfig;

const presentation: Record<string, unknown> = { type, ...config };
if (!dashboardItem.legacy) {
if (!dashboardItem.legacy && 'output' in reportConfig) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having typescript in this package was an absolute godsend! It would have been sooo bug prone to try and make such a change without type safety 😄

const { fetch, transform } = config;
const { aggregations, ...restOfFetch } = fetch;
return { fetch: restOfFetch, aggregate: aggregations, transform };
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Refactored this out as it was common between mapOverlay and dashboardItem logic

width: 100%;
height: 100%;
}
`;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am no css craftsperson 😢 You can see the ticket for an example. But given this should be for internal use only I don't mind it if it looks a bit bad

Copy link
Contributor

Choose a reason for hiding this comment

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

Replace CustomReportName to div will do the work, don't need to attach any css.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah thanks @billli0 !

{ filter: { type: 'facility' } },
);
return [{ value: facilities.length }];
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this testCustomReport as an example for future use and a bit of a proof of concept. @chris-bes feel free to remove this once you add your actual implementation 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @rohan-bes

Copy link
Contributor

@biaoli0 biaoli0 left a comment

Choose a reason for hiding this comment

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

Thanks @rohan-bes for adding this custom report function. Looks great!

@rohan-bes rohan-bes enabled auto-merge (squash) September 6, 2022 04:23
@rohan-bes rohan-bes merged commit c9a31cb into dev Sep 6, 2022
@rohan-bes rohan-bes deleted the rn-484-custom-reports-rs branch September 6, 2022 05:10
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.

3 participants