Skip to content

Commit

Permalink
Fixing popularity transfer download count bug, surfacing additional v…
Browse files Browse the repository at this point in the history 
…alidation info (#9710)

* changed to package registration download count

* added more validation information

* cleaned up and simplified controller and view model

* cleaned up and simplified the view page

* Fixed download count formatting

* Added missing validation case that could have resulted in a transitive relationship
  • Loading branch information
@advay26
advay26 authored Nov 17, 2023
1 parent bc567a1 commit 7561582
Show file tree
Hide file tree
Showing 5 changed files with 155 additions and 78 deletions.
54 changes: 36 additions & 18 deletions src/NuGetGallery/Areas/Admin/Controllers/PopularityTransferController.cs
View file
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
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
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
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
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();
}


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

0 comments on commit 7561582

Please sign in to comment.