diff --git a/lib/administrate/order.rb b/lib/administrate/order.rb index 8f01726417..3fca806d2f 100644 --- a/lib/administrate/order.rb +++ b/lib/administrate/order.rb @@ -10,20 +10,10 @@ def apply(relation) return order_by_association(relation) unless reflect_association(relation).nil? - order = relation.arel_table[sorting_column].public_send(direction) - - tiebreak_key = relation.primary_key - tiebreak_order = relation.arel_table[tiebreak_key].public_send(direction) + return order_by_column(relation) if + column_exist?(relation, sorting_column) - if column_exist?(relation, sorting_column) - if column_exist?(relation, tiebreak_key) && sorting_column.to_s != tiebreak_key.to_s - relation.reorder(order, tiebreak_order) - else - relation.reorder(order) - end - else - relation - end + relation end def ordered_by?(attr) @@ -59,14 +49,26 @@ def opposite_direction (direction == :asc) ? :desc : :asc end + def order_by_column(relation) + order = relation.arel_table[sorting_column].public_send(direction) + with_tiebreak( + relation.reorder(order), + ordered_column: sorting_column + ) + end + def order_by_association(relation) case relation_type(relation) when :has_many - order_by_count(relation) + if relation.primary_key.present? + with_tiebreak(order_by_count(relation)) + else + relation + end when :belongs_to - order_by_belongs_to(relation) + with_tiebreak(order_by_belongs_to(relation)) when :has_one - order_by_has_one(relation) + with_tiebreak(order_by_has_one(relation)) else relation end @@ -77,7 +79,7 @@ def order_by_count(relation) query = klass.arel_table[klass.primary_key].count.public_send(direction) relation .left_joins(attribute.to_sym) - .group(:id) + .group(relation.primary_key) .reorder(query) end @@ -112,6 +114,25 @@ def column_exist?(table, column_name) table.columns_hash.key?(column_name.to_s) end + def with_tiebreak(relation, ordered_column: nil) + tiebreak_key = relation.primary_key + tiebreak_order = tiebreak_order_for(relation) + + if tiebreak_order && ordered_column.to_s != tiebreak_key.to_s + relation.order(tiebreak_order) + else + relation + end + end + + def tiebreak_order_for(relation) + tiebreak_key = relation.primary_key + + if tiebreak_key && column_exist?(relation, tiebreak_key) + relation.arel_table[tiebreak_key].public_send(direction) + end + end + def order_by_id_query(relation) relation.arel_table[association_foreign_key(relation)].public_send(direction) end diff --git a/spec/lib/administrate/order_spec.rb b/spec/lib/administrate/order_spec.rb index e8d56a44b3..1ac1e8831a 100644 --- a/spec/lib/administrate/order_spec.rb +++ b/spec/lib/administrate/order_spec.rb @@ -34,11 +34,14 @@ order = Administrate::Order.new(:name, :asc) relation = relation_with_column(:name) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( - to_sql('"table_name"."name" ASC'), + to_sql('"table_name"."name" ASC') + ) + expect(relation).to have_received(:order).with( to_sql('"table_name"."id" ASC') ) expect(ordered).to eq(relation) @@ -48,11 +51,14 @@ order = Administrate::Order.new(:name, :desc) relation = relation_with_column(:name) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( - to_sql('"table_name"."name" DESC'), + to_sql('"table_name"."name" DESC') + ) + expect(relation).to have_received(:order).with( to_sql('"table_name"."id" DESC') ) expect(ordered).to eq(relation) @@ -62,11 +68,14 @@ order = Administrate::Order.new(:name, :foo) relation = relation_with_column(:name) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( - to_sql('"table_name"."name" ASC'), + to_sql('"table_name"."name" ASC') + ) + expect(relation).to have_received(:order).with( to_sql('"table_name"."id" ASC') ) expect(ordered).to eq(relation) @@ -77,12 +86,14 @@ order = Administrate::Order.new(:id, :asc) relation = relation_with_column(:name) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( to_sql('"table_name"."id" ASC') ) + expect(relation).not_to have_received(:order) expect(ordered).to eq(relation) end end @@ -98,19 +109,21 @@ primary_key: nil ) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( to_sql('"table_name"."name" ASC') ) + expect(relation).not_to have_received(:order) expect(ordered).to eq(relation) end end end context "when relation has_many association" do - it "orders the column by count" do + it "orders the column by count and tiebreaks" do order = Administrate::Order.new(:name) relation = relation_with_association( :has_many, @@ -123,37 +136,124 @@ allow(relation).to receive(:reorder).and_return(relation) allow(relation).to receive(:left_joins).and_return(relation) allow(relation).to receive(:group).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:left_joins).with(:name) - expect(relation).to have_received(:group).with(:id) + expect(relation).to have_received(:group).with("id") expect(relation).to have_received(:reorder).with( to_sql('COUNT("users"."uid") ASC') ) + expect(relation).to have_received(:order).with( + to_sql('"table_name"."id" ASC') + ) expect(ordered).to eq(relation) end + + context "when the parent relation has custom primary key" do + it "groups and tiebreaks by the parent primary key" do + order = Administrate::Order.new(:name) + relation = relation_with_association( + :has_many, + primary_key: "uuid", + klass: double( + table_name: "users", + arel_table: Arel::Table.new("users"), + primary_key: "uid" + ) + ) + allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:left_joins).and_return(relation) + allow(relation).to receive(:group).and_return(relation) + allow(relation).to receive(:order).and_return(relation) + + ordered = order.apply(relation) + + expect(relation).to have_received(:left_joins).with(:name) + expect(relation).to have_received(:group).with("uuid") + expect(relation).to have_received(:reorder).with( + to_sql('COUNT("users"."uid") ASC') + ) + expect(relation).to have_received(:order).with( + to_sql('"table_name"."uuid" ASC') + ) + expect(ordered).to eq(relation) + end + end + + context "when the parent relation has no primary key" do + it "does not apply count ordering" do + order = Administrate::Order.new(:name) + relation = relation_with_association( + :has_many, + primary_key: nil, + klass: double( + table_name: "users", + arel_table: Arel::Table.new("users"), + primary_key: "uid" + ) + ) + allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:left_joins).and_return(relation) + allow(relation).to receive(:group).and_return(relation) + allow(relation).to receive(:order).and_return(relation) + + ordered = order.apply(relation) + + expect(relation).not_to have_received(:left_joins) + expect(relation).not_to have_received(:group) + expect(relation).not_to have_received(:reorder) + expect(relation).not_to have_received(:order) + expect(ordered).to eq(relation) + end + end end context "when relation has belongs_to association" do - it "orders by id" do + it "orders by id and tiebreaks" do order = Administrate::Order.new relation = relation_with_association( :belongs_to, foreign_key: "some_foreign_key" ) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( to_sql('"table_name"."some_foreign_key" ASC') ) + expect(relation).to have_received(:order).with( + to_sql('"table_name"."id" ASC') + ) expect(ordered).to eq(relation) end + context "when the parent relation has no primary key" do + it "orders by id without tiebreaks" do + order = Administrate::Order.new + relation = relation_with_association( + :belongs_to, + primary_key: nil, + foreign_key: "some_foreign_key" + ) + allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) + + ordered = order.apply(relation) + + expect(relation).to have_received(:reorder).with( + to_sql('"table_name"."some_foreign_key" ASC') + ) + expect(relation).not_to have_received(:order) + expect(ordered).to eq(relation) + end + end + context "when `order` argument valid" do - it "orders by the column" do + it "orders by the column and tiebreaks" do order = Administrate::Order.new( :user, nil, @@ -168,18 +268,22 @@ ) allow(relation).to receive(:joins).and_return(relation) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( to_sql('"users"."name" ASC') ) + expect(relation).to have_received(:order).with( + to_sql('"table_name"."id" ASC') + ) expect(relation).to have_received(:joins).with(:user) expect(ordered).to eq(relation) end end context "when `order` argument invalid" do - it "orders by id" do + it "orders by id and tiebreaks" do order = Administrate::Order.new( :user, nil, @@ -193,19 +297,23 @@ ) ) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( to_sql('"table_name"."belongs_to_id" ASC') ) + expect(relation).to have_received(:order).with( + to_sql('"table_name"."id" ASC') + ) expect(ordered).to eq(relation) end end end context "when relation has has_one association" do - it "orders by id" do + it "orders by id and tiebreaks" do order = Administrate::Order.new( :user ) @@ -216,18 +324,48 @@ allow(relation).to receive(:joins).and_return(relation) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( to_sql('"users"."uid" ASC') ) + expect(relation).to have_received(:order).with( + to_sql('"table_name"."id" ASC') + ) expect(relation).to have_received(:joins).with(:user) expect(ordered).to eq(relation) end + context "when the parent relation has no primary key" do + it "orders by id without tiebreaks" do + order = Administrate::Order.new( + :user + ) + relation = relation_with_association( + :has_one, + primary_key: nil, + reflection: {table_name: "users", association_primary_key: "uid"} + ) + + allow(relation).to receive(:joins).and_return(relation) + allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) + + ordered = order.apply(relation) + + expect(relation).to have_received(:reorder).with( + to_sql('"users"."uid" ASC') + ) + expect(relation).not_to have_received(:order) + expect(relation).to have_received(:joins).with(:user) + expect(ordered).to eq(relation) + end + end + context "when `order` argument valid" do - it "orders by the column" do + it "orders by the column and tiebreaks" do order = Administrate::Order.new( :user, nil, @@ -242,18 +380,22 @@ ) allow(relation).to receive(:joins).and_return(relation) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( to_sql('"users"."name" ASC') ) + expect(relation).to have_received(:order).with( + to_sql('"table_name"."id" ASC') + ) expect(relation).to have_received(:joins).with(:user) expect(ordered).to eq(relation) end end context "when `order` argument invalid" do - it "orders by id" do + it "orders by id and tiebreaks" do order = Administrate::Order.new( :user, nil, @@ -268,12 +410,16 @@ ) allow(relation).to receive(:joins).and_return(relation) allow(relation).to receive(:reorder).and_return(relation) + allow(relation).to receive(:order).and_return(relation) ordered = order.apply(relation) expect(relation).to have_received(:reorder).with( to_sql('"users"."pk" ASC') ) + expect(relation).to have_received(:order).with( + to_sql('"table_name"."id" ASC') + ) expect(relation).to have_received(:joins).with(:user) expect(ordered).to eq(relation) end @@ -365,6 +511,80 @@ end end + describe "#with_tiebreak" do + it "returns relation ordered by tiebreak" do + order = Administrate::Order.new(:name) + relation = relation_with_column(:name) + allow(relation).to receive(:order).and_return(relation) + + ordered_relation = order.send(:with_tiebreak, relation, ordered_column: :name) + + expect(relation).to have_received(:order).with( + to_sql('"table_name"."id" ASC') + ) + expect(ordered_relation).to eq(relation) + end + + it "returns relation unchanged when ordered column is primary key" do + order = Administrate::Order.new(:id) + relation = relation_with_column(:id) + allow(relation).to receive(:order).and_return(relation) + + ordered_relation = order.send(:with_tiebreak, relation, ordered_column: :id) + + expect(relation).not_to have_received(:order) + expect(ordered_relation).to eq(relation) + end + + it "returns relation unchanged when relation has no primary key" do + order = Administrate::Order.new(:name) + relation = double( + columns_hash: {"name" => :column_info}, + arel_table: Arel::Table.new("table_name"), + primary_key: nil + ) + allow(relation).to receive(:order).and_return(relation) + + ordered_relation = order.send(:with_tiebreak, relation, ordered_column: :name) + + expect(relation).not_to have_received(:order) + expect(ordered_relation).to eq(relation) + end + end + + describe "#tiebreak_order_for" do + it "returns order by primary key" do + order = Administrate::Order.new(:name) + relation = relation_with_column(:name) + + tiebreak_order = order.send(:tiebreak_order_for, relation) + + expect(tiebreak_order).to to_sql('"table_name"."id" ASC') + end + + it "returns nil when relation has no primary key" do + order = Administrate::Order.new(:name) + relation = double( + columns_hash: {"name" => :column_info}, + arel_table: Arel::Table.new("table_name"), + primary_key: nil + ) + + expect(order.send(:tiebreak_order_for, relation)).to be_nil + end + + it "returns nil when primary key column does not exist" do + order = Administrate::Order.new(:name) + relation = double( + columns_hash: {"name" => :column_info}, + arel_table: Arel::Table.new("table_name"), + primary_key: "id" + ) + + expect(order.send(:tiebreak_order_for, relation)).to be_nil + end + end + def relation_with_column(column) double( klass: double(reflect_on_association: nil), @@ -379,8 +599,12 @@ def relation_with_association( association, foreign_key: "#{association}_id", klass: nil, - reflection: {} + reflection: {}, + primary_key: "id" ) + columns_hash = {"id" => :column_info} + columns_hash[primary_key.to_s] = :column_info if primary_key + double( klass: double( reflect_on_association: double( @@ -393,8 +617,10 @@ def relation_with_association( **reflection ) ), + columns_hash: columns_hash, table_name: "table_name", - arel_table: Arel::Table.new("table_name") + arel_table: Arel::Table.new("table_name"), + primary_key: primary_key ) end end