fix(RHINENG-3106): properly apply default filters in CVEs and SystemsCVEs pages #2124
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2124 +/- ##
==========================================
+ Coverage 66.73% 67.07% +0.34%
==========================================
Files 128 128
Lines 3445 3432 -13
Branches 1069 1064 -5
==========================================
+ Hits 2299 2302 +3
+ Misses 1146 1130 -16 ☔ View full report in Codecov by Sentry. |
| apply({ | ||
| ...(Object.keys(urlParameters).length === 0 ? { advisory_available: 'true' } : {}), | ||
| ...urlParameters | ||
| }); |
There was a problem hiding this comment.
This could be also rewritten in my opinion. Please see the previous comment
| @@ -93,7 +93,7 @@ export const CVEs = ({ rbac }) => { | |||
| }; | |||
|
|
|||
| apply(filtersOnLoad); | |||
| }, [shouldUseHybridSystemFilter]); | |||
| }, [shouldUseHybridSystemFilter. urlParameters]); | |||
There was a problem hiding this comment.
Why urlParameters are needed here in the dependencies list? This useEffect deals with the first load and it seems to me that we should leave it empty completely, or am I wrong?
| <ComponentWithContext store={ store } renderOptions={{ initialEntries: ['?affecting=rpmdnf%2Cedge&advisory_available=true']}}> | ||
| <CVEs /> | ||
| </TestWrapper> | ||
| </ComponentWithContext> | ||
| ); |
There was a problem hiding this comment.
It feels like TestWrapper and ComponentWithContext both deal with the same problem: render a component with the necessary providers and contexts. We have to stop duplicating these two components and just stick to only one. I see that TestWrapper doesn't support initialEntries: let's perhaps extend it and keep the usage of it here in this test file?
There was a problem hiding this comment.
I would remove TestWrapper. Because ComponentWithContext follows the conventions that we have in other apps as well. That is why, I replaced TestWrapper with ComponentWithContext. Let's proceed with the review of this PR, I will, first of all, push the component into FEC so that we can share it among apps, and then adopt it in the vuln app in the next PR.
There was a problem hiding this comment.
Yes, I agree that TestWrapper is the better name for the component rather than ComponentWithContext . While pushing to FEC, I will keep this in mind.
Co-authored-by: Georgii Karataev <[email protected]>
|
@gkarat thank you for the review and nice suggestions. I have applied your suggestions. Have another look! |
|
The last commit fixes the issue in the ticket comments by @mtclinton : I have also noticed that if CVEs without Errata is disabled and I click the modal to "Show CVEs without Errata" the table and filters are not updated to reflect this. I have to manually refresh the page to display the advisory filter and have the table filter correct results. It also has a large amount test cases and improvements. |
There was a problem hiding this comment.
LGTM. Thanks @mkholjuraev, but please take a look at the tests and ideally apply the suggestions before we merge
This fixes the preserving the state of default filters after refresh. The solution is to add the default filters only when user navigates to CVEs or SystemCVEs page by using navigations that have no active filter URL. In this case, default filters are added. In case, there are already default filters applied, the default filter are not applied, URL state dictates the table filtering.