Add the change-name route #74
Conversation
logic/auth.js
Outdated
|
|
||
| changeNameStatus.percent = 100; | ||
| } catch (error) { | ||
| if (error.response.status === constants.STATUS_CODES.UNAUTHORIZED) { |
There was a problem hiding this comment.
| if (error.response.status === constants.STATUS_CODES.UNAUTHORIZED) { | |
| if (error.response.status === constants.STATUS_CODES.FORBIDDEN) { |
There was a problem hiding this comment.
I've seen the same code as you on the previous function and the thing I don't understand is why use the flag forbidden and respond unauthorised = true?
I've done this by "logic" on my reflexion but I can put it back as you mentioned if it's normal
There was a problem hiding this comment.
Yeah you're right.
https://stackoverflow.com/a/6937030
I think we don't need an unauthorised here, because the only error we can get is an fs error (read or write), contrary to the other function which need auth.
Maybe just return Internal server error if something goes wrong?
| const newName = req.body.newName; | ||
|
|
||
| try { | ||
| validator.isString(newName); |
There was a problem hiding this comment.
Maybe we can add other validators, like no special characters?
umbrel-manager/utils/validator.js
Line 10 in d65fafa
or
umbrel-manager/utils/validator.js
Line 19 in d65fafa
There was a problem hiding this comment.
Oh yeah right didn't looked them alpha+space seems a good idea!
There was a problem hiding this comment.
Then this should also be added later in the file, otherwise a name that works for signup won't work if changed to it.
umbrel-manager/routes/v1/account.js
Line 103 in a3b4efc
There was a problem hiding this comment.
If validator.isString(); is all we validate at signup then that's all we should do for name changes too.
Unless there's a specific bug where special characters are causing issues, but that's outside the scope of this PR and should be resolved in a separate PR.
There was a problem hiding this comment.
Okok, noted I will update that
logic/auth.js
Outdated
| changeNameStatus.percent = 1; // eslint-disable-line no-magic-numbers | ||
|
|
||
| changeNameStatus.percent = 40; // eslint-disable-line no-magic-numbers | ||
|
|
||
| try { | ||
| // get user data | ||
| const user = await diskLogic.readUserFile(); | ||
|
|
||
| // update user name | ||
| user.name = name; |
There was a problem hiding this comment.
It won't change a lot of things as it's almost instant, but it seems more logical to me to have the second step (40%) after the file read instead of just after the first step (1%).
| changeNameStatus.percent = 1; // eslint-disable-line no-magic-numbers | |
| changeNameStatus.percent = 40; // eslint-disable-line no-magic-numbers | |
| try { | |
| // get user data | |
| const user = await diskLogic.readUserFile(); | |
| // update user name | |
| user.name = name; | |
| changeNameStatus.percent = 1; // eslint-disable-line no-magic-numbers | |
| try { | |
| // get user data | |
| const user = await diskLogic.readUserFile(); | |
| changeNameStatus.percent = 40; // eslint-disable-line no-magic-numbers | |
| // update user name | |
| user.name = name; |
There was a problem hiding this comment.
Yeah I guess it's near to instant. In fact just before the try it set 40% for both cases, with your change if it fails we do 1 to 100.
What do you think?
I've used the percentage as the change-password function but since it's not set to an UI animation we can eventually do 1 to 100.
There was a problem hiding this comment.
Maybe 1-30-60-100 🧐
It's not really important haha
|
@bguillaumat Can you rebase this PR? |
Verify that name is alphanum+space & remove condition in catch Remove alphanum+space verification Update percentage
bguillaumat
force-pushed
the
add-change-name
branch
from
1ca3386
to
377b9bb
Compare
Done |
Linked to getumbrel/umbrel-dashboard#291