Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace the auth model concern on generator execution #53

Closed
kockok opened this issue Dec 24, 2019 · 5 comments · Fixed by #91
Closed

Replace the auth model concern on generator execution #53

kockok opened this issue Dec 24, 2019 · 5 comments · Fixed by #91
Labels
enhancement New feature or request

Comments

@kockok
Copy link

kockok commented Dec 24, 2019

Question

I see the concern is include DeviseTokenAuth::Concerns::Model
both in the docs and the dummy app.

But the generated one is include DeviseTokenAuth::Concerns::User

class User < ActiveRecord::Base
  # Include default devise modules. Others available are:
  # :confirmable, :lockable, :timeoutable and :omniauthable
  devise :database_authenticatable, :registerable,
         :recoverable, :rememberable, :trackable, :validatable, :confirmable
  include DeviseTokenAuth::Concerns::User
end

So, which one should be used?

@00dav00 00dav00 added the enhancement New feature or request label Dec 24, 2019
@00dav00
Copy link
Contributor

00dav00 commented Dec 24, 2019

Hi @kockok, DeviseTokenAuth::Concerns::Model is a wrapper on top of DeviseTokenAuth::Concerns::User, so for now you can use any of them. Nevertheless I would recommend using DeviseTokenAuth::Concerns::Model since any changes that involve modifying the integration with the model will be placed here.

I think the gem generator should replace this, so I will edit this issue so we can use as reference to make this change.

@00dav00 00dav00 changed the title Concerns for the model Replace the auth model concern on generator execution Dec 24, 2019
@mcelicalderon
Copy link
Member

So, if you take a look at the code, our concern is just an alias for DTA's concern, so as @00dav00 said, you can use any of them for the include in your model. We created GraphqlDevise::Concerns::Model so it might be consistent with this gem and kind of hide the explicit dependency of Devise Token Auth's gem. But there's actually a bug when you use our concern on your controller as described in this issue #47. So for now I would advise sticking to both DeviseTokenAuth concerns as they both work. I'll update the docs to reflect this while we figure out what the problem is with our concern, and also while our generators still generate Devise's module instead of ours.

@kockok
Copy link
Author

kockok commented Dec 26, 2019

If I use include DeviseTokenAuth::Concerns::Model,
it shows this error : NameError: uninitialized constant DeviseTokenAuth::Concerns::Model

@00dav00
Copy link
Contributor

00dav00 commented Dec 26, 2019

@kockok DTA's concern is probably called DeviseTokenAuth::Concerns::User in you case.

@mcelicalderon
Copy link
Member

I'm about to merge #54 which fixes the bug I mentioned before on using our own modules and that I will try to release today. But to make this thread clear (also updated the docs) here's a summary on the subject.

  1. For your models the generator will include DeviseTokenAuth::Concerns::User which is totally valid. Our generator includes that one because we just run internally DTA's generator (we need to fix them to include ours). Your models can also include GraphqlDevise::Concerns::Model which is just an alias for the generated one. We created this so your code doesn't need an explicit dependency to one of our dependencies and for possible extension in the future.
  2. For your controller, the generator will include DeviseTokenAuth::Concerns::SetUserByToken for the same reason (DTA's generator). For this one we also created an alias, but using that one would actually give an error before Fix concern aliases #54 was merged. So you are free to use either with a version that includes that PR. For the current released version you should include DeviseTokenAuth::Concerns::SetUserByToken

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants