Disable floating versions in project level for CPM

[martinrrm]

Bug

Fixes: NuGet/Home#12229

Regression? Last working version:

Description

Floating versions is not enabled for CPM, this change disables this functionality of the combobox.

Project Level

Solution Level

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

[martinrrm]

After creating the PR, I realized that I should add tests for IsCentralPackageManagementEnabled property.

[kartheekp-ms]

disables the ability of installing packages in project level since CPM is a Solution level functionality.

I think in dotnet add package command we add package reference to the project file and package version to the Directory.Packages.Props. I think we should have the same experience across CLI and VS. What do others think? cc @jeffkl

[donnie-msft]

disables the ability of installing packages in project level since CPM is a Solution level functionality.

I think in dotnet add package command we add package reference to the project file and package version to the Directory.Packages.Props. I think we should have the same experience across CLI and VS. What do others think?

@kartheekp-ms I'm not seeing in docs nor in manual testing that dotnet add package supports floating versions. Is it supposed to be supported?

dotnet add package NuGet.Core --version 2.*

error: Value cannot be null. (Parameter 'version')

..but an explicit version does work...

dotnet add package NuGet.Core --version 2.6.0

info : PackageReference for package 'NuGet.Core' version '2.6.0' added to file '[...].csproj'.

[kartheekp-ms]

I'm not seeing in docs nor in manual testing that dotnet add package supports floating versions. Is it supposed to be supported?

Thank you for taking the time to check if dotnet add package supports floating versions right currently. Good that know that using a floating version throws an exception. My comment was related to disabling the ability to install packages in project level since CPM is a Solution level functionality statement. IMHO, CLI and VS should have same user experience for non-floating package versions at least.

[martinrrm]

Thanks @donnie-msft for checking the behavior, you're right floating versions are not supported in CPM, this PR disables floating versions in the CPM and talking with Jeff we thought about disabling the button from the Project Level too to avoid confusion in the users. But as @kartheekp-ms says, adding a package using dotnet inside a project works and the behavior should be the same.

I think we should not disable adding/updating a package in the Project Level

[donnie-msft]

We can disable floating versions controls in PM UI when CPM is enabled, and that's the point of the linked issue and this PR, right?
If we want to support CPM in the PM UI in the future, that to me is an entirely different topic and would warrant a new issue/PR.

This PR is a bug fix, and I'm not seeing how a new feature request is blocking it. Please schedule time with me if I'm still not on the same page.

[ghost]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

[martinrrm]

Updated the PR, this no longer disabled the Update/Install button in project level.

[erdembayar]

If I understand correctly then CPM is enabled for all projects or not, so just checking for 1st project would be suffice.

Suggested change

	private async Task IsCentralPackageManagementEnabledAsync(CancellationToken cancellationToken)
	private async Task IsCentralPackageManagementEnabledAsync(CancellationToken cancellationToken)
	{
	if (!Model.IsSolution)
	{
	await NuGetUIThreadHelper.JoinableTaskFactory.RunAsync(async delegate
	{
	// Go off the UI thread to perform non-UI operations
	await TaskScheduler.Default;
	if (Model.Context.Projects.Any())
	{
	_detailModel.IsCentralPackageManagementEnabled = await Model.Context.Projects.First().IsCentralPackageManagementEnabledAsync(Model.Context.ServiceBroker, cancellationToken);
	}
	});
	}
	}

[donnie-msft]

You can skip the Any() and just use the First() as I suggested here: #5011 (comment)

[erdembayar]

But that case we're making assumption, there would be at least 1 project, Any is for safeguarding any unexpected null exception error.

[martinrrm]

I'll go with First() because as donnie said, we use it on other places and we're not afraid to make that assertion. I would normally go with Any() to avoid errors but the PM UI cannot be opened without a project

[erdembayar]

You still didn't implement First(). For you no need to loop. You can use above code without Any() check too.

[martinrrm]

Sorry I forgot to push new changes.

[zivkan]

If I understand correctly then CPM is enabled for all projects or not

That's not correct. CPM (and PackageReference) is implemented via MSBuild, and MSBuild is strictly per-project. Trying to enforce anything to be "all of solution or none" in MSBuild is effectively impossible.

[erdembayar]

@martinrrm
Have you looked into above scenario? Do we consider it CPM enabled if some projects are CPM enabled but not all? Maybe check with @jeffkl

[martinrrm]

This code will only run in the project PM UI, so if the user has disabled CPM for that project, it can install any version. I understand that disable CPM in just a project is not a desired behavior for this feature but since its msbuild it difficult to restrain users from doing it. @jeffkl can you confirm this?

[martinrrm]

@donnie-msft @erdembayar Can a get a review again? Thanks!