Conversation
|
matthew-white
left a comment
matthew-white
left a comment
There was a problem hiding this comment.
It's looking nice! I've added several comments, but a lot of them are just hints for working with the Frontend codebase.
| (async () => { | ||
| const isPwned = await checkPasswordPwnage(this.request, this.newPassword); |
There was a problem hiding this comment.
I'm noticing the use of this.newPassword here and below, but I'm realizing that the <form-group> isn't disabled during the request. So users could theoretically change this.newPassword between the pwned check and the request to Backend. We don't usually disable form fields during requests, but I think that'd be a reasonable choice here in order to prevent this unlikely but awkward possibility. Alternatively, you could save this.newPassword to a local variable at the start of validate(), then use that local variable instead of the potentially changing this.newPassword.
There was a problem hiding this comment.
We don't usually disable form fields during requests
Why not?
There was a problem hiding this comment.
IIRC issa had a preference along those lines. The idea was to not lock down parts of the UI if we don't need to. If the user wants to edit those fields during the request for some reason (perhaps anticipating an error response), we don't restrict them from doing so.
In terms of code complexity, it's probably a wash. It's a little extra to disable fields during requests, but usually it's just one or two attributes or props. But this is the downside of not disabling them, that we have to consider the possibility of the data property changing during the request.
I don't have a strong preference either way. Maybe it's something we could ask Nicole or the larger team on Slack.
There was a problem hiding this comment.
Now the whole form is disabled while it submits.
There was a problem hiding this comment.
There's a weird race condition here - awaitingResponse is only set true once a request has started, but actually:
- there are 2 separate requests, so it may become true and then false and then true and then false again during a normal submission, and
- any async work done before the first request starts can delay setting of
awaitingResponsefor an unknown period
There was a problem hiding this comment.
Replaced derived awaitingResponse with manual submitInProgress.
| return () => hashes.join('\r\n'); | ||
| }; | ||
|
|
||
| describe.only('UserEditPassword', () => { |
| type="password" :placeholder="$t('field.passwordConfirm')" required | ||
| :has-error="mismatch" autocomplete="new-password"/> | ||
| <button type="submit" class="btn btn-primary" | ||
| :aria-disabled="awaitingResponse"> |
There was a problem hiding this comment.
This is now functionally pointless. It should be removed, but this will require changes to the test for "normal button" behaviour.
This branch has not been deployed
2 participants
Closes getodk/central#2150
What has been done to verify that this works as intended?
Why is this the best possible solution? Were any other approaches considered?
fetch()call?How does this change impact users? Describe intentional behavior changes from code updates. What are the regression risks?
Does this change require updates to user documentation? If so, please file an issue here and include the link below.
I don't think so.