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

Fix scope class name when setting resource by token #1482

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mcelicalderon
Copy link
Contributor

Hi! I'm pushing this change without tests just to make sure the change makes sense before actually writing the specs.

I noticed the scope name is no properly set for models that exist within another model like Users::Customer. I see in places like this that you actually handle that so not doing the same in the concern was probably just overlooked.

The current scope = rc.to_s.underscore.to_sym will generate users/customer as the scope name and this is a problem as the devise mapping name will be users_customer. The problem with this and the reason I saw this is that when using the sign_in method, warden will try to serialize the resource and when trying to do so here and here it will fail to find the custom devise serializer for the model and will simply use serialize. This can (and has happened to me) lead to cookie overflows as the model can be potentially big default serialization will include all columns in the serialized string.

Let me know what you think, happy to add specs if you think this is fine, but I guess I would have to add another model scoped to a module in order to do so.

@MaicolBen
Copy link
Collaborator

It makes sense, so yeah add specs

@mcelicalderon
Copy link
Contributor Author

Sure, will do as soon as possible.

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

Successfully merging this pull request may close these issues.

2 participants