In the file orderreceive.pl there is code depending to Dates.pm:
line 207:  date                  => format_date($order->{entrydate}),

Otherwise the patch looks good! :)

Still applies

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 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'...

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.

(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 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?

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.

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).

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.

... what seemed so weird is just another dateformat setting - I had just reloaded an older database *sigh*.

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>

Patch pushed to master.

Thanks Jonathan!