Hobo Hobo → Hobo 1.3 (Rails 3)

Comments and changes to this ticket

• 

  Domizio Demichelis November 27th, 2010 @ 06:04 PM

  • State changed from “new” to “open”
  • Milestone set to “Hobo 1.3 (Rails 3)”
  • Assigned user set to “Domizio Demichelis”
  • Milestone order changed from “197922” to “0”
• 

  Domizio Demichelis December 19th, 2010 @ 06:21 PM

  I am trying to reproduce it but everything is working as expected. Please, could you check it with the latest version, and eventually give me more clues. Thanks

• 

  Bryan Larsen March 11th, 2011 @ 04:46 PM

  • Milestone order changed from “36” to “0”

  The fix is definitely more intrusive than expected. Matt or Domizio, would you mind reviewing?

  • bug871.patch 2.5 KB
• 

  Domizio Demichelis March 11th, 2011 @ 05:41 PM

  Bryan, please, could you just resume what was the original problem and what the patch is trying to fix specifically? The goal is not so apparent. :-)

• 

  Bryan Larsen March 11th, 2011 @ 07:21 PM

  Sorry, I meant to give an explanation, but I wanted to post before I stepped out.

  Without the patch, linkable?(Foo.new) returns true even though you can't craft a URL to an object without an ID. This causes a couple of problems:

  1) or-cancel takes the wrong path and calls 'a' on an object that isn't linkable.
  2) 'a' messes up and returns almost by accident a link to the root of the subsite, which at least is usually a valid link. But not always, as Martin found out.

  with the patch, or-cancel takes the right path and links to the foos index (if it can). And calling <a with="&Foo.new">Yo</a> will return 'Yo' as plain text, not linked to anything.

  • bug871.patch 3.6 KB
• 

  Domizio Demichelis March 12th, 2011 @ 01:28 PM

  Bryan, if I am not missing something, this would be an obvious solution. Is it breaking something? We could eventually check for the presence of the id, as an alternative.

  • bug871-dd.patch 1 KB
• 

  Bryan Larsen March 14th, 2011 @ 11:28 PM

  It's theoretically possible that a collection or a class would be passed to or-cancel, which would break with your patch, which is a good way to get an index link rather than a link to an item. Or an action that doesn't require an ID could be passed in the attributes.

  <a> has the same bug as <or-cancel>, and fixing that isn't quite as simple as your patch.

• 

  Domizio Demichelis March 14th, 2011 @ 11:38 PM

  Bryan, could you post the code to break my patch and the <a> bug... I am thinking to a possible solution, but I need something to test against. Thank you.

• 

  Bryan Larsen March 14th, 2011 @ 11:59 PM

  Here are a couple.

   <a with="&User" action="new"/>
   <a with="&User"/>    <%# shows the index %>
  

  Hmmm, I can't think of any that should work with an unsaved record at the moment.

• 

  Domizio Demichelis March 15th, 2011 @ 11:38 AM

  Bryan, that doesn't seems to be broken at all. Your specific example could be misleading since if you try it with a non-invite-only app, there are no routes defined for :index. There are also no routes for the :new action even in an invite-only app.

  If you just change the class User to another regular model, both exaples are working as expected, so the tag and linkable? don't seem to be broken at all.

  I would apply my patch (eventually checking if this.respond_to?(:new_record?) first), unless you find any context that might be affected by it.
  Please, let me know.

• 

  Bryan Larsen March 15th, 2011 @ 02:41 PM

  I didn't list the obvious broken one:

   <a href="&Project.new"/>
  

  But that could be fixed by applying your patch to "a" as well.

  But here's one that requires the full power of my patch:

   <a href="&Project.new" action="index"/>
  

  (Think more of a situation where Project is usually, but not always, saved)

  Current behavior would link to the root path (which is broken as demonstrated by the original bug report), your patch would not link it, and mine would link to the Project's index as expected (and as happens when the project not a new record).

  My behaviour might be "more right" than yours, but yours doesn't result in broken links like the current code does, so your patch is "not wrong" in this case. And since it's much less invasive, it definitely might be preferable.

  I would apply a patch like yours to "linkable?" rather than directly to or-cancel and a. That way it stays a single line patch and covers everything.

  I wonder if we could get Matt or Tom to come in and vote.

• 

  Domizio Demichelis March 15th, 2011 @ 03:13 PM

  I think that linkable? should just tell whether a certain route exists or not: it should not incorporate a logic that should belong to the use of linkable?. In other words, the Hobo::Routes module should only know about hobo defined routes.

  If you want to put that logic centralized somewhere, we should probably create a specialized module for that, or I am fine to put the extra contdition in the specific usage we do about linkable?.

  <a href="&Project.new"/>
  <a href="&Project.new" action="index"/>
  

  Is that 'href' a typo for 'with' or is it intended? That should throw an exception.

• 

  Bryan Larsen March 15th, 2011 @ 03:23 PM

  Yes, you're right, I meant to say "with".

  Also, there are two "linkable?" functions. I meant to suggest the patch be added to helper.rb/linkable?, not to Hobo::Routes.linkable?

• 

  Domizio Demichelis March 15th, 2011 @ 04:00 PM

  Ah... right! I forgot the other linkable? :-) That is the right place to include the extra condition.

  Anyway, what action and link should the <a with="&Project.new"/> produce? Without any action attribute, I think it should be :show. Is that correct?

• 

  Bryan Larsen March 15th, 2011 @ 04:17 PM

  Yes that's correct, without an action, the default for an object is :show, and the default for a class is :index. Therefore <a with="&Project.new"> should not produce any link at all since you can't link to an unsaved object.

• You flagged this item as spam.
  

  Tim Griffin March 15th, 2011 @ 04:47 PM

  I'm seeing a related issue with a newly-generated Hobo 1.3.pre30 app (with Invite-only): the Cancel link in the auto <invite-form> should return to /admin/users since /admin does not exist as a default route.

  Tim

• 

  Domizio Demichelis March 15th, 2011 @ 07:17 PM

  This should fix them all with minimum intervention.

  However the condition has to be added in 2 places, because <a> uses object_url which doesn't use the linkable? helper.

  Bryan, let me know if that is ok for you.

  • bug871-dd-1.patch 1.5 KB
• 

  Bryan Larsen March 15th, 2011 @ 07:49 PM

  I forgot about object_url, it's the major difference between the two versions of my patch.

  Yes, your patch looks pretty much exactly like I expected it to, except that I probably would have returned nil from object_url rather than a blank string, since that seems closer to current behaviour. (I probably would also have used try instead of respond_to, but that's a pretty much irrelevant style difference).

  I'm still not totally convinced that we're better off with your patch than mine, but I'm certainly leaning that way.

• 

  Domizio Demichelis March 15th, 2011 @ 09:32 PM

  Just for splitting hairs, I further investigate into that, and try to implement the thing you want with <a href="&Project.new" action="index"/>. That seems to have always been avoided eplicitly by the obj.to_url_path, which for new_record? == true just return the '/' path.

      def to_url_path
        "#{self.class.to_url_path}/#{to_param}" unless new_record?
      end
  

  I think that a new record should never be linkable, but anyway, if passing an "index" action to a new record link makes any sense, we should change the condition in to_url_path. If we don't want to change that we can link to the home page with a little change to my last patch.

• 

  Domizio Demichelis March 15th, 2011 @ 09:39 PM

  the previous attachment is generating a 500 server error... let's see this one

  • bug871-dd-2.patch 859 Bytes
• 

  Domizio Demichelis March 15th, 2011 @ 09:47 PM

  retry

  • bug871-dd-2-copy.patch 859 Bytes
• 

  Domizio Demichelis March 15th, 2011 @ 09:49 PM

  no way... maybe it's better a copy and paste:

  
  diff --git a/hobo/lib/hobo/helper.rb b/hobo/lib/hobo/helper.rb
  index 582002d..f8a1df9 100644
  --- a/hobo/lib/hobo/helper.rb
  +++ b/hobo/lib/hobo/helper.rb
  @@ -54,6 +54,7 @@ module Hobo
       def object_url(obj, *args)
         params = args.extract_options!
         action = args.first._?.to_sym
  +      return if action.nil? && obj.try.new_record?
         options, params = params.partition_hash([:subsite, :method, :format])
         options[:subsite] ||= self.subsite
         subsite, method = options.get :subsite, :method
  @@ -395,6 +396,7 @@ module Hobo
         options = args.extract_options!
         target = args.empty? || args.first.is_a?(Symbol) ? this : args.shift
         action = args.first
  +      return false if action.nil? && target.try.new_record?
   
         if target.respond_to?(:member_class) && (origin = target.try.origin)
           klass = origin.class
  

• 

  Bryan Larsen March 16th, 2011 @ 03:47 PM

  Interesting solution. The assumption in your latter patch is that if an action is explicitly passed in, the user knows what they're doing, which isn't necessarily a bad assumption. It moves the failure mode from this:

  <a with="&Project.new" action="index"/>
  

  to

  <a with="&Project.new" action="show"/>
  

  Of course, people are unlikely to do the second, it is possible they'll use it with user-defined show actions.

  The real problem is that our documentation doesn't actually tell people how to do an index action. These both work:

  <a with="&blog_post" action="index"/>
  <a with="&BlogPost" action="index"/>
  

  Whatever patch we choose, we should fix our documentation to tell people that the latter is how to provide an index link.

  Right now, I'm leaning towards bug871-dd-1.patch along with a documentation fix.

• 

  Domizio Demichelis March 16th, 2011 @ 04:17 PM

  Bryan,
  actually I really don't know what <a href="&Project.new" action="index"/> should return, I've just tried to interpret you, and as usual I am probably wrong :-).

  IMO we are just offering too many possible ways to do simple things. But if we do, an explicit action should override the dryml guessing, as the documentation says. However in that case an index action on a new record should link to the index page of the class of the new records, but now it links to '/'.

• 

  Matt Jones March 16th, 2011 @ 04:29 PM

  I doubt anybody would actually write <a with="&Project.new" action="index"/> deliberately, but implicit context can produce the same thing pretty quick:
  

    # on a new-page
    <a action="index">foo bar</a>
  

  That seems a bit DRYer than having to write:
  

    <a to="&this.class">foo bar</a>
  

• 

  Domizio Demichelis March 16th, 2011 @ 10:57 PM

  Matt,
  I didn't think about a link to index in a new page: that makes sense!
  So the patch to allow that and fix everything else now is:

  diff --git a/hobo/lib/hobo/helper.rb b/hobo/lib/hobo/helper.rb
  index 582002d..f8a1df9 100644
  --- a/hobo/lib/hobo/helper.rb
  +++ b/hobo/lib/hobo/helper.rb
  @@ -54,6 +54,7 @@ module Hobo
       def object_url(obj, *args)
         params = args.extract_options!
         action = args.first._?.to_sym
  +      return if action.nil? && obj.try.new_record?
         options, params = params.partition_hash([:subsite, :method, :format])
         options[:subsite] ||= self.subsite
         subsite, method = options.get :subsite, :method
  @@ -395,6 +396,7 @@ module Hobo
         options = args.extract_options!
         target = args.empty? || args.first.is_a?(Symbol) ? this : args.shift
         action = args.first
  +      return false if action.nil? && target.try.new_record?
   
         if target.respond_to?(:member_class) && (origin = target.try.origin)
           klass = origin.class
  diff --git a/hobo/lib/hobo/model.rb b/hobo/lib/hobo/model.rb
  index 6fd198c..966a8a7 100644
  --- a/hobo/lib/hobo/model.rb
  +++ b/hobo/lib/hobo/model.rb
  @@ -357,7 +357,7 @@ module Hobo
   
   
       def to_url_path
  -      "#{self.class.to_url_path}/#{to_param}" unless new_record?
  +      "#{self.class.to_url_path}/#{to_param}"
       end
  

  Absolutely not invasive and compilant with all the requirement (I think).
  Is that finally ok for everybody? :-)

• 

  Domizio Demichelis March 17th, 2011 @ 07:56 PM

  ooops! I put the condition in the wrong place: object_url is used also for forms... so moving the condition in the right place this should be the one.

  diff --git a/hobo/lib/hobo/helper.rb b/hobo/lib/hobo/helper.rb
  index 582002d..cadcba4 100644
  --- a/hobo/lib/hobo/helper.rb
  +++ b/hobo/lib/hobo/helper.rb
  @@ -395,6 +395,7 @@ module Hobo
         options = args.extract_options!
         target = args.empty? || args.first.is_a?(Symbol) ? this : args.shift
         action = args.first
  +      return false if action.nil? && target.try.new_record?
   
         if target.respond_to?(:member_class) && (origin = target.try.origin)
           klass = origin.class
  diff --git a/hobo/lib/hobo/model.rb b/hobo/lib/hobo/model.rb
  index 6fd198c..966a8a7 100644
  --- a/hobo/lib/hobo/model.rb
  +++ b/hobo/lib/hobo/model.rb
  @@ -357,7 +357,7 @@ module Hobo
   
   
       def to_url_path
  -      "#{self.class.to_url_path}/#{to_param}" unless new_record?
  +      "#{self.class.to_url_path}/#{to_param}"
       end
   
   
  diff --git a/hobo/lib/hobo/rapid/taglibs/rapid_core.dryml b/hobo/lib/hobo/rapid/taglibs/rapid_core.dryml
  index ef00b6f..1e4c03f 100644
  --- a/hobo/lib/hobo/rapid/taglibs/rapid_core.dryml
  +++ b/hobo/lib/hobo/rapid/taglibs/rapid_core.dryml
  @@ -368,7 +368,7 @@ Or a new page if the context is a class:
   
         content = name if content.blank?
   
  -      href = object_url(target, action, (params || {}).merge(:subsite => subsite))
  +      href = object_url(target, action, (params || {}).merge(:subsite => subsite)) unless (action.nil? && target.try.new_record?)
         if href.nil?
           # This target is registered with Hobo::Routes as not linkable
           content
  

• 

  Domizio Demichelis March 22nd, 2011 @ 04:02 PM

  • State changed from “open” to “resolved”

  (from [060c07f4ec7cc50b2eadebbecadd766331f61520]) fix for new record not linkable [#871 state:resolved] https://github.com/tablatom/hobo/commit/060c07f4ec7cc50b2eadebbecad...