Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL fix and addressing feedback from demo #5275

Merged
merged 5 commits into from
Jan 22, 2018
Merged

Conversation

chenriksson
Copy link
Member

Includes the following, with screenshots:

  • Update SQL to return and not migrate when no request match

  • Add informational hint on transform page to indicate what is expected for administrator:

image

  • Add informational message when prompting for new admin sign in:

image

  • Adjust transformation error formatting to align with header:

image

//cc @anangaur , @karann-msft

@@ -62,10 +62,11 @@ public partial class AuthenticationController
/// Sign In\Register view
/// </summary>
[HttpGet]
public virtual ActionResult LogOn(string returnUrl)
public virtual ActionResult LogOn(string returnUrl, string returnUrlMessage = "")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized I'm putting this in url; and should probably move to a header. Will update in a few minutes...

@@ -62,10 +62,11 @@ public partial class AuthenticationController
/// Sign In\Register view
/// </summary>
[HttpGet]
public virtual ActionResult LogOn(string returnUrl)
public virtual ActionResult LogOn(string returnUrl, string returnUrlMessage = "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not TempData? Seems a bit weird to encode an entire message in the URL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, in light of returnUrl it kind of makes sense... I'm torn 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree it should not have been in URL. Fixed by moving to TempData.

</div>
}
<div>
@TempData["TransformError"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this TempData and not part of a view model?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Laziness... fixed by creating strong type viewModel for error message.

@chenriksson
Copy link
Member Author

@joelverhagen Addressed feedback. Also, reminder to review the screenshots... in case you haven't.

@@ -153,9 +151,14 @@ public virtual ActionResult TransformToOrganization()

await _userService.RequestTransformToOrganizationAccount(accountToTransform, adminUser);

// prompt for admin sign-on to confirm transformation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: 2 lines whitespace here instead of one?

@@ -734,6 +734,9 @@ For more information, please contact '{2}'.</value>
<value>Administrator account '{0}' has not confirmed their email address.</value>
</data>
<data name="TransformAccount_RequestExists" xml:space="preserve">
<value>Another tranform request was created on {0} with organization admin '{1}'.</value>
<value>Another tranform request was created on {0} for administrator '{1}'. Note that a new request will overwrite this.</value>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... this request.

@chenriksson chenriksson merged commit 5d29d53 into dev Jan 22, 2018
@chenriksson chenriksson deleted the chenriks-org-migrationfix branch January 22, 2018 03:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants