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 report result filter determination #2366

Merged
merged 7 commits into from
Aug 6, 2020
Merged

Fix report result filter determination #2366

merged 7 commits into from
Aug 6, 2020

Conversation

bjoernricks
Copy link
Contributor

@bjoernricks bjoernricks commented Aug 6, 2020

Don't use the current stored filter of the results list page for loading results at the report details page.

Checklist:

Use camelCase for all variables.
Filter model objects are mutable. Therefore a copy must be created to
not influence other tests.
The FilterProvider is used to load and determine a filter (mostly) for
list pages. In saved the current used filter at a list page in the redux
store under the gmp name e.g. task. This change allows to use a
different name for the filter saved in the redux store.
Revert 60ec7eb and PR #2358 because it
doesn't fix the cause of the original problem.
The page filter is already passed via redux connect and should be used
for loading the report with second highest priority.
When determining the filter for loading a report and its results we need
to ensure that the user settings are considered but not to use the
stored filter of the results list page. Therefore use a different page
name in the FilterProvider to load the report results filter. The
resulting filter is passed as defaultFilter to the load function.
@bjoernricks bjoernricks marked this pull request as ready for review August 6, 2020 13:09
@bjoernricks bjoernricks requested review from a team, sarahd93 and saberlynx August 6, 2020 13:09
@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #2366 into gsa-20.08 will increase coverage by 0.17%.
The diff coverage is 85.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##           gsa-20.08    #2366      +/-   ##
=============================================
+ Coverage      52.76%   52.93%   +0.17%     
=============================================
  Files           1070     1070              
  Lines          25789    25833      +44     
  Branches        7312     7359      +47     
=============================================
+ Hits           13607    13675      +68     
+ Misses         11058    11039      -19     
+ Partials        1124     1119       -5     
Impacted Files Coverage Δ
gsa/src/gmp/commands/wizard.js 9.23% <0.00%> (ø)
gsa/src/web/pages/cpes/details.js 11.11% <ø> (ø)
gsa/src/web/pages/extras/trashactions.js 5.49% <0.00%> (ø)
gsa/src/web/pages/hosts/dialog.js 50.00% <ø> (ø)
gsa/src/web/pages/policies/detailspage.js 100.00% <ø> (ø)
gsa/src/web/pages/policies/row.js 100.00% <ø> (ø)
gsa/src/web/pages/portlists/detailspage.js 34.48% <ø> (ø)
gsa/src/web/pages/portlists/row.js 55.55% <ø> (ø)
gsa/src/web/pages/reportformats/detailspage.js 33.33% <ø> (ø)
gsa/src/web/pages/reportformats/row.js 40.00% <ø> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81e354e...ef85565. Read the comment docs.

Comment on lines +718 to +721
if (!hasValue(filter)) {
// use filter from store
filter = pageFilter;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not 100 percent sure if these lines are really necessary because the page filter will be included in the defaultFilter by the FilterProvider already. Currently this overrides a possible ?filter=... url parameter (named locationQueryFilter in FilterProvider).

@saberlynx saberlynx merged commit 8cc8f34 into greenbone:gsa-20.08 Aug 6, 2020
@bjoernricks bjoernricks deleted the fix-report-result-filter-determination branch August 6, 2020 13:45
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.

2 participants