Project

General

Profile

Actions

Defect #33548

closed

Column header is clickable even when the column is not actually sortable

Added by Vincent Robert over 4 years ago. Updated over 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Issues list
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
Affected version:

Description

Please find below a patch to fix the 'sortable?' method in QueryColumn class.

Currently, the method returns true if @sortable is False.
Therefore, some headers display a link when the column is actually not sortable (multi-value custom-fields columns in Projects page for instance).

Thank you for your work

diff --git a/app/models/query.rb b/app/models/query.rb
index 0dec7d211..c3311ffbf 100644
--- a/app/models/query.rb
+++ b/app/models/query.rb
@@ -51,7 +51,7 @@ class QueryColumn

   # Returns true if the column is sortable, otherwise false
   def sortable?
-    !@sortable.nil?
+    !@sortable.blank?
   end

   def sortable

Files

patch.diff (326 Bytes) patch.diff Vincent Robert, 2020-06-03 17:14
0003-Add-test-for-33548.patch (945 Bytes) 0003-Add-test-for-33548.patch Marius BĂLTEANU, 2021-03-13 15:14
Actions #1

Updated by Go MAEDA over 4 years ago

  • Category set to Issues list
  • Status changed from New to Confirmed
  • Target version set to Candidate for next minor release
Actions #2

Updated by Go MAEDA over 4 years ago

Thank you for the fix. I have confirmed the issue.

Here is a test to catch the issue.

diff --git a/test/unit/query_test.rb b/test/unit/query_test.rb
index e4da5e1aa..c068de162 100644
--- a/test/unit/query_test.rb
+++ b/test/unit/query_test.rb
@@ -1629,6 +1629,8 @@ class QueryTest < ActiveSupport::TestCase
     field.update_attribute :multiple, true

     q = IssueQuery.new
+    column = q.available_columns.detect {|c| c.name.to_s == 'cf_1'}
+    assert !column.sortable?
     assert !q.sortable_columns['cf_1']
   end
Actions #3

Updated by Mischa The Evil over 4 years ago

  • Description updated (diff)

I haven't looked deeply into this, but this makes me wonder: did this method ever worked correctly? And if so: what changed and was it intentional?

Actions #4

Updated by Go MAEDA over 4 years ago

Mischa The Evil wrote:

I haven't looked deeply into this, but this makes me wonder: did this method ever worked correctly? And if so: what changed and was it intentional?

I still don't know the cause, but it appears that 3.3 or later are affected. In my test, 2.6 through 3.2 worked as expected.

Actions #5

Updated by Mischa The Evil over 4 years ago

  • Assignee set to Mischa The Evil

I'll do some research to see if I can dig up some (historical) context for this issue.

Note: if it's already clear for someone what's exactly going on here and what the proper way to fix it would be, feel free to jump-in/take-over. I just want to make sure that we're fixing the cause of the issue at its root instead of quick-fixing it with a patch while it's not well-understood.

Actions #6

Updated by Marius BĂLTEANU over 3 years ago

  • Target version changed from Candidate for next minor release to 4.0.8

I'm assigning this to 4.0.8, we should fix it.

Actions #7

Updated by Marius BĂLTEANU over 3 years ago

I took a look in the code, but I didn't find what changed the behaviour in 3.3.0.

Anyway, looking to the implementation, this line (source:/trunk/app/models/query.rb#L123) never returns nil because of the || false. To be more specific:

self.sortable = custom_field.order_statement || false

If custom_field.order_statement returns nil, then the expression returns false.

From my point of view, the patch proposed by Vincent is safe, but I've added it to the CI to check the test results.

Another fix that should work is the following

   # Returns true if the column is sortable, otherwise false
   def sortable?
-    !@sortable.blank?
+    @sortable
   end

Actions #9

Updated by Markus Boremski over 3 years ago

Should we change the Target-Version?

Actions #10

Updated by Marius BĂLTEANU over 3 years ago

Mischa The Evil wrote:

I'll do some research to see if I can dig up some (historical) context for this issue.

Note: if it's already clear for someone what's exactly going on here and what the proper way to fix it would be, feel free to jump-in/take-over. I just want to make sure that we're fixing the cause of the issue at its root instead of quick-fixing it with a patch while it's not well-understood.

Mischa, please let us know when you find something.

Until then, I'm assigning this to Go Maeda in order to include the fix in 4.0.8. The patch proposed by Vincent is safe from my point of view, I'm attaching a test case. We can refactor then in a major version.

Actions #11

Updated by Go MAEDA over 3 years ago

  • Subject changed from Fix column header when column is not sortable to Column header is clickable even when the column is not actually sortable
  • Status changed from Confirmed to Resolved
  • Resolution set to Fixed

Committed the patch. Thank you.

Actions #12

Updated by Go MAEDA over 3 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF