-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add custom authentication (User model) setup article #2581
Add custom authentication (User model) setup article #2581
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good so far. Some small changes needed, though.
|
||
The rest of this article outlines the steps required should you decide to create | ||
a `User` model from scratch, use an authentication solution like | ||
[Devise][devise], or integrate your application's existing `User` model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing link
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this article does not handle the usage of Devise. Do you plan to extend this article? If not this paragraph should be removed as this confuses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just ran through the requirements in this article with a User
model that I set up with Devise. If someone was already using Devise for their user model they could use this article to integrate it with Solidus.
Why do you feel that mentioning Devise here as an example confuses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok, I misread the comma and the "or". Makes sense. Sorry for the confusion.
|
||
- The `Spree.user_class` is updated to your specified class. | ||
- Authentication helpers are set up for the `solidus_frontend` and | ||
`solidus_backend` views and are sent to the `Rails::ApplicationController`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ApplicationController
is not nested inside the Rails
module
|
||
By default, Solidus provides a [`Spree::LegacyUser` model][legacy-user] that | ||
offers the bare minimum functionality of a user. The model is intended to be | ||
modified by extensions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This user is only meant as test user and should not be modified by extensions. Also it is not suitable for production environments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is just a very old comment then!
# Default implementation of User.
#
# @note This class is intended to be modified by extensions (ex.
# spree_auth_devise)
When I edit this article in my next commits, I'll be sure to update this comment as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
7b1c3f5
to
764350d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left a couple of comments, great job as always. 👍
following columns: | ||
|
||
- `spree_api_key`: A string with a user's API key. This should be limited to 48 | ||
characters (`:limit => 48`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is the text between parenthesis really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😂 Probably not. Thanks for catching that.
your user should also have a `password` column. You can set up a password column | ||
however you see fit. | ||
|
||
### `spree_current_user` <span id="spree-current-user"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we say that this code (spree_current_user
and authentication helpers) goes into a controller? Or is it too obvious?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think that I should mention that. Thank you!
764350d
to
91811f6
Compare
0718b23
to
40b6534
Compare
Thank you to tvdeyen and kennyadsl for their reviews of my pull request.
I wrote an article outlining the requirements for setting up a custom
User
model or some non-solidus_auth_devise
authentication solution. I followed my own instructions while setting up the Passwordless gem with decent results. 👍This is part a larger project to improve Solidus's documentation. See this gist with the high-level table of contents. Where and how this documentation will exist is still up for discussion.