Alias Username fetch on CheckCredential calls#2827
Alias Username fetch on CheckCredential calls#2827ochan1 wants to merge 4 commits intojmoenig:masterfrom
Conversation
|
This Pull Request is in combination with 3 other Pull Requests
Please make sure all three are good at the same time and approve all three at the same time! |
cycomachead
left a comment
cycomachead
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
This should be null, to clearly indicate there's no password. If that causes a problem, then we should use ''.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Doesn't this unset the users previous setting? If not can you add a more clear comment about why?
There was a problem hiding this comment.
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
| user.role, | ||
| response ? JSON.parse(response) : null | ||
| response ? JSON.parse(response) : null, | ||
| user.previous_username_admin |
There was a problem hiding this comment.
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.
ochan1
force-pushed
the
alias_admin_username
branch
from
9d8ceeb to
2f24db3
Compare
2 participants
No description provided.