Hobo / hobo

The web app builder for Rails (moved from tablatom/hobo)
http://hobocentral.net
103 stars 39 forks source link

Using Ajax with transition-buttons in Hobo 2.0 pre7 #18

Closed iox closed 11 years ago

iox commented 11 years ago

When using Ajax, the transition-button does not work. Example:

<show-page:>

  <prepend-content-body:>
    <h3>Without Ajax (it works)</h3>
    <transition-buttons/>

    <h3>With Ajax (it does not work)</h3>
    <div part="mypart">
      <transition-buttons ajax/>  
    </div>
  </prepend-content-body:>

</show-page:>

After some debugging I found out that the server is detecting the call as GET, instead of PUT. It looks like the HTML generated for Ajax requests is not setting the method correctly. I made a small workaround to the transition-button tag to force the method and action:

diff --git a/hobo_rapid/taglibs/buttons/transition_button.dryml b/hobo_rapid/taglibs/buttons/transition_button.dryml
index 6a7f3b3..df47c1a 100644
--- a/hobo_rapid/taglibs/buttons/transition_button.dryml
+++ b/hobo_rapid/taglibs/buttons/transition_button.dryml
@@ -50,9 +50,9 @@ If the transition could not be found, the user does not have permissions for the

     if (!ajax_attributes.empty?) && !has_params
       ajax_attributes[:message] ||= label
-      ajax_attributes[:method] = html_attributes[:method]
-    %><form lifecycle="&transition_name" merge-attrs="&ajax_attributes" class="button_to" param>
+    %><form action="&object_url(this, transition_name)" method="post" lifecycle="&transition_name" merge-attrs="&ajax_attributes" class="button_to" param>
         <input type="hidden" name="key" value="&this.lifecycle.provided_key" if="&this.lifecycle.provided_key"/>
+        <input type="hidden" name="_method" value="put"/>
         <submit label="&label" merge-attributes="&html_attributes" param="button"/>
       </form><%
     else %><%=

I suspect I might be breaking some other use cases. What do you think?

iox commented 11 years ago

I have found something else that it's not working as it should. In the hidden-fields of the transition-button form, the "page_path" is being composed of the action URL + current URL + part options. Is this normal?

<div class="hidden-fields">
  <input type="hidden" value="/facturas/10?page_path=/expedientes/26/edit&amp;render%5B0%5D%5Bpart_context%5D=BAhbCToUbW9zdHJhcl9mYWN0dXJhSSISZXhwZWRpZW50ZToyNgY6BkVGWwBbBkkiD2V4cGVkaWVudGUGOwZG--3e09ac4c6d020996cbbb3820c9df9b67b1f9d547&amp;render%5B0%5D%5Bid%5D=mostrar-factura&amp;_=1355677259511" name="page_path" id="page_path">
  <input type="hidden" value="Ll48Xi+alIbh3HpiaiZPy91OW5TVFPE2ELt0HBfyKO0=" name="authenticity_token">
</div>
iox commented 11 years ago

Ok, I found something about my second issue. That weird page_path hidden field happens when a transition-button (or any form actually) has been rendered in a part updated with a link. Very difficult to describe in words, so here's the example. The transition button will work the first time, but not after clicking on the Ajax link.

<show-page:>
  <prepend-content-body:>
    <h3>Ajax link</h3>
    <a href="/orders/new" update="mypart" params="&{:page_path => '/orders/1'}">
      Link
    </a>

    <h3>Transition Button With Ajax</h3>
    <div part="mypart">
      <transition-buttons ajax/>  
    </div>

  </prepend-content-body:>

</show-page:>

I searched a bit and I think we can safely make the following change in Hobo:

diff --git a/hobo_rapid/app/helpers/hobo_rapid_helper.rb b/hobo_rapid/app/helpers/hobo_rapid_helper.rb
index 5eff39b..91462d0 100644
--- a/hobo_rapid/app/helpers/hobo_rapid_helper.rb
+++ b/hobo_rapid/app/helpers/hobo_rapid_helper.rb
@@ -168,7 +168,7 @@ module HoboRapidHelper
                      end

         unless method == "get"
-          page_path = if (request.post? || request.put? || request.delete?) && params[:page_path]
+          page_path = if params[:page_path]
                         params[:page_path]
                       else
                         request.fullpath

If the page_path is set, why not use it in GET requests?

bryanlarsen commented 11 years ago

1)

the method should be set in this line of code:

    html_attributes[:method] ||= has_params ? :get : :put

and given that the form is only displayed when !has_params, I'm not sure how your first patch is having any effect. Using <form lifecycle="foo" method="put"> should be sufficient. if it isn't, then perhaps we have a problem with our form helper.

2)

AFAICT, the behaviour you see is by design. From the documentation:

Note that <a> does NOT set the page_path parameter the way that <form> does. This means that Hobo will attempt to render a part in the DRYML for your destination path rather than a part in the current DRYML. This will only work if either the source and the destination are the same as in our first example, or if the source and destination have a similar part layout.

This is desired behaviour; <a> usually means ‘go somewhere else’ after all. If you want to set page_path, you’re usually better off using a real form:

<form action="somewhere/else" update="foo"><submit label="update foo"/></form>
ghost commented 11 years ago

1)

I have tried going back on my patch and adding method="put" to the transition-button form, but it still doesn't work.

I added a debugger before if method == "put" || method == "delete" in the form helper and I discovered that the method was being passed as a symbol (:put) instead of a string ("put"). I changed the following line in the transition-button definition and now it works:

- html_attributes[:method] ||= has_params ? :get : :put
+ html_attributes[:method] ||= has_params ? "get" : "put"

Do you think we can safely push this into Hobo?

bryanlarsen commented 11 years ago

If you want to use that code, test against both 1.8.7 and 1.9. Or just add an appropriate to_s to make sure.

Thanks for finding it. On Dec 21, 2012 5:22 PM, "Servicios unoycero.com" notifications@github.com wrote:

1)

I have tried going back on my patch and adding method="put" to the transition-button form, but it still doesn't work.

I added a debugger before if method == "put" || method == "delete" in the form helper and I discovered that the method was being passed as a symbol ( :put) instead of a string ("put"). I changed the following line in the transition-button definition and now it works:

  • html_attributes[:method] ||= has_params ? :get : :put+ html_attributes[:method] ||= has_params ? "get" : "put"

Do you think we can safely push this into Hobo?

— Reply to this email directly or view it on GitHubhttps://github.com/Hobo/hobo/issues/18#issuecomment-11628966.

ghost commented 11 years ago

I don't currently have a 1.8.7 environment to test Hobo 2.0 apps. Could you give me a hint of where the code could fail so I can add a to_s in the right place? To me that change looks quite unoffensive.

bryanlarsen commented 11 years ago

Never mind, I read your original change wrong. Push if the integration tests pass.

Thanks, Bryan On Dec 21, 2012 5:29 PM, "Servicios unoycero.com" notifications@github.com wrote:

I don't currently have a 1.8.7 environment to test Hobo 2.0 apps. Could you give me a hint of where the code could fail so I can add a to_s in the right place? To me that change looks quite unoffensive.

— Reply to this email directly or view it on GitHubhttps://github.com/Hobo/hobo/issues/18#issuecomment-11629134.

iox commented 11 years ago

Thanks, I'll push it ASAP.

By the way, about 2)

I have a use case which I don't know how to handle correctly: It's a short table with records. The user clicks on a table row and additional info about the record appears in a Ajax part below the table.

I'm currently using links inside every cell to simulate this "row-link" behaviour (http://stackoverflow.com/questions/569355). The situation is as follows:

Any suggestion for this situation? Couldn't we let the page_path be used if the user has explicitedly enabled it in a link? Something like this:

        unless method == "get"
          page_path = if params[:page_path]
                        params[:page_path]
                      else
                        if (request.post? || request.put? || request.delete?)
                          request.fullpath
                        else
                          nil
                        end
                      end
          page_path_hidden = hidden_field_tag("page_path", page_path) if page_path
        end
bryanlarsen commented 11 years ago

One can always hide the form and submit it via JavaScript, but that is silly.

Your patch seems reasonable, a may not send page path, but if somebody goes to the trouble of adding it, we should respect it... On Dec 21, 2012 5:43 PM, "Ignacio Huerta" notifications@github.com wrote:

Thanks, I'll push it ASAP.

By the way, about 2)

I have a use case which I don't know how to handle correctly: It's a short table with records. The user clicks on a table row and additional info about the record appears in a Ajax part below the table.

I'm currently using links inside every cell to simulate this "row-link" behaviour (http://stackoverflow.com/questions/569355). The situation is as follows:

  • I can't use links because Hobo won't use the page_path.
  • I don't want to use forms because I won't probably be able to get the same CSS look&feel.
  • I don't want to use JS in a external file because I don't feel I'm using all the "Hobo power"

Any suggestion for this situation? Couldn't we let the page_path be used if the user has explicitedly enabled it in a link? Something like this:

    unless method == "get"
      page_path = if params[:page_path]
                    params[:page_path]
                  else
                    if (request.post? || request.put? || request.delete?)
                      request.fullpath
                    else
                      nil
                    end
                  end
      page_path_hidden = hidden_field_tag("page_path", page_path) if page_path
    end

— Reply to this email directly or view it on GitHubhttps://github.com/Hobo/hobo/issues/18#issuecomment-11629467.

iox commented 11 years ago

Thanks Bryan for your reviews!

I tested the patch I wrote yesterday and it breaks the integration tests :P, so I rewrote it. I've been experimenting with the debugger and I'm pretty confident that removing the (request.post? || request.put? || request.delete?) checks is the best option: