-
Notifications
You must be signed in to change notification settings - Fork 206
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 module to handle italics and multiline processing in policy description #730
Conversation
@isab-m Please update the PR title from the default to a clearer title. Preferably starting with a present tense verb and a short description that explains the purpose of the update. The PR title becomes the merge commit message that lives on in the commit history on the main branch, so title is important as it communicates the actual change to users. |
The italics fix is good, but this PR does not close #604 entirely as it does not address extra line breaks in policy statements rendered in the report. Either split #604 so that a new issue to address line breaks is created outside the scope of this PR or update the PR to address the extra line breaks issue as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a unit test to verify proper transformation.
@schrolla Updated code to reflect multiline processing split into two use cases shown below: Multiline Use Case Visually, can't see much of a difference in this multiline case being present here in MS.DEFENDER.4.6v1 However, upon further investigation into raw text in defender baseline doc, a newline is present in the policy description Multiline case resolves this newline break in markdown, which visually can be shown using a diff tool on the text. List Use Cases |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments below for recommended updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran full Invoke-Scuba -p *
and reviewed output of all reports. Updated rendering looks much better, especially MS.EXO.16.1 with correct multi-line breaks and properly rendered bolded sub-items. All unit tests ran successfully as well. Great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor oversight on my part. The baselines were changed, I'm assuming for testing. I'd recommend reverting/removing those changes so we aren't modifying the baselines for this code update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- An earlier comment as to consider adding/enhancing unit test to check that this capability works (and does not get broke in the future) -- Tests have been added and/or modified to cover the changes in this PR.
- Please fix identified linter errors in CreateReport.psm1
- Need to rebase
Added bold support for the transformation of markdown policy descriptions Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>
All 3 items added to code. Can I get a review on my unit tests @crutchfield? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran again successfully, showing correct multiline, italics, and bold processing. Ran PS and rego unit tests successfully. New unit tests for translation function adequately test for bad input and output values and test valid translations. Looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are a couple comments to look at.
Testing/Unit/PowerShell/CreateReport/Resolve-HTMLMarkdown.Tests.ps1
Outdated
Show resolved
Hide resolved
Added bold support for the transformation of markdown policy descriptions Co-authored-by: Addam Schroll <108814318+schrolla@users.noreply.github.com>
bd15b1c
to
a13f4d7
Compare
* Change italics test string to single underscores * Change test string match from regex to direct compare
…/cisagov/ScubaGear into 604-html-report-markdown-cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addresses immediate needs. Future enhancement will be addressed by #847
@nanda-katikaneni PR is ready for merge. |
🗣 Description
HTML reporting shows Markdown notation and extra line breaks. Markdown notation is not rendered and is shown as is which can make the requirement text harder to interpret or at the very least is less visually appealing. Fixes the HTML reporting translation of italic and bold notation from the baseline's policy description markdown notation. Fixes extra line breaks and list breaks in html reports.
💭 Motivation and context
Closes #604
🧪 Testing
📷 Screenshots (if appropriate)
Italics notation added to baseline using "_"
Italics notation rendered correctly in report descriptions
Bold notation added to baseline policy description MS.AAD.2.3v1 using "**"
Bold notation translation rendered correctly in report descriptions
Bold notation added to baseline policy description MS.AAD.2.3v1 using "**" with italics "_" notation added inside
Bold and Italics notation rendered correctly in report descriptions
Italics notation added to baseline policy description MS.AAD.2.2v1 using "_" with bold "**" notation added inside
Italics and bold notation rendered correctly in report descriptions✅ Pre-approval checklist
in code comments.
to reflect the changes in this PR.
✅ Pre-merge checklist
✅ Post-merge checklist