[Organizations] Include reserved namespaces and package ownership requests in Manage Packages page filtering #5491

scottbommarito · 2018-02-16T00:08:00Z

Previously, only the listed packages and unlisted packages sections were included in the filtering. This change adds the reserved namespaces and package ownership requests sections to the filtering.

A couple notable changes are:

You can now cancel any package ownership request made for another user to be an owner of a package you own from this page. Previously you had to do it from the Manage Package Owners page.
The package owner requests table shows all of the package's owners instead of the user who sent the request like it used to. I could show both, but I came to the conclusion that the requestor is pretty irrelevant information.
If a user tries to accept or reject a package ownership request for a package they are already an owner of, the request is deleted because it's no longer valid.

(the screenshots have the packages sections removed--no actual changes were made to these sections)

All packages:

Filtered by self:

Filtered by organization that self is collaborator of:

(doesn't show any requests because the current user can't do anything about them)

Filtered by organization that self is admin of:

chenriksson · 2018-02-16T16:12:28Z

@scottbommarito Per screenshots, looks like we show logo with org owners in the Owners column, but not the New Owner column. Is this easy to fix?

scottbommarito · 2018-02-16T16:13:04Z

@chenriksson nice catch! should be easy to do

…into sb-orgmanpack

scottbommarito · 2018-02-16T16:17:51Z

@chenriksson done!

chenriksson · 2018-02-16T16:18:27Z

@scottbommarito I'm not sure we need a separate cancel owner request icon... UI is simpler without.

Instead, could 'X' mean reject the request if the user is the receiver, but mean cancel the request if the user is the sender (or another admin)? Both essentially close the request, which is similar enough to the end user... IMHO.

scottbommarito · 2018-02-16T16:20:24Z

@chenriksson smart catch--we only need to show either the reject or the cancel icon, because the end result is the same for the user lol

…into sb-orgmanpack

scottbommarito · 2018-02-16T16:26:32Z

@chenriksson done as well! also updated screenshots

chenriksson · 2018-02-16T16:26:32Z

src/NuGetGallery/Controllers/UsersController.cs

+            var received = userReceived.Union(orgReceived);
+
+            var sent = _packageService.FindPackagesByAnyMatchingOwner(currentUser, true, false)
+                .SelectMany(p => _packageOwnerRequestService.GetPackageOwnershipRequests(package: p.PackageRegistration));


For perf issue #5461, I'm trying to rework the query in FindPackagesByAnyMatchingOwner, which is currently really inefficient. This API fetches the latest version of each registration. Due to our concurrency issue with the IsLatest columns, it ends up fetching all package rows for all matching owners and sorting on NuGetVersion in the client (Gallery).

If possible, can you avoid adding this second call to FindPackagesByAnyMatchingOwner? It's going to make #5461 twice as bad.

good catch! I can reuse the results from when it's called earlier.

chenriksson · 2018-02-16T16:26:58Z

src/NuGetGallery/Scripts/gallery/page-manage-packages.js

            + ' / '
            + downloadsCount.toLocaleString()
-            + ' download' + (downloadsCount == 1 ? '' : 's');
+            + ' download' + (downloadsCount === 1 ? '' : 's');


awesome - thanks!

I was tired of those LINT errors haha

chenriksson · 2018-02-16T16:27:35Z

src/NuGetGallery/Scripts/gallery/page-manage-packages.js

+
+        function formatReservedNamespacesData(namespacesCount) {
+            return namespacesCount.toLocaleString() + " namespace" + (namespacesCount === 1 ? '' : 's');
+        }


I wonder if we can add a pluralize helper in common.js?

I'm a little skeptical about adding something like this given that
1 - the code here is (less than) a one-liner
2 - the actual problem of pluralization is very complex (if someone supplies "octopus" they might expect it to be pluralized to "octopi")

I'm fine with adding a function just for this file because it is used 3 times but beyond that scope might be misleading.

chenriksson · 2018-02-16T16:30:42Z

src/NuGetGallery/Scripts/gallery/page-manage-packages.js

        }

        // Immediately load initial expander data
        showInitialPackagesData("#listed-data", initialData.ListedPackages);
        showInitialPackagesData("#unlisted-data", initialData.UnlistedPackages);
+        showInitialReservedNamespaceData("#namespaces-data", initialData.ReservedNamespaces);
+        showInitialOwnerRequestsData("#requests-received-data", initialData.RequestsReceived);
+        showInitialOwnerRequestsData("#requests-sent-data", initialData.RequestsSent);


Looks great - I know this was a lot of work!

chenriksson · 2018-02-16T16:34:04Z

src/NuGetGallery/Views/Users/Packages.cshtml

                    </text>, expanded: false)
            }
        </div>
+    </div>


We could still consider moving sections into partials, like I did for Account/ManageOrganizations views. It might be nice to reduce the size of this file.

Not sure how to best split this up...I could split the templates into their own partials but there's still gonna be a massive script section at the end.

Feel free to leave as-is, and I can re-evaluate when I do my work for #5461

chenriksson

LGTM!

chenriksson · 2018-02-16T21:34:29Z

