Add UI for validation tables (just orchestrator tables) #4861

joelverhagen · 2017-10-18T01:08:30Z

#4653

Admin page (notice last link)

Initial page

Search by ID

Search by ID + version

Search by validation set tracking ID

Search by validation ID

joelverhagen · 2017-10-18T01:27:30Z

src/NuGetGallery.Core/Entities/IEntityRepository.cs

@@ -7,12 +7,11 @@
 namespace NuGetGallery
 {
    public interface IEntityRepository<T>
-        where T : class, IEntity, new()
+        where T : class, new()


Simplified this interface to enable use on the ValidationEntitiesContext.

loic-sharma · 2017-10-18T15:53:48Z

src/NuGetGallery/UrlExtensions.cs

+            IPackageVersionModel package,
+            bool relativeUrl = true)
+        {
+            return url.RevalidatePackage(package.Id, package.Version);


RevalidatePackage [](start = 23, length = 17)

Pass the relativeUrl down.

loic-sharma · 2017-10-18T15:57:31Z

src/NuGetGallery/Areas/Admin/Controllers/ValidationController.cs

+        }
+
+        [HttpGet]
+        public virtual async Task<ActionResult> Search(string vq)


vq [](start = 62, length = 2)

Nit: vq is a strange name, could we use something like query instead?

validation query. I'll give the parameter a different name but I wanted to keep the query string short.

I would recommend q instead of vq as an abbreviation for the query string :)

I was thinking this would collide with filling in the search box at the top of the page, but this behavior only exists on the package search page. Will fix.

loic-sharma · 2017-10-18T16:00:57Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+                await SearchByPackageIdAsync(validationSets, line);
+            }
+
+            var outputValidationSets = validationSets


outputValidationSets [](start = 16, length = 20)

Nit: return this directly instead of creating a variable?

