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

Showing warning icon in packages list items when package version has one or more vulnerabilities #4161

Merged
merged 7 commits into from
Jul 23, 2021

Conversation

jebriede
Copy link
Contributor

@jebriede jebriede commented Jul 23, 2021

Bug

Fixes: NuGet/Home#10983

Regression? Last working version: N/A

Description

Added logic to package warning icon to show when package has vulnerabilities, added a tooltip to combine vulnerability and deprecation data when applicable, and added code to reload vulnerability data for package item view model.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@jebriede jebriede requested a review from a team as a code owner July 23, 2021 00:14
Copy link
Contributor

@donnie-msft donnie-msft left a comment

Choose a reason for hiding this comment

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

First pass...

@dominoFire
Copy link
Contributor

Can you add an UI screenshot showcasing your changes?

Can you run an a11y test pass using https://accessibilityinsights.io/ to ensure not breaking any a11y guidelines?

Copy link
Contributor

@dominoFire dominoFire left a comment

Choose a reason for hiding this comment

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

Some comments.

@jebriede
Copy link
Contributor Author

jebriede commented Jul 23, 2021

Can you add an UI screenshot showcasing your changes?

Can you run an a11y test pass using https://accessibilityinsights.io/ to ensure not breaking any a11y guidelines?

@dominoFire
image

Accessibility Insights was run and came up clean.

@kartheekp-ms
Copy link
Contributor

kartheekp-ms commented Jul 23, 2021

@jebriede - In the screenshot, you have linked in https://github.com/NuGet/NuGet.Client/pull/4161/files#issuecomment-885871704 comment, I don't see a warning icon next to Installed text in the tab menu. Is that expected or that work is part of another issue?

@kartheekp-ms
Copy link
Contributor

I am sorry If I am missing anything.

  1. Is this PR specific to Install tab? How about other tabs such as Browse or Update tab. For e.g. If I am browsing a package in the Browse tab, I don't see any warning icon when the selected package version has a vulnerability.
  2. I opened your feature branch and loaded it in the experimental instance of VS, occasionally I don't see a warning icon next to the package version in the install tab.
    image

@jebriede
Copy link
Contributor Author

@jebriede - In the screenshot, you have linked in https://github.com/NuGet/NuGet.Client/pull/4161/files#issuecomment-885871704 comment, I don't see a warning icon next to Installed text in the tab menu. Is that expected or that work is part of another issue?

It's part of another issue: NuGet/Home#10982

@donnie-msft
Copy link
Contributor

donnie-msft commented Jul 23, 2021

@jebriede - In the screenshot, you have linked in https://github.com/NuGet/NuGet.Client/pull/4161/files#issuecomment-885871704 comment, I don't see a warning icon next to Installed text in the tab menu. Is that expected or that work is part of another issue?

@kartheekp-ms I also thought the tab warning icon should be shown, if I recall the spec correctly. Is this a bug or is it intentionally missing?
EDIT: I see there's now a link to a separate issue for this. Assuming PM was okay with that, then the screenshot is OK.

@jebriede
Copy link
Contributor Author

I am sorry If I am missing anything.

  1. Is this PR specific to Install tab? How about other tabs such as Browse or Update tab. For e.g. If I am browsing a package in the Browse tab, I don't see any warning icon when the selected package version has a vulnerability.
  2. I opened your feature branch and loaded it in the experimental instance of VS, occasionally I don't see a warning icon next to the package version in the install tab.
    image

I'm taking a look right now.

@jebriede - In the screenshot, you have linked in https://github.com/NuGet/NuGet.Client/pull/4161/files#issuecomment-885871704 comment, I don't see a warning icon next to Installed text in the tab menu. Is that expected or that work is part of another issue?

@kartheekp-ms I also thought the tab warning icon should be shown, if I recall the spec correctly. Is this a bug or is it intentionally missing?

@donnie-msft please see #4161 (comment)

@jebriede
Copy link
Contributor Author

jebriede commented Jul 23, 2021

I am sorry If I am missing anything.

  1. Is this PR specific to Install tab? How about other tabs such as Browse or Update tab. For e.g. If I am browsing a package in the Browse tab, I don't see any warning icon when the selected package version has a vulnerability.
  2. I opened your feature branch and loaded it in the experimental instance of VS, occasionally I don't see a warning icon next to the package version in the install tab.
    image

@kartheekp-ms @dominoFire
Thanks again for noticing this. It was missing a OnPropertyChanged event invocation and after fixing, @dominoFire, you, and I tested this thoroughly offline together and it looks good now. Thanks!

@jebriede jebriede merged commit 747f71d into dev Jul 23, 2021
@jebriede jebriede deleted the dev-jebriede-VulnerabilityWarningInPackageList branch July 23, 2021 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants