Adds feature require_password_to_destroy #4840
Conversation
|
Hey guys! This is a feature I would love to have in my rails app! |
| @@ -38,6 +38,21 @@ | |||
|
|
|||
| <h3>Cancel my account</h3> | |||
|
|
|||
| <p>Unhappy? <%= button_to "Cancel my account", registration_path(resource_name), data: { confirm: "Are you sure?" }, method: :delete %></p> | |||
| <% if resource.class.require_password_to_destroy %> | |||
| <%= form_for(resource, as: resource_name, url: registration_path(resource_name), html: { method: :delete }) do |f| %> | |||
There was a problem hiding this comment.
This form is exactly the same as the previous one. This will generate invalid HTML - multiple elements with the same ID.
One solution that I see is to rename this form so that it won't cause conflicts with the previous one.
| @@ -97,6 +109,10 @@ def update_resource(resource, params) | |||
| resource.update_with_password(params) | |||
| end | |||
|
|
|||
| def destroy_resource(resource, params) | |||
| resource.destroy_with_password(params) | |||
There was a problem hiding this comment.
Here the params parameter is actually only the literal current_password, right? If that's the case, I'd either rename it or change it here to receive the full params hash and then pass only the current_password to resource#destroy_with_password
| end | ||
| else | ||
| resource.destroy | ||
| Devise.sign_out_all_scopes ? sign_out : sign_out(resource_name) |
There was a problem hiding this comment.
WDYT about creating a method to extract this sign out logic? That way we can use it in both scenarios, without the need to duplicate it.
|
|
||
| assert_current_url '/users' | ||
| end | ||
| assert_equal "[email protected]", User.to_adapter.find_first.email |
There was a problem hiding this comment.
I think this assertion can be removed. The email wasn't changed in this test.
| sign_in_as_user | ||
| get edit_user_registration_path | ||
|
|
||
| within(".destroy-password") do |
There was a problem hiding this comment.
I already mentioned the changes needed in the form, but just to add a comment here that ideally, for integration tests we won't include much implementation details - like a CSS class, for example - because that would made the test more fragile - e.g. if we change the CSS class the test would break, even though the system is still functioning the same way.
|
@iorme1 are you still going to work on this? I'm asking to know if I should assign it to someone else. |
tegon
force-pushed
the
master
branch
2 times, most recently
from
90a1ac5 to
4501682
Compare
|
Closing in favor #5029 |
This feature addresses issue #4816. I added a setting for requiring a user's current password in order to destroy their account. This setting is set to false by default. Please let me know if I went about coding this feature according to your typical structure. I tried to follow Devise's code structure as closely as possible. All of Devise's tests including my test additions are passing. I also tried this feature out on my own test app with no issues.