-
-
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 actual support for turning the welcome page off or on #2073
Conversation
- Add the can_edit? method to user model so the app wont 500 on the "users" tab in the back end
- Add configuration setting for show_welcome_page in Refinery::Core - Add the show_welcome_page? method that the guides suggest to monkey patch to the core ApplicationController - Update the docs with the new hotness
@@ -83,7 +87,7 @@ def presenter_for(model, default = BasePresenter) | |||
end | |||
|
|||
def refinery_user_required? |
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 method name is disingenuous as it does not return a boolean or even truthy value. Perhaps it should be create_refinery_user_if_required
or similar.
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.
Or redirect_if_refinery_user_required!
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.
Or require_refinery_user!
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.
Last ones seems best.
We removed it from ApplicationController in master, it's just a thing |
@parndt Do you mean override |
@@ -65,6 +65,10 @@ def login? | |||
|
|||
protected | |||
|
|||
def show_welcome_page? | |||
!!Refinery::Core.show_welcome_page | |||
end |
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 think here you could just use:
delegate :show_welcome_page?, :to => Refinery::Core, :prefix => nil
Or
delegate :show_welcome_page, :to => Refinery::Core, :prefix => nil
alias_method :show_welcome_page?, :show_welcome_page
Or no?
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.
Possibly... I haven't tried. The logic behind putting it here was that the guide instructs you to put it in your application controller (as if it's going to override the same method in Refinery's application controller). It turns out by doing this, what the guide tells you to do actually works.
Please see my refactoring in #2076 too. |
Ok, the state of affairs is that this functionality has been changed slightly on master and the docs are incorrect for master and 2-0-stable. The functionality and naming on master are good and don't need a configuration to make things easier to understand. 2-0-stable shouldn't be adding new features anyway. Therefore, I think the best course of action related to this pull request is to just change the documentation. I imagine something like: We need to also avoid the show_welcome page in test mode or our ExistingApp existing integration tests will suddenly stop working.
NOTE: For branch @2-0-stable@ the relevant method is @refinery_user_required?@ in @app/controllers/application_controller@
<ruby>
class ApplicationController < ActionController::Base
...
def refinery_user_required?
false
end
...
end
</ruby>
NOTE: For branch @master@ the relevant method is @require_refinery_users!@ in @app/controllers/refinery/admin/base_controller@
<ruby>
class Refinery::Admin::BaseController
...
def require_refinery_users!
false
end
...
end
</ruby>
Ok. We should be ready to give it a spin. The intro sentence about this being for testing might need to be changed too. For tests it's easy enough to achieve the same result with a stub. |
Looks good except require_refinery_users! doesn't apply to ApplicationController; it applies to Refinery::AdminController :-) Refinery users aren't required on the frontend anymore from 2.1.0 -> Those notes would read better if you said "on versions before 2.1.0" and "on versions 2.1.0 and onward" so that people don't go "I'm not using git branches". Good summary, thanks! |
The guides suggest if you're using an existing application with devise you can add the
show_welcome_page?
method to your application controller and tell it to returnfalse
it will keep the welcome page from showing. Needless to say, this wasn't a thing. I've made this a thing. It's now a core refinery setting.