From: Mark Wells Date: Fri, 10 Jan 2014 00:37:39 +0000 (-0800) Subject: sales report: fix filtering by report class, #25459, #24776 X-Git-Url: http://git.freeside.biz/gitweb/?p=freeside.git;a=commitdiff_plain;h=5b670255328fbe875196e16bc8dfc57771753e90 sales report: fix filtering by report class, #25459, #24776 --- diff --git a/FS/FS/Report/Table.pm b/FS/FS/Report/Table.pm index da4916176..7f593846e 100644 --- a/FS/FS/Report/Table.pm +++ b/FS/FS/Report/Table.pm @@ -436,7 +436,7 @@ sub cust_bill_pkg_setup { my @where = ( 'pkgnum != 0', $self->with_classnum($opt{'classnum'}, $opt{'use_override'}), - $self->with_report_option($opt{'report_optionnum'}, $opt{'use_override'}), + $self->with_report_option(%opt), $self->in_time_period_and_agent($speriod, $eperiod, $agentnum), ); @@ -463,7 +463,7 @@ sub cust_bill_pkg_recur { my @where = ( 'pkgnum != 0', $self->with_classnum($opt{'classnum'}, $opt{'use_override'}), - $self->with_report_option($opt{'report_optionnum'}, $opt{'use_override'}), + $self->with_report_option(%opt), ); push @where, 'cust_main.refnum = '. $opt{'refnum'} if $opt{'refnum'}; @@ -532,7 +532,7 @@ sub cust_bill_pkg_detail { push @where, $self->with_classnum($opt{'classnum'}, $opt{'use_override'}), $self->with_usageclass($opt{'usageclass'}), - $self->with_report_option($opt{'report_optionnum'}, $opt{'use_override'}), + $self->with_report_option(%opt), ; if ( $opt{'distribute'} ) { @@ -708,44 +708,71 @@ sub with_usageclass { } sub with_report_option { - my ($self, $num, $use_override) = @_; - # $num can be a single number, or a comma-delimited list of numbers, - # or an arrayref. 0 matches the empty set - # or the word 'multiple' for all packages with more than one report class - return '' if !defined($num); - - $num = join(',', @$num) if ref($num); - - # stringify the set of report options for each pkgpart - my $table = $use_override ? 'override' : 'part_pkg'; - my $subselect = " - SELECT replace(optionname, 'report_option_', '') AS num - FROM part_pkg_option - WHERE optionname like 'report_option_%' - AND part_pkg_option.pkgpart = $table.pkgpart - ORDER BY num"; - - my $comparison; - if ( $num eq 'multiple' ) { - $comparison = "(SELECT COUNT(*) FROM ($subselect) AS x) > 1"; - } else { - - my @num = split(/\s*,\s*/, $num); - - #$comparison = "(SELECT COALESCE(string_agg(num, ','), '') FROM ( #Pg 9-ism - $comparison = "(SELECT COALESCE(array_to_string(array_agg(num), ','), '') - FROM ($subselect) AS x - ) = '". join(',', grep $_, @num). "'"; - - $comparison = "( $comparison OR NOT EXISTS ($subselect) )" - if grep !$_, @num; - + my ($self, %opt) = @_; + # %opt can contain: + # - report_optionnum: a comma-separated list of numbers. Zero means to + # include packages with _no_ report classes. + # - not_report_optionnum: a comma-separated list. Packages that have + # any of these report options will be excluded from the result. + # Zero does nothing. + # - use_override: also matches line items that are add-ons to a package + # matching the report class. + # - all_report_options: returns only packages that have ALL of the + # report classes listed in $num. Otherwise, will return packages that + # have ANY of those classes. + + my @num = ref($opt{'report_optionnum'}) + ? @{ $opt{'report_optionnum'} } + : split(/\s*,\s*/, $opt{'report_optionnum'}); + my @not_num = ref($opt{'not_report_optionnum'}) + ? @{ $opt{'not_report_optionnum'} } + : split(/\s*,\s*/, $opt{'not_report_optionnum'}); + my $null; + $null = 1 if ( grep {$_ == 0} @num ); + @num = grep {$_ > 0} @num; + @not_num = grep {$_ > 0} @not_num; + + # brute force + my $table = $opt{'use_override'} ? 'override' : 'part_pkg'; + my $op = ' OR '; + if ( $opt{'all_report_options'} ) { + if ( @num and $null ) { + return 'false'; # mutually exclusive criteria, so just bail out + } + $op = ' AND '; } - if ( $use_override ) { - # then also allow the non-override package to match - $comparison = "( $comparison OR " . $self->with_report_option($num) . ")"; + my @where_num = map { + "EXISTS(SELECT 1 FROM part_pkg_option ". + "WHERE optionname = 'report_option_$_' ". + "AND part_pkg_option.pkgpart = $table.pkgpart)" + } @num; + if ( $null ) { + push @where_num, "NOT EXISTS(SELECT 1 FROM part_pkg_option ". + "WHERE optionname LIKE 'report_option_%' ". + "AND part_pkg_option.pkgpart = $table.pkgpart)"; + } + my @where_not_num = map { + "NOT EXISTS(SELECT 1 FROM part_pkg_option ". + "WHERE optionname = 'report_option_$_' ". + "AND part_pkg_option.pkgpart = $table.pkgpart)" + } @not_num; + + my @where; + if (@where_num) { + push @where, '( '.join($op, @where_num).' )'; + } + if (@where_not_num) { + push @where, '( '.join(' AND ', @where_not_num).' )'; } - $comparison; + + return @where; + # this messes up totals + #if ( $opt{'use_override'} ) { + # # then also allow the non-override package to match + # delete $opt{'use_override'}; + # $comparison = "( $comparison OR " . $self->with_report_option(%opt) . ")"; + #} + } sub with_cust_classnum { diff --git a/httemplate/graph/cust_bill_pkg.cgi b/httemplate/graph/cust_bill_pkg.cgi index 39c972267..1b31955c4 100644 --- a/httemplate/graph/cust_bill_pkg.cgi +++ b/httemplate/graph/cust_bill_pkg.cgi @@ -84,13 +84,19 @@ $bottom_link .= "cust_classnum=$_;" foreach @cust_classnums; #started out as false lazinessish w/FS::cust_pkg::search_sql (previously search/cust_pkg.cgi), but not much left the sane now after #24776 my ($class_table, $name_col, $value_col, $class_param); +my $all_report_options; if ( $cgi->param('class_mode') eq 'report' ) { $class_param = 'report_optionnum'; # CGI param name, also used in the report engine $class_table = 'part_pkg_report_option'; # table containing classes $name_col = 'name'; # the column of that table containing the label $value_col = 'num'; # the column containing the class number -} else { + # in 'exact' mode we want to run the query in ALL mode. + # in 'breakdown' mode want to run the query in ALL mode but using the + # power set of the classes selected. + $all_report_options = 1 + unless $cgi->param('class_agg_break') eq 'aggregate'; +} else { # class_mode eq 'pkg' $class_param = 'classnum'; $class_table = 'pkg_class'; $name_col = 'classname'; @@ -106,10 +112,12 @@ my @classnames = map { if ( $_ ) { } } @classnums; +my @not_classnums; $bottom_link .= "$class_param=$_;" foreach @classnums; -if ( $cgi->param('class_agg_break') eq 'aggregate' ) { +if ( $cgi->param('class_agg_break') eq 'aggregate' or + $cgi->param('class_agg_break') eq 'exact' ) { $title .= ' '. join(', ', @classnames) unless scalar(@classnames) > scalar(qsearch($class_table,{'disabled'=>''})); @@ -117,15 +125,28 @@ if ( $cgi->param('class_agg_break') eq 'aggregate' ) { } elsif ( $cgi->param('class_agg_break') eq 'breakdown' ) { - if ( $cgi->param('mode') eq 'report' ) { - # In theory, a package can belong to any subset of the report classes, - # so the report groups should be all the _subsets_, but for now we're - # handling the simple case where each package belongs to one report - # class. Packages with multiple classes will go into one bin at the - # end. - push @classnames, '(multiple classes)'; - push @classnums, 'multiple'; + if ( $cgi->param('class_mode') eq 'report' ) { + # The new way: + # Actually break down all subsets of the (selected) report classes. + my $powerset = sub { + my @set = []; + foreach my $x (@_) { + @set = map { $_, [ @$_, $x ] } @set; + } + @set; + }; + @classnums = $powerset->(@classnums); + @classnames = $powerset->(@classnames); + # this is pairwise complementary to @classnums, because math + @not_classnums = reverse(@classnums); +warn Dumper(\@classnums, \@classnames, \@not_classnums); + # remove the null set + shift @classnums; + shift @classnames; + shift @not_classnums; } + # else it's 'pkg', i.e. part_pkg.classnum, which is singular on pkgpart + # and much simpler } else { die "guru meditation #434"; @@ -185,7 +206,10 @@ foreach my $agent ( $all_agent || $sel_agent || $FS::CurrentUser::CurrentUser->a 'distribute' => $distribute, ); - if ( $cgi->param('class_agg_break') eq 'aggregate' ) { + if ( $cgi->param('class_agg_break') eq 'aggregate' or + $cgi->param('class_agg_break') eq 'exact' ) { + # the only difference between 'aggregate' and 'exact' is whether + # we pass the 'all_report_options' flag. foreach my $component ( @components ) { @@ -198,14 +222,16 @@ foreach my $agent ( $all_agent || $sel_agent || $FS::CurrentUser::CurrentUser->a my $row_agentnum = $all_agent || $agent->agentnum; my $row_refnum = $all_part_referral || $part_referral->refnum; - push @params, [ + my @row_params = ( @base_params, $class_param => \@classnums, ($all_agent ? () : ('agentnum' => $row_agentnum) ), ($all_part_referral ? () : ('refnum' => $row_refnum) ), - 'charges' => $component, - ]; + 'charges' => $component, + ); + # XXX this is very silly. we should cache it server-side and + # just put a cache identifier in the link my $rowlink = "$link;". ($all_agent ? '' : "agentnum=$row_agentnum;"). ($all_part_referral ? '' : "refnum=$row_refnum;"). @@ -213,6 +239,11 @@ foreach my $agent ( $all_agent || $sel_agent || $FS::CurrentUser::CurrentUser->a "distribute=$distribute;". "use_override=$use_override;charges=$component;"; $rowlink .= "$class_param=$_;" foreach @classnums; + if ( $all_report_options ) { + push @row_params, 'all_report_options', 1; + $rowlink .= 'all_report_options=1'; + } + push @params, \@row_params; push @links, $rowlink; @colorbuf = @agent_colors unless @colorbuf; @@ -223,9 +254,12 @@ foreach my $agent ( $all_agent || $sel_agent || $FS::CurrentUser::CurrentUser->a } elsif ( $cgi->param('class_agg_break') eq 'breakdown' ) { + # if we're working with report options, @classnums here contains + # arrays of multiple classnums for (my $i = 0; $i < scalar @classnums; $i++) { - my $row_classnum = $classnums[$i]; - my $row_classname = $classnames[$i]; + my $row_classnum = join(',', @{ $classnums[$i] }); + my $row_classname = join(', ', @{ $classnames[$i] }); + my $not_row_classnum = join(',', @{ $not_classnums[$i] }); foreach my $component ( @components ) { push @items, 'cust_bill_pkg'; @@ -237,21 +271,30 @@ foreach my $agent ( $all_agent || $sel_agent || $FS::CurrentUser::CurrentUser->a my $row_agentnum = $all_agent || $agent->agentnum; my $row_refnum = $all_part_referral || $part_referral->refnum; - push @params, [ + my @row_params = ( @base_params, $class_param => $row_classnum, ($all_agent ? () : ('agentnum' => $row_agentnum) ), ($all_part_referral ? () : ('refnum' => $row_refnum)), 'charges' => $component, - ]; - - push @links, "$link;". + ); + my $row_link = "$link;". ($all_agent ? '' : "agentnum=$row_agentnum;"). ($all_part_referral ? '' : "refnum=$row_refnum;"). (join('',map {"cust_classnum=$_;"} @cust_classnums)). "$class_param=$row_classnum;". "distribute=$distribute;". "use_override=$use_override;charges=$component;"; + if ( $class_param eq 'report_optionnum' ) { + push @row_params, + 'all_report_options' => 1, + 'not_report_optionnum' => $not_row_classnum, + ; + $row_link .= "all_report_options=1;". + "not_report_optionnum=$not_row_classnum;"; + } + push @params, \@row_params; + push @links, $row_link; @colorbuf = @agent_colors unless @colorbuf; push @colors, shift @colorbuf; diff --git a/httemplate/graph/report_cust_bill_pkg.html b/httemplate/graph/report_cust_bill_pkg.html index 1e54df3ab..c6eb0f2bf 100644 --- a/httemplate/graph/report_cust_bill_pkg.html +++ b/httemplate/graph/report_cust_bill_pkg.html @@ -64,12 +64,15 @@ function class_mode_changed() { var div_pkg = document.getElementById('pkg_class'); var div_report = document.getElementById('report_class'); + var span_exact = document.getElementById('exact_match'); if (mode == 'pkg') { div_pkg.style.display = ''; div_report.style.display = 'none'; + span_exact.style.display = 'none'; } else if (mode == 'report') { div_pkg.style.display = 'none'; div_report.style.display = ''; + span_exact.style.display = ''; } } window.onload = class_mode_changed; @@ -149,6 +152,11 @@ window.onload = class_mode_changed;
<% emt('Breakdown') %> +
+ diff --git a/httemplate/search/cust_bill_pkg.cgi b/httemplate/search/cust_bill_pkg.cgi index f87862a3d..61093d262 100644 --- a/httemplate/search/cust_bill_pkg.cgi +++ b/httemplate/search/cust_bill_pkg.cgi @@ -311,7 +311,9 @@ my $join_pkg = LEFT JOIN part_pkg USING (pkgpart)'; my $part_pkg = 'part_pkg'; -if ( $cgi->param('use_override') ) { #"Separate sub-packages from parents" +# "Separate sub-packages from parents" +my $use_override = $cgi->param('use_override') ? 1 : 0; +if ( $use_override ) { # still need the real part_pkg for tax applicability, # so alias this one $join_pkg .= " LEFT JOIN part_pkg AS override ON ( @@ -342,14 +344,16 @@ if ( $cgi->param('nottax') ) { } if ( grep { $_ eq 'report_optionnum' } $cgi->param ) { - my @nums = grep /^\w+$/, $cgi->param('report_optionnum'); - my $num = join(',', @nums); + my $num = join(',', grep /^[\d,]+$/, $cgi->param('report_optionnum')); + my $not_num = join(',', grep /^[\d,]+$/, $cgi->param('not_report_optionnum')); + my $all = $cgi->param('all_report_options') ? 1 : 0; push @where, # code reuse FTW - FS::Report::Table->with_report_option( $num, $cgi->param('use_override')); - } - - if ( $cgi->param('report_optionnum') =~ /^(\w+)$/ ) { - ; + FS::Report::Table->with_report_option( + report_optionnum => $num, + not_report_optionnum => $not_num, + use_override => $use_override, + all_report_options => $all, + ); } # taxclass