Present references in a table #217

jan-cerny · 2023-11-03T14:55:06Z

We will sort and show the references grouped by reference target. The well-known references will be labeled. This makes the "References" section of rule detail actually useful.

For example:

Fixes: #154

pep8speaks · 2023-11-03T14:55:13Z

Hello @jan-cerny! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2023-12-06 12:48:06 UTC

We will sort and show the references grouped by reference target. The well-known references will be labeled. This makes the "References" section of rule detail actually useful. Fixes: OpenSCAP#154

evgenyz · 2023-11-22T15:46:23Z

It really feels like the code (and developers' personal preferences) is asking for the line length limit to be 120 instead of 100.

jan-cerny · 2023-11-22T16:10:45Z

Not really. All the lines that are flagged as too long are in the list of URLs (KNOWN_REFERENCES) because the URLs are long. I think we can waive this occurrence as a special case. I think it's more useful to keep the URLs as they are instead of splitting them by a backlash because if they are kept as they are they can be easily identified by grep when a need to update them arise.

Honny1

I like the new style of references, but I have some ideas for improvements.

Honny1 · 2023-11-27T18:15:10Z

openscap_report/scap_results_parser/parsers/rule_parser.py

 from dataclasses import replace

 from ..data_structures import Identifier, Reference, Rule, RuleWarning
 from ..namespaces import NAMESPACES
 from .full_text_parser import FullTextParser
 from .remediation_parser import RemediationParser

+KNOWN_REFERENCES = {


Please add comments to the code to turn off long-line warnings for this constant.

I have add the comments.

Pylint is not happy. Please use # pylint: disable=line-too-long before the constant definition and # pylint: enable=line-too-long after the constant definition. You can check this with the tox -e code_style command.

Honny1 · 2023-11-27T18:29:37Z

openscap_report/scap_results_parser/parsers/rule_parser.py

-            references.append(Reference(referenc.get("href"), referenc.text))
-        return references
+        for url, ref_ids in url_to_ref_ids.items():
+            name = KNOWN_REFERENCES.get(url, url)


I would suggest creating a default value for the name if the href attribute is an empty string.
For example, reference SRG-OS-000003-VMM-000030 of the xccdf_org.ssgproject.content_rule_account_disable_post_pw_expiration rule.

Nice catch! You're right, the href attribute of the reference element is optional.

I have add code that accounts for this situation.

However, this situation shouldn't happen in our content. The reference without href makes little sense. Do you have a specific SCAP content or its version where this happens?

I checked the latest content and all the href attributes are not empty strings.

I found this in:
SSG: [0, 1, 63]
Rule: xccdf_org.ssgproject.content_rule_account_disable_post_pw_expiration
Reference: SRG-OS-000003-VMM-000030

Honny1 · 2023-11-27T18:46:38Z

openscap_report/report_generators/html_templates/rule_detail.html

-                    <div class="pf-l-flex pf-m-column"><div class="pf-l-flex__item">
-                    <p class="pf-c-table__text">
-                    <div>
+                    <table>


Please create a responsive table using CSS classes from Patternfly. Missing CCS classes cause a grey bar. See image.

I think that the rows of the internal table with references should span the full width of the table cell with the OVAL definition.

Example rule: xccdf_org.ssgproject.content_rule_account_unique_name

I have add some class name that I have seen around. I'm not sure if it's correct because I don't have any experience with that. Please check this and point me to some resources.

Here is Patternfly documentation. Looks good to me.

These lines are too long according to the PEP8. But, I think we can waive this occurrence as a special case. I think it's more useful to keep the URLs as they are instead of splitting them by a backlash because if they are kept as they are they can be easily identified by grep when a need to update them arise.

The `href` attribute of the `reference` element is optional according to the XCCDF specification. This commit solves the situation when the `href` attribute is not set. In this situation, the reference source is unknown, we don't know where this reference points to so we will put in in a new "unknown" reference category to be displayed in the HTML report.

Honny1 · 2023-11-28T15:25:09Z

openscap_report/scap_results_parser/parsers/rule_parser.py

 from dataclasses import replace

 from ..data_structures import Identifier, Reference, Rule, RuleWarning
 from ..namespaces import NAMESPACES
 from .full_text_parser import FullTextParser
 from .remediation_parser import RemediationParser

+KNOWN_REFERENCES = {


Pylint is not happy. Please use # pylint: disable=line-too-long before the constant definition and # pylint: enable=line-too-long after the constant definition. You can check this with the tox -e code_style command.

jan-cerny · 2023-11-30T08:22:58Z

I have add comments for the pylint tool

Honny1 · 2023-11-30T11:52:26Z

openscap_report/scap_results_parser/parsers/rule_parser.py

+        url_to_ref_ids = collections.defaultdict(list)
+        for reference_el in rule.findall(".//xccdf:reference", NAMESPACES):
+            url = reference_el.get("href")
+            if url is None:


Here is a little catch. I found out that in the reference element is present the href attribute but with an empty string so it looks like this: <reference href="">Example</reference>. So in a variable url is a present empty string. Please add a check for the empty string to the if condition.

jan-cerny · 2023-12-06T12:52:36Z

I have extend the condition to account empty strings in href.

Honny1

I like the new presentation style of references. I have generated a report and checked the new presentation of references. I haven't noticed any problem.

Present references in a table

510a456

We will sort and show the references grouped by reference target. The well-known references will be labeled. This makes the "References" section of rule detail actually useful. Fixes: OpenSCAP#154

jan-cerny force-pushed the references branch from 5ee0c4f to 510a456 Compare November 3, 2023 16:06

jan-cerny requested a review from Honny1 November 27, 2023 12:56

Honny1 requested changes Nov 27, 2023

View reviewed changes

jan-cerny added 3 commits November 28, 2023 10:30

Add special classes to table element

e3b1852

Honny1 self-requested a review November 28, 2023 15:18

Honny1 requested changes Nov 28, 2023

View reviewed changes

Add comments for the pylint tool

5fb4271

Honny1 requested changes Nov 30, 2023

View reviewed changes

Extend the condition to account empty strings in href

74218c0

jan-cerny assigned Honny1 Dec 6, 2023

jan-cerny requested a review from Honny1 December 6, 2023 12:52

Honny1 approved these changes Dec 7, 2023

View reviewed changes

Honny1 merged commit dc74e88 into OpenSCAP:main Dec 7, 2023
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Present references in a table #217

Present references in a table #217

jan-cerny commented Nov 3, 2023

pep8speaks commented Nov 3, 2023 •

edited

evgenyz commented Nov 22, 2023

jan-cerny commented Nov 22, 2023

Honny1 left a comment

Honny1 Nov 27, 2023

jan-cerny Nov 28, 2023

Honny1 Nov 28, 2023

Honny1 Nov 27, 2023

jan-cerny Nov 28, 2023

Honny1 Nov 28, 2023

Honny1 Nov 27, 2023

jan-cerny Nov 28, 2023

Honny1 Nov 28, 2023

Honny1 Nov 28, 2023

jan-cerny commented Nov 30, 2023

Honny1 Nov 30, 2023

jan-cerny commented Dec 6, 2023

Honny1 left a comment

Present references in a table #217

Present references in a table #217

Conversation

jan-cerny commented Nov 3, 2023

pep8speaks commented Nov 3, 2023 • edited

Comment last updated at 2023-12-06 12:48:06 UTC

evgenyz commented Nov 22, 2023

jan-cerny commented Nov 22, 2023

Honny1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jan-cerny commented Nov 30, 2023

Choose a reason for hiding this comment

jan-cerny commented Dec 6, 2023

Honny1 left a comment

Choose a reason for hiding this comment

pep8speaks commented Nov 3, 2023 •

edited