Project

General

Profile

Actions
Copy link

Defect #33548

closed

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

Added by Vincent Robert almost 4 years ago. Updated about 3 years ago.

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

0%

Estimated time:
Resolution:
Fixed
Affected version:
4.1.1

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

Download all 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
Copy link
 #1

Updated by Go MAEDA almost 4 years ago

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

Updated by Go MAEDA almost 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
Copy link
 #3

Updated by Mischa The Evil almost 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
Copy link
 #4

Updated by Go MAEDA almost 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
Copy link
 #5

Updated by Mischa The Evil almost 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
Copy link
 #6

Updated by Marius BĂLTEANU about 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
Copy link
 #7

Updated by Marius BĂLTEANU about 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
Copy link
 #9

Updated by Markus Boremski about 3 years ago

Should we change the Target-Version?

Actions
Copy link
 #10

Updated by Marius BĂLTEANU about 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
Copy link
 #11

Updated by Go MAEDA about 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
Copy link
 #12

Updated by Go MAEDA about 3 years ago

  • Status changed from Resolved to Closed
Actions
Copy link

Also available in: Atom PDF