Skip to content
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

fix(RHINENG-9239, RHINENG-9260): Keep filtering in Hybrid Inventory Tabs separate, filter reset when Add Systems modal closes #2171

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

LiorKGOW
Copy link
Member

@LiorKGOW LiorKGOW commented Apr 8, 2024

Description

This PR addresses issues:
#RHINENG-9239
#RHINENG-9260

Inventory group's details page: Filtering of each tab should be kept separate and saved even when switching between tabs (Conventional and Immutable)
Also, this PR includes 2 commits that change state variable names (tab & activeTabKey) in GroupTabDetails component to improve code readability
4th commit - added Sebastian's suggestion
5th commit - fixed an inconsistency between the different tabs on the "Add Systems" modal

This PR introduces a store instance for each tab, which holds it's own filters for it's InventoryTable. In order to create a new instance of the store for the InventoryTable, you need to provide InventoryTable component with the isolateStore prop to render it with a Provider Wrapper which holds a new store instance alongside with the InventoryTable.
I have removed the component state inside InventoryGroupDetail/GroupTabDetails.js and put the components inside the returned JSX of the component.
Regarding the Immutable tab inside the "Add Systems" Modal, the filters there were handled a bit differently, you can find small comments inside the 5th commit message which solves this inconsistency issue which was created in previous commits

@LiorKGOW LiorKGOW self-assigned this Apr 8, 2024
@LiorKGOW LiorKGOW requested a review from a team as a code owner April 8, 2024 10:42
@LiorKGOW LiorKGOW requested a review from gkarat April 8, 2024 10:43
@codecov-commenter
Copy link

codecov-commenter commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 36.36364% with 35 lines in your changes are missing coverage. Please review.

Project coverage is 57.06%. Comparing base (7b112ea) to head (ece4c0a).

Files Patch % Lines
...c/components/GroupSystems/GroupImmutableSystems.js 0.00% 21 Missing ⚠️
...components/InventoryGroupDetail/GroupTabDetails.js 0.00% 10 Missing ⚠️
src/components/GroupSystems/GroupSystems.js 73.33% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2171   +/-   ##
=======================================
  Coverage   57.06%   57.06%           
=======================================
  Files         207      208    +1     
  Lines        6400     6396    -4     
  Branches     1785     1784    -1     
=======================================
- Hits         3652     3650    -2     
+ Misses       2748     2746    -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LiorKGOW LiorKGOW force-pushed the keep-filtering-of-each-tab-seperate branch from dd5eb8c to 8574b25 Compare April 9, 2024 14:49
@LiorKGOW LiorKGOW changed the title fix(RHINENG-9239): Keep Filtering of Each Tab Seperate (Hybrid Inventory Tabs) fix(RHINENG-9239): Keep Filtering of Each Tab Seperate (Hybrid Inventory Tabs) & fix(RHINENG-9260): filter reset when Add Systems modal closes Apr 9, 2024
@LiorKGOW LiorKGOW changed the title fix(RHINENG-9239): Keep Filtering of Each Tab Seperate (Hybrid Inventory Tabs) & fix(RHINENG-9260): filter reset when Add Systems modal closes fix(RHINENG-9239): Keep Filtering of Each Tab Seperate (Hybrid Inventory Tabs) && fix(RHINENG-9260): filter reset when Add Systems modal closes Apr 9, 2024
@LiorKGOW LiorKGOW requested a review from bastilian April 9, 2024 14:50
@LiorKGOW
Copy link
Member Author

LiorKGOW commented Apr 9, 2024

Hey @bastilian I have went over your comments and updated the PR accordingly
It is ready for another review ✌🏼

@LiorKGOW LiorKGOW force-pushed the keep-filtering-of-each-tab-seperate branch from 8574b25 to c53382c Compare April 9, 2024 15:20
@LiorKGOW
Copy link
Member Author

LiorKGOW commented Apr 9, 2024

Thank you for the quick review @bastilian !
I have updated the PR with your suggestions
It is ready for another review ✌🏼

