[Organizations]: UI for creating migration request #5241

chenriksson · 2018-01-05T23:24:05Z

Screenshot, when request already created (will overwrite):

Screenshot, when admin is not found:

agr · 2018-01-08T20:36:22Z

Was using the word "Transform" discussed? I have a feeling "Convert" would be more appropriate here. As in "Convert to organization account".

cristinamanum · 2018-01-08T20:42:48Z

src/NuGetGallery/Controllers/UsersController.cs

+        [Authorize]
+        public virtual ActionResult Transform()
+        {
+            var accountToTransform = GetCurrentUser();


null check for accountToTransform?

The Authorize attribute should ensure that there is an authenticated user.

cristinamanum · 2018-01-08T20:49:42Z

src/NuGetGallery/Services/UserService.cs

+
+        public async Task RequestTransformToOrganizationAccount(User accountToTransform, User adminUser)
+        {
+            accountToTransform = accountToTransform ?? throw new ArgumentNullException(nameof(accountToTransform));


Will the proposed admin requested to accept the request of becoming an admin?

see PR #5228 for the confirmation

The proposed/pending organization admin must log in and confirm using the generated confirmationToken.

cristinamanum · 2018-01-08T20:51:30Z

src/NuGetGallery/Services/UserService.cs

+
+        public async Task RequestTransformToOrganizationAccount(User accountToTransform, User adminUser)
+        {
+            accountToTransform = accountToTransform ?? throw new ArgumentNullException(nameof(accountToTransform));


Do we handle the cases when the accountToTransform is already an organization ?

Yes, it's handled in UserService.CanTransformToOrganization

cristinamanum · 2018-01-08T20:54:34Z

src/NuGetGallery/Services/UserService.cs

+            accountToTransform.OrganizationMigrationRequest.ConfirmationToken = Crypto.GenerateToken();
+            accountToTransform.OrganizationMigrationRequest.RequestDate = DateTime.UtcNow;
+
+            await UserRepository.CommitChangesAsync();


Does it make sense as the requests to have a Status? Will they be ever completed?

No, the confirmation logic is very similar to email confirmation. The user must be authenticated and verify against the token we generate. Nothing fancy is needed for status.

We considered whether to expire the confirmation after 24hr, but chose to do the same as email confirmation which doesn't expire. We do store RequestDate in case we want to change this.

cristinamanum · 2018-01-08T21:00:44Z

src/NuGetGallery/Services/UserService.cs

+            accountToTransform.OrganizationMigrationRequest.ConfirmationToken = Crypto.GenerateToken();
+            accountToTransform.OrganizationMigrationRequest.RequestDate = DateTime.UtcNow;
+
+            await UserRepository.CommitChangesAsync();


Do we want to add auditing for this scenario?

Yes, there's a separate work item to track auditing which will be done at lower priority.

cristinamanum

LGTM - few comments.

chenriksson · 2018-01-08T21:41:28Z

@agr "Transform" comes from the PM spec, and also matches the terminology used by GitHub.

scottbommarito · 2018-01-09T00:13:13Z

src/NuGetGallery/Strings.resx

+    <value>Administrator account '{0}' does not exist.</value>
+  </data>
+  <data name="TransformAccount_AdminAccountNotConfirmed" xml:space="preserve">
+    <value>Administrator account '{0}' has not validated their email address.</value>


we should use "confirmed" instead of "validated" for this message because that's how it appears elsewhere on the site

For some reason I chose 'validated' instead of 'confirmed' because I thought it was consistent with other strings. Will try to be consistent and run any messages by @anangaur.

scottbommarito · 2018-01-09T00:21:31Z

src/NuGetGallery/Views/Users/Transform.cshtml

+            <div>
+                <aside class="col-md-3 col-md-push-9">
+                    @Html.Label("Logo")
+                    @ViewHelpers.GravatarImage(CurrentUser.EmailAddress, CurrentUser.Username, 332, responsive: true)


A couple thoughts--
1 - I noticed 332 was what we also used on the profile page. Can we put this in a constant of some sort?
2 - Make sure you check how this looks on smaller screens.

scottbommarito · 2018-01-09T00:34:01Z

src/NuGetGallery/Views/Users/Transform.cshtml

+
+                <div class="row form-group col-md-9 col-md-pull-3">
+                    @Html.Label("Organization")
+                    @Html.ShowTextBoxFor(m => CurrentUser.Username, enabled: false)


These look odd as disabled textboxes, especially given that it makes the text harder to see. Can we do what we have in the verify form for these uneditable fields?

E.g.

scottbommarito · 2018-01-09T00:39:36Z

tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs

+            }
+
+            [Fact]
+            public async Task Post_CanTransformReturnsFalse_ShowsError()


i recommend splitting the Get and Post into separate classes and creating a base for the shared logic.

E.g.

public class TheTransformActions : TestContainer { protected UsersController CreateController(string accountToTransform, string canTransformErrorReason = "") { ... } } public class TheGetTransformAction : TheTransformActions { ... } public class ThePostTransformAction : TheTransformActions { ... }

scottbommarito · 2018-01-09T00:45:17Z

tests/NuGetGallery.Facts/Services/UserServiceFacts.cs

+                for (int i = 0; i < (testOverwrite ? 2 : 1); i++)
+                {
+                    // Act
+                    await service.RequestTransformToOrganizationAccount(account, admin);


it may make this test stronger if, for the rewrite test, a different admin user was used

for example, if there was a bug that didn't set the admin of the request to the new admin but still succeeded the request, this test would return a false negative

scottbommarito · 2018-01-09T20:48:34Z

src/NuGetGallery/Controllers/UsersController.cs

+
+        [HttpGet]
+        [Authorize]
+        public virtual ActionResult Transform()


One additional thing I noticed--I think something like TransformAccountToOrganization would be more descriptive and potentially a better name for this action.

chenriksson · 2018-01-09T23:31:53Z

@scottbommarito new screenshot below -

scottbommarito · 2018-01-09T23:40:52Z

@chenriksson Looks much better! Great work!

chenriksson added 7 commits January 2, 2018 14:44

Organizations: transform on confirmation

5a02f4e

Tests and cleanup

930abd4

Fix query

552e728

PR feedback

c8978bb

PR feedback

8cba4dd

Cleanup

12df6a3

[Organizations]: UI for creating migration request

a881e1d

chenriksson requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, ryuyu, cristinamanum, skofman1 and scottbommarito January 5, 2018 23:24

Test fixes

0e0ad2d

chenriksson force-pushed the chenriks-org-transform-ui branch from fe400b7 to 0e0ad2d Compare January 8, 2018 16:53

cristinamanum reviewed Jan 8, 2018

View reviewed changes

cristinamanum approved these changes Jan 8, 2018

View reviewed changes

chenriksson mentioned this pull request Jan 8, 2018

Organizations: transform account on confirmation #5228

Merged

chenriksson changed the title ~~[Organiations]: UI for creating migration request~~ [Organizations]: UI for creating migration request Jan 8, 2018

scottbommarito approved these changes Jan 9, 2018

View reviewed changes

PR feedback

2924613

scottbommarito reviewed Jan 9, 2018

View reviewed changes

Merge from dev

98445be

chenriksson changed the base branch from chenriks-org-transform to dev January 9, 2018 22:17

chenriksson added 2 commits January 9, 2018 14:58

Merge from dev

2d153fd

PR feedback

01e242b

chenriksson merged commit 90e8ada into dev Jan 9, 2018

chenriksson deleted the chenriks-org-transform-ui branch January 9, 2018 23:41

chenriksson mentioned this pull request Jan 26, 2018

S130: Track organizations progress #5361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Organizations]: UI for creating migration request #5241

[Organizations]: UI for creating migration request #5241

chenriksson commented Jan 5, 2018 •

edited

Loading

agr commented Jan 8, 2018

cristinamanum Jan 8, 2018

chenriksson Jan 8, 2018

cristinamanum Jan 8, 2018

chenriksson Jan 8, 2018

cristinamanum Jan 8, 2018

chenriksson Jan 8, 2018

cristinamanum Jan 8, 2018

chenriksson Jan 8, 2018

cristinamanum Jan 8, 2018

chenriksson Jan 8, 2018

cristinamanum left a comment

chenriksson commented Jan 8, 2018

scottbommarito Jan 9, 2018

chenriksson Jan 9, 2018

scottbommarito Jan 9, 2018

scottbommarito Jan 9, 2018

scottbommarito Jan 9, 2018

scottbommarito Jan 9, 2018

chenriksson Jan 9, 2018

scottbommarito Jan 9, 2018 •

edited

Loading

chenriksson commented Jan 9, 2018

scottbommarito commented Jan 9, 2018

[Organizations]: UI for creating migration request #5241

[Organizations]: UI for creating migration request #5241

Conversation

chenriksson commented Jan 5, 2018 • edited Loading

agr commented Jan 8, 2018

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

Choose a reason for hiding this comment

chenriksson commented Jan 8, 2018

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

scottbommarito Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

chenriksson commented Jan 9, 2018

scottbommarito commented Jan 9, 2018

chenriksson commented Jan 5, 2018 •

edited

Loading

scottbommarito Jan 9, 2018 •

edited

Loading