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

Remove references to ReportingCore from Export Type code #158092

Closed
tsullivan opened this issue May 18, 2023 · 2 comments
Closed

Remove references to ReportingCore from Export Type code #158092

tsullivan opened this issue May 18, 2023 · 2 comments
Assignees
Labels
Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)

Comments

@tsullivan
Copy link
Member

tsullivan commented May 18, 2023

Code in x-pack/plugins/reporting/server/export_types is planned to be removed from the reporting plugin and relocated into a new standalone plugin.

A large challenge with this task is to remove references to the ReportingCore object in that code. In the new destination plugin for the export types, there will not be the ReportingCore.

Affected code

# low-hanging fruit
x-pack/plugins/reporting/server/export_types/common/get_custom_logo.ts:  const fakeRequest = reporting.getFakeRequest(headers, spaceId, logger);
x-pack/plugins/reporting/server/export_types/common/get_custom_logo.ts:  const uiSettingsClient = await reporting.getUiSettingsClient(fakeRequest, logger);
x-pack/plugins/reporting/server/export_types/common/get_full_urls.ts:  const serverInfo = reporting.getServerInfo();
x-pack/plugins/reporting/server/export_types/common/get_full_urls.ts:  } = reporting.getConfig();
x-pack/plugins/reporting/server/export_types/common/v2/get_full_redirect_app_url.ts:  } = reporting.getConfig();
x-pack/plugins/reporting/server/export_types/common/v2/get_full_redirect_app_url.ts:  const serverInfo = reporting.getServerInfo();
x-pack/plugins/reporting/server/export_types/common/v2/get_full_redirect_app_url.test.ts:    reporting.getConfig = jest.fn(() => ({ kibanaServer: {} } as unknown as ReportingConfigType));
x-pack/plugins/reporting/server/export_types/common/v2/get_full_redirect_app_url.test.ts:    reporting.getServerInfo = jest.fn(() => ({
x-pack/plugins/reporting/server/export_types/printable_pdf/lib/generate_pdf.ts:  return reporting.getScreenshots(options).pipe(
x-pack/plugins/reporting/server/export_types/printable_pdf_v2/lib/generate_pdf.ts:  const screenshots$ = reporting.getScreenshots({ ...options, urls }).pipe(

# execute_job and create_job code will use a common approach: dependencies will be set into a class instance. See below
x-pack/plugins/reporting/server/export_types/csv_v2/create_job.ts:    const { discover: discoverPluginStart } = await reporting.getPluginStartDeps();
x-pack/plugins/reporting/server/export_types/png_v2/execute_job.ts:    const { encryptionKey } = reporting.getConfig();
x-pack/plugins/reporting/server/export_types/csv_searchsource/execute_job.ts:  const { encryptionKey, csv: csvConfig } = reporting.getConfig();
x-pack/plugins/reporting/server/export_types/csv_searchsource/execute_job.ts:    const fakeRequest = reporting.getFakeRequest(headers, job.spaceId, logger);
x-pack/plugins/reporting/server/export_types/csv_searchsource/execute_job.ts:    const uiSettings = await reporting.getUiSettingsClient(fakeRequest, logger);
x-pack/plugins/reporting/server/export_types/csv_searchsource/execute_job.ts:    const dataPluginStart = await reporting.getDataService();
x-pack/plugins/reporting/server/export_types/csv_searchsource/execute_job.ts:      (await reporting.getEsClient()).asScoped(fakeRequest),
x-pack/plugins/reporting/server/export_types/printable_pdf_v2/execute_job.ts:    const { encryptionKey } = reporting.getConfig();
x-pack/plugins/reporting/server/export_types/csv_v2/execute_job.ts:  const config = reporting.getConfig();
x-pack/plugins/reporting/server/export_types/csv_v2/execute_job.ts:    const fakeRequest = reporting.getFakeRequest(headers, job.spaceId, logger);
x-pack/plugins/reporting/server/export_types/csv_v2/execute_job.ts:    const uiSettings = await reporting.getUiSettingsClient(fakeRequest, logger);
x-pack/plugins/reporting/server/export_types/csv_v2/execute_job.ts:      await reporting.getPluginStartDeps();
x-pack/plugins/reporting/server/export_types/csv_v2/execute_job.ts:      (await reporting.getEsClient()).asScoped(fakeRequest),

# not being moved (need to remove this code asap)
x-pack/plugins/reporting/server/export_types/csv_searchsource_immediate/execute_job.ts:  const { csv: csvConfig } = reporting.getConfig();
x-pack/plugins/reporting/server/export_types/csv_searchsource_immediate/execute_job.ts:    const dataPluginStart = await reporting.getDataService();
x-pack/plugins/reporting/server/export_types/csv_searchsource_immediate/execute_job.ts:    const uiSettings = await reporting.getUiSettingsServiceFactory(savedObjectsClient);
x-pack/plugins/reporting/server/export_types/csv_searchsource_immediate/execute_job.ts:      (await reporting.getEsClient()).asScoped(req),
x-pack/plugins/reporting/server/export_types/png/execute_job/index.ts:    const { encryptionKey } = reporting.getConfig();
x-pack/plugins/reporting/server/export_types/printable_pdf/execute_job/index.ts:    const { encryptionKey } = reporting.getConfig();

Colorized version:
Image

A common approach for removing ReportingCore references from execute_job and create_job functions.

Reporting export types should be redesigned. Today, they are plain objects with stateless functions. They get
state from having the ReportingCore object passed in. By redesigning them as class instances, they can be
constructed with a "fresh" state that isn't dependent on ReportingCore. They can also have lifecycle methods to
receive stateful dependencies from the setup and start phases.

  1. Reporting's export types will not get registered internally in the getExportTypesRegistry function.
  2. Each export type will be created in phases:
    // x-pack/plugins/reporting/server/plugin.ts (export types have not yet been removed from the plugin)
    export class ReportingPlugin
      implements Plugin<ReportingSetup, ReportingStart, ReportingSetupDeps, ReportingStartDeps>
    {
      private logger: Logger;
      private reportingCore?: ReportingCore;
      private pdfExport: PdfExportType; // new
    
      constructor(private initContext: PluginInitializerContext<ReportingConfigType>) {
        this.logger = initContext.logger.get();
        const config = // ;
        this.pdfExport = new PdfExportType(config, this.logger); // new
        this.pngExport = new PngExportType(config, this.logger); // new
        this.csvExport = new CsvExportType(config, this.logger); // new
      }
    
      public setup(core: CoreSetup, plugins: ReportingSetupDeps) {
        const { http, status } = core;
    
        registerExportType(this.pdfExport); // pulled out of server/lib/export_types_registry.ts
        this.pdfExport.setup(core, plugins); // new
        this.pngExport.setup(core, plugins); // new
        this.csvExport.setup(core, plugins); // new
    
        const reportingCore = new ReportingCore(core, this.logger, this.initContext); // no longer given to any export type
        this.reportingCore = reportingCore;
    
        // .. skipped lines
    
        return reportingCore.getContract();
      }
    
      public start(core: CoreStart, plugins: ReportingStartDeps) {
        // ... skipped lines
    
        this.pdfExport.start(core, plugins); // new
        this.pngExport.start(core, plugins); // new
        this.csvExport.start(core, plugins); // new
    
        // ... skipped lines
    
        return reportingCore.getContract();
      }
    
      stop() {
        this.reportingCore?.pluginStop();
      }
    }
  3. Defining export types without ReportingCore: the implementation of an export type definition will change from the plain object into a stateful instance of a
    class:
    class PdfExportType {
      constructor(private config: ReportingConfigType, logger: Logger) {
        this.logger = logger.get('pdf-export');
      }
    
      setup(coreSetup, setupDeps) {
        this.coreSetup = coreSetup;
        this.setupDeps = setupDeps;
      }
    
      start(coreStart, startDeps) {
        this.coreStart = coreStart;
        this.startDeps = startDeps;
      }
    
      createJob(jobParams: JobParamsPdf) {
        // create the payload that gets stored in .kibana-reporting-*
        return {
          ...jobParams,
          forceNow: new Date().toISOString(),
        };
      }
    
      async runTask(jobId, job, cancellationToken, stream) {
        // use the dependencies to execute the job and return the content through the stream
        const encryptionKey = this.config.encryptionKey;
        const headers = await decryptJobHeaders(encryptionKey, job.headers, this.logger)
    
        return this.startDeps.screenshotting.getScreenshots(job, job.locatorParams, { headers })....
      }
    }
@botelastic botelastic bot added the needs-team Issues missing a team label label May 18, 2023
@petrklapka petrklapka added the Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience) label May 18, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/appex-sharedux (Team:SharedUX)

@botelastic botelastic bot removed the needs-team Issues missing a team label label May 18, 2023
rshen91 added a commit that referenced this issue May 22, 2023
## Summary

Part of #158092

Removing calls to `ReportingCore` in export types functions

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
delanni pushed a commit to delanni/kibana that referenced this issue May 25, 2023
## Summary

Part of elastic#158092

Removing calls to `ReportingCore` in export types functions

### Checklist
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
rshen91 added a commit that referenced this issue Jun 27, 2023
## Summary

This partially addresses #158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit that referenced this issue Jun 28, 2023
## Summary

This partially addresses #158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit to rshen91/kibana that referenced this issue Jun 30, 2023
## Summary

This partially addresses elastic#158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit that referenced this issue Jul 5, 2023
## Summary

This partially addresses #158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit to rshen91/kibana that referenced this issue Jul 5, 2023
This partially addresses elastic#158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --fix'

update imports
rshen91 added a commit that referenced this issue Jul 6, 2023
## Summary

This partially addresses #158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit that referenced this issue Jul 6, 2023
## Summary

This is a part of #158092

This PR encompasses both csv_v2 and csv_searchsource as the
CsvExportType and CsvSearchsourceExportType. Based on the reporting
plugin readme, CsvSearchsourceExportType is currently used but
CsvV2ExportType is refactored for when it can reach feature parity to
csv searchsource.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Timothy Sullivan <[email protected]>
rshen91 added a commit that referenced this issue Jul 10, 2023
## Summary

This partially addresses #158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit that referenced this issue Jul 10, 2023
## Summary

This is a part of #158092

This PR encompasses both csv_v2 and csv_searchsource as the
CsvExportType and CsvSearchsourceExportType. Based on the reporting
plugin readme, CsvSearchsourceExportType is currently used but
CsvV2ExportType is refactored for when it can reach feature parity to
csv searchsource.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Timothy Sullivan <[email protected]>
rshen91 added a commit that referenced this issue Jul 11, 2023
## Summary

This partially addresses #158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit that referenced this issue Jul 11, 2023
## Summary

This is a part of #158092

This PR encompasses both csv_v2 and csv_searchsource as the
CsvExportType and CsvSearchsourceExportType. Based on the reporting
plugin readme, CsvSearchsourceExportType is currently used but
CsvV2ExportType is refactored for when it can reach feature parity to
csv searchsource.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Timothy Sullivan <[email protected]>
rshen91 added a commit that referenced this issue Jul 12, 2023
## Summary

This partially addresses #158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit that referenced this issue Jul 12, 2023
## Summary

This is a part of #158092

This PR encompasses both csv_v2 and csv_searchsource as the
CsvExportType and CsvSearchsourceExportType. Based on the reporting
plugin readme, CsvSearchsourceExportType is currently used but
CsvV2ExportType is refactored for when it can reach feature parity to
csv searchsource.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Timothy Sullivan <[email protected]>
rshen91 added a commit that referenced this issue Jul 12, 2023
## Summary

This partially addresses #158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit that referenced this issue Jul 12, 2023
## Summary

This is a part of #158092

This PR encompasses both csv_v2 and csv_searchsource as the
CsvExportType and CsvSearchsourceExportType. Based on the reporting
plugin readme, CsvSearchsourceExportType is currently used but
CsvV2ExportType is refactored for when it can reach feature parity to
csv searchsource.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Timothy Sullivan <[email protected]>
rshen91 added a commit that referenced this issue Jul 12, 2023
## Summary

Partially addresses #158092

Following the Csv and Pdf reports, this PR continues the process with a
PngExportType class instance vs the getExportType() function. This PR
concludes refactoring the export types to class instances. The next
steps will be migrating these to the reporting export types plugin
#161291 and clean up of tests.

Diagnostic tool is timing out for waitForElements - in this PR this is
commented out since the png v1 endpoint has not been refactored yet.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Timothy Sullivan <[email protected]>
rshen91 added a commit that referenced this issue Jul 13, 2023
## Summary

This partially addresses #158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit that referenced this issue Jul 13, 2023
## Summary

This is a part of #158092

This PR encompasses both csv_v2 and csv_searchsource as the
CsvExportType and CsvSearchsourceExportType. Based on the reporting
plugin readme, CsvSearchsourceExportType is currently used but
CsvV2ExportType is refactored for when it can reach feature parity to
csv searchsource.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Timothy Sullivan <[email protected]>
rshen91 added a commit that referenced this issue Jul 13, 2023
## Summary

Partially addresses #158092

Following the Csv and Pdf reports, this PR continues the process with a
PngExportType class instance vs the getExportType() function. This PR
concludes refactoring the export types to class instances. The next
steps will be migrating these to the reporting export types plugin
#161291 and clean up of tests.

Diagnostic tool is timing out for waitForElements - in this PR this is
commented out since the png v1 endpoint has not been refactored yet.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Timothy Sullivan <[email protected]>
rshen91 added a commit to rshen91/kibana that referenced this issue Jul 20, 2023
## Summary

This partially addresses elastic#158092

This PR refactors the generic function getExportType(). This PR is
concerned only with the PDF export type and refactors it into a class
instance. This class is then used in the request handler and the execute
report functionality in Reporting.

The execute_report.test.ts is removed in this PR and will be part of the
clean up needed in a subsequent PR once CSV and PNG are also refactored
to class instances instead of the getExportType() function.
rshen91 added a commit to rshen91/kibana that referenced this issue Jul 20, 2023
## Summary

This is a part of elastic#158092

This PR encompasses both csv_v2 and csv_searchsource as the
CsvExportType and CsvSearchsourceExportType. Based on the reporting
plugin readme, CsvSearchsourceExportType is currently used but
CsvV2ExportType is refactored for when it can reach feature parity to
csv searchsource.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Timothy Sullivan <[email protected]>
rshen91 added a commit to rshen91/kibana that referenced this issue Jul 20, 2023
## Summary

Partially addresses elastic#158092

Following the Csv and Pdf reports, this PR continues the process with a
PngExportType class instance vs the getExportType() function. This PR
concludes refactoring the export types to class instances. The next
steps will be migrating these to the reporting export types plugin
elastic#161291 and clean up of tests.

Diagnostic tool is timing out for waitForElements - in this PR this is
commented out since the png v1 endpoint has not been refactored yet.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Timothy Sullivan <[email protected]>
@rshen91
Copy link
Contributor

rshen91 commented Jul 31, 2023

This issue is completed 🥳

@rshen91 rshen91 closed this as completed Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:SharedUX Team label for AppEx-SharedUX (formerly Global Experience)
Projects
None yet
Development

No branches or pull requests

4 participants