-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Reporting] Fix and test for Listing of Reports #74453
Conversation
@@ -9,6 +9,6 @@ import { FtrProviderContext } from '../../ftr_provider_context'; | |||
export default ({ loadTestFile }: FtrProviderContext) => { | |||
describe('reporting management app', function () { | |||
this.tags('ciGroup7'); | |||
loadTestFile(require.resolve('./report_delete_pagination')); | |||
loadTestFile(require.resolve('./report_listing')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed the file since it tests more than just deleting a report in the listing
@@ -50,7 +56,7 @@ export function registerJobInfoRoutes(reporting: ReportingCore) { | |||
page: queryPage = '0', | |||
size: querySize = '10', | |||
ids: queryIds = null, | |||
} = req.query as ListQuery; | |||
} = req.query as ListQuery; // NOTE: type inference is not working here. userHandler breaks it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to understand why the type of req.query
can not be inferred from the validation. I didn't take much time to look into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to know as well. I spent a good deal of time trying to get types to pass through with the higher order handler (https://github.com/elastic/kibana/blob/master/x-pack/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts#L15), but there's some interesting effects it might have trying to get type information to pass through properly. This was a pretty big change in our codebase to get everything working properly, as a lot of our utilities relied on augmented Hapi request objects since we used the pre
handlers fairly extensively. I can take a look at this since I wrote the wrapper handler. Unfortunately nothing is jumping out at me since I pretty much copy/pasted the migration docs for this "higher-order" handler.
Pinging @elastic/kibana-reporting-services (Team:Reporting Services) |
@@ -35,7 +35,13 @@ export function registerJobInfoRoutes(reporting: ReportingCore) { | |||
router.get( | |||
{ | |||
path: `${MAIN_ENTRY}/list`, | |||
validate: false, | |||
validate: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note from @restrry: "platform doesn’t provide request params / body / query unless validation schema is provided"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of remarks
- page & size should be
number
, you parse them below userHandler
might break type inference logic, at least it doesn't implement the same interface as wrappers in docs https://github.com/elastic/kibana/blob/e57f4d2b25ac95183e876ea0c305138ab119ea26/src/core/MIGRATION_EXAMPLES.md#migrating-hapi-pre-handlers.
I'd recommend either implement it as RequestHandlerWrapper or pass user viaRequestHandlerContext
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our HoH has roughly the same type passthrough interface. Is there something obvious I'm missing?
I had to add Instead, there should probably be a paging test subject on the page that we can wait to find, to know that we're on the right page - instead of just waiting a hard 2 seconds Edit: resolved in 9c7ac46d3f |
9c7ac46
to
a39f8af
Compare
💚 Build SucceededBuild metricspage load bundle size
History
To update your PR or re-run it, just comment with: |
Hi @restrry, I'm not exactly clear on the advice in #74453 (comment)
Does this mean the route validates them as number? And also that it will still be a string in my handler and that I should still parse the string into a number?
Is this advice for something to change in each of our route handlers? Or is something not set up right in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Since this is a blocker (IMO), I'm OK with following up with the np changes in a follow up PR
I agree with this |
* [Reporting] Fix and test for Listing of Reports * add sleeps * await selector instead of sleep * reduce changes * cleanup after csv generated * fix snapshot
* [Reporting] Fix and test for Listing of Reports * add sleeps * await selector instead of sleep * reduce changes * cleanup after csv generated * fix snapshot
* [Reporting] Fix and test for Listing of Reports (#74453) * [Reporting] Fix and test for Listing of Reports * add sleeps * await selector instead of sleep * reduce changes * cleanup after csv generated * fix snapshot * fix test asserts
…nes/processor-forms-a-d * 'master' of github.com:elastic/kibana: (25 commits) [ML] Removing full lodash library imports (elastic#74742) [Search] Server strategy example (elastic#71679) [Reporting] Fix and test for Listing of Reports (elastic#74453) [maps] fix drawing shapes (elastic#74689) [Resolver] Improve simulator. Add more click-through tests and panel tests. (elastic#74601) Deprecate schema-less specs in Vega (elastic#73805) [Security Solution] Rename Administration > Hosts subtab to Endpoints (elastic#74287) Timelion deprecation doc (elastic#74508) [Functional Tests] Adds a wait time between setting the index pattern and the time field on TSVB (elastic#74736) [Lens] Add styling options for x and y axes on the settings popover (elastic#71829) [Maps] add initial location option that fits to data bounds (elastic#74583) theme function (elastic#73451) [data.ui.query] Write filters to query log from default editor. (elastic#74474) [data.search.SearchSource] Move some SearchSource dependencies to the server. (elastic#74607) [Canvas][tech-debt] Convert renderers (elastic#74134) [security solutions][lists] Adds end to end tests (elastic#74473) pluralized for occurrences vs occurrence (elastic#74564) Update links that pointed to CONTRIBUTING.md (elastic#74757) [Ingest pipelines] Implement tabs in processor flyout (elastic#74469) [Event log] Use Alerts client & Actions client when fetching these types of SOs (elastic#73257) ... # Conflicts: # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/field_components/text_editor.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/manage_processor_form.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/append.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/bytes.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/circle.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/field_name_field.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/common_fields/ignore_missing_field.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/convert.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/csv.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/date_index_name.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dissect.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/dot_expander.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/drop.tsx # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/index.ts # x-pack/plugins/ingest_pipelines/public/application/components/pipeline_processors_editor/components/manage_processor_form/processors/shared.ts
* master: [Vega] add functional tests for Vega visualization (elastic#74097) [ML] Removing full lodash library imports (elastic#74742) [Search] Server strategy example (elastic#71679) [Reporting] Fix and test for Listing of Reports (elastic#74453) [maps] fix drawing shapes (elastic#74689) [Resolver] Improve simulator. Add more click-through tests and panel tests. (elastic#74601) Deprecate schema-less specs in Vega (elastic#73805) [Security Solution] Rename Administration > Hosts subtab to Endpoints (elastic#74287) Timelion deprecation doc (elastic#74508)
* [Reporting] Fix and test for Listing of Reports (#74453) * [Reporting] Fix and test for Listing of Reports * add sleeps * await selector instead of sleep * reduce changes * cleanup after csv generated * fix snapshot * same as 7.9 fix
yes, it has ben fixed in #74879
yes, |
Summary
Closes: #74799
Fix an issue where the Reporting API route for showing the listing of pages was not working correctly, due to query parameters not being parsed from the request.
Skipping release notes since this bug was not released in the current latest version.
Checklist
Delete any items that are not applicable to this PR.
For maintainers