src/NuGetGallery/Views/Users/Packages.cshtml

+                <tbody data-bind="foreach: Requests">
+                    <tr class="manage-package-listing" data-bind="visible: Visible">
+                        <td class="align-middle hidden-xs">
+                            <img class="package-icon img-responsive" aria-hidden="true" alt=""


accessibility requires alt for images, so I assume we should use non-empty here.

alt was empty for the package icon in the packages lists. Not sure what to put there but I'll figure it out.

chenriksson · 2018-02-16T21:39:02Z

src/NuGetGallery/Views/Users/Packages.cshtml

+            <table class="table">
+                <thead>
+                    <tr class="manage-package-headings">
+                        <th class="hidden-xs"></th>


see: Accessibility PR #5426

Keros complained about empty header columns, so I put hidden spans with a text description. I wonder if we should do that here too.

cristinamanum · 2018-03-01T22:20:32Z

src/NuGetGallery/Controllers/PackagesController.cs

@@ -1301,6 +1301,8 @@ public virtual Task<ActionResult> RejectPendingOwnershipRequest(string id, strin

            if (package.Owners.Any(o => o.MatchesUser(user)))
            {
+                // If the user is already an owner, clean up the invalid request.
+                await _packageOwnershipManagementService.DeletePackageOwnershipRequestAsync(package, user);


Probably this should not throw.

Scott Bommarito added 3 commits February 15, 2018 15:29

Expand owners filter to reserved namespaces and ownership requests

60e0257

filter received and sent categories separately

5d0ab6a

add test for removing invalid ownership request

1247894

scottbommarito requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, nugetlurker, ryuyu, anangaur, chenriksson, cristinamanum and skofman1 February 16, 2018 00:08

Merge branch 'dev' into sb-orgmanpack

4326914

scottbommarito changed the title ~~[Organizations] Include reserved namespaces and package ownership requests in Manage Packages Page filtering~~ [Organizations] Include reserved namespaces and package ownership requests in Manage Packages page filtering Feb 16, 2018

Scott Bommarito added 3 commits February 16, 2018 08:16

show org icon for new owner in package owner requests

62bd96d

Merge branch 'sb-orgmanpack' of https://github.com/NuGet/NuGetGallery …

c3ea995

…into sb-orgmanpack

Merge branch 'dev' into sb-orgmanpack

8529734

Scott Bommarito added 2 commits February 16, 2018 08:24

dont' show cancel icon if the user can accept/reject

e610989

Merge branch 'sb-orgmanpack' of https://github.com/NuGet/NuGetGallery …

43da5b4

…into sb-orgmanpack

chenriksson reviewed Feb 16, 2018

View reviewed changes

perf fix

6943e08

chenriksson approved these changes Feb 16, 2018

View reviewed changes

Scott Bommarito added 2 commits February 20, 2018 09:40

accessibility fixes

6da8985

Merge branch 'dev' into sb-orgmanpack

6067daa

scottbommarito merged commit d8371bd into dev Mar 1, 2018

scottbommarito deleted the sb-orgmanpack branch March 1, 2018 20:41

cristinamanum reviewed Mar 1, 2018

View reviewed changes

scottbommarito restored the sb-orgmanpack branch July 22, 2019 23:19

scottbommarito deleted the sb-orgmanpack branch July 22, 2019 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Organizations] Include reserved namespaces and package ownership requests in Manage Packages page filtering #5491

[Organizations] Include reserved namespaces and package ownership requests in Manage Packages page filtering #5491

scottbommarito commented Feb 16, 2018 •

edited

Loading

chenriksson commented Feb 16, 2018

scottbommarito commented Feb 16, 2018

scottbommarito commented Feb 16, 2018

chenriksson commented Feb 16, 2018

scottbommarito commented Feb 16, 2018

scottbommarito commented Feb 16, 2018 •

edited

Loading

chenriksson Feb 16, 2018

scottbommarito Feb 16, 2018

chenriksson Feb 16, 2018

scottbommarito Feb 16, 2018

chenriksson Feb 16, 2018

scottbommarito Feb 20, 2018

chenriksson Feb 16, 2018

chenriksson Feb 16, 2018

scottbommarito Feb 16, 2018

chenriksson Feb 16, 2018

chenriksson left a comment

chenriksson Feb 16, 2018

scottbommarito Feb 20, 2018

chenriksson Feb 16, 2018

scottbommarito Feb 20, 2018

cristinamanum Mar 1, 2018

[Organizations] Include reserved namespaces and package ownership requests in Manage Packages page filtering #5491

[Organizations] Include reserved namespaces and package ownership requests in Manage Packages page filtering #5491

Conversation

scottbommarito commented Feb 16, 2018 • edited Loading

chenriksson commented Feb 16, 2018

scottbommarito commented Feb 16, 2018

scottbommarito commented Feb 16, 2018

chenriksson commented Feb 16, 2018

scottbommarito commented Feb 16, 2018

scottbommarito commented Feb 16, 2018 • 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

chenriksson left a 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

scottbommarito commented Feb 16, 2018 •

edited

Loading

scottbommarito commented Feb 16, 2018 •

edited

Loading