loic-sharma · 2017-10-18T16:08:29Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+                while ((line = stringReader.ReadLine()) != null)
+                {
+                    var normalizedLine = Regex.Replace(line.Trim(), @"\s+", " ");
+                    lines.Add(line);


line [](start = 30, length = 4)

Add the normalizedLine, not the line. Also, skip normalizedLines that are empty.

loic-sharma · 2017-10-18T16:10:00Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+                string line;
+                while ((line = stringReader.ReadLine()) != null)
+                {
+                    var normalizedLine = Regex.Replace(line.Trim(), @"\s+", " ");


Replace [](start = 47, length = 7)

Could you run the Regex.Replace once on the entire query instead?

loic-sharma · 2017-10-18T16:14:24Z

src/NuGetGallery/Areas/Admin/Views/Validation/Index.cshtml

+    @{ bool isFirst = true; }
+    @foreach (var package in Model.Packages)
+    {
+        if (!isFirst)


isFirst [](start = 13, length = 7)

Should this be if (isFirst)?

Removed entirely.

loic-sharma · 2017-10-18T16:15:19Z

src/NuGetGallery/Areas/Admin/Views/Validation/Index.cshtml

+                            </thead>
+                            <tbody>
+                                @foreach (var validation in set.PackageValidations)
+                        {


{ [](start = 24, length = 1)

Curly brace is off here.

cristinamanum · 2017-10-18T16:16:52Z

src/NuGetGallery/Areas/Admin/Controllers/HomeController.cs

-            return View();
+            var viewModel = new HomeViewModel(
+                showValidation: _config.Current.AsynchronousPackageValidationEnabled);
+


Will this ever be false?

The default git clone case is this (and NuGet.org, today).

cristinamanum · 2017-10-18T16:28:15Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+            _packages = packages ?? throw new ArgumentNullException(nameof(packages));
+        }
+
+        public async Task<IReadOnlyList<PackageValidationSet>> SearchAsync(string query)


It would be great to have comments at the method level.

cristinamanum · 2017-10-18T16:36:46Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+            }
+        }
+
+        private async Task SearchByValidationSetKeyAsync(Dictionary<long, PackageValidationSet> validationSets, string line)


line argument is more like a key. Can it be renamed to key?

Line is not the key in all cases. It is just something that will be parsed for a DB query. I would rather keep the parameter name consistent.

cristinamanum · 2017-10-18T16:38:58Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+                    .Include(x => x.PackageValidations)
+                    .Where(x => x.Key == integer)
+                    .FirstOrDefaultAsync();
+


What is the state if the collection has more than 1 element? Should this case be asserted? (same for the other similar methods)

This is not possible since Key is the primary key of the PackageValidationSets table.

cristinamanum · 2017-10-18T16:44:35Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+                await SearchByValidationIdAsync(validationSets, line);
+                await SearchByValidationSetKeyAsync(validationSets, line);
+                await SearchByPackageIdAndVersionAsync(validationSets, line);
+                await SearchByPackageIdAsync(validationSets, line);


Does the order of invocation for these methods matter? Could be cases when they will override dictionary data for one call to the other?

Order does not matter since the dictionary contains the whole package validation set.

skofman1 · 2017-10-18T16:44:43Z

src/NuGetGallery/Web.config

@@ -172,6 +172,7 @@
  <connectionStrings>
    <add name="Gallery.SqlServer" connectionString="Data Source=(localdb)\mssqllocaldb; Initial Catalog=NuGetGallery; Integrated Security=True; MultipleActiveResultSets=True" providerName="System.Data.SqlClient" />
    <add name="Gallery.SupportRequestSqlServer" connectionString="Data Source=(localdb)\mssqllocaldb; Initial Catalog=SupportRequest; Integrated Security=True; MultipleActiveResultSets=True" providerName="System.Data.SqlClient" />
+    <add name="Validation.SqlServer" connectionString="Data Source=(localdb)\mssqllocaldb; Initial Catalog=Validation; Integrated Security=True; MultipleActiveResultSets=True" providerName="System.Data.SqlClient" />


what would be the F5 experience of this? Will someone running the gallery on a clean PC have issue? We don't run migrations for Validation DB as part of the setup-devenvironment.ps1.

F5 will not see this validation UI since asynchronous validation is disabled. The setup script should not create this database since it's not the default scenario (IMO).

skofman1 · 2017-10-18T16:48:21Z

src/NuGetGallery/Areas/Admin/Models/DeletedStatus.cs

+
+namespace NuGetGallery.Areas.Admin.Models
+{
+    public enum DeletedStatus


PackageDeletedStatus for clearity

skofman1 · 2017-10-18T16:50:30Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+                string line;
+                while ((line = stringReader.ReadLine()) != null)
+                {
+                    var normalizedLine = Regex.Replace(line.Trim(), @"\s+", " ");


can u add a timeout to the regex. Barry will like it :)

Good catch.

skofman1 · 2017-10-18T16:51:52Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+        private async Task SearchByValidationSetTrackingIdAsync(Dictionary<long, PackageValidationSet> validationSets, string line)
+        {
+            Guid guid;
+            if (Guid.TryParse(line, out guid))


nit: u can define the guid inline

chenriksson · 2017-10-18T16:40:59Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+        private async Task SearchByValidationSetTrackingIdAsync(Dictionary<long, PackageValidationSet> validationSets, string line)
+        {
+            Guid guid;
+            if (Guid.TryParse(line, out guid))


nit: the SearchByX methods are very similar; I wonder if you could refactor for both code reuse and slight optimization.

For instance, if the caller does TryParse it would know whether to call specific SearchByX methods. Also, then you may be able to combine the core _validationSets SearchBy query into a single method that takes a filter.

I chose not to perform these optimizations until we experience some painful performance. As the code is today, I think it's more readable.

chenriksson · 2017-10-18T16:50:30Z

src/NuGetGallery/Areas/Admin/Services/ValidationEntityRepository.cs

+    {
+        private readonly ValidationEntitiesContext _entities;
+
+        public ValidationEntityRepository(ValidationEntitiesContext entities)


Could EntityRepository be made more generic so that it could be reused for Validation? Maybe introduce an IEntitiesContextBase interface for the APIs shared across all EntityContexts:

SaveChangesAsync

Set

DeleteOnCommit

SetCommandTimeout

GetDatabase

I don't think an new base interface would help here. I will try to have a shared base class (implementation, not interface).

These cannot share a base class since there is not common interface between IEntitiesContext and ValidationEntitiesContext.

skofman1 · 2017-10-18T16:55:58Z

src/NuGetGallery/Areas/Admin/Services/ValidationAdminService.cs

+
+namespace NuGetGallery.Areas.Admin.Services
+{
+    public class ValidationAdminService


didnt see UTs for this code

Spoke extensively offline about this. I think they are not necessary in this case since the code is pretty simple but I would rather set the pattern here as there is no rush.

skofman1 · 2017-10-18T16:58:11Z

src/NuGetGallery/Areas/Admin/Views/Home/Index.cshtml

@@ -105,5 +106,19 @@
                Manage Namespace Reservation
            </p>
        </li>
+        @if (Model.ShowValidation)


I like this approach. We show items in this menu that are not relevant for certain configs, and we shouldn't.

Address #4653

joelverhagen requested review from agr, loic-sharma, xavierdecoster, shishirx34, ryuyu, dtivel, chenriksson, cristinamanum, skofman1 and scottbommarito October 18, 2017 01:08

dnfclas added the cla-already-signed label Oct 18, 2017

joelverhagen commented Oct 18, 2017

View reviewed changes

loic-sharma reviewed Oct 18, 2017

View reviewed changes

loic-sharma approved these changes Oct 18, 2017

View reviewed changes

cristinamanum reviewed Oct 18, 2017

View reviewed changes

skofman1 reviewed Oct 18, 2017

View reviewed changes

chenriksson reviewed Oct 18, 2017

View reviewed changes

skofman1 reviewed Oct 18, 2017

View reviewed changes

Add UI for validation tables (just orchestrator tables) (#4861)

1ecbca2

Address #4653

joelverhagen merged commit 1ecbca2 into NuGet:dev Oct 19, 2017

joelverhagen deleted the val-admin branch October 19, 2017 18:13

Add UI for validation tables (just orchestrator tables) #4861

Add UI for validation tables (just orchestrator tables) #4861

Conversation

joelverhagen commented Oct 18, 2017

Admin page (notice last link)

Initial page

Search by ID

Search by ID + version

Search by validation set tracking ID

Search by validation ID

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

loic-sharma Oct 18, 2017 • 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

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

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

loic-sharma Oct 18, 2017 •

edited

Loading