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

Support username casing change in Admin UI #9748

Draft
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

RiadGahlouz
Copy link
Contributor

@RiadGahlouz RiadGahlouz commented Dec 8, 2023

Summary of the changes (in less than 80 characters):
This PR adds support for username casing change in the Admin UI. There are no UI changes, simply controller flow changes.

Addresses https://github.com/NuGet/Engineering/issues/5057

@RiadGahlouz RiadGahlouz self-assigned this Dec 8, 2023
@RiadGahlouz RiadGahlouz requested a review from a team as a code owner December 8, 2023 23:09
lyndaidaii
lyndaidaii previously approved these changes Dec 8, 2023
mariaghiondea
mariaghiondea previously approved these changes Dec 8, 2023
erdembayar
erdembayar previously approved these changes Dec 8, 2023
advay26
advay26 previously approved these changes Dec 9, 2023
@advay26
Copy link
Member

advay26 commented Dec 9, 2023

Should this be the linked issue? https://github.com/NuGet/Engineering/issues/5057

mariaghiondea
mariaghiondea previously approved these changes Dec 11, 2023
Copy link
Contributor

@dannyjdev dannyjdev left a comment

Choose a reason for hiding this comment

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

Apart from updating the record, there is a previous step we perform when changing username casing. We check if the user have available packages (check the playbook for more context). I think this could be added by adding a column on the first form (Verify Account) to include if the user has available packages. And also, please update the playbook with this new changes.

@@ -83,14 +87,28 @@ public ActionResult VerifyAccount(string accountEmailOrUsername)
}

[HttpGet]
public ActionResult ValidateNewUsername(string newUsername)
public ActionResult ValidateNewUsername(string newUsername, bool checkOwnedPackages, string oldUsername = "")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where we're passing true for checkOwnedPackages other than unit test. How we're passing value here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@RiadGahlouz
Have you looked into my question? After this I can approve this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's coming from a form input:

<input type="checkbox" id="checkNonDeletedOwnedPackages" data-bind="checked: checkOwnedPackages" />

<input type="text" placeholder="New username" autocomplete="off" autofocus style="width: 200px;" rows="1" data-bind="value: newUsername" />
<input type="checkbox" id="checkNonDeletedOwnedPackages" data-bind="checked: checkOwnedPackages" />
<label for="checkNonDeletedOwnedPackages">Check Non-Deleted Owned Packages</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

If setting this checkbox just adds a list of owned packages, how about changing it to "List ..."?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe even enable it automatically if new/old only differs by the case?

return new ValidateUsernameResult()
{
IsFormatValid = UsernameValidationRegex.IsMatch(username),
IsAvailable = foundUser == null || (requestor.Key == foundUser.Key && foundUser.Username != username) // The username check is in the event where we found a user in the DB but we're doing a cAsIng change
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
IsAvailable = foundUser == null || (requestor.Key == foundUser.Key && foundUser.Username != username) // The username check is in the event where we found a user in the DB but we're doing a cAsIng change
IsAvailable = foundUser == null || (requestor.Key == foundUser.Key && foundUser.Username != username) // The username check is in the event where we found a user in the DB but we're doing a casing change

{
var oldUsername = IndividualAccount.Username;

var result = await ChangeUsernameController.ChangeUsername(IndividualAccount.Username, IndividualAccount.Username.ToUpper()) as JsonResult;
Copy link
Contributor

Choose a reason for hiding this comment

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

IndividualAccount.Username

nit: "IndividualAccount.Username" -> "oldUsername"

@@ -83,14 +87,28 @@ public ActionResult VerifyAccount(string accountEmailOrUsername)
}

[HttpGet]
public ActionResult ValidateNewUsername(string newUsername)
public ActionResult ValidateNewUsername(string newUsername, bool checkOwnedPackages, string oldUsername = "")
Copy link
Contributor

Choose a reason for hiding this comment

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

= ""

I am thinking whether this default value is needed, if this "oldUsername" will be passed to query DB anyway. Should the caller always pass an "oldUsername"? The API shape is changed here, so I guess that the PR updates all the callers who use this API.

{
if (string.IsNullOrEmpty(newUsername))
{
return Json(HttpStatusCode.BadRequest, "Username cannot be null or empty.", JsonRequestBehavior.AllowGet);
}

var result = ValidateUsername(newUsername);
var oldAccount = _userService.FindByUsername(oldUsername);
Copy link
Contributor

@zhhyu zhhyu Jan 24, 2024

Choose a reason for hiding this comment

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

oldAccount

nit: oldAccount -> account

It may be better to use the same name for local variables under the same class. For example, line 130 uses "account". We can follow the same name convention and also the account itself is still the same but the name will be changed. Personally I think this is more readable and easier to maintain in the future.

@@ -116,39 +134,44 @@ public async Task<ActionResult> ChangeUsername(string oldUsername, string newUse
return Json(HttpStatusCode.NotFound, "Old username account was not found.", JsonRequestBehavior.AllowGet);
}

var newUsernameValidation = ValidateUsername(newUsername);
var newUsernameValidation = ValidateUsernameChange(account, newUsername);
Copy link
Contributor

Choose a reason for hiding this comment

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

newUsernameValidation

nit: newUsernameValidation -> validatedResult

The same at line 103.

@erdembayar erdembayar marked this pull request as draft May 22, 2024 23:01
@erdembayar
Copy link
Contributor

erdembayar commented May 22, 2024

@RiadGahlouz
I changed to draft mode since there has been no update in the past few months and there few comments not addressed. Feel free to switch to Ready to review mode when the PR is ready.

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.

8 participants