reporter template can misinterpet expected input for an aggregate filter

Bug #1858114 reported by Galen Charlton
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Evergreen
Medium
Unassigned
3.5
Medium
Unassigned
3.6
Medium
Unassigned

Bug Description

Consider a report that has a filter with the following properties:

- operator is greater than or the like
- field transform is "count"

The generated SQL produces a HAVING clause that looks something like:

  HAVING ((COUNT("f4ec076b93f2f159d6941c11b83555ff"."id")) IS NULL OR COUNT("f4ec076b93f2f159d6941c11b83555ff"."id") > $_20322$XXX$_20322$)...

In context, the value of XXX should just be a number. However, the interface for creating a report has two problems:

- if a filter value is set in the template, that filter value is ignored
- a selection widget is constructed based on the class and field; the only appropriate filter value for a HAVING COUNT(*) is just a plain number

The expected result: such an aggregate filter should only result in a numeric input in the report editor (if no filter value is set) or use the value set in the template.

Evergreen 3.2+

Revision history for this message
Galen Charlton (gmc) wrote :

Example of a report template that exercises the bug

description: updated
Revision history for this message
Mike Rylander (mrylander) wrote :

The problem here is that there's logic to override the filter widget for all columns that have a selector defined, such as the ID column of many tables. When we transform the column with an aggregate transform, though, we don't want to pick a particular selector-labeled value.

While I can imagine using a min or max aggregate to get a representative value for a group, when other dependent filters would make the value stable or where the value in particular doesn't matter but isn't needed in a count (say), all the other aggregate transforms that are currently supported are specifically numeric (sum, average, count, and count_distinct). Because of that, and the most likely use case of matching a count to "0" or "1", we should only present the "remote object widget" for non-aggregates, and just use the baseline text string input here.

Branch forthcoming.

Changed in evergreen:
assignee: nobody → Mike Rylander (mrylander)
Revision history for this message
Mike Rylander (mrylander) wrote :
tags: added: pullrequest
Changed in evergreen:
assignee: Mike Rylander (mrylander) → nobody
Revision history for this message
Mike Rylander (mrylander) wrote :

I've updated the above branch with another commit to allow the "having" clause to actually save and use template-level filter values provided by template creators. Previous to that commit, aggregate columns required report-level filter parameters in all cases.

Revision history for this message
Mike Rylander (mrylander) wrote :

And, finally, I've pushed one more commit to that branch which fixes a thinko from [checks notes] 2007 that prevented template filter values on aggregate filter columns ("having" clause) from showing up in the filter area.

Here's to hoping that's the last time we have to touch the bespoke-OO-JS reports UI!

Revision history for this message
Angela Kilsdonk (akilsdonk) wrote :

I have tested this code and consent to signing off on it with my name, Angela Kilsdonk, and my email address, akilsdonk[at]equinoxinitiative.org.

tags: added: signedoff
Changed in evergreen:
milestone: none → 3.6.1
Changed in evergreen:
milestone: 3.6.1 → 3.6.2
Jason Boyer (jboyer)
Changed in evergreen:
milestone: 3.6.2 → 3.7-beta
Revision history for this message
Jason Boyer (jboyer) wrote :

Pushed to master, rel_3_6, and rel_3_5. Thanks Mike and Angela!

Galen Charlton (gmc)
Changed in evergreen:
status: New → Fix Committed
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers