-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[ML] Edit text for deleting calendars & add functional tests for Calendars and Filter Lists #77566
Conversation
Pinging @elastic/ml-ui (:ml) |
x-pack/plugins/ml/public/application/settings/calendars/edit/new_event_modal/new_event_modal.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/settings/calendars/edit/new_event_modal/new_event_modal.js
Outdated
Show resolved
Hide resolved
.../ml/public/application/settings/filter_lists/components/add_item_popover/add_item_popover.js
Outdated
Show resolved
Hide resolved
.../ml/public/application/settings/filter_lists/components/add_item_popover/add_item_popover.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/settings/filter_lists/edit/edit_filter_list.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/ml/public/application/settings/filter_lists/edit/edit_filter_list.js
Outdated
Show resolved
Hide resolved
config/kibana.yml
Outdated
@@ -40,8 +40,8 @@ | |||
# the username and password that the Kibana server uses to perform maintenance on the Kibana | |||
# index at startup. Your Kibana users still need to authenticate with Elasticsearch, which | |||
# is proxied through the Kibana server. | |||
#elasticsearch.username: "kibana_system" | |||
#elasticsearch.password: "pass" | |||
elasticsearch.username: "kibana_system" |
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.
You must remove these edits before merging!
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.
This is why we added a codeowner on this file actually 😄 . Once reverted, you can remove the review request from the platform team.
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.
Thanks @pgayvallet - I have reverted the change and removed review request. Sorry about that.
…emButton, update snapshots, add more assertions
…rlist-tests # Conflicts: # x-pack/plugins/ml/public/application/settings/filter_lists/edit/edit_filter_list.js # x-pack/plugins/ml/public/application/settings/filter_lists/list/table.js # x-pack/test/functional/services/ml/settings_filter_list.ts
@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.
Functional test code LGTM with a few last comments.
@@ -124,7 +124,7 @@ export function MachineLearningSettingsCalendarProvider( | |||
await testSubjects.existOrFail('mlPageCalendarEdit'); | |||
}, | |||
|
|||
async assertApplyToAllJobsSwitchEnabled(expectedValue: boolean) { | |||
async assertApplyToAllJobsSwitchCheckedState(expectedValue: boolean) { |
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.
This method is not validating the checked state, but rather that the control is enabled or disabled for the user, i.e. the user is able to toggle it vs the control is grayed out. So I think this change should be reverted. If you still think that the old method name is misleading, I'm open for other suggestions.
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.
My apologies. I switched it back here db1bdd7
await this.assertCalendarEventDescriptionValue(eventDescription); | ||
}, | ||
|
||
async createNewCalendarEvent(eventDescription: string) { |
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 discussed on slack: I think we don't need this method any more.
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.
Removed here db1bdd7
await ml.settingsFilterList.saveFilterList(); | ||
await ml.settingsFilterList.assertFilterListRowExists(filterId); | ||
} | ||
await ml.testExecution.logTestStep('filter list creation creates new filter lists'); |
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 think this line could be removed as you're already logging more detailed test steps in the next lines.
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.
Removed here db1bdd7
@elasticmachine merge upstream |
…ndarEvent, remove redundant log step
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.
Latest edits LGTM
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededBuild metricsasync chunks size
History
To update your PR or re-run it, just comment with: |
…ndars and Filter Lists (elastic#77566) Co-authored-by: Elastic Machine <[email protected]>
…r Calendars and Filter Lists (#77566) (#78198) Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
This PR adds functional test for the calendar and filter list. It also updates the delete modal when deleting calendar(s).
Checklist
Delete any items that are not applicable to this PR.