-
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
adds pagination on Alert Instances list on Alert Details page #57524
adds pagination on Alert Instances list on Alert Details page #57524
Conversation
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
* master: add `absolute` option to `getUrlForApp` (elastic#57193) [Telemetry] Migrate public to NP (elastic#56285) address flaky test where instances might have different start… (elastic#57506) fix(NA): support legacy plugins path in plugins (elastic#57472) build immutable bundles for new platform plugins (elastic#53976) [SIEM] [Detection Engine] Reject if duplicate rule_id in request payload (elastic#57057) Add autocomplete="off" for input type="password" to appease the scanners (elastic#56922) Use default spaces suffix for signals index if spaces disabled (elastic#57244) [Alerting] Create alert design cleanup (elastic#56929)
@elasticmachine merge upstream |
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.
As much as I understand this stuff, LGTM. Made some nit-level comments about the code, and had some questions about testing the result of the pagination which seem like they could be flaky.
...triggers_actions_ui/public/application/sections/alert_details/components/alert_instances.tsx
Show resolved
Hide resolved
|
||
const { alertInstances } = await alerting.alerts.getAlertState(alert.id); | ||
|
||
const [firstItem] = await pageObjects.alertDetailsUI.getAlertInstancesList(); |
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.
should it check the # of items in the list? Should be 10, right?
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.
Ah fair, I'll add that 👍
@@ -135,22 +135,36 @@ export function AlertInstances({ | |||
unmuteAlertInstance, | |||
requestRefresh, | |||
}: AlertInstancesProps) { | |||
const [pagination, setPagination] = useState<Pagination>({ index: 0, size: 10 }); |
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.
there are a couple of 10
literals throughout the PR; should these be a constant defined somewhere?
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.
Well, only once in the plugin itself, the others are tests where the duplication makes sense to me. But that said, alert list has it too, I'll pull both into a constant.
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 tested using 100 alert instances - pagination, changing page size, skipping pages all working as expected. I would agree with Patrick maybe add a const DEFAULT_PAGE_SIZE = 10
otherwise LGTM.
* master: (22 commits) skip flaky suite (elastic#50018) skip settings tests (elastic#57608) skip failing suite (elastic#44631) [SIEM] [Case] Initial UI (elastic#57283) handle viewing sample dashboards on default dist (elastic#57510) Fix detection of "system requests" in plugins (elastic#57149) [ML] New Platform server shim: update job service schema (elastic#57614) skip flaky suite (elastic#44631) [APM] Update monospace font family variable (elastic#57555) skip flaky test (elastic#57377) Skip save query tests (elastic#57589) [Maps] allow simultaneous opening of multiple tooltips (elastic#57226) [Uptime] Fix/host connected components (elastic#56969) [logs][metrics][docs] Update screenshots for 7.6 (elastic#57254) [ML] New Platform server shim: update job service routes to use new platform router (elastic#57403) [Maps] Fix document source top hits split by scripted field (elastic#57481) Use log4j pattern syntax (elastic#57433) [ML] Categorization field example endpoint tests (elastic#57471) [Lens] Filter out pinned filters from saved object of Lens (elastic#57197) Lens client side shim cleanup (elastic#56976) ...
…bana into alerting/instance-pagination * 'alerting/instance-pagination' of github.com:gmmorris/kibana:
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
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
Running functional tests 42x to ensure no flakiness: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/192/. |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…c#57524) * added pagination on alert instances page * extracted default page size to a constant for alerting UI as a whole * Fix test failure Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Mike Côté <[email protected]>
#58045) * added pagination on alert instances page * extracted default page size to a constant for alerting UI as a whole * Fix test failure Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Mike Côté <[email protected]> Co-authored-by: Gidi Meir Morris <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
adds pagination on Alert Instances list on Alert Details page
Checklist
Delete any items that are not applicable to this PR.
[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorialsFor maintainers