Bug 14946 - Remove C4::Dates from files acqui/*.pl
Summary: Remove C4::Dates from files acqui/*.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 15166
Blocks: 14870
  Show dependency treegraph
 
Reported: 2015-10-02 20:35 UTC by Marc Véron
Modified: 2016-06-21 21:39 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14946 - Remove C4::Dates from files acqui/*.pl (13.26 KB, patch)
2015-10-02 20:50 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14946 - Remove C4::Dates from files acqui/*.pl (14.19 KB, patch)
2015-10-05 15:00 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14946 - Remove C4::Dates from files acqui/*.pl (14.21 KB, patch)
2015-10-25 16:17 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14946 - Remove C4::Dates from files acqui/*.pl (16.72 KB, patch)
2015-11-02 20:16 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14946 - Remove C4::Dates from files acqui/*.pl (16.78 KB, patch)
2015-11-06 17:27 UTC, Mirko Tietgen
Details | Diff | Splinter Review
Bug 14946: (followup) Remove C4::Dates from files acqui... (7.62 KB, patch)
2015-11-10 15:18 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14946: Remove C4::Dates from acqui/* files (23.87 KB, patch)
2015-11-10 18:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14946: Remove C4::Dates from acqui/* files (24.02 KB, patch)
2015-11-10 21:57 UTC, Marc Véron
Details | Diff | Splinter Review
Screenshot of invoices page (21.83 KB, image/png)
2015-11-17 18:40 UTC, Katrin Fischer
Details
Screenshot of invoice page (12.56 KB, image/png)
2015-11-17 18:40 UTC, Katrin Fischer
Details
[PASSED QA] Bug 14946: Remove C4::Dates from acqui/* files (24.07 KB, patch)
2015-11-17 23:39 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marc Véron 2015-10-02 20:35:54 UTC
This patch removes C4::Dates from:
- acqui/histsearch.pl
- acqui/invoice.pl
- acqui/invoices.pl
- acqui/orderreceive.pl
- acqui/parcel.pl
- acqui/parcels.pl
- acqui/transferorder.pl
Comment 1 Marc Véron 2015-10-02 20:50:10 UTC Comment hidden (obsolete)
Comment 2 Joonas Kylmälä 2015-10-05 08:35:20 UTC
In the file orderreceive.pl there is code depending to Dates.pm:
line 207:  date                  => format_date($order->{entrydate}),

Otherwise the patch looks good! :)
Comment 3 Marc Véron 2015-10-05 15:00:15 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2015-10-25 16:17:49 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2015-10-29 14:10:13 UTC
Still applies
Comment 6 Mark Tompsett 2015-11-02 14:13:08 UTC
Comment on attachment 43954 [details] [review]
Bug 14946 - Remove C4::Dates from files acqui/*.pl

Review of attachment 43954 [details] [review]:
-----------------------------------------------------------------

Some of these comments happen multiple times in this patch. I just picked on a couple.

::: acqui/finishreceive.pl
@@ +63,5 @@
>  my $new_ordernumber  = $ordernumber;
>  
> +$datereceived = eval { output_pref( { dt => dt_from_string( $datereceived) , dateonly => 1, dateformat => 'iso' } ); };
> +$datereceived = output_pref( { dt => dt_from_string( $datereceived) , dateonly => 1, dateformat => 'iso' } )
> +    if (! $datereceived );

Either both lines will succeed or both will fail. Why is there $datereceived in the second line call to dt_from_string?

::: acqui/histsearch.pl
@@ +101,2 @@
>  }
> +if ( $d = $input->param('to') ) {

Why is this 'to' and the other is 'iso'? I don't see a template change in this patch.

::: acqui/invoice.pl
@@ +84,5 @@
> +    my $billingdate        = '';
> +
> +    if ( $input->param('shipmentdate') ) {
> +        $shipmentdate = eval { dt_from_string( $input->param('shipmentdate') ) };
> +        $shipmentdate = output_pref( { dt => $shipmentdate, dateformat => 'iso',  dateonly => 1 } ) if ( $shipmentdate );

Post-fix if required because param('shipmentdate') could be a bad date?

::: acqui/orderreceive.pl
@@ +222,4 @@
>      invoiceid             => $invoice->{invoiceid},
>      invoice               => $invoice->{invoicenumber},
>      datereceived          => $datereceived,
> +    datereceived_iso      => output_pref( { dt => $datereceived, dateonly => 1, dateformat => 'iso' } ),

Formatting a date to ISO seems like a GUI issue. Is there not a way to format dates ISO in the Template? If not, ignore this. :)

::: acqui/parcels.pl
@@ +146,5 @@
>  my @parcels = GetInvoices(
>      supplierid => $booksellerid,
>      invoicenumber => $code,
> +    ( $datefrom ? ( eval { shipmentdatefrom => output_pref({ dt => dt_from_string($datefrom), dateformat => 'iso' }); } ) : () ),
> +    ( $dateto   ? ( eval { shipmentdateto   => output_pref({ dt => dt_from_string($dateto),   dateformat => 'iso' }); } )    : () ),

Nested code like this is difficult to read, and the ? makes it worse. Please set some local variables outside the parameter list to use.
Comment 7 Marc Véron 2015-11-02 20:16:01 UTC Comment hidden (obsolete)
Comment 8 Mark Tompsett 2015-11-03 00:48:01 UTC
Comment on attachment 44295 [details] [review]
Bug 14946 - Remove C4::Dates from files acqui/*.pl

Review of attachment 44295 [details] [review]:
-----------------------------------------------------------------

You missed a point. I'm assuming that your lack of comment about "Post-fix if required because param('shipmentdate') could be a bad date?" is because this is true.

::: acqui/histsearch.pl
@@ +95,4 @@
>      }
>  );
>  
> +my ( $from_iso, $to_iso );

Nice improvement removing the $d.

@@ +102,2 @@
>  }
> +if ( $input->param('to') ) {

... and yet this says 'to'. You didn't explain why the change, or correct it.

@@ -102,2 @@
>  }
> -if ( $d = $input->param('iso') ) {

This says 'iso'...
Comment 9 Marc Véron 2015-11-03 09:12:34 UTC
Thanks again M.Tompsett for reviewing the code. Sorry for any missing comments from my part.

I comment in below.

Could you already test the functionality?


(In reply to M. Tompsett from comment #8)
> Comment on attachment 44295 [details] [review] [review]
> Bug 14946 - Remove C4::Dates from files acqui/*.pl
> 
> Review of attachment 44295 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> You missed a point. I'm assuming that your lack of comment about "Post-fix
> if required because param('shipmentdate') could be a bad date?" is because
> this is true.

MV: Yes.

> 
> ::: acqui/histsearch.pl
> @@ +95,4 @@
> >      }
> >  );
> >  
> > +my ( $from_iso, $to_iso );
> 
> Nice improvement removing the $d.

MV: Thanks. BTW my aim is to remove C4:Dates for finally have our official date format dd.mm.yyyy (it is really important) - and not to make the code more elegant. In my very long journey through Koha files I saw a lot of places where such improvements could be done :-)

> 
> @@ +102,2 @@
> >  }
> > +if ( $input->param('to') ) {
> 
> ... and yet this says 'to'. You didn't explain why the change, or correct it.
> 
> @@ -102,2 @@
> >  }
> > -if ( $d = $input->param('iso') ) {
> 
> This says 'iso'...

MV: It's a fix - there is no incoming param 'iso'. We have a param 'from' and a param 'to' resulting in $from_iso and $to_iso. I tested several times to make sure that my change does not break the behaviour.
Comment 10 Mirko Tietgen 2015-11-06 17:27:20 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2015-11-06 18:27:54 UTC
(In reply to Marc Véron from comment #9)
[SNIP]
> > @@ +102,2 @@
> > >  }
> > > +if ( $input->param('to') ) {
> > 
> > ... and yet this says 'to'. You didn't explain why the change, or correct it.
> > 
> > @@ -102,2 @@
> > >  }
> > > -if ( $d = $input->param('iso') ) {
> > 
> > This says 'iso'...
> 
> MV: It's a fix - there is no incoming param 'iso'. We have a param 'from'
> and a param 'to' resulting in $from_iso and $to_iso. I tested several times
> to make sure that my change does not break the behaviour.

That's all I was looking for. :) Since, Mirko has signed off. I'll let it progress to Needs QA. :)
Comment 12 Jonathan Druart 2015-11-10 11:08:16 UTC
Comment on attachment 44596 [details] [review]
Bug 14946 - Remove C4::Dates from files acqui/*.pl

Review of attachment 44596 [details] [review]:
-----------------------------------------------------------------

::: acqui/finishreceive.pl
@@ +63,5 @@
>  my $new_ordernumber  = $ordernumber;
>  
> +my $datereceived_dt = eval { dt_from_string( $datereceived) };
> +$datereceived_dt = dt_from_string unless ( $datereceived_dt );
> +$datereceived = output_pref( { dt => $datereceived_dt , dateonly => 1, dateformat => 'iso' } ) ;

I don't understand how this change could be useful.

::: acqui/histsearch.pl
@@ +126,5 @@
>      );
>  }
>  
> +my $from_date = output_pref( { dt => $from_placed_on, dateformat => 'iso',  dateonly => 1 } );
> +my $to_date = output_pref( { dt => $to_placed_on, dateformat => 'iso',  dateonly => 1 } );

This is wrong, why do you want to display in iso format?
Prefer to do that on TT side.

::: acqui/invoice.pl
@@ +90,5 @@
> +
> +    if ( $input->param('billingdate') ) {
> +        $billingdate = eval { dt_from_string( $input->param('billingdate') ) };
> +        $billingdate = output_pref( { dt => $billingdate, dateformat => 'iso',  dateonly => 1 } ) if ( $billingdate );
> +    }

This is not exactly the same as before but is an improvement.
If the date is wrong, don't set it as today, but keep is undef (not important atm, the input is readonly).

::: acqui/invoices.pl
@@ +68,5 @@
> +
> +my $shipmentdatefrom_iso = '';
> +my $shipmentdateto_iso = '';
> +my $billingdatefrom_iso = '';
> +my $billingdateto_iso = '';

prefer my ($var1, $var2, ..., $varN). No need to assign them an empty string.

@@ +162,5 @@
> +    suppliername     => $suppliername,
> +    shipmentdatefrom => $shipmentdatefrom_iso,
> +    shipmentdateto   => $shipmentdateto_iso,
> +    billingdatefrom  => $billingdatefrom_iso,
> +    billingdateto    => $billingdateto_iso,

This is wrong, the dates are displayed in iso format.

::: acqui/orderreceive.pl
@@ -74,4 @@
>  use C4::Biblio;
>  use C4::Suggestions;
>  
> -use Koha::Acquisition::Bookseller;

Why do you remove it?

@@ +193,5 @@
>  
>  my $datereceived = $order->{datereceived} ? dt_from_string( $order->{datereceived} ) : dt_from_string;
>  
> +my $orderentrydate =  eval{ dt_from_string( $order->{entrydate} ); };
> +$orderentrydate = output_pref( { dt => $orderentrydate, dateonly => 1 } ) if ( $orderentrydate );

I don't understand these changes.

::: acqui/parcels.pl
@@ +169,5 @@
>  my @parcels = GetInvoices(
> +    supplierid       => $booksellerid,
> +    invoicenumber    => $code,
> +    shipmentdatefrom => $datefrom_iso,
> +    shipmentdateto   => $dateto_iso,

I'd keep the previous behavior, don't pass shipmentdateto/from if no need to pass them.
Why did you change this part of code?
Comment 13 Marc Véron 2015-11-10 15:18:17 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-11-10 18:00:29 UTC Comment hidden (obsolete)
Comment 15 Marc Véron 2015-11-10 21:03:08 UTC
Tests:

1/ Search for orders using the different filters
  => OK
2/ Create an invoice, try with setting and leaving empty the date
fields.
Default should be an undefined value (not today)
  => OK
4/ Receive an order, on creating the invoice, the default date should be
today.
  => OK

3/ Search for invoices and use the 4 different filters.
Close and reopen invoices.
The filters should be kept from one page to the other (not that it does
not work with shipment date before this patch).
  => Not OK
  I have invoices with shipment dates 02/11/2015, 11/11/2015,10/11/2015 
  Set filter for shipment date from 01/11/2015 to 11/11/2015 
  Result: Line with "Shipment date: From 01/11/2015 To 11/11/2015 ", 
  but no  results.
  Same with billing dates: one with 30/09/2015, three with 02/10/2015
  If I set the filter for billing date from 30/09/2015 to 02/10/2015 I get
  a line with "Billing date: From 30/09/2015 To 02/10/2015", but no results.
Comment 16 Marc Véron 2015-11-10 21:53:36 UTC
3/ Search for invoices and use the 4 different filters.
Close and reopen invoices.
The filters should be kept from one page to the other (not that it does
not work with shipment date before this patch).

=> Works OK (With 15166 applied, sorry I was not aware).
Comment 17 Marc Véron 2015-11-10 21:57:53 UTC Comment hidden (obsolete)
Comment 18 Katrin Fischer 2015-11-17 18:39:42 UTC
There is something very weird on the invoices and invoice pages - see screenshots attached. The dates in the database are correct and the dateformat setting is set to DD/MM/YYYY.
Comment 19 Katrin Fischer 2015-11-17 18:40:20 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2015-11-17 18:40:52 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2015-11-17 18:44:52 UTC
... what seemed so weird is just another dateformat setting - I had just reloaded an older database *sigh*.
Comment 22 Katrin Fischer 2015-11-17 23:39:06 UTC
Created attachment 44936 [details] [review]
[PASSED QA] Bug 14946: Remove C4::Dates from acqui/* files

This patch replaces all use of C4::Dates with Koha::DateUtils, which is
the best practice to follow.

It also fixes a bug on the invoice search, the shipment date (from and
to) were not passed correctly from one page to the other.

Test plan:
1/ Search for orders using the different filters
2/ Create an invoice, try with setting and leaving empty the date
fields.
Default should be an undefined value (not today)
3/ Search for invoices and use the 4 different filters.
Close and reopen invoices.
The filters should be kept from one page to the other (not that it does
not work with shipment date before this patch).
4/ Receive an order, on creating the invoice, the default date should be
today.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Tomás Cohen Arazi 2015-11-18 02:46:38 UTC
Patch pushed to master.

Thanks Jonathan!