Account delete user flow #4977

cristinamanum · 2017-11-09T19:38:17Z

The user flow for account delete.

scottbommarito · 2017-11-09T20:56:50Z

src/NuGetGallery/Services/MessageService.cs

@@ -647,6 +647,32 @@ public void SendPackageAddedNotice(Package package, string packageUrl, string pa
            }
        }

+        public void SendAccountDeleteNotice(MailAddress mailAddress, string account)
+        {
+            string body = @"We received a request to delete your account {0}. If you did not initiate this request please contact the {1} team.


The formatting here could be much better. Also there's a couple oddly worded phrases in here.

You should also make similar modifications to the page itself.

We received a request to delete your account {0}. If you did not initiate this request please contact the {1} team immediately. When your account will be deleted, we will: - revoke your API key(s) - remove your ownership from any packages you own - remove your ownership from any ID prefix reservations and delete any ID prefix reservations that you were the only owner of We will not delete any packages associated with your account. Thanks, The {1} Team

For comparison, it is currently:

We received a request to delete your account {0}. If you did not initiate this request please contact the {1} team. When your account will be deleted, we will:revoke your API key(s), remove you as the owner for any package you own, dissociate all previously existing ID prefix reservation with this account. If you were the only account tied to a particular ID prefix reservation the prefix will be available to anyone. We will not delete the NuGet packages associated with the account. Thanks, The {1} Team

scottbommarito · 2017-11-09T20:57:56Z

src/NuGetGallery/Views/Packages/ContactOwners.cshtml

-                @ViewHelpers.AlertWarning(
-                    @<text>
-                        Sorry, the owners of this package do not allow contacting them through this form.
+                if ((bool)ViewData["HasOwners"])


Put this in the Model, don't use ViewData.

scottbommarito · 2017-11-09T21:00:02Z

src/NuGetGallery/Views/Packages/ContactOwners.cshtml

+                {
+                    @ViewHelpers.AlertWarning(
+                        @<text>
+                            Sorry, this package has no owners and is not being actively maintained. You can try to find a contact for "@Model.PackageId" through its <a href="@Model.ProjectUrl">Project Site</a>


check that the package has a ProjectUrl before you ask them to go to it like above

scottbommarito · 2017-11-09T21:00:23Z

src/NuGetGallery/Views/Packages/DisplayPackage.cshtml

@@ -626,17 +626,24 @@

            <h2>Owners</h2>
            <ul class="list-unstyled owner-list">
-                @foreach (var owner in Model.Owners)
+                @if (Model.Owners.Count() == 0)


Use .Any() instead of .Count() == 0

scottbommarito · 2017-11-09T21:01:13Z

src/NuGetGallery/Views/Packages/DisplayPackage.cshtml

-                            @owner.Username
-                        </a>
-                    </li>
+                        @ViewHelpers.AlertWarning(@<text>@Strings.PackageHasNoOwnersMessage</text>)


We don't use strings for other user-facing strings in the .cshtml, no need to put it in a string.

I think that is good to target having them in resource file. It will make easier to localize the site. Do you see any disadvantages?

The disadvantages are
1 - if we actually were to localize the site, it would require a huge effort anyway because this convention isn't used anywhere else
2 - it's harder to work with (you have to work with the .resx file instead of the rest of the code and you have to recompile if you want to change the string unlike the rest of razor) with little gain

If we reused this string somewhere else I'd agree that it was probably worth, but given that we don't, I don't think it's worth it.

scottbommarito · 2017-11-09T21:08:24Z

src/NuGetGallery/Controllers/UsersController.cs

+            {
+                return HttpNotFound("User not found.");
+            }
+            bool pendingRequest = _supportRequestService.GetIssues(assignedTo: null, reason: null, issueStatusId: null, galleryUsername: user.Username)


If someone cancels a request to delete their account, won't this still find the existing request?

It seems a bit awkward to query the support request service for whether or not a user requested deletion. Additionally, when we move off our support request system, this will need to be changed. I would rather store this in the database somehow.

For the first question - no they will not find it because it was not created.
For the second question, the result of the user action to submit the request to have the package deleted is that a new support request is created.
As discussed offline updated to use the issuekey.

scottbommarito · 2017-11-09T21:13:42Z

src/NuGetGallery/Views/Users/DeleteAccount.cshtml

+                            {
+                                @Html.AntiForgeryToken()
+                                <p>
+                                    <strong>By proceeding, I understand that I am relinquishing ownership of the package/s listed above, and any existent package ID reservation/s will be dissociated with this account.</strong>


Can we use less verbose terms than relinquish and dissociate? losing should be perfectly understandable

Also, there is no need for the passive voice here: "am relinquishing" -> "will relinquish" (or "will lose").

This is as per spec. Following up with the spec owner.

scottbommarito · 2017-11-09T21:15:11Z

src/NuGetGallery/Views/Users/DeleteAccount.cshtml

+                            {
+                                @Html.AntiForgeryToken()
+                                <p>
+                                    <strong>By proceeding, I understand that I am relinquishing ownership of the package/s listed above, and any existent package ID reservation/s will be dissociated with this account.</strong>


We should show all the reserved namespaces associated with the account in the same way we show packages. This will make it clearer for users which reserved namespaces will be deleted and which ones have multiple users.

Implementation is as spec-ed. In think the info and warnings that are showed are enough for the user.

scottbommarito · 2017-11-09T21:17:31Z

tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs

+            public void DeleteAccountRequestView (bool withPendingIssues)
+            {
+                // Arrange
+                string userName = "DeletedUser";


this test (and the rest of the account deletion tests) should use Fakes

var fakes = Get<Fakes>(); var userToDelete = fakes.Owner; var packageToDelete = fakes.Package;

What are the benefits of getting the user through fakes versus creating it directly?

Fakes is used in some of our tests to make it easier to get users. Unless your test requires that you create special users (for example, those that own multiple packages or are in multiple organizations), if your test is in a file that uses Fakes, your test should use Fakes.

scottbommarito · 2017-11-09T21:18:06Z

tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs

+            [Theory]
+            [InlineData(false)]
+            [InlineData(true)]
+            public void DeleteAccountRequestView (bool withPendingIssues)


This is not a test for the TheDeleteAccountAction, so it should have its own method.

loic-sharma · 2017-11-09T23:36:46Z

src/NuGetGallery/Views/Users/DeleteAccount.cshtml

+                {
+                    @ViewHelpers.AlertWarning(@<text>
+                        <b class="keywords">Request submitted.</b> <br />
+                        It may take up to 5 business days to process Account deletion requests.<a href="https://github.com/NuGet/Home/wiki/NuGet-Account-Deletion-Workflow">Read more.</a><br />


Add a space between the period and the link here.

Super nit: the capitalization on "Account" seems weird - could we leave it lower cased?

It is implemented as spec-ed. IMO it looks good with capital letter.

Fair enough. You should still add a space between "requests." and the "Read more." link though :)

loic-sharma · 2017-11-09T23:41:56Z

src/NuGetGallery/App_Start/Routes.cs

@@ -280,6 +280,11 @@ public static void RegisterUIRoutes(RouteCollection routes)
                new { controller = "Users", action = "Delete" });

            routes.MapRoute(
+                RouteName.UserDeleteAccount,
+               "account/delete",


The spacing looks off here.

loic-sharma · 2017-11-09T23:42:54Z

src/NuGetGallery/Controllers/PackagesController.cs

@@ -775,6 +775,7 @@ public virtual ActionResult ContactOwners(string id)
                CopySender = true,
            };

+            ViewData["HasOwners"] = package.PackageRegistration.Owners.Count > 0;


.Owners.Count > 0 -> .Owners.Any()

loic-sharma · 2017-11-09T23:44:30Z

src/NuGetGallery/Controllers/UsersController.cs

+            }
+            bool pendingRequest = _supportRequestService.GetIssues(assignedTo: null, reason: null, issueStatusId: null, galleryUsername: user.Username)
+                                    .Any(issue => string.Equals(issue.IssueTitle, Strings.AccountDelete_SupportRequestTitle, StringComparison.CurrentCultureIgnoreCase) && 
+                                         issue.IssueStatus.Key != 3 && 


What does issue.IssueStatus.Key != 3 mean? A constant for 3 could help explain this code.

The code was updated. It represents the status for a specific issue. ( 3 == resolved)

loic-sharma · 2017-11-09T23:51:39Z

tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs

+
+                List<Package> userPackages = new List<Package>() { userPackage };
+                List<Issue> issues = new List<Issue>();
+                if(withPendingIssues)


A space is missing between the if and (

skofman1 · 2017-11-10T05:15:25Z

src/NuGetGallery/Controllers/UsersController.cs

+        [HttpPost]
+        [Authorize]
+        [ValidateAntiForgeryToken]
+        public virtual async Task<ActionResult> SendAccountDeleteNotificationAsync()


RequestAccountDeletionAsync?

I don't think we use Async in controller action names

The async suffix was added to follow the naming convention for the async methods. The observation that @scottbommarito made is correct. The name was changed to RequestAccountDeletion to be in sync with the current pattern.

skofman1 · 2017-11-10T05:16:20Z

src/NuGetGallery/Controllers/UsersController.cs

+        public virtual async Task<ActionResult> SendAccountDeleteNotificationAsync()
+        {
+            var user = GetCurrentUser();
+            bool createSupportRequestStatus = await _supportRequestService.AddNewSupportRequestAsync(Strings.AccountDelete_SupportRequestTitle,


There are no checks here:
if (user == null || user.IsDeleted)

scottbommarito · 2017-11-14T18:55:31Z

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

@@ -58,6 +58,11 @@ public IReadOnlyCollection<Issue> GetIssues(int? assignedTo = null, string reaso
            return queryable.ToList();
        }

+        public IReadOnlyCollection<Issue> GetOpenIssues( Func<Issue,bool> filter )


It's weird to have a Func<Issue, bool> as a parameter when you can just return an IQueryable<Issue> or IEnumerable<Issue> instead. Is there any substantial benefit to returning IReadOnlyCollection<Issue>?

Another option is to add parameters for the user the request is for and the title of the support request so that you can just specify those values instead and you can still return IReadOnlyCollection<Issue>

One last thing--you could make the key of the issue a parameter of the function so that you don't need to make functions for GetResolvedIssues, GetWaitingForCustomerIssues, etc

The issues key is private and I preferred to not change it as I think that was a reason for making it private. Why is weird to have a predicate that enables filtering?

As I said in another comment, IssueKeys should be public. There's no reason to have "magic values" around the code when we can just make the enum public and get rid of them. I think it was probably private before because it wasn't needed outside of that class, but now it is so it should be changed.

It's weird to have a predicate because
1 - syntactically, predicates aren't as elegant or powerful as LINQ
2 - it requires every caller to write a filtering function when the filtering is likely done on the same fields (the user the request is for and the title, for example)

Updated to make IssuesKeys public and use them on this code path.

scottbommarito · 2017-11-14T19:14:45Z

src/NuGetGallery/Controllers/UsersController.cs

+                return HttpNotFound("User not found.");
+            }
+
+            bool createSupportRequestStatus = await _supportRequestService.AddNewSupportRequestAsync(Strings.AccountDelete_SupportRequestTitle,


nit: I think bool isSupportRequestCreated is a better name for this

scottbommarito · 2017-11-14T19:16:24Z

src/NuGetGallery/Services/MessageService.cs

@@ -647,6 +647,36 @@ public void SendPackageAddedNotice(Package package, string packageUrl, string pa
            }
        }

+        public void SendAccountDeleteNotice(MailAddress mailAddress, string account)
+        {
+            string body = @"We received a request to delete your account {0}. If you did not initiate this request please contact the {1} team imediatelly.


We received a request -> We have received a request

your account {0} -> your account, '{0}'

imediatelly -> immediately

scottbommarito · 2017-11-14T19:20:16Z

src/NuGetGallery/Services/MessageService.cs

+        public void SendAccountDeleteNotice(MailAddress mailAddress, string account)
+        {
+            string body = @"We received a request to delete your account {0}. If you did not initiate this request please contact the {1} team imediatelly.
+{2}When your account will be deleted, we will:


We haven't needed to use Environment.NewLines in previous emails. Have you tested that the email sent looks correctly and doesn't have extra newlines?

I think that the email looks good(attached).

I agree it looks good.

However, it might not look consistent with the rest of our emails, because they don't have newlines between every line.

scottbommarito · 2017-11-14T19:24:01Z

src/NuGetGallery/Strings.resx

@@ -618,4 +618,13 @@ For more information, please contact '{2}'.</value>
  <data name="AccountDelete_Success" xml:space="preserve">
    <value>The account:{0} was deleted succesfully.</value>
  </data>
+  <data name="AccountDelete_CreateSupportRequestFails" xml:space="preserve">
+    <value>The request failed to be submitted. Please try again or contact nuget support.</value>


Two things
1 - nuget -> NuGet
2 - This should use IAppConfiguration.Brand so that if somebody deploys the gallery themselves and changes the brand, it doesn't say NuGet support anyway

An easy alternative may be just remove the use of nuget and just show contact support.

Updated to not use NuGet in the notification.

scottbommarito · 2017-11-14T19:26:46Z

src/NuGetGallery/Views/Packages/ContactOwners.cshtml

+                {
+                    @ViewHelpers.AlertWarning(
+                        @<text>
+                            Sorry, this package has no owners and is not being actively maintained.


I would say it's not being actively maintained ON the NuGet Gallery. Someone might still be working on it on GitHub or some other site, but is not maintaining this package on NuGet.

is not being actively maintained on @Config.Brand.

scottbommarito · 2017-11-14T19:27:56Z

src/NuGetGallery/Views/Packages/ContactOwners.cshtml

+
+                            @if (!string.IsNullOrEmpty(Model.ProjectUrl))
+                            {
+                                @:You can try contacting the package owners of "@Model.PackageId" through its <a href="@Model.ProjectUrl">Project Site</a>


We just said it doesn't have any package owners, so we should say

You can try contacting someone maintaining "@Model.PackageId" through its <a href="@Model.ProjectUrl">Project Site</a>

Also in the else

You'll have to find someone maintaining the package through other means.

The first is as spec-ed. The second, the scenario is about finding the owners of the package. I think that the message should refer to the owners.

It's a minor grammatical change. I think it's ok to change despite how it was spec'ed

My confusion is just that the statement above says "this package has no owners" (and not "this package has no owners on NuGet.org"), but then we are talking about it having owners again. It's not too big of a deal but I think changing the phrasing as detailed would improve the clarity of the statement.

The code updated to be : Sorry, this package has no owners and is not being actively maintained on NuGet.org

scottbommarito · 2017-11-14T19:31:10Z

src/NuGetGallery/Views/Users/DeleteAccount.cshtml

+                {
+                    @ViewHelpers.AlertWarning(@<text>
+                        <b class="keywords">Request submitted.</b> <br />
+                        It may take up to 5 business days to process Account deletion requests. <a href="https://go.microsoft.com/fwlink/?linkid=862770">Read more.</a><br />


Account -> account

(I don't think account should be capitalized here.)

Agree - updated.

scottbommarito · 2017-11-14T19:33:51Z

tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs

+            {
+                // Arrange
+                string userName = "DeletedUser";
+                string emailAddress = $"{userName}@coldmail.com";


coldmail.com...that's brilliant hahaha

scottbommarito · 2017-11-14T19:34:14Z

tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs

+                    {
+                        IssueTitle = Strings.AccountDelete_SupportRequestTitle,
+                        OwnerEmail = emailAddress,
+                        IssueStatus = new IssueStatus() { Key = 1, Name = "OneIssue" }


Use the IssueKeys constant instead of 1.

IssuesKeys is private. I prefer to not change it.

IssueKeys should be public...there's no reason to have "magic values" around the code when we can just make the enum public and get rid of them.

Updated to make the keys public and use these values here instead of 1.

* Account delete user flow * PR feedback * PR feedback * PR feedback

Account delete user flow

e909727

dnfclas added the cla-already-signed label Nov 9, 2017

cristinamanum requested review from chenriksson, shishirx34, agr, joelverhagen, ryuyu, scottbommarito, skofman1, loic-sharma and xavierdecoster November 9, 2017 19:38

scottbommarito suggested changes Nov 9, 2017

View reviewed changes

loic-sharma reviewed Nov 9, 2017

View reviewed changes

skofman1 reviewed Nov 10, 2017

View reviewed changes

skofman1 approved these changes Nov 10, 2017

View reviewed changes

cristinamanum added 3 commits November 13, 2017 09:30

PR feedback

3d30a49

Merge branch 'dev' into cmanu-userdeleteaccount

f635792

PR feedback

bc6530a

scottbommarito approved these changes Nov 14, 2017

View reviewed changes

cristinamanum added 2 commits November 15, 2017 12:35

PR feedback

6756442

Merge branch 'dev' into cmanu-userdeleteaccount

27243fb

cristinamanum merged commit 9036b12 into dev Nov 16, 2017

shishirx34 mentioned this pull request Nov 17, 2017

Deployment 2017-11-30 #4950

Closed

10 tasks

cristinamanum deleted the cmanu-userdeleteaccount branch January 11, 2018 18:09

scottbommarito pushed a commit that referenced this pull request Jul 22, 2019

Account delete user flow (#4977)

14f6c29

* Account delete user flow * PR feedback * PR feedback * PR feedback

Account delete user flow #4977

Account delete user flow #4977

Conversation

cristinamanum commented Nov 9, 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

loic-sharma Nov 9, 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

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

cristinamanum Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

cristinamanum commented Nov 9, 2017 •

edited

Loading

loic-sharma Nov 9, 2017 •

edited

Loading

cristinamanum Nov 15, 2017 •

edited

Loading