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

reports: HTML add sequence support #5918

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kingthorin
Copy link
Member

@kingthorin kingthorin commented Nov 13, 2024

Overview

TBC

Related Issues

Checklist

  • Update help
  • Update changelog
  • Run ./gradlew spotlessApply for code formatting
  • Write tests
  • Check code coverage
  • Sign-off commits
  • Squash commits
  • Use a descriptive title

@kingthorin kingthorin force-pushed the rpt-html-seq branch 2 times, most recently from 5fd3c37 to 476c7ed Compare November 13, 2024 20:25
@kingthorin
Copy link
Member Author

Probably needs further tests implemented.
Raised primarily for initial feedback.

Signed-off-by: kingthorin <kingthorin@users.noreply.github.com>
@kingthorin
Copy link
Member Author

I'm quite confused about the needed white space changes and why the report files seem so inconsistent on spaces vs tabs. Especially when spotless seems to impact them as well.

Copy link
Member

@psiinon psiinon left a comment

Choose a reason for hiding this comment

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

Great start!

th:if="${reportData.reportObjects.get('seqAScanData') != null}">
<h3 th:text="#{report.sequences.summary.name}">Summary of
Sequences</h3>
<sup>For each step: result (Pass/Fail) - risk (of highest
Copy link
Member

Choose a reason for hiding this comment

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

Needs to be i18ned

@@ -518,6 +518,116 @@ <h3 th:text="#{report.alerts.detail}">Alert Detail</h3>
<div class="spacer"></div>
</th:block>
</th:block>

<th:block th:if="${reportData.isIncludeSection('sequencedetails')}">
<th:block
Copy link
Member

Choose a reason for hiding this comment

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

I would have the summary block much higher, maybe after "Summary of Alerts" ?

<th:block th:each="step, stepState: ${seq.getSteps()}">
<td th:class="(${step.isPass()} ? 'pass' : 'fail')"
th:with="riskLvl=(${step.getHighestAlert()})"><span
th:text="(${step.isPass()} ? 'Pass' : 'Fail')">isPass</span> <span
Copy link
Member

Choose a reason for hiding this comment

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

If we use text it should be i18n but tbh I'd prefer not to have any text. We could have tick / cross if the colour isnt sufficient.
How about also having a seq id which links to the relevant step below?

<th:block
th:each="seq, seqState: ${reportData.reportObjects.get('seqAScanData').seqData}">
<table class="alerts">
<tr>
Copy link
Member

Choose a reason for hiding this comment

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

For long sequences we should spread this over multiple lines, eg if a sequence has 60 steps this wouldnt look good?

<th:block th:each="step, stepState: ${seq.getSteps()}">

<h4
th:text="'Step ' + ${stepState.index + 1} + ': ' + ${step.getOriginalMsg().getRequestHeader().getMethod()} + ' - ' + ${step.getOriginalMsg().getRequestHeader().getURI().toString()}">stepDesc</h4>
Copy link
Member

Choose a reason for hiding this comment

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

Could be a real link?

</div>

<table>
<th colspan="2" th:text="#{report.sequences.step.original}">Original</th>
Copy link
Member

Choose a reason for hiding this comment

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

This is black on dark grey in light mode, so v difficult to read.

@psiinon
Copy link
Member

psiinon commented Nov 14, 2024

there were a few other texts that should be i18n'ed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants