Persist selected filters in checks results inside Redux #1445

dottorblaster · 2023-05-23T16:16:45Z

Saving applied filters into a redux slice so when the user comes back we just restore previously applied state

How was this tested?

Jest test added

nelsonkopliku

Great job!

Just a couple of suggestions if they help.

Additionally, as we discussed, just noting that the filter in the query string gets wiped out when navigating among the pages.
However this does not impact the feature which works just fine. I am ok with the tradeoff if it's too much of a hassle re-applying the qs when we have saved filters and we navigate back to the executions results.

nelsonkopliku · 2023-05-24T15:03:31Z

assets/js/components/ExecutionResults/ChecksResultFilters.jsx

@@ -4,14 +4,25 @@ import Filter from '@components/Table/Filter';

 export const RESULT_FILTER_FIELD = 'result';

-function ChecksResultFilters({ onChange }) {
+const getFilters = (savedFilters, searchParams) =>
+  savedFilters &&


If I am getting it right in the worst case savedFilters comes as an empty array, so this initial check might be not necessary.

Could come as undefined, updated it to have a default prop 👍

nelsonkopliku · 2023-05-24T15:36:55Z

assets/js/components/ExecutionResults/ChecksResultFilters.jsx

+const getFilters = (savedFilters, searchParams) =>
+  savedFilters &&
+  savedFilters.length >= 0 &&
+  searchParams.getAll('health').length === 0


We could extract this searchParams.getAll('health') outside of getFilters function.
I am aware it wouldn't change much 😅, so not a biggie. However if you want to give it a go here's the changes it would require.

function ChecksResultFilters() { //... const qsFilters = searchParams.getAll('health'); useEffect(() => { const selectedFilters = getFilters(savedFilters, qsFilters); setFiltersForField({ //... }); }, [searchParams, savedFilters]); //... return ( <div className="flex"> <Filter //... value={getFilters(savedFilters, qsFilters)} /> </div> ); }

I tried and the component exploded in my face 😅

nelsonkopliku · 2023-05-24T15:47:58Z

assets/js/components/ExecutionResults/ExecutionResults.test.jsx

+      />
+    );
+
+    expect(screen.getAllByText('test-cluster')).toHaveLength(2);


What about squashing this test and the previous one in a it.each version with two scenarios?
One scenario that does not pass a savedFilters (or an empty one) but it provides the route with querystring, another scenario that does provide the savedFilters without querystring.

At the end the expectations are the same for the two tests 🤔

If it calls for too much refactor that you don't feel like addressing, keep it as it is.

arbulu89

@dottorblaster The code looks good.
I only commented the naming part (our beloved naming).
I think we could generalize this. We will have sooner than later checks that are not only cluster based, so what about making the state already generic?
The usage would be exactly the same, we just need to build the payload correctly, that's all.

arbulu89 · 2023-05-25T06:43:19Z

assets/js/state/clusterChecksResultsFilters.js

@@ -0,0 +1,17 @@
+import { createSlice } from '@reduxjs/toolkit';


I would rename this file to checksResultsFilters. Surely we will want to use this for future visualizations, where we have hosts based results, but I expect the execution results view being the same, as data will be coming from wanda in the same way.

Yes, you're right

arbulu89 · 2023-05-25T06:45:38Z

assets/js/state/clusterChecksResultsFilters.js

+  name: 'clusterChecksResultsFilters',
+  initialState,
+  reducers: {
+    setClusterFilters: (state, { payload: { clusterID, filters } }) => {


What about making this generic setChecksResultsFilters.
Getting the { id: filters } payload. I'm just thinking for future uses where we don't only have checks for clusters.
All of our resouces (clusters, hosts, sap systesms) are identified by uuid, so there wouldn't be any collision.

Right again :D

arbulu89 · 2023-05-25T06:46:06Z

assets/js/state/clusterChecksResultsFilters.js

+  name: 'clusterChecksResultsFilters',
+  initialState,
+  reducers: {
+    setClusterFilters: (state, { payload: { clusterID, filters } }) => {


I know, the code is ultra simple, but what about adding a test?

arbulu89 · 2023-05-25T06:48:30Z

assets/js/components/ExecutionResults/ExecutionResults.jsx

@@ -101,9 +101,11 @@ function ExecutionResults({
  executionData,
  executionError,
  clusterSelectedChecks = [],
+  savedFilters,


Could we maybe add an empty list as default?
To make things a bit more secure, and in fact it would even help to story book

Let me check if nothing explodes, sure

I managed to assign a default prop but I had to declare it outside the inline scope. Left a comment about that, hope it's ok

EMaksy

Nice, well done :D

Besides @nelsonkopliku suggestions nothing to add.

LGTM.

arbulu89

@dottorblaster It looks great!
I would simply change clusterID by resourceID/groupID to finish making it generic now that we changed the files/state names.
(Maybe run find all thing in the changes hehe, as clusterID is everywhere, specially on the tests)

arbulu89 · 2023-05-29T11:56:16Z

assets/js/state/selectors/checksResultsFilters.js

@@ -0,0 +1,4 @@
+export const getSelectedFilters =
+  (clusterID) =>


I would change clusterID by groupID/resourceID once we started making it generic

arbulu89 · 2023-05-29T11:56:45Z

assets/js/state/checksResultsFilters.js

+  name: 'checksResultsFilters',
+  initialState,
+  reducers: {
+    setSelectedFilters: (state, { payload: { clusterID, filters } }) => {


clusterID by resourceID maybe?

arbulu89 · 2023-05-29T11:57:07Z

assets/js/components/ExecutionResults/ExecutionResultsPage.jsx

      onStartExecution={(clusterId, hosts, selectedChecks, navigate) =>
        dispatch(executionRequested(clusterId, hosts, selectedChecks, navigate))
      }
+      onSaveFilters={(filters) =>
+        dispatch(setSelectedFilters({ clusterID, filters }))


`setSelectedFilters({ resourceID: clusterID, filters })

Save filters in redux when filtering checks results

dottorblaster added the enhancement New feature or request label May 23, 2023

dottorblaster self-assigned this May 23, 2023

dottorblaster force-pushed the persist-checks-results-filters-redux branch from fe9e421 to ddde3a1 Compare May 24, 2023 10:28

dottorblaster marked this pull request as ready for review May 24, 2023 13:19

dottorblaster requested review from arbulu89, nelsonkopliku and EMaksy May 24, 2023 13:19

nelsonkopliku reviewed May 24, 2023

View reviewed changes

arbulu89 reviewed May 25, 2023

View reviewed changes

dottorblaster force-pushed the persist-checks-results-filters-redux branch from 7c62a09 to 585ca15 Compare May 25, 2023 14:06

EMaksy approved these changes May 25, 2023

View reviewed changes

This comment was marked as duplicate.

Sign in to view

dottorblaster force-pushed the persist-checks-results-filters-redux branch 3 times, most recently from 528b971 to 8cc32fa Compare May 29, 2023 08:15

nelsonkopliku approved these changes May 29, 2023

View reviewed changes

arbulu89 approved these changes May 29, 2023

View reviewed changes

dottorblaster force-pushed the persist-checks-results-filters-redux branch from 1d09f71 to 9190c5f Compare May 29, 2023 13:58

Persist selected filters in checks results inside Redux

a43a4ff

Save filters in redux when filtering checks results

dottorblaster force-pushed the persist-checks-results-filters-redux branch from 9190c5f to a43a4ff Compare May 29, 2023 14:28

dottorblaster merged commit e9c5856 into main May 29, 2023

dottorblaster deleted the persist-checks-results-filters-redux branch May 29, 2023 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist selected filters in checks results inside Redux #1445

Persist selected filters in checks results inside Redux #1445

dottorblaster commented May 23, 2023 •

edited

Loading

nelsonkopliku left a comment

nelsonkopliku May 24, 2023

dottorblaster May 29, 2023

nelsonkopliku May 24, 2023

dottorblaster May 29, 2023

nelsonkopliku May 24, 2023

nelsonkopliku May 29, 2023

arbulu89 left a comment

arbulu89 May 25, 2023

dottorblaster May 25, 2023

arbulu89 May 25, 2023

dottorblaster May 25, 2023

arbulu89 May 25, 2023

arbulu89 May 25, 2023

dottorblaster May 25, 2023

dottorblaster May 25, 2023

EMaksy left a comment

This comment was marked as duplicate.

arbulu89 left a comment

arbulu89 May 29, 2023

arbulu89 May 29, 2023

arbulu89 May 29, 2023

		@@ -0,0 +1,17 @@
		import { createSlice } from '@reduxjs/toolkit';

		@@ -0,0 +1,4 @@
		export const getSelectedFilters =
		(clusterID) =>

Persist selected filters in checks results inside Redux #1445

Persist selected filters in checks results inside Redux #1445

Conversation

dottorblaster commented May 23, 2023 • edited Loading

How was this tested?

nelsonkopliku left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EMaksy left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dottorblaster commented May 23, 2023 •

edited

Loading