@LiorKGOW LiorKGOW requested a review from bastilian April 9, 2024 15:21
@bastilian
Copy link
Member

@LiorKGOW The code looks good to me, however, while testing I noticed an inconsistency. On the Systems tab both the Conventional and the Immutable Systems persist the filters properly, but in the modal when it closes and the filters should not persist the Immutable tab still keeps them after closing the modal:

immu.mp4

@gkarat gkarat added the bug Something isn't working label Apr 10, 2024
@LiorKGOW
Copy link
Member Author

LiorKGOW commented Apr 11, 2024

@LiorKGOW The code looks good to me, however, while testing I noticed an inconsistency. On the Systems tab both the Conventional and the Immutable Systems persist the filters properly, but in the modal when it closes and the filters should not persist the Immutable tab still keeps them after closing the modal:

immu.mp4

@bastilian you are right, I have not noticed that
Let me have a look at it. I'll ping you once this is fixed 👍🏼

@LiorKGOW LiorKGOW removed the bug Something isn't working label Apr 11, 2024
@LiorKGOW LiorKGOW added the bug Something isn't working label Apr 11, 2024
@LiorKGOW LiorKGOW force-pushed the keep-filtering-of-each-tab-seperate branch from 674b383 to 821c1b2 Compare April 24, 2024 13:06
@LiorKGOW LiorKGOW removed the bug Something isn't working label Apr 24, 2024
@LiorKGOW
Copy link
Member Author

@bastilian I have added the code that fixes the inconsistency problem you mentioned, you can find it in the last commit 👍🏼

@LiorKGOW LiorKGOW force-pushed the keep-filtering-of-each-tab-seperate branch from 821c1b2 to 44a4138 Compare April 25, 2024 10:50
@LiorKGOW
Copy link
Member Author

LiorKGOW commented Apr 25, 2024

Rebased on top of latest master and repushed

@LiorKGOW
Copy link
Member Author

/retest

@LiorKGOW LiorKGOW force-pushed the keep-filtering-of-each-tab-seperate branch 4 times, most recently from 92c15d8 to ece4c0a Compare April 30, 2024 07:51
@bastilian bastilian requested a review from a team April 30, 2024 09:33
@gkarat gkarat changed the title fix(RHINENG-9239): Keep Filtering of Each Tab Seperate (Hybrid Inventory Tabs) && fix(RHINENG-9260): filter reset when Add Systems modal closes fix(RHINENG-9239, RHINENG-9260): Keep filtering inHybrid Inventory Tabs separate, filter reset when Add Systems modal closes May 3, 2024
@LiorKGOW LiorKGOW changed the title fix(RHINENG-9239, RHINENG-9260): Keep filtering inHybrid Inventory Tabs separate, filter reset when Add Systems modal closes fix(RHINENG-9239, RHINENG-9260): Keep filtering in Hybrid Inventory Tabs separate, filter reset when Add Systems modal closes May 5, 2024
@johnsonm325 johnsonm325 force-pushed the keep-filtering-of-each-tab-seperate branch from ece4c0a to bbc1d36 Compare May 8, 2024 14:15
+ fix(RHINENG-9260): filter reset when Add Systems modal closes

+ Remove component state variable
+ Remove useMemo from the content of Hybrid inventory Tabs
+ Remove the conditional rendering of both GroupSystems & GroupImmutableSystems components

Links to issues:
1. https://issues.redhat.com/browse/RHINENG-9239
2. https://issues.redhat.com/browse/RHINENG-9260
The systems table of ImmutableDevicesView, which uses Edge's DevicesView, persists filters in the Session Storage
@bastilian bastilian force-pushed the keep-filtering-of-each-tab-seperate branch from bbc1d36 to f00a4c4 Compare May 10, 2024 08:26
Copy link
Contributor

@gkarat gkarat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one regression: if URL contains pagination, it's no longer applied for conventional tab. I will try to find out what caused it and fix it

@gkarat gkarat assigned gkarat and unassigned LiorKGOW May 16, 2024
@gkarat gkarat marked this pull request as draft May 21, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants