amethyst icon indicating copy to clipboard operation
amethyst copied to clipboard

before_action not calling method

Open drujensen opened this issue 8 years ago • 17 comments

So, I'm probably doing something wrong, but I have a before_action :authorize in my controller and it doesn't seem to call the def authorize method.

I see the macros defined in the controllers for before_action. Any idea's?

drujensen avatar Aug 13 '15 18:08 drujensen

it works for me , make sure authorize is the last def in the class , its something to do with how the macro is working , if you still have issues I'll add a code sample from a working project

bararchy avatar Aug 13 '15 19:08 bararchy

Thanks for you quick response!

Here is my controller: https://github.com/drujensen/crystal-blog/blob/master/app/controllers/post_controller.cr

When I inspect the @callback_methods, it doesn't have any of the @actions I list. It has the @types instead. Not sure how this works but it seems like you would want to register the actions instead?

drujensen avatar Aug 13 '15 19:08 drujensen

I have a fix but I'm not happy with it. Basically, to register the action in the macro, you need the list of actions that the before filter applies too. Here is the fix in the Base::Controller.

  macro before_action(callback, *actions)
    {% for action in actions %}
      def _before_{{action.id}}_{{callback.id}}
      @before_callbacks["{{action.id}}"] = [] of (-> Bool) unless @before_callbacks["{{action.id}}"]?
      @before_callbacks["{{action.id}}"] << ->{{callback.id}}
      end
    {% end %}
  end

Before, this method was creating callbacks for the @types instead of the @actions.

Let me know if you have a better way of getting the list of actions. Can you grab them from the other macro called action?

drujensen avatar Aug 13 '15 20:08 drujensen

Well, macros aren't my strong side, I'll leave that to @Sdogruyol or @Codcore .

But, I know you have two options regarding before_action

# before_action :method_to_execute_before_actions, only: [:methods, :to, :use, :the, :before, :action]
before_action :authorized?, only: [:execute, :new, :notifications, :dash_board] 

Or

# This will apply "authorized?" to every action 
before_action :authorized?

bararchy avatar Aug 13 '15 20:08 bararchy

I verified that the only: option works. The before_action :authorized? doesn't work.

Specifically:

  macro before_action(callback, only=[] of Symbol)
    {% if only.empty? %}
      {% only = @type.methods.map(&.name.stringify) %}
    {% end %}

If you don't specify the only:, you can see that it tries to get the list for you. The problem is that it is creating the list from @types instead of @actions.

So the callbacks are indexed on any types that are defined in the controller, instead of the actions defined in the other macro called actions. Not sure how to get that list of actions from the other macro.

Is there a way to do this in Crystal?

drujensen avatar Aug 13 '15 21:08 drujensen

Hey @drujensen

This may help you understand the actions https://github.com/Codcore/amethyst/blob/master/src/amethyst/base/controller.cr#L54-L60

sdogruyol avatar Aug 13 '15 21:08 sdogruyol

Thanks @Sdogruyol . How do I get the list of actions in the other macro called before_action? Can one macro use the results of another?

drujensen avatar Aug 13 '15 21:08 drujensen

Well first you have to register it via actions macro and then i think you can access the actions via @actions

sdogruyol avatar Aug 13 '15 21:08 sdogruyol

unfortunately, @actions is not available in the scope of the macro.

drujensen avatar Aug 13 '15 21:08 drujensen

I see we need to tinker about macro loading order :(

sdogruyol avatar Aug 13 '15 21:08 sdogruyol

Also we need specs for before_action callbacks

sdogruyol avatar Aug 13 '15 21:08 sdogruyol

@Sdogruyol are you saying that it should be available in the macros? Let me try some things if thats true.

drujensen avatar Aug 13 '15 21:08 drujensen

Yeah.If you register it as an action first then it should be available.

sdogruyol avatar Aug 13 '15 21:08 sdogruyol

EDIT: Nope, false alarm. Its still not available even when I register the action before the before action

drujensen avatar Aug 13 '15 21:08 drujensen

We have to rethink about before_callback implementation and have some proper specs :+1:

sdogruyol avatar Aug 13 '15 21:08 sdogruyol

@Sdogruyol Ok. Thanks for your help. For now, I will use the only: option and pass in all the actions.

drujensen avatar Aug 13 '15 21:08 drujensen

Your welcome @drujensen i'll tackle this issue

sdogruyol avatar Aug 13 '15 21:08 sdogruyol