Skip to content

Loading…

Fixes #1585. #1882

Merged
merged 2 commits into from

3 participants

@robyurkowski

Sorting 11 items without reordering should be a no-op. However, if we sort the array by the given keys—which are strings—this will actually sort the 11th entry after the 2nd (producing an lft of 5, instead of an expected 21).

EDIT: I cannot count.

@robyurkowski

This will fix #1585.

@travisbot

This pull request fails (merged dc36f082 into 5e22bab).

@travisbot

This pull request fails (merged 4c85486 into 5e22bab).

@parndt parndt merged commit 4c85486 into master

1 check failed

Details default The Travis build failed
@parndt
Refinery member

And cherry-picked to 2-0-stable for you.

@robyurkowski robyurkowski added a commit that referenced this pull request
@robyurkowski robyurkowski Add changelog to respect #1882. a930dd8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Showing with 18 additions and 1 deletion.
  1. +1 −1 core/lib/refinery/crud.rb
  2. +17 −0 core/spec/lib/refinery/crud_spec.rb
View
2 core/lib/refinery/crud.rb
@@ -269,7 +269,7 @@ def update_positions
# After we drop Ruby 1.8.x support the following line can be changed back to
# list.each do |index, hash|
# because there won't be an ordering issue (see https://github.com/resolve/refinerycms/issues/1585)
- list.sort.map { |item| item[1] }.each_with_index do |hash, index|
+ list.sort_by {|k, v| k.to_i}.map { |item| item[1] }.each_with_index do |hash, index|
moved_item_id = hash['id'].split(/#{singular_name}\_?/).reject(&:empty?).first
@current_#{singular_name} = #{class_name}.find_by_id(moved_item_id)
View
17 core/spec/lib/refinery/crud_spec.rb
@@ -96,6 +96,23 @@ module Refinery
dummy.lft.should eq(9)
dummy.rgt.should eq(10)
end
+
+ # Regression test for https://github.com/resolve/refinerycms/issues/1585
+ it "sorts numerically rather than by string key" do
+ dummy, dummy_params = [], {}
+
+ # When we have 11 entries, the 11th index will be #10, which will be
+ # sorted above #2 if we are sorting by strings.
+ 11.times do |n|
+ dummy << Refinery::CrudDummy.create!
+ dummy_params["#{n}"] = {"id" => "crud_dummy_#{dummy[n].id}"}
+ end
+
+ post :update_positions, { "ul" => { "0" => dummy_params } }
+
+ dummy = dummy.last.reload
+ dummy.lft.should eq(21)
+ end
end
end
Something went wrong with that request. Please try again.