Edgewall Software
Modify

Opened 5 years ago

Closed 3 years ago

Last modified 17 months ago

#11837 closed enhancement (fixed)

support "optional" parameters in TracReports

Reported by: Christian Boos Owned by: Christian Boos
Priority: normal Milestone: 1.3.1
Component: report system Version: 1.1dev
Severity: normal Keywords:
Cc: leho@…, trac.edgewall.org@… Branch:
Release Notes:

Default values for TracReports dynamic variables can be specified in the SQL query itself, inside SQL comments.

API Changes:

Description (last modified by Christian Boos)

Currently if a report uses a user-defined parameter and that parameter is not defined, a warning is displayed (see e.g. {14}).

In case of complex reports with many user-defined parameters, that's a bit unfortunate as it's very easy to cope with missing values in the SQL query, e.g.

...
WHERE
    ($AUTHOR_ IS '' OR author LIKE $AUTHOR_)
    ...

Original proposal

I propose to consider parameter names ending with "_" to be considered as "optional":

  • they don't trigger the warning if left empty
  • their name is rendered specially in the Parameters box to show that they're optional (with the example above, it would be: (AUTHOR))

I also made the "foldable" legend use a bigger font size.

example with two optional parameters and a mandatory one

Updated proposal

The idea about the trailing "_" was abandoned in favor of specifying the default value for the dynamic variables in the SQL query, inside a SQL comment. See comment:5.

Attachments (3)

TracReports-optional-parameters.png (6.2 KB ) - added by Christian Boos 5 years ago.
example with two optional parameters and a mandatory one
0001-TracReports-parameter-names-ending-with-_-become-opt.patch (2.5 KB ) - added by Christian Boos 5 years ago.
proposed changes on top of r13350
T11837-Report-argument-defaults-in-comments.patch (1.7 KB ) - added by Peter Suter 5 years ago.

Download all attachments as: .zip

Change History (23)

by Christian Boos, 5 years ago

example with two optional parameters and a mandatory one

by Christian Boos, 5 years ago

proposed changes on top of r13350

comment:1 by lkraav <leho@…>, 5 years ago

Cc: leho@… added

comment:2 by Ryan J Ollos, 5 years ago

Tested and works well. One place for a documentation update is TracReports#AdvancedReports:DynamicVariables.

comment:3 by Peter Suter, 5 years ago

Is there a way to specify the default value for parameters? That would be very useful.

comment:4 by Christian Boos, 5 years ago

Having the possibility to define a default value would indeed by useful. However I don't see where. In TracIni you'd need to be admin to do that and anyway having to edit the report in two different places is not optimal. In the report itself, there's no obvious place either:

  • as a SQL comment? -- REVIEWER_=$USER. Easy enough, but looks a bit hackish.
  • as a Wiki parameter? {{{#!param name="REVIEWER_" value="$USER"}}}… Well, it's not implemented yet ;-)

in reply to:  4 comment:5 by Peter Suter, 5 years ago

Replying to cboos:

  • as a SQL comment? -- REVIEWER_=$USER. Easy enough, but looks a bit hackish.

I'd be happy with that. Also, you could drop the underscore, right? All parameters with such defaults would be optional.

comment:6 by Peter Suter, 5 years ago

This patch implements the simple case -- REVIEWER=psuter. (Supporting -- REVIEWER=$USER sounded easy, but I got stuck preventing -- REVIEWER=%s raising ProgrammingError: Incorrect number of bindings supplied.)

in reply to:  6 ; comment:7 by Christian Boos, 5 years ago

Replying to psuter:

Supporting -- REVIEWER=$USER sounded easy, but I got stuck preventing -- REVIEWER=%s raising ProgrammingError: Incorrect number of bindings supplied.)

Removing the default parameter assignments from the SQL solves that part.

See cboos.git@t11837-default-param-for-report.

in reply to:  7 comment:8 by Peter Suter, 5 years ago

Sounds good. But defaults should not replace explicit arguments:

  • trac/ticket/report.py

    diff -r ab60c8583e04 trac/ticket/report.py
    a b  
    812812
    813813    def get_default_var_args(self, report_args, sql):
    814814        def extract_default_var(fullmatch):
    815             report_args[fullmatch.group(1)] = fullmatch.group(2)
     815            report_args.setdefault(fullmatch.group(1), fullmatch.group(2))
    816816        return self.arg_default_re.sub(extract_default_var, sql)
    817817
    818818    def sql_sub_vars(self, sql, args):

Another option to implement the indirection would be:

defaultval = sub_vars(fullmatch.group(2), report_args)
report_args.setdefault(fullmatch.group(1), defaultval)
Last edited 5 years ago by Peter Suter (previous) (diff)

comment:9 by Ryan J Ollos, 5 years ago

Milestone: 1.1.31.1.4

comment:10 by Ryan J Ollos, 5 years ago

Milestone: 1.1.41.1.5

comment:11 by Ryan J Ollos, 5 years ago

Milestone: 1.1.51.1.6

Retargeting tickets that I think won't be completed for 1.1.5, but please move back if I'm incorrect.

comment:12 by Ryan J Ollos, 4 years ago

Milestone: 1.1.6next-dev-1.1.x

comment:13 by Ryan J Ollos, 4 years ago

Milestone: next-dev-1.1.x1.2
Owner: changed from Christian Boos to Ryan J Ollos
Status: newassigned

comment:14 by Christian Boos, 4 years ago

Owner: changed from Ryan J Ollos to Christian Boos

I'll finish this one in time for 1.2.

comment:15 by Ryan J Ollos, 4 years ago

Would you like to push the changes this week, or should we retarget it to milestone:1.3.1?

comment:16 by Christian Boos, 4 years ago

Milestone: 1.21.3.1

Very busy month for me as well, unfortunately. Yeah, this one shouldn't hold back the release.

comment:17 by Christian Boos, 3 years ago

Description: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Focusing on psuter's solution (comment:5), I removed the special handling of trailing _.

Implemented in r15216.

comment:18 by Robert Becker <trac.edgewall.org@…>, 3 years ago

Is it possible to backport this feature to the next 1.2.x release? The changeset only affects 3 files, so the risk of conflicts is small. I fear that we would have to wait long for this feature otherwise, as 1.2 already took 4 years to be released.

comment:19 by Robert Becker <trac.edgewall.org@…>, 3 years ago

Cc: trac.edgewall.org@… added

comment:20 by Ryan J Ollos, 17 months ago

Release Notes: modified (diff)

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Christian Boos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.