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.

Sign up for GitHub

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

Jump to bottom

[Cases] Cases List - Refactor Solution Filter #170851

Merged
merged 4 commits into from Nov 9, 2023
Merged

[Cases] Cases List - Refactor Solution Filter #170851

merged 4 commits into from Nov 9, 2023

Conversation

jcger
Copy link
Contributor

@jcger jcger commented Nov 8, 2023

Meta #167651

Refactors the Solution Filter to use the same component as the rest of the filters

Screenshot

QA

Add Sample web logs data and click on View Data > ML Jobs
Screenshot

Then on the 3 dots right side of the Anomaly timeline > Add to case > Overall

@jcger jcger added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.12.0 labels Nov 8, 2023
@jcger jcger marked this pull request as ready for review November 8, 2023 12:03
@jcger jcger requested a review from a team as a code owner November 8, 2023 12:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

cnasikas
cnasikas approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Great cleanup 🚀. I tested with different users and permissions and everything is working as expected.

Should we move the tests about Solutions from x-pack/plugins/cases/public/components/all_cases/all_cases_list.test.tsx to x-pack/plugins/cases/public/components/all_cases/solution_filter.test.tsx?

@jcger
Copy link
Contributor Author

jcger commented Nov 8, 2023 •
edited

Great cleanup 🚀. I tested with different users and permissions and everything is working as expected.

Should we move the tests about Solutions from x-pack/plugins/cases/public/components/all_cases/all_cases_list.test.tsx to x-pack/plugins/cases/public/components/all_cases/solution_filter.test.tsx?

This are the tests in all_cases_list:

Solutions
✓ should set the owner to all available solutions when deselecting all solutions (512 ms)
✓ should hide the solutions filter if the owner is provided (252 ms)
✓ should call useGetCases with the correct owner on initial render (246 ms)

The first one is repeated but it's testing it from a integration point of view, so why not?
The second one definitely makes sense as that's not implemented in the solution filter
The third one also makes sense also because it's something that is not implemented in the solution filter

So I would say no, let's keep it as it is. There are broken now because the test ids changed so I'll fix them and we should be good to go 🚀

Edit: I'll add more tests in the solution_filter.test, for example, I'll check what happens depending on the owner

adcoelho
adcoelho reviewed Nov 8, 2023
View reviewed changes
x-pack/plugins/cases/public/components/all_cases/all_cases_list.tsx
}
: {}),
});
setFilterOptions(newFilterOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this 😁

adcoelho
adcoelho approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

Also tested, working as expected 👍

@cnasikas
Copy link
Member

cnasikas commented Nov 8, 2023

Great cleanup 🚀. I tested with different users and permissions and everything is working as expected.
Should we move the tests about Solutions from x-pack/plugins/cases/public/components/all_cases/all_cases_list.test.tsx to x-pack/plugins/cases/public/components/all_cases/solution_filter.test.tsx?

This are the tests in all_cases_list:

Solutions ✓ should set the owner to all available solutions when deselecting all solutions (512 ms) ✓ should hide the solutions filter if the owner is provided (252 ms) ✓ should call useGetCases with the correct owner on initial render (246 ms)

The first one is repeated but it's testing it from a integration point of view, so why not? The second one definitely makes sense as that's not implemented in the solution filter The third one also makes sense also because it's something that is not implemented in the solution filter

So I would say no, let's keep it as it is. There are broken now because the test ids changed so I'll fix them and we should be good to go 🚀

Edit: I'll add more tests in the solution_filter.test, for example, I'll check what happens depending on the owner

You are right! Thanks for the clarification.

@kibana-ci
Copy link
Collaborator

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] Defend Workflows Cypress Tests #5 / Unenroll agent from fleet changing agent policy when agent tamper protection is enabled but then is switched to a policy with it also enabled "before each" hook for "should unenroll from fleet without issues" "before each" hook for "should unenroll from fleet without issues"
  • [job] [logs] Defend Workflows Cypress Tests #4 / Unenroll agent from fleet changing when agent tamper protection is enabled but then is switched to a policy with it disabled "before each" hook for "should unenroll from fleet without issues" "before each" hook for "should unenroll from fleet without issues"

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [375d638]

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jcger jcger merged commit ac0bdb3 into elastic:cases/167651 Nov 9, 2023
31 of 32 checks passed
@jcger jcger mentioned this pull request Nov 30, 2023
Merged
jcger added a commit that referenced this pull request Dec 4, 2023
@jcger @adcoelho @kibanamachine
[Cases] Cases Table: Configure Available Filters Including Custom Fie…
90d6358 
…lds (#172276)

Meta issue #167651
Fixes: #167651

## Summary
Previous PRs merged into this feature branch:
- #169356
- #169371
- #170851
- #171102
- #171176

## Release notes
Case list filter bar can now be customised. Filters can be removed and
custom fields can be used as filters

## Pending issues
- Table in modal shouldn’t load in local storage saved filter options of
status/severity
- Status & Severity filters in url. Filters must be activated if the
user has them deactivated
- UI overflow when to much filters are active
- Race condition: When a user has a custom field active with an option
selected and this custom field gets removed in settings, it includes the
removed custom field when refreshing. This request will fail, triggering
a second one which won't include the removed custom field
- Found during QA. In the modal, when trying to select all options in
the solutions filter, when checking the last unchecked option, it resets
and there is no checked option anymore

## Flaky test runner link

https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4128

---------

Co-authored-by: Antonio <antoniodcoelho@gmail.com>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adcoelho adcoelho adcoelho approved these changes

@cnasikas cnasikas cnasikas approved these changes

Assignees
No one assigned
Labels
Feature:Cases Cases feature release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jcger @elasticmachine @cnasikas @kibana-ci @adcoelho