ChiliProject is not maintained anymore. Please be advised that there will be no more updates.
We do not recommend that you setup new ChiliProject instances and we urge all existing users to migrate their data to a maintained system, e.g. Redmine. We will provide a migration script later. In the meantime, you can use the instructions by Christian Daehn.
Wrong filters for int and float custom fields (Bug #498)
Description
int and float custom fields get the same filters as strings (default for custom fields in "unknown formats") instead as the ones for "integer custom fields".
Related issues
| duplicated by Bug #387: Filter for integer custom field has options for searching... | Duplicate | 2011-05-09 |
Associated revisions
Correct filters for int,float custom fields. #498
History
Updated by Felix Schäfer at 2011-06-30 12:21 pm
I have a fix for this, will post it shortly.
Updated by Felix Schäfer at 2011-07-04 02:04 pm
Ok, so this was a little longer in the making than I had thought because of how custom fields work (wouldn't it have been easier to have a text field for "character"-based custom fields and a float/numeric field for numeric custom fields?…). Anyway, here's my proposed patch:
1diff --git a/app/models/query.rb b/app/models/query.rb
2index 1772d68..f7abb19 100644
3--- a/app/models/query.rb
4+++ b/app/models/query.rb
5@@ -586,9 +586,17 @@ class Query < ActiveRecord::Base
6 sql = "#{db_table}.#{db_field} IS NOT NULL"
7 sql << " AND #{db_table}.#{db_field} <> ''" if is_custom_filter
8 when ">="
9- sql = "#{db_table}.#{db_field} >= #{value.first.to_i}"
10+ if is_custom_filter
11+ sql = "#{db_table}.#{db_field} != '' AND CAST(#{db_table}.#{db_field} AS decimal(60,3)) >= #{value.first.to_f}"
12+ else
13+ sql = "#{db_table}.#{db_field} >= #{value.first.to_f}"
14+ end
15 when "<="
16- sql = "#{db_table}.#{db_field} <= #{value.first.to_i}"
17+ if is_custom_filter
18+ sql = "#{db_table}.#{db_field} != '' AND CAST(#{db_table}.#{db_field} AS decimal(60,3)) <= #{value.first.to_f}"
19+ else
20+ sql = "#{db_table}.#{db_field} <= #{value.first.to_f}"
21+ end
22 when "o"
23 sql = "#{IssueStatus.table_name}.is_closed=#{connection.quoted_false}" if field == "status_id"
24 when "c"
25@@ -628,6 +636,8 @@ class Query < ActiveRecord::Base
26
27 custom_fields.select(&:is_filter?).each do |field|
28 case field.field_format
29+ when "int", "float"
30+ options = { :type => :integer, :order => 20 }
31 when "text"
32 options = { :type => :text, :order => 20 }
33 when "list"
While we're at it, I'd like to change the :integer filter type to :numeric to make it clear it doesn't work only for integers.
- Target version set to 2.1.0
- Assignee deleted (
Felix Schäfer) - Status changed from Open to Ready for review
Updated by Holger Just at 2011-07-04 02:34 pm
Hmmm, this results in a full table scan as the casts can not use any index. This might be an issue if you have a couple of custom values. Also I'd use a scale of a least 4 digits in the decimal to accomodate common currency precision. Using this, I think this is a nice workaround
That said, I think a much cleaner variant would be to provide more columns on the custom fields to be used for different data types. Those columns should by correctly typed (and indexed) in the database. Thus, there could be the following columns:
- :text for actual strings and serialized complex values
- :integer for well,integers
- :currency which could be a
decimal(60,4)in the database - *:float@, well floating point stuff. This one might not be necessary as the decimal could be sufficient
Additionally, a type field would select the actually value column to be used. There would only be one column used, the others would be force to contain null.
The slight storage overhead shouldn't really matter compared to the performance opportunitues of actually using indexes.
Updated by Felix Schäfer at 2011-07-04 10:20 pm
Holger Just wrote:
Hmmm, this results in a full table scan as the casts can not use any index. This might be an issue if you have a couple of custom values.
The current custom field structure doesn't allow anything better, more on that below.
Also I'd use a scale of a least 4 digits in the decimal to accomodate common currency precision. Using this, I think this is a nice workaround
Well, I just copied the precision from source:/app/models/custom_field.rb#L116, we can bump both to DECIMAL(60,4) or even DECIMAL(65,10) though, I don't think the performance will be much different.
That said, I think a much cleaner variant would be to provide more columns on the custom fields to be used for different data types. Those columns should by correctly typed (and indexed) in the database. Thus, there could be the following columns:
- :text for actual strings and serialized complex values
- :integer for well,integers
- :currency which could be a
decimal(60,4)in the database- *:float@, well floating point stuff. This one might not be necessary as the decimal could be sufficient
Additionally, a type field would select the actually value column to be used. There would only be one column used, the others would be force to contain null.
The slight storage overhead shouldn't really matter compared to the performance opportunitues of actually using indexes.
Regarding that, I really (really really) dislike that the types are hardcoded and just differentiated by a bunch of case statements. I've spent a few minutes pondering this, and I think the nicest solution would be to have sort of a CustomField namespace and have say CustomField::String, CustomField::Text, CustomField::Integer, CustomField::Date, CustomField::Users and so on, and each could provide/decide how to store values (maybe CustomValue::Text, CustomValue::Numeric, CustomValue::Association (for stuff referencing core objects and/or (multiselect) lists)), how to sort them, how to sort objects using them, and so on. This would also have the advantage of every field type being able to provide it's own form for the admin section instead of relying on (unextensible) js, that you wouldn't need to declare custom field types in lib/redmine.rb as is done now (just scan for CustomField:: types), and all that combined would make adding new types even from a plugin a breeze.
The biggest snag I've hit though is that that would need some sort of doubly polymorphic many-to-many relation: Customizable many-to-many CustomField, where Customizable can be anything and sees custom fields as "CustomizableCustomField" (e.g. IssueCustomField), and CustomField being a CustomField::String or CustomField::Users and so on. The backend shouldn't be too much of a problem, but I'm not sure how good rails will play with that.
Anyway, already much too much discussion about that for the scope of this ticket, I'll open a forum thread tomorrow to discuss all this, I'm also pretty sure there's a nicer way that what I have in mind currently, but I guess my ruby-foo isn't good enough for it yet :-)
Updated by Felix Schäfer at 2011-07-05 09:21 am
Also see the custom field rework proposal for more info.
Updated by Felix Schäfer at 2011-07-08 07:41 pm
Committed the proposed patch with DECIMAL(60,3) changed to DECIMAL(60,4), this will at least get us working filters until we have a better solution.
- Status changed from Ready for review to Closed