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

Fixing popularity transfer download count bug, surfacing additional validation info #9710

Merged
merged 6 commits into from
Nov 17, 2023
Merged

Fixing popularity transfer download count bug, surfacing additional validation info #9710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a6dd2bf
changed to package registration download count
advay26 Oct 12, 2023
d62adaa
added more validation information
advay26 Oct 13, 2023
599d28f
cleaned up and simplified controller and view model
advay26 Oct 13, 2023
798787f
cleaned up and simplified the view page
advay26 Oct 17, 2023
e29fb9a
Fixed download count formatting
advay26 Oct 20, 2023
b8aa851
Added missing validation case that could have resulted in a transitiv…
advay26 Oct 27, 2023
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
54 changes: 36 additions & 18 deletions src/NuGetGallery/Areas/Admin/Controllers/PopularityTransferController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,37 +98,55 @@ public ActionResult ValidateInputs(string packagesFromInput, string packagesToIn
}

// create validated input result
var input = new PopularityTransferItem(CreatePackageSearchResult(packageFrom.Packages.First()),
CreatePackageSearchResult(packageTo.Packages.First()),
packageFrom.Key,
packageTo.Key);
result.ValidatedInputs.Add(new PopularityTransferItem(packageFrom, packageTo));

result.ValidatedInputs.Add(input);
// checking for existing entries in the PackageRenames table
// 1. 'From' input that already has a 'From' entry in the PackageRenames table -- Conflict
var existingRenames = _packageRenameService.GetPackageRenames(packageFrom);

// check for existing entries in the PackageRename table for the 'From' packages
var existingRenamesFrom = _packageRenameService.GetPackageRenames(packageFrom);

if (existingRenamesFrom.Any())
if (existingRenames.Any())
{
if (existingRenamesFrom.Count == 1)
if (existingRenames.Count == 1)
{
var existingRenamesMessage = $"{packageFrom.Id} already has 1 entry in the PackageRenames table. This will be removed with this operation.";
result.ExistingPackageRenames.Add(existingRenamesMessage);
result.ExistingPackageRenamesMessagesConflict.Add($"{packageFrom.Id} already has 1 entry in the PackageRenames table. This will be removed with this operation.");
}
else
{
var existingRenamesMessage = $"{packageFrom.Id} already has {existingRenamesFrom.Count} entries in the PackageRenames table. These will be removed with this operation.";
result.ExistingPackageRenames.Add(existingRenamesMessage);
result.ExistingPackageRenamesMessagesConflict.Add($"{packageFrom.Id} already has {existingRenames.Count} entries in the PackageRenames table. These will be removed with this operation.");
}

foreach (var existingRename in existingRenames)
{
result.ExistingPackageRenamesConflict.Add(new PopularityTransferItem(existingRename.FromPackageRegistration, existingRename.ToPackageRegistration));
}
}

// 2. 'From' input that already has a 'To' entry in the PackageRenames table -- Transitive
existingRenames = _packageRenameService.GetPackageRenamesTo(packageFrom);

if (existingRenames.Any())
{
var existingRenamesMessage = $"{packageFrom.Id} already has entries in the PackageRenames table. This popularity transfer will result in a new transitive relationship. Please look at the PackageRenames table and verify your input before proceeding.";
result.ExistingPackageRenamesMessagesTransitive.Add(existingRenamesMessage);

foreach (var existingRename in existingRenames)
{
result.ExistingPackageRenamesTransitive.Add(new PopularityTransferItem(existingRename.FromPackageRegistration, existingRename.ToPackageRegistration));
}
}

// check for existing entries in the PackageRename table for the 'To' packages
var existingRenamesTo = _packageRenameService.GetPackageRenames(packageTo);
// 3. 'To' input that already has a 'From' entry in the PackageRenames table -- Transitive
existingRenames = _packageRenameService.GetPackageRenames(packageTo);

if (existingRenamesTo.Any())
if (existingRenames.Any())
{
var existingRenamesMessage = $"{packageTo.Id} already has entries in the PackageRenames table. This popularity transfer will result in a new transitive relationship. Please look at the PackageRenames table and verify your input before proceeding.";
result.ExistingPackageRenames.Insert(0, existingRenamesMessage);
result.ExistingPackageRenamesMessagesTransitive.Add(existingRenamesMessage);

foreach (var existingRename in existingRenames)
{
result.ExistingPackageRenamesTransitive.Add(new PopularityTransferItem(existingRename.FromPackageRegistration, existingRename.ToPackageRegistration));
}
}
}

