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

Update the title to display summary instead of id #2137

Conversation

zahraaalizadeh
Copy link
Collaborator

@zahraaalizadeh zahraaalizadeh commented Apr 26, 2024

Prior to this change, in the vulnerability page, id of the vulnerability was displayed as title.

This change updates the title to display the human readable summary for the title, and adds ID as an data description.

resolves #2078

example 1 - Before:
before1

After:
after1

Example 2 - Before:
Screenshot 2024-04-26 at 3 40 44 pm

After:
after2

@oliverchang
Copy link
Collaborator

Thanks! Could we instead add the summary to the main part of the page, above "details" ?

IMO the title should remain the vulnerability ID. summary can get pretty long.

@zahraaalizadeh
Copy link
Collaborator Author

@oliverchang Sometimes details include a Summary section too. Adding a field like Summary or Brief summary might seem duplicated? WDYT?

@oliverchang
Copy link
Collaborator

@oliverchang Sometimes details include a Summary section too. Adding a field like Summary or Brief summary might seem duplicated? WDYT?

I think that's OK. We should accurately render what's in the source JSON.

@zahraaalizadeh zahraaalizadeh force-pushed the issue-2078/replace-vulnerability-id-with-summary branch from 190476b to d4bbecd Compare May 1, 2024 08:13
@zahraaalizadeh
Copy link
Collaborator Author

zahraaalizadeh commented May 1, 2024

@oliverchang I updated it as per suggestion. Now there's a Summary field that comes before Details.

@another-rex
Copy link
Contributor

It is starting to look a bit busy, can you add a gap between summary and the Modified field above it (similar to the gap between references and details). This way we have 3 mini sections separated with some padding:

  • the metadata
  • the text (summary and description)
  • references

Also, for some entries summary is empty, in those cases I think we should just hide the summary field (though that might be confusing, maybe a greyed out [none] or something similar could be better)

Prior to this change, if there were no summary provided
in the schema json it displayed nothing, which is a bit
confusing.

This change shows a grayout [none] when the summary is
not included in the json.
@zahraaalizadeh
Copy link
Collaborator Author

@another-rex Thanks for your suggestions, I applied them in dc46044 and
566aaec.

@zahraaalizadeh zahraaalizadeh requested review from andrewpollock, oliverchang and another-rex and removed request for andrewpollock May 7, 2024 05:35
@another-rex
Copy link
Contributor

/gcbrun

@zahraaalizadeh zahraaalizadeh self-assigned this May 8, 2024
@oliverchang oliverchang merged commit 149934b into google:master May 10, 2024
10 of 11 checks passed
@hogo6002
Copy link
Contributor

It seems not all entries have summary field. Example: https://test.osv.dev/vulnerability/CVE-2024-34397 Screenshot 2024-05-10 at 2 58 54 PM

@G-Rath G-Rath deleted the issue-2078/replace-vulnerability-id-with-summary branch May 10, 2024 05:06
@andrewpollock
Copy link
Contributor

It seems not all entries have summary field

This is definitely to be expected for CVEs, which don't have clean correlation between OSV's summary versus details fields. I think the current treatment isn't too bad. An improvement would be to omit the Summary field entirely from the per-vulnerability page.

The vulnerability listing page is deferring to the full record in the absence of a summary...

@zahraaalizadeh
Copy link
Collaborator Author

@hogo6002 Displaying the summary as grayout [none] is intentional based on this comment.

Please let me know if you think we could handle it better in the vulnerability page.

@andrewpollock
Copy link
Contributor

An improvement would be to omit the Summary field entirely from the per-vulnerability page.

Apologies, that was a poorly formed statement... I meant to say it could be omitted when it is empty/undefined, but that inconsistency could also be visually jarring and may be worse than what we have now.

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.

Details Page Does Not Display Vulnerability Summary
5 participants