Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Find account record from resourceful route when user model is overridden #377

Closed
wants to merge 1 commit into from

4 participants

Dan Buettner Ben Orenstein Derek Prior Greg Lazarev
Dan Buettner

The password reset function breaks when the user model is overridden, as the record ID from the resourceful route is expected to be available as :user_id but instead is e.g. :person_id

This corrects that.

I'm not wild about the dummy class definitions in the password controller spec file; suggestions for improving that aspect welcomed.

Ben Orenstein r00k commented on the diff
spec/controllers/passwords_controller_spec.rb
((9 lines not shown))
describe Clearance::PasswordsController do
+
+ context "custom user model adaptations" do
+ before do
Ben Orenstein Admin
r00k added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Ben Orenstein
Admin

I had one comment. Once that's addressed I'm fine with merging this.

Greg Lazarev gylaz commented on the diff
spec/controllers/passwords_controller_spec.rb
((35 lines not shown))
+ end
+
+ it "is :person_id" do
+ controller.send(:user_model_identifier).should eql(:multi_word_human_being_klass_id)
+ end
+ end
+ end
+
+ context "when the user model is User" do
+ describe "#user_model_identifier" do
+ it "is :user_id" do
+ controller.send(:user_model_identifier).should eql(:user_id)
+ end
+ end
+ end
+ end
Greg Lazarev Admin
gylaz added a note

Generally, we prefer not to write tests for private methods. Isntead, could you write a controller (regression) test that demonstrates the bug/failure that you found, and passes with this new fix?

Derek Prior Admin

I'm trying to do this myself right now. It's difficult because you need to override the routes in the controller spec as well. I'm not sure there's an easy way to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Derek Prior derekprior referenced this pull request from a commit
Derek Prior derekprior Get user_model id parameter from configuration
The password reset controller was previously assuming that the id
parameter would be available as 'user_id', but if you have customized
the user model this won't be the case. Get the proper name from the
configuration.

Supersedes #377
dd51b8e
Derek Prior
Admin

I went in a different, simpler-to-test direction with #395. Closing this.

Derek Prior derekprior closed this
Derek Prior derekprior referenced this pull request from a commit
Derek Prior derekprior Get user_model id parameter from configuration
The password reset controller was previously assuming that the id
parameter would be available as 'user_id', but if you have customized
the user model this won't be the case. Get the proper name from the
configuration.

Supersedes #377
0f71807
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
6 app/controllers/clearance/passwords_controller.rb
View
@@ -51,7 +51,7 @@ def password_reset_params
def find_user_by_id_and_confirmation_token
Clearance.configuration.user_model.
- find_by_id_and_confirmation_token params[:user_id], params[:token].to_s
+ find_by_id_and_confirmation_token params[user_model_identifier], params[:token].to_s
end
def find_user_for_create
@@ -100,4 +100,8 @@ def url_after_create
def url_after_update
Clearance.configuration.redirect_url
end
+
+ def user_model_identifier
+ "#{Clearance.configuration.user_model.to_s.underscore.downcase}_id".to_sym
+ end
end
49 spec/controllers/passwords_controller_spec.rb
View
@@ -1,6 +1,55 @@
require 'spec_helper'
+class Person
+end
+
+class MultiWordHumanBeingKlass
+end
+
describe Clearance::PasswordsController do
+
+ context "custom user model adaptations" do
+ before do
Ben Orenstein Admin
r00k added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ @original_user_model = Clearance.configuration.user_model
+ end
+ after do
+ Clearance.configuration.user_model = @original_user_model
+ end
+
+ context "when the user model is Person" do
+ describe "#user_model_identifier" do
+ before do
+ Clearance.configuration.user_model = Person
+ end
+
+ it "is :person_id" do
+ controller.send(:user_model_identifier).should eql(:person_id)
+ end
+ end
+ end
+
+ context "when the user model is MultiWordHumanBeingKlass" do
+ describe "#user_model_identifier" do
+ before do
+ Clearance.configuration.user_model = MultiWordHumanBeingKlass
+ end
+
+ it "is :person_id" do
+ controller.send(:user_model_identifier).should eql(:multi_word_human_being_klass_id)
+ end
+ end
+ end
+
+ context "when the user model is User" do
+ describe "#user_model_identifier" do
+ it "is :user_id" do
+ controller.send(:user_model_identifier).should eql(:user_id)
+ end
+ end
+ end
+ end
Greg Lazarev Admin
gylaz added a note

Generally, we prefer not to write tests for private methods. Isntead, could you write a controller (regression) test that demonstrates the bug/failure that you found, and passes with this new fix?

Derek Prior Admin

I'm trying to do this myself right now. It's difficult because you need to override the routes in the controller spec as well. I'm not sure there's an easy way to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+
describe 'a signed up user' do
before do
@user = create(:user)
Something went wrong with that request. Please try again.