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

[Transitive's in Solution] Vulnerability indicators in solution level details pane for top-level packages #6010

Conversation

martinrrm
Copy link
Contributor

@martinrrm martinrrm commented Sep 4, 2024

Bug

Fixes: NuGet/Home#13760

Description

This PR enables calculating the vulnerabilities for all the packages that we have a metadata, in the previous PR for this feature we allowed the PackageItemViewModel to have data for all the installed versions in the Solution and store a copy of that metadata.

Vulnerability indicators are also being added to the Solution Detail Pane so users are able to identify vulnerable installed packages more easily.

image

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests, missing tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@martinrrm martinrrm marked this pull request as ready for review September 4, 2024 22:36
@martinrrm martinrrm requested a review from a team as a code owner September 4, 2024 22:36
private IEnumerable<NuGetVersion> _vulnerableVersions = [];
public IEnumerable<NuGetVersion> VulnerableVersions
private Dictionary<NuGetVersion, int> _vulnerableVersions = [];
public Dictionary<NuGetVersion, int> VulnerableVersions
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This were mistakenly added in the previous PR and were never used.

Copy link
Contributor

@jebriede jebriede left a comment

Choose a reason for hiding this comment

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

Looks good so far! Requesting a few changes.

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.

Nice! Thanks for doing a little refactoring to make the converter more generic and updating tests!
Left some minor points that you could address before going to dev.

@jebriede jebriede self-requested a review September 10, 2024 22:35
Copy link
Contributor

@jebriede jebriede left a comment

Choose a reason for hiding this comment

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

Approving with suggestion. Just one change then 🚀!

Comment on lines 905 to 912
if (maxSeverity > -1)
{
VulnerableVersions.Add(version, maxSeverity);
}

VulnerabilityMaxSeverity = Math.Max(VulnerabilityMaxSeverity, maxSeverity);

OnPropertyChanged(nameof(Status));
Copy link
Contributor

Choose a reason for hiding this comment

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

For SetVulnerabilityMaxSeverity, would we want to only calculate and set the VulnerabilityMaxSeverity and call OnPropertyChanged if the maxSeverity is > -1?

@martinrrm martinrrm force-pushed the dev-martinrrm-vulnerabilities-top-level-solution branch from cc328a9 to 949ba9f Compare September 17, 2024 22:02
@martinrrm martinrrm merged commit 34cc432 into feature-TransitivePkgsInSolutionPMUI Sep 18, 2024
28 checks passed
@martinrrm martinrrm deleted the dev-martinrrm-vulnerabilities-top-level-solution branch September 18, 2024 15:50
martinrrm added a commit that referenced this pull request Sep 23, 2024
… details pane for top-level packages (#6010)

* Vulnerability indicators for top level in solution
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.

4 participants