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(SystemsPage): RHINENG-5977 - Fix URL params filters handling #2129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bastilian
Copy link
Member

@bastilian bastilian commented Jul 24, 2024

This fixes the issue with filter params and adds them correctly to the URL.

How to test:

  1. Open the Systems or CVESystems page
  2. Filter the table
  3. Verify the URL params are maintained properly and in sync with the filter state

Copy link

jira-linking bot commented Jul 24, 2024

Referenced Jiras:
https://issues.redhat.com/browse/RHINENG-5977

@bastilian bastilian force-pushed the RHINENG-5977 branch 7 times, most recently from 8edcd8e to 6811d96 Compare July 26, 2024 12:13
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.16%. Comparing base (40d2bae) to head (6811d96).
Report is 8 commits behind head on master.

Files Patch % Lines
...ponents/SmartComponents/SystemsPage/SystemsPage.js 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2129   +/-   ##
=======================================
  Coverage   67.15%   67.16%           
=======================================
  Files         128      129    +1     
  Lines        3440     3441    +1     
  Branches     1067     1068    +1     
=======================================
+ Hits         2310     2311    +1     
  Misses       1130     1130           

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

@bastilian bastilian marked this pull request as ready for review August 29, 2024 11:49
@bastilian bastilian requested a review from a team as a code owner August 29, 2024 11:49
Copy link
Contributor

@mkholjuraev mkholjuraev left a comment

Choose a reason for hiding this comment

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

Codewise looks good. However, while testing I found out that if I apply some filters and open another tab with the same URL, those applied filters are lost. This needs to be fixed

import { useCallback } from 'react';
import { useSearchParams } from 'react-router-dom';

// TODO DRY:similar to constructParameters
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is time to finally to implement this TODO. Can we try to consolidate this function with constructParameters and get rid of the duplicate code?

const [searchParams, setSearchParams] = useSearchParams();

const setUrlParams = useCallback((parameters) => {
const para = constructURLParameters(parameters, allowedParams);
Copy link
Contributor

Choose a reason for hiding this comment

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

NITPICK: It looks like a typo on the first look.

Suggested change
const para = constructURLParameters(parameters, allowedParams);
const param = constructURLParameters(parameters, allowedParams);

describe('MiscHelper', () => {
it('useUrlParams', async () => {
const { result } = renderHook(() => useUrlParams(['a', 'b']), {
wrapper: createTestWrapper(MemoryRouter, { initialEntries: ['/'] })
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use the TestWrapper which has already MemortRouter provided?

@@ -22,4 +22,10 @@ TestWrapper.propTypes = {
store: propTypes.object
};

export const createTestWrapper = (Wrapper = TestWrapper, props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use the shared wrapper, this is not needed.

@mkholjuraev
Copy link
Contributor

@bastilian Just a friendly ping on this :)

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.

3 participants