search

songkick / oauth2-provider

Simple OAuth 2.0 provider toolkit
MIT License
529 stars 148 forks source link

Alter attr-accessible-fix patch so that spec passes #38

Closed pixeltrix closed 11 years ago

pixeltrix commented 11 years ago

Use an association extension module to set inverse association to the same instance as the :inverse_of association setting is only available in Active Record 3.x.

jcoglan commented 11 years ago

Sorry, I'm having difficulty separating this from the other changes in the pixeltrix-patches branch, which I'm trying to review separately. Could you line the commits up so they apply against master?

jcoglan commented 11 years ago

I might have spoken too soon. No need to do anything, I can probably sort this out myself, just having a git mental block.

jcoglan commented 11 years ago

Merged. git-cherry-pick ftw.

pixeltrix commented 11 years ago

You've probably worked it out but I merged you're recent changes from master (including the spec) into my local tracking branch of pixeltrix-patches, made the spec pass and pushed the branch to my fork. I then created the PR from the pixeltrix-patches branch in my fork against pixeltrix-patches here.

The upshot is you can merge this into pixeltrix-patches, test that branch on travis and if it's green and acceptable merge pixeltrix-patches into master.

jcoglan commented 11 years ago

Though, the tests changed in https://github.com/songkick/oauth2-provider/commit/f6a448d489e7b3520e16541a8bb8984c63217f6c are no longer sufficient: the mock expectations do not indicate that the authorization is saved, or that it is related to the client. Can this be fixed?

jcoglan commented 11 years ago

I'm basically cherry-picking out of pixeltrix-patches as I review things.

pixeltrix commented 11 years ago

Though, the tests changed in f6a448d4 are no longer sufficient: the mock expectations do not indicate that the authorization is saved, or that it is related to the client. Can this be fixed?

Changing the expectation from :create to :new on the should_not makes no difference because it's testing for not creating an authorization. The other one can be modified to add a authorization.new_record?.should be_false and a authorization.owner.should be_equal(@client)

jcoglan commented 11 years ago

The Authorization is does not get an ID, and appears to still be a new record. This spec fails: https://github.com/songkick/oauth2-provider/blob/master/spec/oauth2/model/resource_owner_spec.rb#L14

pixeltrix commented 11 years ago

I've added some additional specs here which pass. I suspect the mock on OAuth2::Model::Authorization is interfering with the Active Record association code - that's why I changed it from :create to :new because the build_association method was returning nil instead of a new instance.

jcoglan commented 11 years ago

This whole setup is not giving me much confidence -- it's more complicated than what I was doing before, and is leaving me second-guessing ActiveRecord. I don't see why stubbing Base.new should stop the record from being saved properly, but these specs just don't do what I expect.

In this method, you assign the owner and client and then don't save the object, which is a requirement. Despite this, I can't make any reasonable test for this stuff fail as I'd expect. Tests of the Provider layer are fine because Provider::Exchange calls save! on the record, but the model layer must not rely on this.

https://github.com/songkick/oauth2-provider/blob/eccb7c7d8f3739cfb51572e5a0163c66e5654a2e/lib/oauth2/model/resource_owner.rb#L10-13

pixeltrix commented 11 years ago

I don't see why stubbing Base.new should stop the record from being saved

Building models via the association has a couple of places where the block passed to a method is passed to another method inside a nested block. I think the stub was breaking the return value of that nested block so the model doesn't get returned. I'm not sure whether that's clear but the problem is here

I can't make any reasonable test for this stuff fail as I'd expect

What do you want to fail? For example if I change the find_or_create_ to find_or_initialize_ then the new_record?.should be_false fails.

In this method, you assign the owner and client and then don't save the object, which is a requirement

The object has been saved by the find_or_create_by_client_id, the assigns are just to set the instances to be the current ones rather than reloading from the database. If it could rely on :inverse_of then they wouldn't be needed.

jcoglan commented 11 years ago

Okay, I've tweaked the tests into a shape I'm happy with. Thanks for the patch :)