-
Notifications
You must be signed in to change notification settings - Fork 46
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
feat(RHIF-283): extend CVE systems filter with edge #2002
Conversation
c6e0521
to
a17db6a
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2002 +/- ##
==========================================
+ Coverage 70.13% 71.42% +1.29%
==========================================
Files 127 127
Lines 3328 3353 +25
Branches 1019 1028 +9
==========================================
+ Hits 2334 2395 +61
+ Misses 994 958 -36 ☔ View full report in Codecov by Sentry. |
@@ -23,6 +23,11 @@ jest.mock('../../../Store/Actions/Actions', () => ({ | |||
}) } | |||
)); | |||
|
|||
jest.mock('../../../Helpers/Hooks', () => ({ | |||
...jest.requireActual('../../../Helpers/Hooks'), | |||
useFeatureFlag: () => true |
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.
It would also make sense to add tests with the feature set to false.
We might also want to investigate a wrapper function that we can use to run tests with both the feature turned on and off without having to duplicate the tests.
4de6731
to
8b5c364
Compare
@@ -9,7 +9,8 @@ import { useSelector } from 'react-redux'; | |||
/* Mock function returning permissions */ | |||
jest.mock('../../../Helpers/Hooks', () => ({ | |||
...jest.requireActual('../../../Helpers/Hooks'), | |||
useRbac: () => [[true, true, true], false] | |||
useRbac: () => [[true, true, true], false], | |||
useFeatureFlag: () => true |
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.
The tests (all in this PR) should by default run with the feature turned off/false
. and tests added for the feature should turn them on explicitly.
This way we can ensure that the functionality stays the same until the feature is realeased.
@@ -13,6 +13,7 @@ const affectingFilter = (apply, currentFilter = {}) => { | |||
}); | |||
}; | |||
|
|||
const filterItems = getAffectingFilterOptions(isEdgeParityEnabled); |
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.
It might be better to filter out the unwanted items with the feature turned off here and not have to pass down the feature flag into all the functions.
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 also went this way so that I have minimal tech-debt. But there are multiple places where affecting filter values are used e.g. affecting filter, report custom filter, and chip builders. The changes were not clean to read and brought more tech debt. Thus, I changed the approach to use the feature flag in components so that when we get rid of the flag, we can easily remove the flags
@bastilian I have disabled the feature flag in the ReportPage test file. |
@mkholjuraev Yeah, but now there aren't any with it enabled, and just for this one test:
|
37eea76
to
8e896b3
Compare
8e896b3
to
3a9fe9d
Compare
3a9fe9d
to
9d60037
Compare
@@ -25,8 +25,14 @@ const customMiddleWare = store => next => action => { | |||
next(action); | |||
} | |||
|
|||
jest.mock('../../../Helpers/Hooks', () => ({ | |||
...jest.requireActual('../../../Helpers/Hooks'), | |||
useHybridSystemFilterFlag: () => false |
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 am planning to write meaningful tests for changes in ReportConficModal when I am working on RHIF-302.
@@ -23,6 +25,11 @@ jest.mock('../../../Store/Actions/Actions', () => ({ | |||
}) } | |||
)); | |||
|
|||
jest.mock('../../../Helpers/Hooks', () => ({ | |||
...jest.requireActual('../../../Helpers/Hooks'), | |||
useHybridSystemFilterFlag: jest.fn(() => true) |
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.
Shouldn't this be default false
as well and set to true only on line 232
?
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.
@bastilian Yup, I have made the change and also added test cases covering the flag is both off and on. PTAL
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.
@bastilian can I ask for your review on this? Thanks
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.
LGTM! Thank you @mkholjuraev!
…ts#2002) Fixes https://issues.redhat.com/browse/ESSNTL-5253. This is another patch for ESSNTL-5253 that makes sure that all of the params are passed to the tabs components and the tabs are loaded correctly.
Resolves: https://issues.redhat.com/browse/RHIF-283
We need to extend the filter options of the Systems filter on the CVEs table to have
1 or more Conventional systems (RPM-DNF)
1 or more Immutable (OSTree)
None
mockup