Fix ordering HasOne fields without explicit order (#2584)

Ordering by a primary key of the HasOne field wasn't working
and resulted in invalid sql, because it did not join the table.

This unifies the code paths to use the same logic to add the ordering
from another table and use reflection to get relevant table information.

lib/administrate/order.rb

@@ -75,34 +75,24 @@ def order_by_count(relation)
 
     def order_by_belongs_to(relation)
       if ordering_by_association_column?(relation)
-        order_by_attribute(relation)
+        order_by_association_attribute(relation)
       else
         order_by_id(relation)
       end
     end
 
     def order_by_has_one(relation)
       if ordering_by_association_column?(relation)
-        order_by_attribute(relation)
+        order_by_association_attribute(relation)
       else
         order_by_association_id(relation)
       end
     end
 
-    def order_by_attribute(relation)
-      relation.joins(
-        attribute.to_sym
-      ).reorder(order_by_attribute_query)
-    end
-
     def order_by_id(relation)
       relation.reorder(order_by_id_query(relation))
     end
 
-    def order_by_association_id(relation)
-      relation.reorder(order_by_association_id_query)
-    end
-
     def ordering_by_association_column?(relation)
       association_attribute &&
         column_exist?(
@@ -115,16 +105,26 @@ def column_exist?(table, column_name)
     end
 
     def order_by_id_query(relation)
-      relation.arel_table[foreign_key(relation)].public_send(direction)
+      relation.arel_table[association_foreign_key(relation)].public_send(direction)
+    end
+
+    def order_by_association_id(relation)
+      order_by_association_column(relation, association_primary_key(relation))
+    end
+
+    def order_by_association_attribute(relation)
+      order_by_association_column(relation, association_attribute)
     end
 
-    def order_by_association_id_query
-      Arel::Table.new(association_table_name)[:id].public_send(direction)
+    def order_by_association_column(relation, column_name)
+      relation.joins(
+        attribute.to_sym
+      ).reorder(order_by_association_column_query(relation, column_name))
     end
 
-    def order_by_attribute_query
-      table = Arel::Table.new(association_table_name)
-      table[association_attribute].public_send(direction)
+    def order_by_association_column_query(relation, column)
+      table = Arel::Table.new(association_table_name(relation))
+      table[column].public_send(direction)
     end
 
     def relation_type(relation)
@@ -135,12 +135,16 @@ def reflect_association(relation)
       relation.klass.reflect_on_association(attribute.to_s)
     end
 
-    def foreign_key(relation)
+    def association_foreign_key(relation)
       reflect_association(relation).foreign_key
     end
 
-    def association_table_name
-      attribute.tableize
+    def association_primary_key(relation)
+      reflect_association(relation).association_primary_key
+    end
+
+    def association_table_name(relation)
+      reflect_association(relation).table_name
     end
   end
 end

spec/lib/administrate/order_spec.rb

@@ -116,14 +116,14 @@
       context "when `order` argument valid" do
         it "orders by the column" do
           order = Administrate::Order.new(
-            double(to_sym: :user, tableize: "users"),
+            :user,
             nil,
             association_attribute: "name"
           )
           relation = relation_with_association(
             :belongs_to,
+            reflection: { table_name: :users },
             klass: double(
-              table_name: "users",
               columns_hash: {"name" => :value}
             )
           )
@@ -134,14 +134,15 @@
           expect(relation).to have_received(:reorder).with(
             to_sql('"users"."name" 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
           order = Administrate::Order.new(
-            double(table_name: "users", to_sym: :user),
+            :user,
             nil,
             association_attribute: "invalid_column_name"
           )
@@ -152,7 +153,6 @@
               columns_hash: {name: :value}
             )
           )
-          allow(relation).to receive(:joins).and_return(relation)
           allow(relation).to receive(:reorder).and_return(relation)
 
           ordered = order.apply(relation)
@@ -168,30 +168,36 @@
     context "when relation has has_one association" do
       it "orders by id" do
         order = Administrate::Order.new(
-          double(to_sym: :user, tableize: "users")
+          :user
+        )
+        relation = relation_with_association(
+          :has_one,
+          reflection: {table_name: 'users', association_primary_key: 'uid'}
         )
-        relation = relation_with_association(:has_one)
+
+        allow(relation).to receive(:joins).and_return(relation)
         allow(relation).to receive(:reorder).and_return(relation)
 
         ordered = order.apply(relation)
 
         expect(relation).to have_received(:reorder).with(
-          to_sql('"users"."id" ASC')
+          to_sql('"users"."uid" ASC')
         )
+        expect(relation).to have_received(:joins).with(:user)
         expect(ordered).to eq(relation)
       end
 
       context "when `order` argument valid" do
         it "orders by the column" do
           order = Administrate::Order.new(
-            double(to_sym: :user, tableize: "users"),
+            :user,
             nil,
             association_attribute: "name"
           )
           relation = relation_with_association(
             :has_one,
+            reflection: { table_name: 'users' },
             klass: double(
-              table_name: "users",
               columns_hash: {"name" => :value}
             )
           )
@@ -202,21 +208,22 @@
           expect(relation).to have_received(:reorder).with(
             to_sql('"users"."name" 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
           order = Administrate::Order.new(
-            double(to_sym: :user, tableize: "users"),
+            :user,
             nil,
             association_attribute: "invalid_column_name"
           )
           relation = relation_with_association(
             :has_one,
+            reflection: { table_name: 'users', association_primary_key: 'pk' },
             klass: double(
-              table_name: "users",
               columns_hash: {name: :value}
             )
           )
@@ -226,8 +233,9 @@
           ordered = order.apply(relation)
 
           expect(relation).to have_received(:reorder).with(
-            to_sql('"users"."id" ASC')
+            to_sql('"users"."pk" ASC')
           )
+          expect(relation).to have_received(:joins).with(:user)
           expect(ordered).to eq(relation)
         end
       end
@@ -330,15 +338,19 @@ def relation_with_column(column)
   def relation_with_association(
     association,
     foreign_key: "#{association}_id",
-    klass: nil
+    klass: nil,
+    reflection: {}
   )
     double(
       klass: double(
         reflect_on_association: double(
           "#{association}_reflection",
           macro: association,
           foreign_key: foreign_key,
-          klass: klass
+          table_name: association.to_s.tableize,
+          klass: klass,
+          association_primary_key: :id,
+          **reflection
         )
       ),
       table_name: "table_name",