Disable Updating of User Emails #917
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- For the edited .erb files, `"disabled": true` has been set for the email fields section of forms (copies approach used in `app/views/org_admin/users/edit.html.erb`)
- For added security, we are also removing `:email` from the .permit() update params of the corresponding controllers
- Notes regarding changes to app/controllers/registrations_controller.rb:
- Because `:email` was removed from the .permit() params, all of the `update_params[:email]`-related code needed to be addressed/removed (lines 161, 174, and 200)
- The removal of `:email` from the update params allows for the removal of a lot of other code in the file.
- The `if require_password` check in the `do_update` method is no longer needed. The comment on 197 stated `# user is changing email or password`. However, line 204-206 pointed out how this case is never actually reached for password changes (`def do_update_password` is executed instead). Since we are no longer allowing for the email to be changed either, it follows that `require_password` should always evaluate to false.
- As result, we only need the code that corresponded to the `else` statement for `if require_password` (previously lines 225-228, now 185-188)
- Also, because `require_password` is now always false, it follows that we can remove both the `require_password` arg from `def do_update` as well as the entirety of `def needs_password?`
These layout changes are being made to align with the changes in commit 74c960896 (i.e. setting `"disabled": true` to user.email in "Edit Profile")
Generated by 🚫 Danger |
lagoan
requested changes
Oct 1, 2024
|
|
||
| <div class="form-group col-xs-8"> | ||
| <%= f.label(:email, _('Email'), class: 'control-label') %> | ||
| <%= f.email_field(:email, class: "form-control", "aria-required": true, value: @user.email) %> | ||
| <%= f.email_field(:email, class: "form-control", "aria-required": true, value: @user.email, "disabled": true) %> |
There was a problem hiding this comment.
I like how we are still showing the information here.
lagoan
requested changes
Oct 8, 2024
This was referenced Nov 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #916
Changes proposed in this PR:
"disabled": truewithinf.email_fieldfor all forms (the same approach is already used inapp/views/org_admin/users/edit.html.erb):emailfrom the .permit() update params of the controllers corresponding to the formsNotes regarding changes to
app/controllers/registrations_controller.rb::emailwas removed from the .permit() params, all of theupdate_params[:email]- related code needed to be addressed/removed (lines 161, 174, and 200):emailfromupdate_paramsenabled the removal of a lot of other code in the file.if require_passwordcheck in thedo_updatemethod is no longer needed. The comment on 197 stated# user is changing email or password. However, line 204-206 pointed out how this case is never actually reached for password changes (def do_update_passwordis executed instead). Since we are no longer allowing for the email to be changed either, it follows thatrequire_passwordshould always evaluate to false.elsestatement forif require_password(previously lines 225-228, now 185-188)require_passwordis now always false, it follows that we can remove both therequire_passwordarg fromdef do_updateas well as the entirety ofdef needs_password?