Expand Down
65 changes: 44 additions & 21 deletions src/NuGetGallery/Areas/Admin/ViewModels/PopularityTransferViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
// 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.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Web;
using System.Web.Mvc;
using NuGet.Services.Entities;

namespace NuGetGallery.Areas.Admin.ViewModels
{
Expand All @@ -12,12 +15,18 @@ public class PopularityTransferViewModel
public PopularityTransferViewModel()
{
ValidatedInputs = new List<PopularityTransferItem>();
ExistingPackageRenames = new List<string>();
ExistingPackageRenamesConflict = new List<PopularityTransferItem>();
ExistingPackageRenamesTransitive = new List<PopularityTransferItem>();
ExistingPackageRenamesMessagesConflict = new List<string>();
ExistingPackageRenamesMessagesTransitive = new List<string>();
SuccessMessage = string.Empty;
}

public List<PopularityTransferItem> ValidatedInputs { get; set; }
public List<string> ExistingPackageRenames { get; set; }
public List<PopularityTransferItem> ExistingPackageRenamesConflict { get; set; }
public List<PopularityTransferItem> ExistingPackageRenamesTransitive { get; set; }
public List<string> ExistingPackageRenamesMessagesConflict { get; set; }
public List<string> ExistingPackageRenamesMessagesTransitive { get; set; }
public string SuccessMessage { get; set; } = string.Empty;
}

Expand All @@ -28,35 +37,49 @@ public PopularityTransferItem()
FromOwners = new List<UserViewModel>();
ToOwners = new List<UserViewModel>();
}

public PopularityTransferItem(
PackageSearchResult packageFrom,
PackageSearchResult packageTo,
int fromKey,
int toKey)

public PopularityTransferItem(PackageRegistration packageFrom, PackageRegistration packageTo)
{
FromId = packageFrom.PackageId;
FromUrl = UrlHelperExtensions.Package(new UrlHelper(HttpContext.Current.Request.RequestContext), packageFrom.PackageId);
FromDownloads = packageFrom.DownloadCount;
FromOwners = packageFrom.Owners;
FromKey = fromKey;

ToId = packageTo.PackageId;
ToUrl = UrlHelperExtensions.Package(new UrlHelper(HttpContext.Current.Request.RequestContext), packageTo.PackageId);
ToDownloads = packageTo.DownloadCount;
ToOwners = packageTo.Owners;
ToKey = toKey;
FromId = packageFrom.Id;
FromUrl = UrlHelperExtensions.Package(new UrlHelper(HttpContext.Current.Request.RequestContext), packageFrom.Id);
FromDownloads = packageFrom.DownloadCount.ToNuGetNumberString();
FromOwners = packageFrom
.Owners
.Select(u => u.Username)
.OrderBy(u => u, StringComparer.OrdinalIgnoreCase)
.Select(u => new UserViewModel
{
Username = u,
ProfileUrl = UrlHelperExtensions.User(new UrlHelper(HttpContext.Current.Request.RequestContext), u),
})
.ToList();
FromKey = packageFrom.Key;

ToId = packageTo.Id;
ToUrl = UrlHelperExtensions.Package(new UrlHelper(HttpContext.Current.Request.RequestContext), packageTo.Id);
ToDownloads = packageTo.DownloadCount.ToNuGetNumberString();
ToOwners = packageTo
.Owners
.Select(u => u.Username)
.OrderBy(u => u, StringComparer.OrdinalIgnoreCase)
.Select(u => new UserViewModel
{
Username = u,
ProfileUrl = UrlHelperExtensions.User(new UrlHelper(HttpContext.Current.Request.RequestContext), u),
})
.ToList();
ToKey = packageTo.Key;
}

public string FromId { get; set; }
public string FromUrl { get; set; }
public long FromDownloads { get; set; }
public string FromDownloads { get; set; }
public IReadOnlyList<UserViewModel> FromOwners { get; set; } = new List<UserViewModel>();
public int FromKey { get; set; }

public string ToId { get; set; }
public string ToUrl { get; set; }
public long ToDownloads { get; set; }
public string ToDownloads { get; set; }
public IReadOnlyList<UserViewModel> ToOwners { get; set; } = new List<UserViewModel>();
public int ToKey { get; set; }
}
Expand Down
102 changes: 63 additions & 39 deletions src/NuGetGallery/Areas/Admin/Views/PopularityTransfer/Index.cshtml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,36 @@
}
@ViewHelpers.AjaxAntiForgeryToken(Html)

@helper AddPopularityTransferItemTable(string inputName, string tableId, string tableLabel)
{
<table class="table form-group col-md-12" id="@(tableId)" style="display:none" data-bind="visible: @(inputName)().length > 0" aria-label="@(tableLabel)">
<thead>
<tr>
<th class="col-group-from-header col-group-1">Package ID<br />(From)</th>
<th class="col-group-from-header col-group-2">Downloads<br />(From)</th>
<th class="col-group-from-header col-group-3">Owners<br />(From)</th>
<th class="col-group-to-header col-group-1">Package ID<br />(To)</th>
<th class="col-group-to-header col-group-2">Downloads<br />(To)</th>
<th class="col-group-to-header col-group-3">Owners<br />(To)</th>
</tr>
</thead>
<tbody data-bind="foreach: @(inputName)">
<tr>
<td class="col-group-from-data col-group-1"><a href="#" data-bind="text: FromId, attr: { href: FromUrl }"></a></td>
<td class="col-group-from-data col-group-2"><span data-bind="text: FromDownloads"></span></td>
<td class="col-group-from-data col-group-3" data-bind="foreach: FromOwners">
<a data-bind="text: Username, attr: { href: ProfileUrl }" />
</td>
<td class="col-group-to-data col-group-1"><a href="#" data-bind="text: ToId, attr: { href: ToUrl }"></a></td>
<td class="col-group-to-data col-group-2"><span data-bind="text: ToDownloads"></span></td>
<td class="col-group-to-data col-group-3" data-bind="foreach: ToOwners">
<a data-bind="text: Username, attr: { href: ProfileUrl }" />
</td>
</tr>
</tbody>
</table>
}

<section role="main" class="container main-container page-admin-popularity-transfers">
<div class="row col-md-12" style="display:none" data-bind="visible: successMessage">
<div id="popularityTransferSuccessMessage" class="alert alert-success" role="alert" data-bind="text: successMessage"></div>
Expand Down Expand Up @@ -34,44 +64,29 @@
</div>
</div>

<div style="display: none" data-bind="visible: doneValidateInputs() && !errorInputs()">
<div class="form-horizontal" style="display: none" data-bind="visible: doneValidateInputs() && !errorInputs()">
<h3>Preview of changes</h3>

<div class="form-horizontal">
<table class="table form-group col-md-12" id="validatedInputResult" aria-label="preview of popularity transfer changes">
<thead>
<tr>
<th class="col-group-from-header col-group-1">Package ID<br />(From)</th>
<th class="col-group-from-header col-group-2">Downloads<br />(From)</th>
<th class="col-group-from-header col-group-3">Owners<br />(From)</th>
<th class="col-group-to-header col-group-1">Package ID<br />(To)</th>
<th class="col-group-to-header col-group-2">Downloads<br />(To)</th>
<th class="col-group-to-header col-group-3">Owners<br />(To)</th>
</tr>
</thead>
<tbody data-bind="foreach: validatedInputs">
<tr>
<td class="col-group-from-data col-group-1"><a href="#" data-bind="text: FromId, attr: { href: FromUrl }"></a></td>
<td class="col-group-from-data col-group-2"><span data-bind="text: FromDownloads"></span></td>
<td class="col-group-from-data col-group-3" data-bind="foreach: FromOwners">
<a data-bind="text: Username, attr: { href: ProfileUrl }" />
</td>
<td class="col-group-to-data col-group-1"><a href="#" data-bind="text: ToId, attr: { href: ToUrl }"></a></td>
<td class="col-group-to-data col-group-2"><span data-bind="text: ToDownloads"></span></td>
<td class="col-group-to-data col-group-3" data-bind="foreach: ToOwners">
<a data-bind="text: Username, attr: { href: ProfileUrl }" />
</td>
</tr>
</tbody>
</table>

<div class="form-group col-md-12" style="display:none" data-bind="visible: existingPackageRenames().length > 0, foreach: existingPackageRenames">
@ViewHelpers.AlertWarning(@<text><span data-bind="text: $data"></span></text>)
</div>

<div class="form-group col-md-12">
<button type="submit" class="btn btn-block btn-primary" data-bind="click: submitInputs">Submit changes</button>
</div>
@AddPopularityTransferItemTable("validatedInputs", "validatedInputResult", "preview of popularity transfer changes")

<div class="form-group col-md-12" style="display:none" data-bind="visible: existingPackageRenamesMessagesConflict().length > 0, foreach: existingPackageRenamesMessagesConflict">
@ViewHelpers.AlertWarning(@<text><span data-bind="text: $data"></span></text>)
</div>

<h3 style="display:none" data-bind="visible: existingPackageRenamesConflict().length > 0">Entries that will be removed from the PackageRenames table with this operation</h3>

