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

CVEs landing page redesign #13382

Merged
merged 10 commits into from
Dec 13, 2023
Merged

Conversation

mtruj013
Copy link
Contributor

@mtruj013 mtruj013 commented Nov 30, 2023

Done

Future work

The releases links will be handled in a separate PR, as will the advanced search link.

QA

Issue / Card

Fixes https://warthogs.atlassian.net/browse/WD-5145

@webteam-app
Copy link

Demo starting at https://ubuntu-com-13382.demos.haus

Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Merging #13382 (8c26568) into cve-overhaul (895bfb3) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@              Coverage Diff              @@
##           cve-overhaul   #13382   +/-   ##
=============================================
  Coverage         74.41%   74.41%           
=============================================
  Files               107      107           
  Lines              2838     2838           
  Branches            946      946           
=============================================
  Hits               2112     2112           
  Misses              702      702           
  Partials             24       24           

@mtruj013 mtruj013 changed the base branch from main to cve-overhaul December 5, 2023 08:41
@mtruj013 mtruj013 changed the title WIP - CVEs landing page redesign CVEs landing page redesign Dec 5, 2023
@juanruitina
Copy link
Contributor

juanruitina commented Dec 11, 2023

  • Minor visual comment: please make the ruler above "By Ubuntu release" the same width as the row below (9 columns) as per the mockup.
  • Forgot to mention: please re-add the list of blog posts at the end of the page, similar to what we have live now.

Other than that, great work. Love how the cards downscale now on smaller viewports :)

@petesfrench
Copy link
Contributor

In the design, on medium screens, the button remains to the right of the search input:
image

Copy link
Contributor

@petesfrench petesfrench left a comment

Choose a reason for hiding this comment

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

Couple minor comments but looks good to me

</div>
<div class="col-3 col-medium-2">
{% set status = cve.summarized_status %}
{% if status.name == "Some fixed" %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are the 'status.name' values set? Is it possible they could include variations of capitalization, trailing white-space etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're set in the get_summarized_statuses function in the helpers.py file. The names are hard coded depending on which check is valid for the cve as a whole, so no white spaces or variations in capitalization, eg from line 55:


                cve["summarized_status"] = {
                    "name" : "Some fixed",
                    "fixed_count" : total_fixed,
                    "total_count" : total_fixable
                }

{% elif loop.index == 6 %}
and {{ cve.packages | length - 5 }} more
{% endif %}
{% elif cve.packages | length < 6 %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{% elif cve.packages | length < 6 %}
{% else %}

You can assume length is small than 6 in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes lol

<li class="p-list__item"><a href="/blog/running-openssl-1-1-1-after-eol-with-ubuntu-pro">Running OpenSSL 1.1.1 after EOL? Stay secure with Ubuntu Pro.</a></li>
<li class="p-list__item"><a href="/blog/ubuntu-23-10-restricted-unprivileged-user-namespaces">Restricted unprivileged user namespaces are coming to Ubuntu 23.10</a></li>
<li class="p-list__item"><a href="/blog/securing-open-source-software-dependencies-in-the-public-cloud">Securing open source software dependencies in the public cloud</a></li>

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 246 to 247
</div>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the indentation is a little off here, might be missing a closing tag

for package in cve["packages"]:
# Check if all statuses for all packages are the same, excluding DNE and empty data
if (len({d["status"] for d in package["statuses"] if d["status"] not in {"DNE", ""}}) == 1):
# todo: set status
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# todo: set status
# todo: set status

do you need this still?

@mtruj013 mtruj013 merged commit 45624ba into canonical:cve-overhaul Dec 13, 2023
15 of 16 checks passed
@mtruj013 mtruj013 deleted the cves-landing-page branch December 13, 2023 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants