Extract application (aka client) to a module #181

Closed
wants to merge 19 commits into
from

Conversation

Projects
None yet
8 participants
Contributor

felipeelias commented Dec 16, 2012

Big change.

The Application model is now responsibility of the rails app. In a comparison with devise, this will be very similar to what the User model is.

This will affect most of what the current gem does and will break some stuff when upgrading.

The advantage is that there will be no need of monkey patching if you need to customize what are the model fields of Application which before was a bit of pain.

The generators were split into two. One for default models and the other one to generate the "client" model:

rails generate active_record:doorkeeper_tables
rails generate active_record:doorkeeper_client MODEL

Similar generators were created for Mongoid and MongoMapper

Within the client, the generator will add doorkeeper_client! method. For now it simply extend the current class into doorkeeper client's behaviour. This method will allow customization of which modules are integrated:

class Client < ActiveRecord::Base
  # future wise, this method will allow module customization 
  doorkeeper_client!

  attr_accessible :name, :redirect_uri
end

NOTE: Application ownership was removed. It is much more simpler to define what is the relationship in your own model than extending a module that cannot be customizable.

NOTE 2: If you generated the views, it is required to re-generate them since they had to be adapted to this feature.

+ module Models
+ module ActiveRecord
+ def doorkeeper_client!(options = {})
+ Doorkeeper.client = self
@piotrj

piotrj Dec 16, 2012

Owner

How about some check that the client is not yet set and raising exception when it is.
That could prevent a situation when someone by accident adds doorkeeper_client! to 2 models and one of those models will get lost.

@felipeelias

felipeelias Dec 17, 2012

Contributor

Added that in the setter for client since we have different places where this option gets set (b398e6e)

This config seems misplaced and I might move it somewhere else in the future

maletor commented Dec 18, 2012

Nice!

I didn't see anything in the generator to force load the user's client model (so that doorkeeper_client! will be executed) - intentional and to_be_documented? Otherwise when classes are lazy loaded in development mode calls to doorkeeper endpoints will fail until the client model gets loaded.

Contributor

felipeelias commented Dec 26, 2012

@smarterclayton it was intentional since I didn't expect that to happen. Did you run into any issues with that?

One way to solve this (I guess) would move the doorkeeper_client! logic to a config option in the initializer so the gem will know which model is the client. I'm not sure whether this is the best idea or not.

Any suggestions?

It might make sense to let someone set the string/class on the config and then do a lazy constantize when Doorkeeper.client is called.

It was fairly mucky to handle this in a doorkeeper initializer, since I needed to require doorkeeper/models/mongoid.rb before loading my model so that doorkeeper_client! is available. I saw that enable_orm loads the models but not the mongoid.rb file, and the initializer will load it, but not when rails lazily loads mongoid in dev mode. It may make sense to use an activesupport::on_load hook for requiring mongoid.rb (I believe that exists but haven't checked which one) instead of the initializer.

Contributor

felipeelias commented Jan 13, 2013

@smarterclayton I found out that Mongoid2 has no call to run_load_hooks whilst Mongoid3 has, so it's kinda tricky to make this work with Mongoid2

Contributor

felipeelias commented Jan 13, 2013

This was already merged into 1.0 branch.

gigr commented Oct 14, 2013

👍

gigr commented Oct 14, 2013

Is there any plan for merging this? I'd love to see it make its way in.

I second wanting this merged in.

Owner

tute commented Apr 12, 2014

Last commit in this PR was a year ago. While I'm sure ideas here are super valuable I'm currently the only maintainer for doorkeeper and I won't be able to resurrect it, so I'll close it for now.

@tute tute closed this Apr 12, 2014

@tute tute deleted the extract-application branch Jan 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment