Vote #80904
完了Column header is clickable even when the column is not actually sortable
0%
説明
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
journals
--------------------------------------------------------------------------------
Thank you for the fix. I have confirmed the issue.
Here is a test to catch the issue.
<pre><code class="diff">
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
</code></pre>
--------------------------------------------------------------------------------
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?
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
I'm assigning this to version:"4.0.8", we should fix it.
--------------------------------------------------------------------------------
I took a look in the code, but I didn't find what changed the behaviour in version:"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:
<pre><code class="ruby">
self.sortable = custom_field.order_statement || false
</code></pre>
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
<pre><code class="ruby">
# Returns true if the column is sortable, otherwise false
def sortable?
- !@sortable.blank?
+ @sortable
end
</code></pre>
--------------------------------------------------------------------------------
The tests pass with both fixes:
1. https://gitlab.com/redmine-org/redmine/-/pipelines/262768044
2. https://gitlab.com/redmine-org/redmine/-/pipelines/262768066
--------------------------------------------------------------------------------
Should we change the Target-Version?
--------------------------------------------------------------------------------
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 version:"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.
--------------------------------------------------------------------------------
Committed the patch. Thank you.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------