Re: Proposal, v1

On Sun, Apr 10, 2016 at 12:03 PM, Erik Huelsmann <..hidden..> wrote:

Hi Yves,

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};

die "Bad interval: $interval" if undefined $delta_name;

$date->date->add($delta_name => $n);

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;

Does this one here mean that there's always going to be a comparison? (The current report defaults to "no comparison"; I'm not sure we want to step away from that?)

+ 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};
+ }
+ }

Would it be an idea to abstract out this comparison-handling to the Comparison(to be created) or Dates role?

+ $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,

One quick procedural request: could you Cc the mailing list on mails you send to me with these review requests? That way the broader community can follow what we do and even chime in; most people aren't on IRC and those on IRC aren't always on at the times we are.

Thanks for your work!

--

Bye,

Erik.

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

Robust and Flexible. No vendor lock-in.