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.

Sign up for GitHub

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

Jump to bottom

Gsa sets default filter #1631

Merged
merged 25 commits into from
Sep 30, 2019
Merged

Gsa sets default filter #1631

merged 25 commits into from
Sep 30, 2019

Conversation

swaterkamp
Copy link
Member

Checklist:

@swaterkamp swaterkamp self-assigned this Sep 17, 2019
@swaterkamp swaterkamp marked this pull request as ready for review September 17, 2019 12:51
@codecov
Copy link

codecov bot commented Sep 17, 2019 •
edited
Loading

Codecov Report

Merging #1631 into master will decrease coverage by 0.79%.
The diff coverage is 60.16%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1631     +/-   ##
=========================================
- Coverage   42.34%   41.55%   -0.8%     
=========================================
  Files        1047     1048      +1     
  Lines       23984    24051     +67     
  Branches     6684     6712     +28     
=========================================
- Hits        10156     9994    -162     
- Misses      12589    12754    +165     
- Partials     1239     1303     +64
Impacted Files Coverage Δ
gsa/src/web/entities/container.js 1.74% <0%> (-39.72%) ⬇️
gsa/src/web/pages/usersettings/usersettingspage.js 3.36% <0%> (-0.1%) ⬇️
gsa/src/web/store/utils.js 100% <100%> (ø) ⬆️
gsa/src/gmp/utils/identity.js 100% <100%> (ø) ⬆️
gsa/src/web/store/usersettings/defaults/actions.js 100% <100%> (ø) ⬆️
gsa/src/gmp/models/filter.js 100% <100%> (ø) ⬆️
gsa/src/gmp/commands/gmp.js 100% <100%> (ø) ⬆️
gsa/src/web/entities/filterprovider.js 100% <100%> (ø)
gsa/src/web/pages/results/listpage.js 50% <100%> (+7.14%) ⬆️
...c/web/store/usersettings/defaultfilters/actions.js 100% <100%> (ø) ⬆️
... and 14 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 488b885...0eeedca. Read the comment docs.

bjoernricks
bjoernricks requested changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

I am sorry but these changes need tests. We must ensure that the provided filter is the expected filter. Currently this can break without noticing. Also with tests it would be much clearer how this is supposed to work.

gsa/src/gmp/commands/users.js Outdated Show resolved Hide resolved
gsa/src/web/entities/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/store/usersettings/defaultfilters/actions.js Outdated Show resolved Hide resolved
gsa/src/web/store/usersettings/defaults/actions.js Outdated Show resolved Hide resolved
gsa/src/web/store/usersettings/defaults/actions.js Outdated Show resolved Hide resolved
gsa/src/web/pages/usersettings/usersettingspage.js Outdated Show resolved Hide resolved
gsa/src/web/entities/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/entities/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/entities/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/entities/filterprovider.js Outdated Show resolved Hide resolved
bjoernricks
bjoernricks requested changes Sep 19, 2019
View reviewed changes
Copy link
Contributor

@bjoernricks bjoernricks left a comment

Choose a reason for hiding this comment

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

Looks very very good to me. Just some additional cleanups.

gsa/src/web/entities/__tests__/filterprovider.js Show resolved Hide resolved
gsa/src/web/entities/__tests__/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/entities/__tests__/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/entities/__tests__/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/entities/__tests__/filterprovider.js Show resolved Hide resolved
gsa/src/web/entities/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/entities/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/entities/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/entities/filterprovider.js Outdated Show resolved Hide resolved
gsa/src/web/pages/policies/__tests__/listpage.js Outdated Show resolved Hide resolved
@swaterkamp
Copy link
Member Author

Known issue which needs a fix: If a locationQuery is used and the filter is changed by the user, it does not change the URL, so at the next rerender the query from the URL will be used again instead of the user's filter.

@swaterkamp
Add isNull function to utils/identity
17aaa9f
@swaterkamp
Add tests for isNull function
b5657e8
@swaterkamp
Allow to load single default usersetting in store
928710d
@swaterkamp
Set store filters to null when loaded but non-existent
c0d67b0
@swaterkamp
If getSetting() returns array, use first value
162d7da 
gvmd returns two settings with the same ID for the rowsPerPage setting. 
The first one is the current user setting and the second the global 
setting. Therefore use the first.
@swaterkamp
Don't crash UserSettingsPage when defaultFilters are null
5e56480
@swaterkamp
Add new FilterProvider component
ede0d16
@swaterkamp
Use FilterProvider with EntitiesContainer
fb7a282
@swaterkamp
Set sort-reverse=severity as fallbackFilter for Results
79811cd 
Basically meant as a PoC and example.
@swaterkamp
Update tests
6dc0f0d
@swaterkamp
Expect fallbackFilter to be of type Filter
e15e31c
@swaterkamp
Simplify code
34659b6
@swaterkamp
Merge new default settings into store and don't overwrite all settings
f36ee1e
@swaterkamp
Update reducer test
5e6332f 
Since single settings should be merged now, the test 'should override 
existing settings' is obsolete
@swaterkamp
Update filterIdentifier
131ffeb
@swaterkamp
Add tests for FilterProvider
40907bf
@swaterkamp
Fix propType for locationQuery
8419d51
@swaterkamp
Add test for user command getSetting()
9d0e55d 
This tests if only one setting is returned in case there are >1 with the 
same ID.
@bjoernricks bjoernricks merged commit 51552fa into greenbone:master Sep 30, 2019
@swaterkamp swaterkamp deleted the gsaSetsDefaultFilter branch February 20, 2020 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjoernricks bjoernricks bjoernricks approved these changes

Assignees

@swaterkamp swaterkamp

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@swaterkamp @bjoernricks