Skip to content

[Cases] Cases List - Refactor Solution Filter - #170851

Merged
jcger merged 4 commits into
elastic:cases/167651from
jcger:issue-167651-refactor-solution-filter
Nov 9, 2023
Merged

[Cases] Cases List - Refactor Solution Filter#170851
jcger merged 4 commits into
elastic:cases/167651from
jcger:issue-167651-refactor-solution-filter

Conversation

@jcger

@jcger jcger commented Nov 8, 2023

Copy link
Copy Markdown
Contributor

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 Platform ResponseOps team (formerly the Cases and Alerting teams) t// 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

elasticmachine commented Nov 8, 2023

Copy link
Copy Markdown
Contributor

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

@elasticmachine

elasticmachine commented Nov 8, 2023

Copy link
Copy Markdown
Contributor

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

cnasikas
cnasikas approved these changes Nov 8, 2023
View reviewed changes

@cnasikas cnasikas left a comment

Copy link
Copy Markdown
Member

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

jcger commented Nov 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
Comment thread x-pack/plugins/cases/public/components/all_cases/all_cases_list.tsx
}
: {}),
});
setFilterOptions(newFilterOptions);

@adcoelho adcoelho Nov 8, 2023

Copy link
Copy Markdown
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

@adcoelho adcoelho left a comment

Copy link
Copy Markdown
Contributor

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

cnasikas commented Nov 8, 2023

Copy link
Copy Markdown
Member

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

kibana-ci commented Nov 8, 2023

Copy link
Copy Markdown

💔 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
@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 Platform ResponseOps team (formerly the Cases and Alerting teams) t// v8.12.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jcger @elasticmachine @cnasikas @kibana-ci @adcoelho