Re: Proposal, v1

Looks like you found the right tree to bark at :-)

There are a few comments in line below.

=========================================================================================================================
diff --git a/LedgerSMB/Report/Balance_Sheet.pm
b/LedgerSMB/Report/Balance_Sheet.pm
index d3af20e..b9aa94c
--- a/LedgerSMB/Report/Balance_Sheet.pm
+++ b/LedgerSMB/Report/Balance_Sheet.pm
@@ -42,6 +42,23 @@ Boolean, true if the regular hierarchies need to be
ignored,

has legacy_hierarchy => (is => 'rw', isa => 'Bool');

+=item comparison_periods
+
+This is the number of periods to compare to
+
+=cut
+
+has comparison_periods => (is => 'ro', isa =>'Int',
+ required =>1, default => 1);
+
+=item comparison_type
+
+This is either by number of periods or by dates
+
+=cut
+
+has comparison_type => (is => 'ro', isa =>'Str', required =>0);
+
=item column_path_prefix

diff --git a/LedgerSMB/Report/PNL/Income_Statement.pm
b/LedgerSMB/Report/PNL/Income_Statement.pm
index ddba1d1..cd0d522
--- a/LedgerSMB/Report/PNL/Income_Statement.pm
+++ b/LedgerSMB/Report/PNL/Income_Statement.pm
@@ -31,6 +31,22 @@ This is either 'cash' or 'accrual'

has basis => (is => 'ro', isa =>'Str', required => 1);

+=item comparison_periods
+
+This is the number of periods to compare to
+
+=cut
+
On Sun, Apr 10, 2016 at 2:18 AM, Yves Lavoie, GaYLi <..hidden..> wrote:
Hi Erik,

This ain't complete but the PNL works as expected, producing from 1 to 9
years of comparison from a from_date and looking backward n years.
I still have to work on dialog though.

Yves

+has comparison_periods => (is => 'ro', isa =>'Int', required =>0);
+
+=item comparison_type
+
+This is either by number of periods or by dates
+
+=cut
+
+has comparison_type => (is => 'ro', isa =>'Str', required =>1);
+
=item ignore_yearend

You're duplicating the same fields here which makes me wonder if this should be part of a "Comparison" Moose role, or that it could/should be included in the LedgerSMB::Report::Dates role. What do you think?

This is 'none', 'all', or 'last'
diff --git a/LedgerSMB/Scripts/pnl.pm b/LedgerSMB/Scripts/pnl.pm
index 976fe75..3212dcc 100755
--- a/LedgerSMB/Scripts/pnl.pm
+++ b/LedgerSMB/Scripts/pnl.pm
@@ -32,6 +32,42 @@ use LedgerSMB::Report::PNL::Invoice;
use LedgerSMB::Report;
use LedgerSMB::App_State;

+use LedgerSMB::PGDate;
+use Data::Dumper;

For now Data::Dumper is fine, of course, but please don't forget to remove before final commit.

+
+sub date_interval {
+ my ($date,$interval,$n) = @_;
+ $date = LedgerSMB::PGDate->from_input($date);
+ $n //= 1; # Default to 1

Is this one comparison (i.e. 2 dates)? Or one date?

+ if ($interval eq 'day'){
+ if ( $n > 0 ) {
+ $date->date->add(days => 1 * $n);
+ } else {
+ $date->date->subtract(days => 1 * -$n);
+ }

Reading the documentation on add() and subtract(), they are syntactic sugar for add_duration() and subtract_duration(). Reading the docs on subtract_duration(), it's simply a wrapper around add($duration->invert), which suggests to me that this code can be half as long by depending on "add(days => $n)" == "subtract(days => -$n)".

In addition, I personally like to use hashes to do input mapping; in this case:

my %delta_names = (

day => 'days',

month => 'months',

year => 'years',
);

my $delta_name = $delta_names{$interval};

+ } elsif ($interval eq 'month'){
+ if ( $n > 0 ) {
+ $date->date->add(months => 1 * $n);
+ } else {
+ $date->date->subtract(months => 1 * -$n);
+ }
+ } elsif ($interval eq 'quarter'){
+ if ( $n > 0 ) {
+ $date->date->add(months => 3 * $n);
+ } else {
+ $date->date->subtract(months => 3 * -$n);
+ }
+ } elsif ($interval eq 'year'){
+ if ( $n > 0 ) {
+ $date->date->add(years => 1 * $n);
+ } else {
+ $date->date->subtract(years => 1 * -$n);
+ }
+ } else {
+ die "Bad interval: '$interval'";
+ }
+ return $date;
+}

