Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

Use new API to Generate Reports from Existing Definitions #213

Conversation

davidcui1225
Copy link
Contributor

Issue #, if available:
N/A
Description of changes:
Refactored to call new API for generating reports from existing definitions- instead of passing in the entire metadata of creating a report, we only have to pass in the reportDefinitionId now.

Usages:

  • Creating a report after an on-demand report definition is created
  • The Generate report button on the Report definition details page for an on-demand definition.
  • The download icon under File format within Report definition details.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Comment on lines 467 to 469
reportDefinitionRawResponse.report_definition.report_params.core_params
.time_duration;
const fromDate = getRelativeStartDate(duration);
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need all these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, good catch. Will remove

Copy link
Member

Choose a reason for hiding this comment

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

Can you verify if it's the same situation for logic in create_report_defintion.ts? I always have an impression that lots of code can be removed with this new API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean create_report_definition.tsx? If so, yes. From the code change in the PR, the only parameter that is created is a reportDefinitionId constant based on the response from the Create API call

Copy link
Member

@zhongnansu zhongnansu left a comment

Choose a reason for hiding this comment

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

LGTM, merge after github action test and build is passed

@davidcui1225 davidcui1225 merged commit 0bbab87 into opendistro-for-elasticsearch:dev Nov 24, 2020
zhongnansu pushed a commit to zhongnansu/kibana-reports that referenced this pull request Dec 4, 2020
zhongnansu pushed a commit to zhongnansu/kibana-reports that referenced this pull request Dec 7, 2020
zhongnansu pushed a commit to zhongnansu/kibana-reports that referenced this pull request Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants