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

Alias Username fetch on CheckCredential calls #2827

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ochan1
Copy link

@ochan1 ochan1 commented May 4, 2021

No description provided.

@ochan1
Copy link
Author

ochan1 commented May 4, 2021

This Pull Request is in combination with 3 other Pull Requests

  1. Add Banner and Cloud reads for Alias user snap-cloud/SnapSite#105 (SnapSite)
  2. Add Banner and Cloud reads for Alias user snap-cloud/snapCloud#285 (snapCloud)
  3. Alias Username fetch on CheckCredential calls #2827 (Snap)

Please make sure all three are good at the same time and approve all three at the same time!

Copy link
Collaborator

@cycomachead cycomachead left a comment

Choose a reason for hiding this comment

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

A couple minor suggestions.

src/cloud.js Outdated
if (user.username && user.previous_username_admin) {
myself.login(
user.previous_username_admin,
0, // password is irrelevant
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be null, to clearly indicate there's no password. If that causes a problem, then we should use ''.

Copy link
Author

Choose a reason for hiding this comment

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

0 was used during the alias login section of the website as well

I can try null or ' ' and see what happens

src/cloud.js Outdated
myself.login(
user.previous_username_admin,
0, // password is irrelevant
false, // ignored
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this unset the users previous setting? If not can you add a more clear comment about why?

Copy link
Author

Choose a reason for hiding this comment

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

I'll make more clear in the comments that I saved the previous user setting of the alias and use that, so no matter what choice is ticked here it won't be used

src/cloud.js Outdated
}
if (onSuccess) {
onSuccess.call(
null,
user.username,
user.role,
response ? JSON.parse(response) : null
response ? JSON.parse(response) : null,
user.previous_username_admin
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's hard to tell where the on success callback is defined, but this doesn't feel like it should come after the response body. IMO it should come before -- though ideally we use keyword args here.

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.

None yet

2 participants