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

Report load from store #923

Merged
merged 16 commits into from
Sep 11, 2018
Merged

Report load from store #923

merged 16 commits into from
Sep 11, 2018

Conversation

bjoernricks
Copy link
Contributor

Load the report data from store and adjust auto reload for report details.

@codecov
Copy link

codecov bot commented Sep 7, 2018

Codecov Report

Merging #923 into master will increase coverage by 0.12%.
The diff coverage is 22.92%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #923      +/-   ##
=========================================
+ Coverage    7.37%   7.49%   +0.12%     
=========================================
  Files         826     826              
  Lines       26873   26908      +35     
  Branches     5731    5740       +9     
=========================================
+ Hits         1982    2018      +36     
+ Misses      22449   22443       -6     
- Partials     2442    2447       +5
Impacted Files Coverage Δ
gsa/src/web/pages/reports/resultstab.js 0% <ø> (ø) ⬆️
gsa/src/web/pages/reports/emptyresultsreport.js 0% <ø> (ø) ⬆️
gsa/src/web/entities/container.js 0% <0%> (ø) ⬆️
gsa/src/web/utils/constants.js 0% <0%> (ø) ⬆️
gsa/src/web/store/entities/reducers.js 0% <0%> (ø) ⬆️
gsa/src/gmp/commands/reports.js 9.37% <0%> (-0.63%) ⬇️
gsa/src/web/pages/reports/listpage.js 0% <0%> (ø) ⬆️
gsa/src/web/pages/reports/detailscontent.js 0% <0%> (ø) ⬆️
gsa/src/web/pages/reports/row.js 0% <0%> (ø) ⬆️
gsa/src/web/pages/reports/detailspage.js 0% <0%> (ø) ⬆️
... and 2 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 c20aba9...ebd7666. Read the comment docs.

@@ -499,7 +499,7 @@ const PageContent = ({
onFilterDecreaseMinQoDClick={onFilterDecreaseMinQoDClick}
onFilterRemoveSeverityClick={onFilterRemoveSeverityClick}
onFilterEditClick={onFilterEditClick}
onFilterResetClick={onFilterResetClick}
onFilterRemoveClick={onFilterRemoveClick}
onInteraction={onInteraction}
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep both options? Removing the reset-option makes it impossible to easily apply the default filter

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've only fixed the current behavior of the displayed box. If you think we should add another box when no results are available to actually reset the filter to the detauls a new PR is always welcome 😄

@@ -153,6 +159,20 @@ class ReportDetails extends React.Component {
}
}

startMeasurement() {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe these methods could get a more meaningful name. It's not clear from the title alone that time for auto-updates is measured. Could also be "number of scrolled pixels" or basically anything else...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it could measure all kind of durations but currently it's only used for the load duration. So I suppose you are right with changing the name would be better.

Copy link
Member

Choose a reason for hiding this comment

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

As it can measure any kind of duration something like startDurationMeasurement() would be quite informative. We don't need to restrict this method to startLoadDurationMeasurement() or anything like that.

Copy link
Member

@swaterkamp swaterkamp left a comment

Choose a reason for hiding this comment

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

Not really a request for changes, but imho two points worth thinking about before merging.

The test description wasn't changed after the method got renamed.
Allow to pass a filter for the results when loading a report.

Update report loadEntity tests accordingly.
Allow to handle delta reports in the redux store.
Refactor the report details page to load all data from the redux store.
Use new camelCase data from the report details page at report details
page content.
Resetting meand using the default results filter which may still not
show all results. Therefore allow the user to remove the current applied
filter completely.
It's not possible to get the target from the store directly after
loading it into the store when opening the edit dialog. The correct
target isn't passed to the details page before the dialog is open.

Therefore bypass the store and load the target directly.
If the filter isn't reset the report list would keep the task_id filter
when visiting the report list again.
Automatically reload the report if it is active.
Rename start/endMeasurement to start/endDurationMeasurement
@swaterkamp swaterkamp merged commit f5e594e into greenbone:master Sep 11, 2018
@bjoernricks bjoernricks deleted the report-load-from-store branch September 11, 2018 07:30
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