apotonick / apotomo

MVC Components for Rails.
http://apotomo.de
654 stars 68 forks source link

url_for_event does not work with nested routes. #67

Open bradphelan opened 12 years ago

bradphelan commented 12 years ago

Say I have an meeting management website. To access a meeting you go to URL

http://meeting.com/meeting/1

and you are presented with the web interface for a meeting with id = 1. Each meeting can have a comments thread and the comments thread is embedded within an Apotomo cell. All is good :). My meetings controller is

class EventsController < ApplicationController
  respond_to :html, :js

  # CANCAN should load the resource
  load_resource :only => :render_event_response

  has_widgets do |root|
    root << panel = widget("comments/panel", :comments, :meeting => @meeting)
  end

   ...
end

The @meeting object is loaded via CANCA and passed to the main widget. However I now have a problem in that I wish to have an Apotomo event called for deleting a comment. In my view I create a link for my delete button.

  = link_to url_for_event(:destroy,
         :source => widget_id, 
         :id => @meeting.id, 
         :comment_id => comment.id),
         remote: true

The extra parameter :id => @meeting.id should not be necessary as url_for_event is being called in the context of http://meeting.com/meeting/1 and the url should either be generated as

http://meeting.com/meeting/1/render_event_response?.... params ...

or

http://meeting.com/meeting/render_event_response?id=1&....params....

or am I doing something totally back to front here. Having to put the

             :id => @meeting.id, 

explicity in the call to url_for_event is breaking encapsulation as meeting should be passed into the widget from the top level.

bradphelan commented 12 years ago

I think it can be fixed here

----------------------
/Users/bradphelan/.rvm/gems/ruby-1.9.2-p180@gobbq/gems/apotomo-1.2.2/lib/apotomo/widget.rb
----------------------
133     def address_for_event(type, options={})
134       options.reverse_merge!  :source     => name,
135                               :type       => type,
136                               :controller => parent_controller.controller_path  # DISCUSS: dependency to parent_controller.  
137     end
bradphelan commented 12 years ago

Fix turns out to be trivial. Just need to add another route to the apotomo routing

----------------------
/Users/bradphelan/.rvm/gems/ruby-1.9.2-p180@gobbq/gems/apotomo-1.2.2/config/routes.rb
----------------------
  1 Rails.application.routes.draw do
  2   match ":controller/render_event_response", :to => "#render_event_response", :as => "apotomo_event"
  3   match ":controller/:id/render_event_response", :to => "#render_event_response", :as => "apotomo_event"
  4 end
bradphelan commented 12 years ago

I have an even simpler solution as the above route matching breaks under several circumstances. I have a base widget with the following implementation

# A BaseWidget to apply stuff to widgets across
# the board. All widgets should inherit from this
class ApplicationWidget < Apotomo::Widget
  include ActionController::RecordIdentifier # so I can use `dom_id`

  def url_for_event type, options={}
    p = {}
    parent_controller.request.parameters.each do |k,v|
      if k.end_with? "_id" or k == "id"
        p[k] = v
      end
    end
    super type, p.merge(options)
  end

end

What I do is forward any parameters that are either id or _id* as URL parameters to url_for_event. This catches most cases for needing to render a widget at a specific resource and wanting that resource identifier carried automatically over in the render_event_response** call.

apotonick commented 12 years ago

Ah, no ifs, that'll be magic :-) This is definitely a routing problem - we must fix that with Rails.

bradphelan commented 12 years ago

This is interesting as it also suggests a solution to state handling between events without using the session. url_for event can be hijacked to add arbitrary parameters to the the event url. I've added the following changes to by base application widget.

class ApplicationWidget <  Apotomo::Widget

      def self.stateful_option option
        @stateful_options ||= []
        @stateful_options << option
      end

      def self.stateful_options
        @stateful_options
      end

      # Make sure that all id or *_id parameters
      # are forwarded to the event URL so that 
      # resources are recovered correctly when 
      # the event is executed.
      def url_for_event type, opts={}
        p = HashWithIndifferentAccess.new
        parent_controller.request.parameters.each do |k,v|
          if k.end_with? "_id" or k == "id"
            p[k] = v
          end
        end

        so = {}
        self.class.stateful_options.each do |opt|
          so[opt] = options[opt]
        end

        opts = p.merge(opts).merge :state => {
          widget_id => so
        }

        super type, p.merge(opts)
      end

      def options
        unless @stateful_options
          @stateful_options = super
          self.class.stateful_options.each do |opt|
            if params[:state] and params[:state][widget_id]
              @stateful_options[opt] = params[:state][widget_id][opt]
            end
          end
        end
        @stateful_options
      end
end

and I can then declare options to persist into the events

class PublicEventsWidget < ApplicationWidget

  responds_to_event :refresh

  stateful_option :past

  <snip>

end

a call to

url_for_event(:refresh)

will generate the URL

http://localhost:3000/events/render_event_response?radius=1000&source=public_events&state%5Bpublic_events%5D%5Bpast%5D=&type=refresh

or when decoded

http://localhost:3000/events/render_event_response?radius=1000&source=public_events&state[public_events][past]=true&type=refresh

The overridden options method processes the params list looking for an item with the same widget_id and key in the state parameter and then adds this to the options.

The URL length limit is about 2000 characters so this approach has it's limit.

bradphelan commented 12 years ago

The other way to do it would be to hijack widget_div and inject json into an html5 data attribute and pick the data up appended to the ajax post.

<div class="widget" id="public_event" data-state="{past:true}">
    </snip>

    <a href="/events/render_event_response" data-remote="true" data-widget="true"> foo </a>
</div>

then we can hook the jquery ujs driver with

 $(document).ready =>
  apply = =>

    $('body').on 'ajax:before', '*',  (e)->
      e.stopPropagation()
      state = $(@).closest(".widget").data('state')
      $(@).data('params', state)

  apply()

Just verified that the above works very nicely in my code.

bradphelan commented 12 years ago

Actually I had to override widget_div with the following implementation to get the above working

module Apotomo::Rails::ViewHelper
  def widget_tag(tag, options={}, &block)
    if options[:class]
      options[:class] = "#{options[:class]} widget"
      options['data-state'] = stateful_options.to_json
    end

    options.reverse_merge!(:id => widget_id) 
    content_tag(tag, options, &block)
  end

  def widget_div(options={}, &block)
    widget_tag "div", options, &block
  end

end
bradphelan commented 12 years ago

The above solution ( proposed by me ) is wrong. I don't believe it is the widgets job to specify if any of it's options are persisted or not. I think this should be done in the by whatever creates the widget. For example. I have a standard rails controller that needs a widget.

class EventsController < ApplicationController

    has_widgets do
        root << widget(:events , 
                     'my_events', 
                     :geolocate => false, 
                     :filter => my_events, 
                     :title => :as_host,
                     :past => params[:past] )
         end

  end

Now the problem here is that if we call url_for_event in the context of any of the views of :events widget then the handler will be missing the :past parameter. Who's responsibility is it to make the :past parameter stateful. I would argue that it is not the widgets responsibility as in other circumstances I might hard code the :past parameter or have some other algorithm based on the position of the moon etc to set it. Obviously it is whoever/whatever instantiates the widget to decide which parameters are stateful and which are not. I propose a simple solution for this

class EventsController < ApplicationController

    has_widgets do
        root << widget(:events , 
                     'my_events', 
                     :geolocate => false, 
                     :filter => my_events, 
                     :title => :as_host,
                     :past => remember(params[:past] ) )
         end

  end

In fact I would say that the only widget that should ever use the remember function would be a root widget. All other widgets should derive their state from either the database or the state of their parent widget.

I have a solution that works for this based on some of the code of my previous notes.

We need a javascript helper

$(document).ready =>

      rootWidget = (leaf)=>
        leaf.parents(".widget").last()

      apply = =>

        $('body').on 'ajax:before', '*',  (e)->

          target = $(@)
          e.stopPropagation()

          # Only the root widget has state 
          # parameters
          widget = rootWidget(target)

          state = widget.data 'state'
          id = widget.attr 'id'
          params = target.data('params')
          params = {} unless params?
          params['state'] = {}
          params['state'][id] = state

          target.data('params', params)

      apply()

Our widget_tag and widget_div helpers now look like

module Apotomo::Rails::ViewHelper
  def widget_tag(tag, options={}, &block)
    if options[:class]
      options[:class] = "#{options[:class]} widget"
      options['data-state'] = stateful_options.to_json
    else
      options[:class] = "widget"
    end

    options.reverse_merge!(:id => widget_id) 
    content_tag(tag, options, &block)
  end

  def widget_div(options={}, &block)
    widget_tag "div", options, &block
  end

end

and we need to override options and provide stateful_options methods within the Widget (sub)class

  def stateful_options
    options.select do |k,v|
      v.is_a? Apotomo::StatefulOption 
    end
  end
  helper_method :stateful_options

  def options
    unless @options_with_state
      @options_with_state = super
      if params[:state]
        if params[:state][widget_id]
          @options_with_state = @options_with_state.with_indifferent_access.merge params[:state][widget_id].with_indifferent_access
        end
      end
    end
    @options_with_state
  end

  def options
    unless @options_with_state
      @options_with_state = super
      if params[:state]
        if params[:state][widget_id]
          @state = params[:state][widget_id].each do |k,v|
            @options_with_state[k.to_sym] = Apotomo::StatefulOption.new(v)
          end
        end
      end
    end
    @options_with_state
  end

Finally we need to create the remember function which is pretty easy as we just use the SimpleDelegator ruby class to mark it. There are other way to do this I guess but this works.

# lib/apotomo/stateful_option.rb

require 'delegate'

class Apotomo::StatefulOption < SimpleDelegator
end

class ActionController::Base
  def remember val

    Apotomo::StatefulOption.new(val) unless val.blank?
  end
end

I think those are all the moving parts and it works very nicely for me now.

apotonick commented 12 years ago

Good stuff :-) We were already designing a stateful engine for Apotomo with adapters for session storage and the way you propose where options get saved in the page and then appended to URLs. Basically it would work like this (note that it is the widget's job to define stateful attributes!).

class CommentsWidget < Apotomo::Widget
  include Stateful

  stateful_attrs :comment_ids, :user_id
end

apotomo-stateful would now worry about making those instance variables available in the next request. This is very close to what you wrote.

The biggest problem I have is writing the JavaScript that appends data to all URLs - maybe you can help out here?

bradphelan commented 12 years ago

I'm still not sure about defining state attributes within the widget. Perhaps some use cases would clarify. I think pagination is probably an internal state like use case.

It's pretty easy to attach json to html objects. In rails ( haml )

.widget{ :id => "comment_widget" :data-state => @state.to_json}

    = some_cool_widget_stuff

In javascript ( coffeescript)

# Find a widget

widget = $("#comment_widget")

# Get it's state data as json

state = widget.data('state')

Sending back json with a remote link in

%a{href=>url_event_for(:click), :remote => true, :data-param=>@state.json}

Note that the rails UJS driver automagically picks up the html5 data param and delivers it as extra post / get data. That is the basics of it. Most of the more complex code I wrote above was just about correctly namespacing and collecting the data.

A small change to the javascript code above will collect state data from all widgets from the root of the widget tree and add it to the target element for ujs to deliver to the server. ( Not tested but I think it will work )

$(document).ready =>

  allWidgetsFromRoot = (leaf)=>
    # First find the root widget then find
    # all widgets from the root
    root = leaf.parents(".widget").last()
    all_children = root.find(".widget")

  apply = =>

    $('body').on 'ajax:before', '*',  (e)->

      target = $(@)
      e.stopPropagation()

      # Preserve hard coded params
      params = target.data('params')
      params = {} unless params?
      params['state'] = {}

      # Collect all state parameters and 
      # provide them to the UJS driver via
      # the 'data-params' attribute
      allWidgetsFromRoot(target).map (widget)=>
        state = widget.data 'state'
        id = widget.attr 'id'
        params['state'][id] = state

      target.data('params', params)

  apply()

I'd be happy to help out :)

kristianmandrup commented 11 years ago

@bradphelan and @apotonick what is the status of this work? I would love to get this into apotomo and working. I'm a bit sad to see that apotomo seems to have stalled while cells is progressing ahead. What is the reason behind this? Cheers!

apotonick commented 11 years ago

The thing is that Cells is becoming more and more popular whereas Apotomo is still a "secret treasure", used by many people, but not as actively propagated as Cells since it completely changes the "Rails way". I'd love to see it evolve more but I have 4 big gems to maintain and I only have limited resources. If you're up for polishing it with my help I am ready to go!!!

kristianmandrup commented 11 years ago

I'm ready to go! I'm using it in a large scale project that is split up in multiple engines. I love the more OO approach of Cells and Widgets instead of the very "flat" Rails way, which has little to do with OOP, even with the Rails 3 architecture, except for a very primitive use of MVC (also the idea of "thick" models is "sick" IMO).

Seems like script programmers prefer the procedural approach, managing their code by simply subdividing into modules, causing all kinds of mayhem and disorganisation IMO. Typical beginner approach I think.

If you look at my fork, you will see I have made some major changes, such as restructuring how the widget tree is built. This is still an experiment. I would also like to see some of the ideas discussed in this particular issue (67) to be put into action. What are your ideas for improvement and main concerns?

kristianmandrup commented 11 years ago

On a side note, I was just reading through the Cells docs and wiki to get a better grasp of the fundamentals. I stumbled onto this:

`Using the *_url helpers implies accessing the request instance, which kinda breaks encapsulation.``

Could you explain what you mean by this? I would assume you are trying to say, that inside a cell there would normally be no need to generate urls, since it should only render the views that relate directly to its actions?