@AddPopularityTransferItemTable("existingPackageRenamesConflict", "existingRenamesConflictResult", "existing popularity transfers for the selected packages")

<div class="form-group col-md-12" style="display:none" data-bind="visible: existingPackageRenamesMessagesTransitive().length > 0, foreach: existingPackageRenamesMessagesTransitive">
@ViewHelpers.AlertWarning(@<text><span data-bind="text: $data"></span></text>)
</div>

<h3 style="display:none" data-bind="visible: existingPackageRenamesTransitive().length > 0">Existing entries in the PackageRenames table that will result in a new transitive relationship</h3>

@AddPopularityTransferItemTable("existingPackageRenamesTransitive", "existingRenamesTransitiveResult", "existing popularity transfers for the selected packages")

<div class="form-group col-md-12">
<button type="submit" class="btn btn-block btn-primary" data-bind="click: submitInputs">Submit changes</button>
</div>
</div>
</section>
Expand All @@ -91,7 +106,10 @@
this.errorInputs = ko.observable('');

this.validatedInputs = ko.observableArray([]);
this.existingPackageRenames = ko.observableArray([]);
this.existingPackageRenamesConflict = ko.observableArray([]);
this.existingPackageRenamesMessagesConflict = ko.observableArray([]);
this.existingPackageRenamesTransitive = ko.observableArray([]);
this.existingPackageRenamesMessagesTransitive = ko.observableArray([]);

this.successMessage = ko.observable('');
this.errorSubmitChanges = ko.observable('');
Expand All @@ -100,7 +118,10 @@
$self.doneValidateInputs(false);
$self.errorInputs('');
$self.validatedInputs([]);
$self.existingPackageRenames([]);
$self.existingPackageRenamesConflict([]);
$self.existingPackageRenamesMessagesConflict([]);
$self.existingPackageRenamesTransitive([]);
$self.existingPackageRenamesMessagesTransitive([]);
$self.successMessage('');
$self.errorSubmitChanges('');

Expand All @@ -117,7 +138,10 @@
dataType: 'json',
success: function (result) {
$self.validatedInputs(result.ValidatedInputs);
$self.existingPackageRenames(result.ExistingPackageRenames);
$self.existingPackageRenamesConflict(result.ExistingPackageRenamesConflict);
$self.existingPackageRenamesMessagesConflict(result.ExistingPackageRenamesMessagesConflict);
$self.existingPackageRenamesTransitive(result.ExistingPackageRenamesTransitive);
$self.existingPackageRenamesMessagesTransitive(result.ExistingPackageRenamesMessagesTransitive);
}
})
.fail(function (xhr, textStatus, errorMessage) {
Expand Down
1 change: 1 addition & 0 deletions src/NuGetGallery/Services/IPackageRenameService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,6 @@ namespace NuGetGallery
public interface IPackageRenameService
{
IReadOnlyList<PackageRename> GetPackageRenames(PackageRegistration package);
IReadOnlyList<PackageRename> GetPackageRenamesTo(PackageRegistration package);
}
}
11 changes: 11 additions & 0 deletions src/NuGetGallery/Services/PackageRenameService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,17 @@ public IReadOnlyList<PackageRename> GetPackageRenames(PackageRegistration packag
{
return _packageRenameRepository.GetAll()
.Where(pr => pr.FromPackageRegistrationKey == packageRegistration.Key)
.Include(pr => pr.FromPackageRegistration)
.Include(pr => pr.ToPackageRegistration)
.ToList();
Copy link
Contributor

Choose a reason for hiding this comment

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

.ToListAsync()? Below, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

@advay26
Are you going to address this? After this I can approve again.

Copy link
Member Author

Choose a reason for hiding this comment

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

.GetAll() doesn't currently support ToListAsync(). EF Core would allow us to use this, but regular Entity Framework doesn't. I spoke to @agr offline, and we won't look to fix this right now with this change.

For context, this is the error it throws when I tried to make the call async:

The source IQueryable doesn't implement IDbAsyncEnumerable<NuGet.Services.Entities.PackageRename>. Only sources that implement IDbAsyncEnumerable can be used for Entity Framework asynchronous operations.

}


public IReadOnlyList<PackageRename> GetPackageRenamesTo(PackageRegistration packageRegistration)
{
return _packageRenameRepository.GetAll()
.Where(pr => pr.ToPackageRegistrationKey == packageRegistration.Key)
.Include(pr => pr.FromPackageRegistration)
.Include(pr => pr.ToPackageRegistration)
.ToList();
}
Expand Down