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

Use new access denied extension point. #12

Merged
merged 1 commit into from
Jul 16, 2015
Merged

Use new access denied extension point. #12

merged 1 commit into from
Jul 16, 2015

Conversation

adammathys
Copy link
Member

Class attribute added in solidusio/solidus#179 allows us to more easily define the behaviour we want for the extension.

@gmacdougall
Copy link
Member

👍

@athal7
Copy link

athal7 commented Jul 14, 2015

i'm not a huge fan of this, can we find another way to decorate solidus?

Class attribute added in solidusio/solidus#179 allows us to more easily
define the behaviour we want for the extension.
@adammathys adammathys changed the title Override the right method. Use new access denied extension point. Jul 15, 2015
@adammathys
Copy link
Member Author

@athal7: Updated to hook into Solidus a bit nicer. What do you think?

Tests will fail until solidusio/solidus#179 is merged. Running locally everything is green.

return "Spree::#{const_name}".constantize
end
nil
def model_class
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm still curious why this is here as opposed to in core, though that's not central to what you're doing here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither am I. Wanted to limit the number of changes in this pull-request so I didn't spend any time looking into whether this is even necessary.

@athal7
Copy link

athal7 commented Jul 16, 2015

looks great!

@jhawthorn
Copy link
Contributor

👍

jhawthorn added a commit that referenced this pull request Jul 16, 2015
Use new access denied extension point.
@jhawthorn jhawthorn merged commit 3da8870 into solidusio:master Jul 16, 2015
@BenMorganIO
Copy link
Contributor

Writing rescue_from CanCan::AccessDenied always felt accessible to me. I can't seem to figure out how to extend this in decorators.

# GET /admin/orders

Spree::BaseController.unauthorized_redirect = -> { redirect_to main_app.root_path }

Spee::BaseController.class_eval do
  unauthorized_redirect = -> do
    redirect_to main_app.root_path
  end

  def self.unauthorized_redirect
    -> { redirect_to main_app.root_path }
  end
end

Spree::Admin::BaseController.unauthorized_redirect = -> { redirect_to main_app.root_path }

Spee::Admin::BaseController.class_eval do
  unauthorized_redirect = -> do
    redirect_to main_app.root_path
  end

  def self.unauthorized_redirect
    -> { redirect_to main_app.root_path }
  end
end

# => GET /admin/login

None of these worked for me, however, using rescue_from brought me some desired results, but still not up to what I needed.

Spree::Admin::BaseController.class_eval do
  rescue_from CanCan::AccessDenied do |_exception|
    render file: 'public/404.html', layout: false, status: 404
  end unless Rails.env.development?
end

# GET /admin
# => 404

# GET /admin/orders
# => 302

@BenMorganIO
Copy link
Contributor

Note to self and to anyone stumbling across via Google, don't forget to use self:

Spee::Admin::BaseController.class_eval do
  self.unauthorized_redirect = -> { ... }
end

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.

5 participants