fix(GlobalFilters): RHINENG-6296 push correct params to url

[xmicha82]

Description

Associated Jira ticket: # RHINENG-6296

Fixed pushing global filters to URL

How to test the PR

1. Open Patch systems page
2. Apply Global filters
3. Global Filters should be added to the URL

Before the change

After the change

Checklist:

• The commit message has the Jira ticket linked
• PR has a short description
• Screenshots before and after the change are added
• Tests for the changes have been added
• README.md is updated if necessary
• Needs additional dependent work

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 64.04%. Comparing base (fdcced0) to head (d164f1f).

Files	Patch %	Lines
src/SmartComponents/Systems/SystemsTable.js	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1185      +/-   ##
==========================================
+ Coverage   64.01%   64.04%   +0.02%     
==========================================
  Files         124      124              
  Lines        3235     3243       +8     
  Branches      831      833       +2     
==========================================
+ Hits         2071     2077       +6     
- Misses       1164     1166       +2     

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

[bastilian]

Explain.

[xmicha82]

Since apply is now used inside the SystemsTable (before, it was just passed along to other functions), the tests fail if apply is undefined. thus i'm just passing a plain function into the component

[bastilian]

We will need to make this work even when no apply is provided.

[bastilian]

Can you elaborate a bit more on what the issue was?

[xmicha82]

basically, SystemsTable did not use the hook for managing URL params which is used in other places in patch. using this hook fixed the issue

[bastilian]

@xmicha82 This doesn't feel like a fix, but a work around. We should also not need this firstMount state.

[xmicha82]

@bastilian this change is consistent with other pages in patch, which also use the historyPusher. could you please explain why do you think it's not the right way to fix this issue?

[bastilian]

@xmicha82 I took a quick look at this. I used master without any changes, but this in the SystemsTable:

    const location = useLocation();
    console.log('LOCATION CHANGE', location);

Here is what I saw:

patch_global_filter.mp4

If you pay attention to the console you will notice that, first of all, there are probably updates to the URL multiple times per render and re renders, and if you look that location after the global filter is set you'll see that at some point the hash is lost.

A proper fix for this issue will require us to take a closer look and see why, how and where changes to the URL happen when the SystemsTable loads and ensure that all of these places do not change the hash and preserve the global filter.

Ideally we would also look into reducing the amount of updates to just one update to the URL per filter change, but for now just making sure that the global filter hash is not lost should be fix enough.

[bastilian]

A better place to start debugging might be the SystemsMainContent where we use useSearchParams. Adding this log output can give you an idea of how many times the URL is updated:

    const setSearchParams = (...args) => {
        console.log('SetSearch', ...args);
        setSearchParamsOrig(...args);
    };

The setSerachParams function provided by react router also allows navigate options as a second parameter, this might allow us to set the hash of the current location, that includes the global filter. No it doesn't help.

We might need to move those search param changes to use useNavigate instead.

[bastilian]

@xmicha82 Have you had time to revise this issue? If you want we can debug this together and see where it starts loosing the hash property.