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.

Sign up for GitHub

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

IconUrl deprecation message. #7556

Merged
merged 3 commits into from
Sep 24, 2019
Merged

IconUrl deprecation message. #7556

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e8111cb
IconUrl deprecation message.
agr Sep 13, 2019
a21f1b9
Removed comment.
agr Sep 17, 2019
37ec438
Changed wording for license and icon url warnings.
agr Sep 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/NuGetGallery/NuGetGallery.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@
<DependentUpon>201906262145129_EmbeddedIconFlag.cs</DependentUpon>
</Compile>
<Compile Include="Services\ConfigurationIconFileProvider.cs" />
<Compile Include="Services\IconUrlDeprecationValidationMessage.cs" />
<Compile Include="Services\IconUrlTemplateProcessor.cs" />
<Compile Include="Services\IIconUrlTemplateProcessor.cs" />
<Compile Include="Services\IIconUrlProvider.cs" />
Expand Down
17 changes: 17 additions & 0 deletions src/NuGetGallery/Services/IconUrlDeprecationValidationMessage.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

namespace NuGetGallery
{
/// <summary>
/// IconUrl element deprecation message.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Not a very useful comment, as it just restates the name of the class. I would prefer either getting rid of this comment or saying more about IconUrl and its deprecation.

/// </summary>
public class IconUrlDeprecationValidationMessage : IValidationMessage
{
public string PlainTextMessage => $"{Strings.UploadPackage_IconUrlDeprecated} https://aka.ms/deprecateIconUrl";

public bool HasRawHtmlRepresentation => true;

public string RawHtmlMessage => $"{Strings.UploadPackage_IconUrlDeprecated.Replace("<", "&lt;").Replace(">", "&gt;")} <a href=\"https://aka.ms/deprecateIconUrl\">{Strings.UploadPackage_LearnMore}</a>.";
}
}
13 changes: 9 additions & 4 deletions src/NuGetGallery/Services/PackageUploadService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
using NuGetGallery.Diagnostics;
using NuGetGallery.Helpers;
using NuGetGallery.Packaging;
using NuGetGallery.Services;

namespace NuGetGallery
{
Expand Down Expand Up @@ -148,7 +149,7 @@ public async Task<PackageValidationResult> ValidateBeforeGeneratePackageAsync(
return result;
}

result = await CheckIconMetadataAsync(nuGetPackage, currentUser);
result = await CheckIconMetadataAsync(nuGetPackage, warnings, currentUser);
if (result != null)
{
//_telemetryService.TrackIconValidationFailure();
Expand Down Expand Up @@ -341,18 +342,22 @@ private async Task<PackageValidationResult> CheckLicenseMetadataAsync(PackageArc
return null;
}

private async Task<PackageValidationResult> CheckIconMetadataAsync(PackageArchiveReader nuGetPackage, User user)
private async Task<PackageValidationResult> CheckIconMetadataAsync(PackageArchiveReader nuGetPackage, List<IValidationMessage> warnings, User user)
{
var nuspecReader = GetNuspecReader(nuGetPackage);

var iconElement = nuspecReader.IconElement;
var embeddedIconsEnabled = _featureFlagService.AreEmbeddedIconsEnabled(user);

if (iconElement == null)
{
if (embeddedIconsEnabled && nuspecReader.GetIconUrl() != null)
{
warnings.Add(new IconUrlDeprecationValidationMessage());
}
return null;
}

if (!_featureFlagService.AreEmbeddedIconsEnabled(user))
if (!embeddedIconsEnabled)
{
return PackageValidationResult.Invalid(Strings.UploadPackage_EmbeddedIconNotAccepted);
}
Expand Down
9 changes: 9 additions & 0 deletions src/NuGetGallery/Strings.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions src/NuGetGallery/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1150,4 +1150,7 @@ The {1} Team</value>
<data name="SiteAdminNotLoggedInWithRequiredTenant" xml:space="preserve">
<value>The site admins are required to sign in with the '{0}' tenant only.</value>
</data>
<data name="UploadPackage_IconUrlDeprecated" xml:space="preserve">
<value>&lt;iconUrl&gt; element will be deprecated, please consider switching to specifying the icon in the package.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering that we're warning on icon URLs already, isn't it already deprecated? What do you think of this messaging?

Suggested change
<value>&lt;iconUrl&gt; element will be deprecated, please consider switching to specifying the icon in the package.</value>
<value>The &lt;iconUrl&gt; element is deprecated, please use the icon element instead.</value>

This is message doesn't mirror the client's message, but I would prefer a clearer message if possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message that client currently produces is:

WARNING: NU5048: The 'PackageIconUrl'/'iconUrl' element is deprecated. Consider using the 'PackageIcon'/'icon' element instead. Learn more at https://aka.ms/deprecateIconUrl

And for licenseUrl:

WARNING: NU5125: The 'licenseUrl' element will be deprecated. Consider using the 'license' element instead.

</data>
</root>
51 changes: 51 additions & 0 deletions tests/NuGetGallery.Facts/Services/PackageUploadServiceFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1141,6 +1141,53 @@ public async Task AcceptsPackagesWithEmbeddedIconForFlightedUsers()
Assert.Empty(result.Warnings);
}

[Fact]
public async Task WarnsAboutPackagesWithIconUrlForFlightedUsers()
{
_nuGetPackage = GeneratePackageWithUserContent(
iconUrl: new Uri("https://nuget.test/icon"),
iconFilename: null,
licenseExpression: "MIT",
licenseUrl: new Uri("https://licenses.nuget.org/MIT"));
_featureFlagService
.Setup(ffs => ffs.AreEmbeddedIconsEnabled(_currentUser))
.Returns(true);

var result = await _target.ValidateBeforeGeneratePackageAsync(
_nuGetPackage.Object,
GetPackageMetadata(_nuGetPackage),
_currentUser);

Assert.Equal(PackageValidationResultType.Accepted, result.Type);
Assert.Null(result.Message);
var warning = Assert.Single(result.Warnings);
Assert.IsType<IconUrlDeprecationValidationMessage>(warning);
Assert.StartsWith("<iconUrl> element will be deprecated, please consider switching to specifying the icon in the package.", warning.PlainTextMessage);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks easy to break this test if we use the hard coded string. Maybe directly using the "Strings.UploadPackage_IconUrlDeprecated" is better for future maintenances.
I find some other examples in this facts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the point of this check. To break if text changes. Using Strings.UploadPackage_IconUrlDeprecated directly will hide such changes.

Copy link

Choose a reason for hiding this comment

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

Like that?
image

Saying that element is deprecated when we didn't release a "stable" version of the client supporting it sounds a bit weird though.

@agr Do you need to update your tests?

Assert.StartsWith("&lt;iconUrl&gt; element will be deprecated, please consider switching to specifying the icon in the package.", warning.RawHtmlMessage);
}

[Fact]
public async Task DoesntWarnAboutPackagesWithIconUrl()
{
_nuGetPackage = GeneratePackageWithUserContent(
iconUrl: new Uri("https://nuget.test/icon"),
iconFilename: null,
licenseExpression: "MIT",
licenseUrl: new Uri("https://licenses.nuget.org/MIT"));
_featureFlagService
.Setup(ffs => ffs.AreEmbeddedIconsEnabled(_currentUser))
.Returns(false);

var result = await _target.ValidateBeforeGeneratePackageAsync(
_nuGetPackage.Object,
GetPackageMetadata(_nuGetPackage),
_currentUser);

Assert.Equal(PackageValidationResultType.Accepted, result.Type);
Assert.Null(result.Message);
Assert.Empty(result.Warnings);
}

[Theory]
[InlineData("<icon><something/></icon>")]
[InlineData("<icon><something>icon.png</something></icon>")]
Expand Down Expand Up @@ -2218,6 +2265,7 @@ protected static Mock<TestPackageReader> GeneratePackageWithUserContent(
bool isSigned = true,
int? desiredTotalEntryCount = null,
Func<string> getCustomNuspecNodes = null,
Uri iconUrl = null,
Uri licenseUrl = null,
string licenseExpression = null,
string licenseFilename = null,
Expand All @@ -2232,6 +2280,7 @@ protected static Mock<TestPackageReader> GeneratePackageWithUserContent(
isSigned: isSigned,
desiredTotalEntryCount: desiredTotalEntryCount,
getCustomNuspecNodes: getCustomNuspecNodes,
iconUrl: iconUrl,
licenseUrl: licenseUrl,
licenseExpression: licenseExpression,
licenseFilename: licenseFilename,
Expand All @@ -2249,6 +2298,7 @@ protected static MemoryStream GeneratePackageStream(
bool isSigned = true,
int? desiredTotalEntryCount = null,
Func<string> getCustomNuspecNodes = null,
Uri iconUrl = null,
Uri licenseUrl = null,
string licenseExpression = null,
string licenseFilename = null,
Expand All @@ -2265,6 +2315,7 @@ protected static MemoryStream GeneratePackageStream(
desiredTotalEntryCount: desiredTotalEntryCount,
getCustomNuspecNodes: getCustomNuspecNodes,
licenseUrl: licenseUrl,
iconUrl: iconUrl,
licenseExpression: licenseExpression,
licenseFilename: licenseFilename,
licenseFileContents: GetBinaryLicenseFileContents(licenseFileBinaryContents, licenseFileContents),
Expand Down