sub generate_income_statement {
my ($request) = @_;
@@ -44,18 +80,32 @@ sub generate_income_statement {
} elsif ($request->{pnl_type} eq 'product'){
$rpt = LedgerSMB::Report::PNL::Product->new(%$request);
} else {
- $rpt =LedgerSMB::Report::PNL::Income_Statement->new(
+ my $counts = $request->{comparison_periods} || 1;
+ warn $counts;
+ if ( $request->{comparison_type} eq 'by_periods' ) {
+ # to_date = from_date + 1 period - 1 day
+ my $date =
date_interval(date_interval($request->{from_date},$request->{interval}),'day',-1);
+ $request->{"to_date"} = $date->to_output;
+ for my $c_per (1 .. $counts) { # Comparison are backward
+ $date =
date_interval($request->{from_date},$request->{interval},-$c_per);
+ $request->{"from_date_$c_per"} = $date->to_output;
+ $date =
date_interval(date_interval($date,$request->{interval}),'day',-1);
+ $request->{"to_date_$c_per"} = $date->to_output;
+ $request->{"interval_$c_per"} = $request->{interval};
+ }
+ }
+ $rpt = LedgerSMB::Report::PNL::Income_Statement->new(
%$request,
column_path_prefix => [ 0 ]);
$rpt->run_report;
- for my $c_per (1 .. 3) {
+ for my $c_per (1 .. $counts) {
my $found = 0;
- for (qw(from_month from_year from_date to_date interval)){
- $request->{$_} = $request->{"${_}_$c_per"};
- delete $request->{$_} unless defined $request->{$_};
- $found = 1 if defined $request->{$_} and $_ ne 'interval';
- }
- next unless $found;
+ for (qw(from_month from_year from_date to_date interval)){
+ $request->{$_} = $request->{"${_}_$c_per"};
+ delete $request->{$_} unless defined $request->{$_};
+ $found = 1 if defined $request->{$_} and $_ ne 'interval';
+ }
+ next unless $found;
my $comparison =
LedgerSMB::Report::PNL::Income_Statement->new(
%$request,
diff --git a/LedgerSMB/Scripts/reports.pm b/LedgerSMB/Scripts/reports.pm
index 7f04687..b45a3ea
--- a/LedgerSMB/Scripts/reports.pm
+++ b/LedgerSMB/Scripts/reports.pm
@@ -181,7 +181,8 @@ sub balance_sheet {
%$request,
column_path_prefix => [ 0 ]);
$report->run_report;
- for my $count (1 .. 3){
+ my $counts = $request->{comparison_periods};
+ for my $count (1 .. $counts){
next unless $request->{"to_date_$count"};
$request->{to_date} = $request->{"to_date_$count"};
my $comparison =
diff --git a/UI/Reports/filters/income_statement.html
b/UI/Reports/filters/income_statement.html
index e5ff91f..0a26d2c
--- a/UI/Reports/filters/income_statement.html
+++ b/UI/Reports/filters/income_statement.html
@@ -119,17 +119,50 @@ PROCESS input element_data = {
</table>
</td>
</tr>
+ <tr>
+ <th><?lsmb text('Comparison selection') ?></th>
+ <td>
+ <?lsmb PROCESS input element_data={
+ type = 'radio',
+ name = 'comparison_type',
+ id = 'comparison_by_periods',
+ value = 'by_periods',
+ label = text('By # of Periods'),
+ checked = 'checked',
+ } -?>
+ <?lsmb PROCESS input element_data={
+ type = 'radio',
+ name = 'comparison_type',
+ id = 'By_dates',
+ value = 'by_dates',
+ label = text('By dates'),
+ } -?>
+ </td>
+ </tr>
+ <tr>
+ <td><?lsmb PROCESS input element_data = {
+ label = text('Comparison Periods')
+ name = 'comparison_periods'
+ text_attr = 'description'
+ value_attr = 'text'
+ size = '3'
+ default_values = 1
+ };
+ ?></td>
+ </tr>
<tr>
<td>
<table>
<?lsmb PROCESS business_classes;
PROCESS date_row; # TODO add comparisons ?>
+<?lsmb IF detailed_checked ?>
<tr>
<th colspan="7"><?lsmb text('Enter up to 3 date ranges for
comparison') ?></th>
</tr>
<?lsmb FOREACH c IN [1, 2, 3];
PROCESS date_row SUFFIX='_' _ c;
END ?>
+<?lsmb END ?>
</table>
</td>
</tr>

--

Bye,

Erik.

http://efficito.com -- Hosted accounting and ERP.

Robust and Flexible. No vendor lock-in.