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

Add HTML exporting feature for reports #183

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

mster
Copy link
Contributor

@mster mster commented Aug 13, 2019

See: https://github.com/nodesource/process/issues/36

Description:
Allows for users to generate and export a NCM project report in HTML (as per customer request).

  • Report contains project information similar to the --long report.
  • Users can specify the location and filename when exporting via --html path/to/report.html (default exports to cwd with a timestamped report name).

TODO:

  • Implementation approval
  • Determine & implement design
  • Add tests

Screenshot(s): a very ugly duckling
Screen Shot 2019-08-12 at 7 48 19 PM

@mster mster self-assigned this Aug 13, 2019
@mster mster requested a review from edsadr August 13, 2019 19:03
@mster mster added this to the OP Sprint 5 (8/13 - 9/2) milestone Aug 13, 2019
Copy link
Member

@edsadr edsadr left a comment

Choose a reason for hiding this comment

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

There are two significant things to refactor:

  • The logic generating the HTML report is a bit hard to follow, I think an approach of having a single function putting the required variables inside a unique Html template would be better, as the SVG template in the console here: https://github.com/nodesource/nsolid-console-ng/blob/master/lib/svg-template.js. This would be easier to maintain; lets split the template and the logic to create it in different files to create a separation of concerns.

  • Remove any third-party dependencies for the CSS and JS embedded in the template, we want a report that could be used offline, also a whole framework is kind of an overkill for what we need here.

module.exports = htmlReport

async function htmlReport (report, whitelist, dir, output) {
if (output === true) output = path.join(process.cwd(), `ncm-report-${Date.now()}.html`)
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to include the name of the app in the name

@mster
Copy link
Contributor Author

mster commented Aug 19, 2019

Changes completed as per your review @edsadr

@mster
Copy link
Contributor Author

mster commented Aug 19, 2019

Updated Example Screenshot:
bad-package-report

@mster mster changed the title [WIP] report: concept for html report feature (needs design) Add HTML exporting feature for long reports Aug 29, 2019
}

return vulns
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Migration of formatting logic for reusability in the HTML reporting feature.

Copy link
Member

@edsadr edsadr left a comment

Choose a reason for hiding this comment

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

The report is working, great work on this @mster, just some items to consider:

  • Currently, the report contains different information than the one being generated by the CLI, this could confuse the customer. let's use the same information and also support all filters like --filter=compliance

  • I know I suggested not to include external assets, but let's add the styles to use our fonts, if the customer has it it will render properly, if not fall back to another font, ask @pdevay about it.

@mster
Copy link
Contributor Author

mster commented Aug 30, 2019

Filtering now supported.

report-c

report-s

(and depending on security vulnerability level, as the cli does)

@mster mster changed the title Add HTML exporting feature for long reports Add HTML exporting feature for reports Aug 30, 2019
@mster
Copy link
Contributor Author

mster commented Aug 30, 2019

Reflection of changes

report3

@edsadr edsadr assigned Gioyik and unassigned mster Jan 11, 2021
@Gioyik Gioyik removed this from the OP Sprint 5 (8/13 - 8/30) milestone Jan 12, 2021
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.

4 participants