Transitive packages and vulnerability indicators in Solution PM UI #6044

martinrrm · 2024-09-23T16:48:56Z

Bug

Description

This feature brings to the Solution-level PM UI the Transitive Packages experience that we previously added to the Project-level PM UI. Additionally, we added new UX to the projects / versions grid in the Details Pane to show the package level of the selected package, as well as any vulnerabilities.

Accessibility localization PR #6051

Accessibility

Details pane

It has a minimum size so the columns will always look "good"

PR Checklist

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

…t and installed tab vulnerability warning (#5994) First step to show transitive packages in Solution PM UI and show vulnerability warnings in Solution PM UI Installed Tab

…level (#5998) * Get all the installed packages metadata in the Solution level

…n tooltip (#6021)

… details pane for top-level packages (#6010) * Vulnerability indicators for top level in solution

…#6034) * Show transitive's data in Details Pane

…ransitive in the solution (#6040)

#6043) * Update vulnerability warning tooltips

kartheekp-ms · 2024-09-24T17:47:46Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Converters/IsGreaterThanToVisibilityConverter.cs

@@ -9,16 +9,20 @@

 namespace NuGet.PackageManagement.UI
 {
-    public class DownloadCountToVisibilityConverter : IValueConverter
+    public class IsGreaterThanToVisibilityConverter : IValueConverter


For some reason, I am having a hard time understanding the name of the class :) If my understanding is correct, the visibility of some control in the UI is dependent on the count. If the count is greater than the some threshold the control will be enabled. Is that correct? Maybe we can try VS Copilot's smart rename feature.

You are correct, @kartheekp-ms. The Converter checks if the value is greater than a threshold to determine visibility. Perhaps GreaterThanThresholdToVisibilityConverter would be a better name? Thoughts?

donnie-msft

Great to see this being added, nice work!
I'd like to see some accessibility testing to ensure we're not introducing a11y bugs for the new UI elements (https://github.com/NuGet/NuGet.Client/blob/dev/docs/ui-guidelines.md#accessibility-testing).

My main UI behavior concern is whether the calculations for columns and scrolling works well at different resolutions.

More unit tests to cover more of the package feeds and Services logic changes would be good as well.

donnie-msft · 2024-09-24T17:53:20Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/SolutionView.xaml

+                  <TextBlock
+                    VerticalAlignment="Bottom"
+                    Text="{Binding InstalledVersion, Converter={StaticResource VersionToStringConverter}}" />
+                  <imaging:CrispImage


Could the warning icon be put before the installed version? This would reduce visual clutter by aligning the icons. Also, it's more consistent with the positioning in the packages list (warning icon on the left of installed version).

rough mockup (left is from this PR, right is my suggestion):

When I presented this to the UX board the warning icons where on the left* side, but their suggestion was to move to the right* side so it matches the Installed tab warning icon at the top

@martinrrm do you mean vice versa? The icons are currently on the right side and I believe when you presented this initially you had them on the left and the advice was to put the warning icons on the right side. Please correct me if I'm wrong.

Mb, I corrected the message, but yeah they suggested to change it so both installed "headers" have the icon at the same place, and have them mixed (rows with icon to the left and header to the right) would look weird

Ok, in that case, the Installed tab could also have probably been changed. We still have 2 different approaches, again, the packages list shows on the left.

Thanks Donnie, that a great suggestion, we have been getting UX approval for specific features and not considering the PM UI consistency. We should probably get back to the UX board and look at the whole PM UI. During my last UX meeting we discussed a lot of opportunities we could do improve the PM UI experience, I'll send a link to the meeting recording.

Look forward to seeing that! In the meantime, if this is "ok" with UX board, then I think it could be easily changed later and not be too jarring to customers.

Not for this PR, but could it make sense to move the vulnerability information to a column of its own? This would help align the icons and we could also add additional icons to denote the max severity of the vulnerabilities.

✅ - No Vul
⚠️ - Low/Moderate/High
🔴 - Critical
❔ - Unkown

src/NuGet.Clients/NuGet.PackageManagement.UI/Xamls/SolutionView.xaml.cs

...ients/NuGet.PackageManagement.VisualStudio/PackageFeeds/InstalledAndTransitivePackageFeed.cs

donnie-msft · 2024-09-24T18:21:27Z

...ts/NuGet.PackageManagement.VisualStudio.Test/Feeds/InstalledAndTransitivePackageFeedTests.cs

                // Returns latest transitive package version
-                e => Assert.Equal(new PackageIdentity("transitivePackageC", new NuGetVersion("0.0.2")), e.Identity));
+                e => Assert.Equal(new PackageIdentity("transitivePackageC", new NuGetVersion("0.0.2")), e.Identity),
+                // Returns all the transtive packages that are not the latest verion


Could the comment be expanded to explain why this assertion is made?

Suggested change

// Returns all the transtive packages that are not the latest verion

// Returns all the transitive packages that are not the latest version because.....

So this PackageCollection is sorted, first we display the Installed Packages and then the TransitivePackages, and in each "group" they are sorted by latest.

In this test I'm trying to test that they are sorted as expected

If they are sorted, why is transitivePackageC 0.0.2 asserted before version 0.0.1 ?

Are you planning to fix the typos I suggested?

donnie-msft · 2024-09-24T18:51:04Z

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageItemViewModel.cs

+            set
+            {
+                _vulnerableVersions = value;
+                OnPropertyChanged(nameof(VulnerableVersions));


Is this necessary? I don't see where the entire dictionary is being replaced, so I don't think it's being used.

Good point. I think we can delete the Setter altogether since the Dictionary is instantiated and only added to and this setter is never used. Then, the private _vulnerableVersions can be readonly.

@donnie-msft I addressed this now in the latest commit. I moved the OnPropertyChanged to a place where we add items to the VulnerableVersions dictionary, and also added an OnPropertyChanged for the VulnerableVersionsString which was previously missing. I removed the setter here because it was not being used. Thank you!

jebriede · 2024-09-24T21:38:18Z

src/NuGet.Clients/NuGet.PackageManagement.UI/PackageItemLoader.cs

-                    {
-                        versionOverride = versionOverrides[0];
-                    }
+                    existingListItem.UpdateInstalledPackagesVulnerabilities(new PackageIdentity(packageId, packageVersion));


@martinrrm Am I remembering correctly that we changed this to accept metadataContextInfo.Identity rather than instantiate a new PackageIdentity? I'm wondering how this made its way back here.

I fixed this in the latest commit.

jgonz120 · 2024-09-24T22:45:11Z

src/NuGet.Clients/NuGet.PackageManagement.UI/Models/PackageSolutionDetailControlModel.cs

@@ -181,18 +211,20 @@ private async Task UpdateInstalledVersionsAsync(CancellationToken cancellationTo
        /// This method is called from several methods that are called from properties and LINQ queries
        /// It is likely not called more than once in an action.
        /// </summary>
-        private async Task<IPackageReferenceContextInfo> GetInstalledPackageAsync(
+        private async Task<(IPackageReferenceContextInfo TopLevelPackage, IPackageReferenceContextInfo TransitivePackage)> GetInstalledAndTransitivePackagesAsync(


Trying to make sure I'm understanding things here, this is returning the package information for a project and a specific packageId?

Is it possible for a single packageId to be both a toplevel and transitive package for a project? If not then I think this should be renamed to GetInstalledOrTransitivePackageAsync.

private async Task<(IPackageReferenceContextInfo Package, bool IsTransitive)> GetPackageContextAsync

@jgonz120 It's not possible for a package to be both top-level and transitive in a single project. It's only possible for a package to be top-level and transitive in a across the solution, i.e. top-level in one project, transitive in another.

@martinrrm I think the suggested rename is a good idea because it looks like we're getting package information for a project/package combo. Could you confirm this is what we're doing and what you think of the proposed rename?

I put this private async Task<(IPackageReferenceContextInfo Package, bool IsTransitive)> GetPackageContextAsync and didn't explain it.

If only one package will ever be returned we can simplify the return with a bool to indicate if the package found is transitive or top level.

Or even private async Task<(IPackageReferenceContextInfo Package, PackageLevel PackageLevel)> GetPackageContextAsync

@jgonz120: I created #6065 to incorporate your feedback. Please let me know if that's what you had in mind and leave any feedback on that PR. Thanks!

src/NuGet.Clients/NuGet.PackageManagement.UI/PackageInstallationInfo.cs

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageItemViewModel.cs

jgonz120 · 2024-09-24T23:21:57Z

...ients/NuGet.PackageManagement.VisualStudio/PackageFeeds/InstalledAndTransitivePackageFeed.cs

+            IEnumerable<PackageCollectionItem> transitivePkgsWithOrigins = GetTransitivePackagesWithOrigins(_transitivePackages);
+
+            // Get the metadata for the latest version of the installed packages and transitive packages
+            IEnumerable<PackageCollectionItem> installedPackagesLatest = GetLatestPackages(_installedPackages);


Nit: can this be updated to match the other variable name? latestInstalledPackages

jgonz120

Looks good to me!

…mprove localization of table headers (#6051)

kartheekp-ms · 2024-09-25T18:25:04Z

...t.Clients/NuGet.PackageManagement.UI/Converters/GreaterThanThresholdToVisibilityConverter.cs

                {
-                    return Visibility.Visible;
+                    long intValue = System.Convert.ToInt64(value, culture);


Convert.ToInt64() will throw an exception if the value is not a valid long value. The example in the doc has catch blocks for various exceptions this method can throw.

I'm guessing this is done so an object or a string can be converted to the correct value. Would it worth creating a bool TryConvertToInt64(obj, formatter, out long? convertedValue) method somewhere?

@kartheekp-ms thanks for the feedback. @jgonz120 I like this suggestion. I've got this ready and will push changes in a moment.

@kartheekp-ms @jgonz120 pushed an update that addresses this case and has a new test for invalid conversion scenarios.

...t.Clients/NuGet.PackageManagement.UI/Converters/GreaterThanThresholdToVisibilityConverter.cs

jgonz120 · 2024-09-25T18:45:21Z

...t.Clients/NuGet.PackageManagement.UI/Converters/GreaterThanThresholdToVisibilityConverter.cs

                {
-                    return Visibility.Visible;
+                    long intValue = System.Convert.ToInt64(value, culture);


I'm guessing this is done so an object or a string can be converted to the correct value. Would it worth creating a bool TryConvertToInt64(obj, formatter, out long? convertedValue) method somewhere?

…sion cases. - Only adding to VulnerableVersions dictionary if it does not already exist because the same version is installed in another project and only if it's added should we fire OnPropertyChanged.

kartheekp-ms · 2024-09-25T21:54:24Z

Looking at the screenshot mentioned in the PR description, I think Uninstall button should be invisible for transitive packages. Only Install button might be applicable because the install action promotes a transitive dependency to top-level dependency. I am unsure if this feedback is out of scope for this PR.

donnie-msft

Great work and can't wait to see how customers use this!
Left some questions and nits.

donnie-msft · 2024-09-25T20:58:37Z

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageItemViewModel.cs

+        private readonly Dictionary<NuGetVersion, int> _vulnerableVersions = [];
+        public Dictionary<NuGetVersion, int> VulnerableVersions => _vulnerableVersions;


Is anything actually listening to VulnerableVersions ? Maybe I'm missing it in a binding somewhere?

It's used here: https://github.com/NuGet/NuGet.Client/pull/6044/files#diff-5c15ee840d9063ec5f345761223159c7ecde0f058e0e0190e359adcfee761290R170

It was also supposed to be used in the XAML for the VulnerableVersions.Count but there's typo. I'm fixing the XAML binding now.

Fixing in: #6056

donnie-msft · 2024-09-25T20:58:53Z

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageItemViewModel.cs

+            }
+        }
+
+        private readonly Dictionary<NuGetVersion, int> _vulnerableVersions = [];


Is this field being used? I think it doesn't need both the field & property.

donnie-msft · 2024-09-25T21:01:25Z

src/NuGet.Clients/NuGet.PackageManagement.UI/ViewModels/PackageItemViewModel.cs

+                    OnPropertyChanged(nameof(VulnerableVersions));
+                    OnPropertyChanged(nameof(VulnerableVersionsString));


nit: Ideally, a CollectionChanged event could be used to fire the appropriate OnPropertyChanged events when the collection gets changed.

That's a really good point. This should be an observable set / dictionary that fires CollectionChanged when it gets updated, and we listen to that and fire any additional OnPropertyChanged as needed.

donnie-msft · 2024-09-25T21:40:23Z

...ts/NuGet.PackageManagement.VisualStudio.Test/Feeds/InstalledAndTransitivePackageFeedTests.cs

                // Returns latest transitive package version
-                e => Assert.Equal(new PackageIdentity("transitivePackageC", new NuGetVersion("0.0.2")), e.Identity));
+                e => Assert.Equal(new PackageIdentity("transitivePackageC", new NuGetVersion("0.0.2")), e.Identity),
+                // Returns all the transtive packages that are not the latest verion


If they are sorted, why is transitivePackageC 0.0.2 asserted before version 0.0.1 ?

Are you planning to fix the typos I suggested?

donnie-msft · 2024-09-25T22:02:39Z

src/NuGet.Clients/NuGet.PackageManagement.UI/PackageItemLoader.cs

@@ -262,85 +262,105 @@ public IEnumerable<PackageItemViewModel> GetCurrent()
                return Enumerable.Empty<PackageItemViewModel>();
            }

-            var listItemViewModels = new List<PackageItemViewModel>(capacity: _state.ItemsCount);
+            var listItemViewModels = new Dictionary<string, PackageItemViewModel>();


I'm trying to understand what the dictionary is being used for. Could someone explain how the same package would exist in _state.Results.PackageSearchItems multiple times? Ideally, a comment would be good here as well.

@donnie-msft the changes made here: https://github.com/NuGet/NuGet.Client/pull/6044/files#diff-b1b2dd9d0be30f84d9309d08db86156cff27b5df8e43b54c13e90bfa5fbf72f9R38-R43 are related to this.

Let's see if I can explain this in a way that makes sense in a comment. :-) (This is actually the main reason I did the live code review meeting to explain this part). The Solution PM UI needs to know about all the versions installed across the solution, in each project, not just the latest version. For the Project PM UI, this will always only be the latest version because of how restore works. But limiting it to just the latest doesn't work in the solution because we can have the same package installed at different versions in different projects and since one of those can be vulnerable, not just the latest, we need to know all of the package versions for each package. As such, the InstalledAndTransitivePackageFeed will now return all the package versions. However, we want to create a single PackageItemViewModel so it shows up only once in the packages list. We keep track of the PackageItemViewModels we've created for that Package Id (the string key) and retrieve the view model if it was already created, so we can add the additional versions to it.

I hope this explanation helps! I can probably explain it better in real time so if you have questions or want a deeper dive, please set up a quick meeting and we'll go over this in more detail!

jebriede · 2024-09-25T22:04:14Z

Looking at the screenshot mentioned in the PR description, I think Uninstall button should be invisible for transitive packages. Only Install button might be applicable because the install action promotes a transitive dependency to top-level dependency. I am unsure if this feedback is out of scope for this PR.

@kartheekp-ms We already fixed this. I thought the PR was completed but it looks like it's not merged yet. This fixes the issue: #6050. I'll ask @martinrrm to retarget dev and submit this fix after this PR goes in.

jebriede and others added 7 commits September 23, 2024 17:28

First step to fetch transitive packages in Solution PM UI package lis…

c7908b8

…t and installed tab vulnerability warning (#5994) First step to show transitive packages in Solution PM UI and show vulnerability warnings in Solution PM UI Installed Tab

Get the packages metadata for all installed packages in the solution …

e99d092

…level (#5998) * Get all the installed packages metadata in the Solution level

Represent all transitive origins for all installed package versions i…

9e077cb

…n tooltip (#6021)

[Transitive's in Solution] Vulnerability indicators in solution level…

04559cc

… details pane for top-level packages (#6010) * Vulnerability indicators for top level in solution

[Transitive's in Solution] Transitive's data in Solution Details Pane (…

1c3b623

…#6034) * Show transitive's data in Details Pane

Show transitive package details for packages that are top-level and t…

286a239

…ransitive in the solution (#6040)

[Transitive's in Solution PM UI] Update vulnerability warning tooltips (

85f658a

#6043) * Update vulnerability warning tooltips

martinrrm force-pushed the feature-TransitivePkgsInSolutionPMUI branch from 8cb61fa to 85f658a Compare September 23, 2024 23:40

martinrrm marked this pull request as ready for review September 24, 2024 17:25

martinrrm requested a review from a team as a code owner September 24, 2024 17:25

martinrrm changed the title ~~Feature transitive pkgs in solution pm UI~~ Transitive packages and vulnerability indicators in Solution PM UI Sep 24, 2024

kartheekp-ms reviewed Sep 24, 2024

View reviewed changes

donnie-msft reviewed Sep 24, 2024

View reviewed changes

jebriede reviewed Sep 24, 2024

View reviewed changes

jgonz120 reviewed Sep 24, 2024

View reviewed changes

Code cleanup and addressing PR feedback.

1b34007

jgonz120 previously approved these changes Sep 25, 2024

View reviewed changes

Update ProjectInstallationInfo ToString() on Details Pane table and i…

aae9f6d

…mprove localization of table headers (#6051)

jebriede dismissed jgonz120’s stale review via aae9f6d September 25, 2024 18:23

kartheekp-ms reviewed Sep 25, 2024

View reviewed changes

...t.Clients/NuGet.PackageManagement.UI/Converters/GreaterThanThresholdToVisibilityConverter.cs Outdated Show resolved Hide resolved

jgonz120 previously approved these changes Sep 25, 2024

View reviewed changes

- Adding exception handling for converter and test for invalid conver…

d211fe0

…sion cases. - Only adding to VulnerableVersions dictionary if it does not already exist because the same version is installed in another project and only if it's added should we fire OnPropertyChanged.

jebriede dismissed jgonz120’s stale review via d211fe0 September 25, 2024 19:34

jgonz120 approved these changes Sep 25, 2024

View reviewed changes

donnie-msft approved these changes Sep 25, 2024

View reviewed changes

jebriede merged commit 7b232e7 into dev Sep 25, 2024
28 checks passed

jebriede deleted the feature-TransitivePkgsInSolutionPMUI branch September 25, 2024 23:10

This was referenced Sep 27, 2024

Refactoring the method that fetches the project context in the Solution PM UI #6065

Merged

Addressing PR comments for Transitives in Solution PM UI #6064

Merged

zivkan modified the milestone: 6.12 Oct 25, 2024

	// Returns all the transtive packages that are not the latest verion
	// Returns all the transitive packages that are not the latest version because.....

		private readonly Dictionary<NuGetVersion, int> _vulnerableVersions = [];
		public Dictionary<NuGetVersion, int> VulnerableVersions => _vulnerableVersions;

		OnPropertyChanged(nameof(VulnerableVersions));
		OnPropertyChanged(nameof(VulnerableVersionsString));

Transitive packages and vulnerability indicators in Solution PM UI #6044

Transitive packages and vulnerability indicators in Solution PM UI #6044

Conversation

martinrrm commented Sep 23, 2024 • edited Loading

Bug

Description

Accessibility

Details pane

PR Checklist

kartheekp-ms Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnie-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinrrm Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donnie-msft Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebriede Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jgonz120 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kartheekp-ms commented Sep 25, 2024 • edited Loading

donnie-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebriede Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebriede Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jebriede commented Sep 25, 2024

martinrrm commented Sep 23, 2024 •

edited

Loading

kartheekp-ms Sep 24, 2024 •

edited

Loading

martinrrm Sep 24, 2024 •

edited

Loading

donnie-msft Sep 24, 2024 •

edited

Loading

jebriede Sep 27, 2024 •

edited

Loading

kartheekp-ms commented Sep 25, 2024 •

edited

Loading

jebriede Sep 26, 2024 •

edited

Loading

jebriede Sep 26, 2024 •

edited

Loading