LG-3738: Resolve parse error on password change screen#4497
Conversation
app/javascript/packs/pw-strength.js
Outdated
There was a problem hiding this comment.
oooo I have not seen .dataset I'm just used to .getAttribute('data-forbidden-passwords')
There was a problem hiding this comment.
Open Questions: Is there a better way to ensure consistency of or consolidate how the password edit view is prepared? Similarly, there appear to be a few different variations of users (token_user, build_user, resource) within the ResetPasswordsController class, not all of which are available to reference in every method handler.
One thing we could do is have the template raise if @forbidden_passwords is nil, defined?
I don't like this but it would definitely mean we find out sooner rather than later....
| 'forbidden-passwords': @forbidden_passwords, | |
| 'forbidden-passwords': @forbidden_passwords.tap { |p| raise 'missing forbidden_passwords' if p.blank? }, |
There was a problem hiding this comment.
One thing we could do is have the template raise if
@forbidden_passwordsis nil, defined?
I don't like this but it would definitely mean we find out sooner rather than later....
Hmm. I think this would help give us visibility where disparate setup logics fail to assign the expected view variable, but I'd worry if the instinct to resolve that would be to patch where it's missing, vs. to avoid having multiple setups in the first place.
I was thinking more along the lines of: Could we redirect the user to the edit handler? Or otherwise reuse the logic of the edit method where this is set up initially?
There was a problem hiding this comment.
Another option would be to add a helper method like load_forbidden_passwords which would:
- raise an error if undefined
- highlight a need to include the helper in the controller class so it's available?
There was a problem hiding this comment.
Another option would be to add a helper method like
load_forbidden_passwordswhich would:
Part of the challenge with this is that the source of those values often changes (e.g. user lookup by token, etc).
One minor improvement I considered is to avoid referencing the controller variable from the partial, which I've read is a common Rails anti-pattern obscuring where these variables are referenced.
In 2d0c49a, I revised to pass the value to the partial from the controller view, which gives a bit more transparency to where we're expecting it to be assigned. Notably, in doing this, I noticed another case where we weren't passing the value: In the event disavowal form. This is resolved in 5bae171.
I was curious why the event disavowal path wasn't one of the original routes included in the error being resolved here (LG-3738). It turns out that there's another error preventing it from getting to the point where the original reported error would occur 😅 ("Cannot read property 'addEventListener' of null"). This is fixed in 2271b9b.
It's improved now, though this all seems to reinforce the fact that manually preparing the variable (and hard-coded references to password input IDs) is rather brittle.
There was a problem hiding this comment.
Using a local variable to the partial seems like a good compromise
|
Forgot to update tests with changes in cdafa30. Will plan to do that in the morning. At least we have confidence they're working 😅 |
**Why**: It should be fine to consider a "try" as in a purely optional sense; if it fails, so be it, no need for ceremony. In fact, the opposite could be argued to be more problematic: Forcing a catch body could encourage all-encompassing try blocks, which are unnecessarily broad in scope and likely to overlook potential error cases.
**Why**: Allow rails to manage data tag value rendering
**Why:** In case the attribute is not set or improperly formatted, password strength check shouldn't be affected.
**Why**: Expected by view. Consistently prevent user from attempting to use these passwords.
**Why**: More obvious when not passed
aduth
force-pushed
the
aduth-json-parse-error
branch
from
7d1215a to
2271b9b
Compare
| const input = document.querySelector( | ||
| '#password_form_password, #reset_password_form_password, #update_user_password_form_password', | ||
| [ | ||
| '#password_form_password', | ||
| '#password_reset_from_disavowal_form_password', | ||
| '#reset_password_form_password', | ||
| '#update_user_password_form_password', | ||
| ].join(','), |
There was a problem hiding this comment.
Now that we have the partial reliably setting data-forbidden what if we used that to query for instead of these specific IDs?
There was a problem hiding this comment.
Or maybe we add a specific attribute like data-password-strength?
There was a problem hiding this comment.
The partial is separate from the inputs targeted by these selectors. Right now, the partial only represents the feedback text for password strength. The input itself is rendered outside the partial in the form.
I do think it would probably make most sense to consider the password field with associated strength feedback as a single unit, which would make what you propose a bit more obvious.
2 participants
Why: User may be forbidden from choosing certain passwords which would be easily guessable. Specifically, the flow where a user enters an invalid password will not assign the expected view variable
forbidden_passwords.Open Questions: Is there a better way to ensure consistency of or consolidate how the password edit view is prepared? Similarly, there appear to be a few different variations of users (
token_user,build_user,resource) within theResetPasswordsControllerclass, not all of which are available to reference in every method handler.