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

make safety table formats look better #479

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Conversation

ahouseholder
Copy link
Contributor

@ahouseholder ahouseholder commented Feb 19, 2024

This PR modifies the text description strings in src/ssvc/decision_points/safety_impact.py, the python module for the safety impact decision point, to improve readability when presented in tabular format in the web site. A screenshot of the new look is given below

Note: I did not bump the version number since 2.0.0 is already a new version of the decision point in the current release (based on #439) so it's still malleable until we tag the release later.

Before:
Screenshot 2024-02-19 at 1 14 09 PM

After:
Screenshot 2024-02-19 at 1 09 08 PM

I think this is an improvement in the HTML presentation.

Caveat: This change makes the json uglier

However, because the json version is just concatenating the raw data strings from the python dataclass, that means the description field now contains embedded HTML (e.g., <br/><br/>) and some comparatively unobtrusive markdown (e.g., - *Physical harm*: instead of Physical harm:) whereas previously it was entirely unformatted text.

The <br/>s are necessary to make the markdown-to-html rendering recognize the bulleted list within a markdown table cell. If the <br/>s in the description are too much, we could remove them and the bullets and consider just using either italics or bold (e.g., *Physical harm*: ...Physical harm:)

I've included a snippet of the JSON with embedded HTML below:

{
  "namespace": "ssvc",
  "version": "2.0.0",
  "key": "SI",
  "name": "Safety Impact",
  "description": "The safety impact of the vulnerability. (based on IEC 61508)",
  "values": [
    {
      "key": "N",
      "name": "Negligible",
      "description": "Any one or more of these conditions hold.<br/><br/>- *Physical harm*: Minor injuries at worst (IEC 61508 Negligible).<br/>- *Operator resiliency*: Requires action by system operator to maintain safe system state as a result of exploitation of the vulnerability where operator actions would be well within expected operator abilities; OR causes a minor occupational safety hazard.<br/>- *System resiliency*: Small reduction in built-in system safety margins; OR small reduction in system functional capabilities that support safe operation.<br/>- *Environment*: Minor externalities (property damage, environmental damage, etc.) imposed on other parties.<br/>- *Financial*: Financial losses, which are not readily absorbable, to multiple persons.<br/>- *Psychological*: Emotional or psychological harm, sufficient to be cause for counselling or therapy, to multiple persons."
    },
...

which renders as


Any one or more of these conditions hold.

- Physical harm: Minor injuries at worst (IEC 61508 Negligible).
- Operator resiliency: Requires action by system operator to maintain safe system state as a result of exploitation of the vulnerability where operator actions would be well within expected operator abilities; OR causes a minor occupational safety hazard.
- System resiliency: Small reduction in built-in system safety margins; OR small reduction in system functional capabilities that support safe operation.
- Environment: Minor externalities (property damage, environmental damage, etc.) imposed on other parties.
- Financial: Financial losses, which are not readily absorbable, to multiple persons.
- Psychological: Emotional or psychological harm, sufficient to be cause for counselling or therapy, to multiple persons.


The alternative with no HTML and only bold markdown would look like:

{
  "namespace": "ssvc",
  "version": "2.0.0",
  "key": "SI",
  "name": "Safety Impact",
  "description": "The safety impact of the vulnerability. (based on IEC 61508)",
  "values": [
    {
      "key": "N",
      "name": "Negligible",
      "description": "Any one or more of these conditions hold. **Physical harm**: Minor injuries at worst (IEC 61508 Negligible). **Operator resiliency**: Requires action by system operator to maintain safe system state as a result of exploitation of the vulnerability where operator actions would be well within expected operator abilities; OR causes a minor occupational safety hazard. **System resiliency**: Small reduction in built-in system safety margins; OR small reduction in system functional capabilities that support safe operation. **Environment**: Minor externalities (property damage, environmental damage, etc.) imposed on other parties. **Financial**: Financial losses, which are not readily absorbable, to multiple persons. **Psychological**: Emotional or psychological harm, sufficient to be cause for counselling or therapy, to multiple persons."
    },

and would render as:


Any one or more of these conditions hold. Physical harm: Minor injuries at worst (IEC 61508 Negligible). Operator resiliency: Requires action by system operator to maintain safe system state as a result of exploitation of the vulnerability where operator actions would be well within expected operator abilities; OR causes a minor occupational safety hazard. System resiliency: Small reduction in built-in system safety margins; OR small reduction in system functional capabilities that support safe operation. Environment: Minor externalities (property damage, environmental damage, etc.) imposed on other parties. Financial: Financial losses, which are not readily absorbable, to multiple persons. Psychological: Emotional or psychological harm, sufficient to be cause for counselling or therapy, to multiple persons.


It appears that for now we've got a choice1 of either maintaining "pure" text-only json descriptions or making it look good when rendered as HTML.

Footnotes

  1. There are other more involved options, which seem likely to include significantly more code changes than the above proposals, which are limited to modifying text strings in the python (and then regenerating the downstream content). For example, we could decide that we want to represent two versions of the description with parallel fields like description: for pure text or description_md for a markdown version. This feels kludgy to me. Alternatively, we could consider modifying the description field just before rendering out to json such that it strips out any embedded HTML or markdown from the resulting json blob. That's not a terrible option to consider, but it's at least a few hours more work than this PR was, so I'm hesitant to start that unless we decide that this PR is unworkable.

@ahouseholder ahouseholder added documentation Improvements or additions to documentation bug Something isn't working enhancement New feature or request labels Feb 19, 2024
@ahouseholder ahouseholder added this to the SSVC 202403 milestone Feb 19, 2024
@ahouseholder
Copy link
Contributor Author

ahouseholder commented Feb 19, 2024

If we were to go the route mentioned at the end of the footnote in the description, this tip on Stack Overflow could be relevant to the implementation: https://stackoverflow.com/questions/753052/strip-html-from-strings-in-python

We also have the option to merge this PR as-is and then leave an issue behind to implement the html stripping from the json format as a future improvement.

Copy link
Contributor

@cgyarbrough cgyarbrough left a comment

Choose a reason for hiding this comment

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

I think the readability is much better in the latest improvement. I get the JSON will be messy, but the output is much clearer and the pagination is much more intuitive. I approve this change.

@ahouseholder ahouseholder merged commit 054789b into main Feb 20, 2024
4 checks passed
@ahouseholder ahouseholder deleted the 380-safety-table-fix branch February 20, 2024 18:56
@ahouseholder
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Revise Safety Impact decision point for better integration with generated documentation
2 participants