fix(OperatingSystemFilter): RHINENG-10345 - Fix OS inventory filter conversion #1152
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1152 +/- ##
==========================================
- Coverage 64.14% 64.00% -0.14%
==========================================
Files 124 124
Lines 3207 3234 +27
Branches 818 830 +12
==========================================
+ Hits 2057 2070 +13
- Misses 1150 1164 +14 ☔ View full report in Codecov by Sentry. |
bastilian
force-pushed
the
RHINENG-2912-2
branch
from
16737e4 to
10838b9
Compare
bastilian
force-pushed
the
RHINENG-2912-2
branch
from
10838b9 to
3fb4157
Compare
bastilian
force-pushed
the
RHINENG-2912-2
branch
2 times, most recently
from
9fd240b to
bc453aa
Compare
bastilian
changed the title
| ...operatingSystemFilter ? { | ||
| filters: [...(osFilter || [])] | ||
| } : {}, |
There was a problem hiding this comment.
I think this is something we could cover with tests: there is SystemTable.test.js available where we could add a scenario where query parameters contain filter[os]. And given that InventoryTable receives customFilters, we can assert on what the mocked component was called with and make sure osFilter is passed in the expected format. WDYT?
| filter: queryParamsFilter, search, page, perPage, sort | ||
| } = queryParams; | ||
| const { os: operatingSystemFilter, ...filter } = queryParamsFilter || {}; | ||
| const osFilter = operatingSystemFilter && [{ |
There was a problem hiding this comment.
Can we extract this into some utility function? The os filter exists in Advisory systems table as well. Probably we need to make same changes there as welll
There was a problem hiding this comment.
Yeah, a similar change is in advisor. We could look into moving this to a shared function, but the apps have always slightly different ways of providing the filter to their APIs and I'm not sure how much we could really share.
There was a problem hiding this comment.
Aha, advisory application is a different example of how sharing might help. But, I meant the AdvisorySystems table in the patch itself
There was a problem hiding this comment.
@mkholjuraev but, ähm, we are not showing the filter there yet. Do we want to enable it there as well now?
There was a problem hiding this comment.
There we don't need the change, because the table is not querying against the Patch API, but the inventory API and the filter comes directly from there.
There was a problem hiding this comment.
oh, I'm confused. ok looking more. before I say more. :D
|
@bastilian can you please have a look at this PR. The OS filter is broken in both stage and prod and we are receiving more and more complaints unfortunately. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
🎉 This PR is included in version 1.67.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This fixes issues with syncing the state and URL of filters in the Systems table.
Description
Associated Jira ticket: # (issue)
Please include a summary of the change, what this fixes/creates/improves.
How to test the PR
Please include steps to test your PR.
Before the change
After the change
Dependent work link
Checklist: