Fix sorting of menu items whose labels are procs #2023

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

Contributor
exviva commented Mar 26, 2013

Commit 452956e introduced a bug in MenuItem#<=>: even if the label was a proc, it would not compare label.call <=> other.label.call, but instead label.to_s <=> other.label.to_s, and Proc#to_s returns some irrelevant metadata about the Proc object.

This bug would show when the menu items got randomly rearranged on every refresh of the admin area.

@seanlinsley seanlinsley commented on an outdated diff Mar 27, 2013
lib/active_admin/menu_item.rb
@@ -71,7 +71,11 @@ def ancestors
def <=>(other)
result = priority <=> other.priority
- result = label.to_s <=> other.label.to_s if result == 0
+ if result == 0
+ label_value = label.is_a?(Proc) ? label.call : label
+ other_label_value = other.label.is_a?(Proc) ? other.label.call : other.label
+ result = label_value <=> other_label_value
+ end
result
seanlinsley
seanlinsley Mar 27, 2013 Owner

That's really ugly. Here's my take on the problem:

class MenuItem
  # This is the sorting operator for Menu Items.
  # When sorting, any manually-set priority has higher... priority.
  # Otherwise, we sort by the string value of the label (if a proc, it's eval'd).
  def <=>(other)
    result = render_or_call(label) <=> render_or_call(other.label)
    result = priority <=> other.priority
    result
  end
end

# Added into the existing view_helpers/method_or_proc_helper.rb
module MethodOrProcHelper
  # Simply returns a string value, no matter the data type.
  # This is for when you don't want to evaluate the Proc in a special context.
  def render_or_call(string_or_proc)
    case string_or_proc
    when String, Symbol
      string_or_proc.to_s
    when Proc
      string_or_proc.call.to_s
    end
  end
end

Now as it is, I'm not really sure what any of these versions of <=> is doing, since we're setting the result twice. It doesn't seem like the first comparison would ever be used.

Contributor
exviva commented Mar 27, 2013

@Daxter have a look at the commit I've mentioned - MenuItem#label used to handle that logic - now it no longer can, since the original label proc is needed elsewhere in the code.

Now as it is, I'm not really sure what any of these versions of <=> is doing, since we're setting the result twice. It doesn't seem like the first comparison would ever be used.

It's first comparing priorities, and only if they're equal, it's comparing labels.

Contributor
exviva commented Mar 27, 2013

@Daxter I've taken a slightly different approach, let me know what you think.

Owner

It's first comparing priorities, and only if they're equal, it's comparing labels.

Oh I see. I hadn't noticed that if result == 0 before.

Could you add a comment above the <=> method that makes this clearer? Something like this:

# Sort menu items first by their priority attribute, then by their labels.

Also could you add to_s to the proc caller? Devs might not always return a string.

case label
when String then label
when Proc   then label.call.to_s
end 
@exviva exviva Fix sorting of menu items whose labels are procs
Commit 452956e introduced
a bug in `MenuItem#<=>`: if the label was a proc, it would
not compare `label.call <=> other.label.call`, but instead
`label.to_s <=> other.label.to_s`, and `Proc#to_s` returns
some irrelevant metadata about the Proc object.
7705466
Contributor
exviva commented Mar 28, 2013

@Daxter done. BTW I think the whole reason why the original commit removed @label.call from MenuItem is that it wanted the proc to be instance_exec'd in the view context. This would mean that the proc would now be evaluated in 2 different contexts, whether it'd be for rendering, or for sorting, which is really confusing.

Owner

Then maybe it would be best to only sort by the priority, because this will break if the proc depends on the view context.

def <=>(other)
  priority <=> other.priority
end 

/cc @macfanatic

Owner

I don't know about you @exviva, but I always set manual priorities on my menu items for AA apps.

Contributor
exviva commented Mar 28, 2013

@Daxter I'm not sure if we're following SemVer, but if so, I think that would need a major version bump, since it breaks backwards compatibility.

So far I've never set priorities, I (and my users I managed to talk to) expect menus to appear in alphabetical order. Of course, I have no idea about the majority of AA users, and I wouldn't mind having to explicitly set priorities (even though it sounds a bit cumbersome).

How about instance_exec'ing the label proc gets reverted (as it hasn't been released yet), since that commit actually broke BC and introduced that bug in the first place?

Owner

I just opened a PR that has an alternative solution to the problem ⬆️

The big change is that it moves sorting up into TabbedNavigation where the view context is accesible.

Owner

Closing in favor of #2031.

@seanlinsley seanlinsley closed this Apr 1, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment