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

Load "small" before "full" report #1697

Merged
merged 13 commits into from
Oct 16, 2019

Conversation

swaterkamp
Copy link
Member

Checklist:

@swaterkamp swaterkamp requested a review from a team October 15, 2019 14:18
@swaterkamp swaterkamp self-assigned this Oct 15, 2019
@codecov
Copy link

codecov bot commented Oct 15, 2019

Codecov Report

Merging #1697 into gsa-8.0 will decrease coverage by <.01%.
The diff coverage is 25.71%.

Impacted file tree graph

@@             Coverage Diff             @@
##           gsa-8.0    #1697      +/-   ##
===========================================
- Coverage    39.52%   39.52%   -0.01%     
===========================================
  Files          965      965              
  Lines        22095    22120      +25     
  Branches      6237     6230       -7     
===========================================
+ Hits          8734     8743       +9     
- Misses       12098    12111      +13     
- Partials      1263     1266       +3
Impacted Files Coverage Δ
gsa/src/web/pages/reports/resultstab.js 16.66% <ø> (ø) ⬆️
gsa/src/web/pages/reports/scaninfo.js 5.35% <0%> (-0.2%) ⬇️
gsa/src/gmp/commands/reports.js 32.35% <0%> (ø) ⬆️
gsa/src/web/pages/reports/detailscontent.js 9.7% <0%> (-1.05%) ⬇️
...a/src/web/pages/reports/reportentitiescontainer.js 4.61% <0%> (ø) ⬆️
gsa/src/web/pages/reports/detailspage.js 3.1% <0%> (-0.05%) ⬇️
gsa/src/web/store/entities/reports.js 100% <100%> (ø) ⬆️

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 733bc11...2219310. Read the comment docs.

@swaterkamp swaterkamp force-pushed the ReportDetailsPerformance8 branch from e66282f to 3a7fb5c Compare October 15, 2019 14:38
gsa/src/web/pages/reports/detailspage.js Show resolved Hide resolved
@@ -181,8 +183,8 @@ class ReportEntitiesContainer extends React.Component {

ReportEntitiesContainer.propTypes = {
children: PropTypes.func,
counts: PropTypes.counts.isRequired,
entities: PropTypes.array.isRequired,
counts: PropTypes.oneOfType([PropTypes.counts, PropTypes.object]).isRequired,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why should an object be passed as counts here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I need an object instead of counts, because if I pass a new CollectionCount() its construction sets all fields to 0 resulting in faulty information on the detailspage (it will tell me that the report is empty instead of showing Loading).

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not passing undefined instead? undefined has a clear meaning.

Copy link
Member Author

Choose a reason for hiding this comment

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

For example in ResultsTab I would have to check again, if it's undefined when calling counts.filtered or counts.all. in order not to have to add several tests like that i just pass an empty object.

Copy link
Contributor

Choose a reason for hiding this comment

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

you could init the counts instead when passing undefined

{counts = {}} = props

Seems to be much cleaner to me :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

In ReportEntitiesContainer i would still have to accept CollectionCount and Object then.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. i'll take a look after the pr is merged.

gsa/src/web/pages/reports/scaninfo.js Outdated Show resolved Hide resolved
gsa/src/web/pages/reports/scaninfo.js Outdated Show resolved Hide resolved
gsa/src/web/store/entities/__tests__/reports.js Outdated Show resolved Hide resolved
gsa/src/web/store/entities/__tests__/reports.js Outdated Show resolved Hide resolved
gsa/src/web/store/entities/__tests__/reports.js Outdated Show resolved Hide resolved
gsa/src/web/store/entities/__tests__/reports.js Outdated Show resolved Hide resolved
gsa/src/web/store/entities/__tests__/reports.js Outdated Show resolved Hide resolved
gsa/src/web/store/entities/__tests__/reports.js Outdated Show resolved Hide resolved
@swaterkamp swaterkamp force-pushed the ReportDetailsPerformance8 branch from 7a43411 to 2219310 Compare October 16, 2019 09:03
@bjoernricks bjoernricks merged commit 4397958 into greenbone:gsa-8.0 Oct 16, 2019
@swaterkamp swaterkamp deleted the ReportDetailsPerformance8 branch October 16, 2019 11:40
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