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

Change options_for from class method to instance method #147

Merged
merged 1 commit into from
Jul 2, 2015
Merged

Change options_for from class method to instance method #147

merged 1 commit into from
Jul 2, 2015

Conversation

rupurt
Copy link

@rupurt rupurt commented Jun 17, 2015

Based on the thread from #146

This will help with:

  • Allowing a controller to use instance data e.g. current_user
  • Allowing a controller instance to override the default keys set in ::SecureHeaders::Configuration.configure

@oreoshake
Copy link
Contributor

@rupurt
Copy link
Author

rupurt commented Jun 17, 2015

Because I moved the method from public to private I was thinking the existing test coverage would suffice. If that doesn't work what kind of a test would you like to see?

@oreoshake
Copy link
Contributor

In one of the fixture apps, override the default behavior for a single action that would alter the policy and verify the value on the way out. It will at least show that per-action results are possible.

e.g. add an action to https://github.com/twitter/secureheaders/blob/master/fixtures/rails_4_1_8/app/controllers/things_controller.rb, override options_for or whatever with something like return something if params[:action] == :my_new_action, check the policy in https://github.com/twitter/secureheaders/blob/master/fixtures/rails_4_1_8/spec/controllers/things_controller_spec.rb

@oreoshake
Copy link
Contributor

btw, I submitted an alternative: #148

I do plan on shipping both options. I like #148 because it keeps all of the related code in the config file. That handles the case where the logic isn't too complex. I still like this approach for more complicated scenarios.

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