Just wondering: we configure a delay value for each vendor, can this be used in the script? I think it would make sense and have the delay value as a possible overwrite if you haven't used the field maybe?

It's called grace period actually in the interface.

The thought is that you can treat vendors differently. Your local bookstore will be usually faster than something you ordered from outside the country.

Created attachment 71274 [details] [review]
Bug 20125 - Add a script to claim late orders (ordersClaim.pl)

Test plan:

 - Put some orders late from different baskets by changing the closedate
   column in aqbasket table,

Testing delay parameters:
 - launch "perl misc/cronjobs/ordersClaim.pl" with the following
   parameters:
   - delay: Up to the number of days from the basket's closedate,
   - claimed-for: no matter for the first claim,
 - Check that there is one entry in message_queue *for each basket with
   late order(s)*
 - Check these late orders in database:
   - claims_count column should be set to 1,
   - claimed_date should be today

Testing claimed-for parameter:
 - change the claimed_date column in database for all orders (i.e 2 days
   back),
 - launch "perl misc/cronjobs/ordersClaim.pl" again with the following
   parameters:
   - delay: Same than for the first test,
   - claimed-for: 3 (if set claimed_date 2 days back),
   - max-claims: at least 2
 - check that there is no new entry in message_queue
 - launch "perl misc/cronjobs/ordersClaim.pl" again by moving only
   claimed-for to 1
 - There should be new entry in message_queue
 - claims_count column should be set to 2
 - claimed_date should be today

Testing max-claims parameter:
 - change the claimed_date column in database for all orders (i.e 2 days
   back),
 - launch "perl misc/cronjobs/ordersClaim.pl" again with the following
   parameters:
   - delay: Same than for the first and second test,
   - claimed-for: 1 (if set claimed_date 2 days back),
   - max-claims: 1 and 2 (test the two options),
 - There should be new entry in message_queue
 - launch "perl misc/cronjobs/ordersClaim.pl" again by moving only
   max-claims to 3
 - There should be new entry in message_queue
 - claims_count column should be set to 3
 - claimed_date should be today

(In reply to Katrin Fischer from comment #2)
> Just wondering: we configure a delay value for each vendor, can this be used
> in the script? I think it would make sense and have the delay value as a
> possible overwrite if you haven't used the field maybe?

The delay paramter of the script is set to the vendor's delivery time by default (if not used in the command)

Ah, so it's optional? Great :)

Created attachment 74767 [details] [review]
Bug 20125 - Add a script to claim late orders (ordersClaim.pl)

Test plan:

 - Put some orders late from different baskets by changing the closedate
   column in aqbasket table,

Testing delay parameters:
 - launch "perl misc/cronjobs/ordersClaim.pl" with the following
   parameters:
   - delay: Up to the number of days from the basket's closedate,
   - claimed-for: no matter for the first claim,
 - Check that there is one entry in message_queue *for each basket with
   late order(s)*
 - Check these late orders in database:
   - claims_count column should be set to 1,
   - claimed_date should be today

Testing claimed-for parameter:
 - change the claimed_date column in database for all orders (i.e 2 days
   back),
 - launch "perl misc/cronjobs/ordersClaim.pl" again with the following
   parameters:
   - delay: Same than for the first test,
   - claimed-for: 3 (if set claimed_date 2 days back),
   - max-claims: at least 2
 - check that there is no new entry in message_queue
 - launch "perl misc/cronjobs/ordersClaim.pl" again by moving only
   claimed-for to 1
 - There should be new entry in message_queue
 - claims_count column should be set to 2
 - claimed_date should be today

Testing max-claims parameter:
 - change the claimed_date column in database for all orders (i.e 2 days
   back),
 - launch "perl misc/cronjobs/ordersClaim.pl" again with the following
   parameters:
   - delay: Same than for the first and second test,
   - claimed-for: 1 (if set claimed_date 2 days back),
   - max-claims: 1 and 2 (test the two options),
 - There should be new entry in message_queue
 - launch "perl misc/cronjobs/ordersClaim.pl" again by moving only
   max-claims to 3
 - There should be new entry in message_queue
 - claims_count column should be set to 3
 - claimed_date should be today

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Works as described, thanks for this very clear test plan !

Comment on attachment 74767 [details] [review]
Bug 20125 - Add a script to claim late orders (ordersClaim.pl)

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

Hi Alex, 

a few small things pointed out by QA script:

 FAIL	misc/cronjobs/ordersClaim.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   FAIL	  pod
		
		 in file misc/cronjobs/ordersClaim.pl
		*** ERROR: 
		=over on line 54 without closing =back
		 at line EOF
   OK	  spelling
   OK	  valid


Processing additional checks

	* Commit title does not start with 'Bug XXXXX: ' - 18e290c

And some questions and notes from first review:

::: misc/cronjobs/ordersClaim.pl
@@ +18,5 @@
> +# along with Koha; if not, see <http://www.gnu.org/licenses>.
> +
> +=head1 NAME
> +
> +ordersClaim.pl - cron script that put late orders in message queue grouped by basket.

... that puts
Or maybe:
con script to automatically generate late order claims in the message queue grouped by basket.

@@ +129,5 @@
> +    next unless @$orders;
> +
> +    my $letter = GetPreparedLetter(
> +        module => 'claimacquisition',
> +        letter_code => 'ACQCLAIM',

The letter code for claiming acquisitions can be different from the sample provided by Koha. Koha only checks for the module and allows you to have multiple messages (for example in different languages). Can we make this a script option to be more flexible? Should at lest be documented.

@@ +139,5 @@
> +        },
> +        repeat => $orders,
> +    ) or next;
> +
> +    my $admin_address = C4::Context->preference('KohaAdminEmailAddress');

Maybe use the library from the basket if set first and only fall back to KohaAdminEmailAddress?

Created attachment 74838 [details] [review]
Bug 20125: Add a script to claim late orders (ordersClaim.pl)

Test plan:

 - Put some orders late from different baskets by changing the closedate
   column in aqbasket table,

Testing delay parameters:
 - launch "perl misc/cronjobs/ordersClaim.pl" with the following
   parameters:
   - delay: Up to the number of days from the basket's closedate,
   - claimed-for: no matter for the first claim,
 - Check that there is one entry in message_queue *for each basket with
   late order(s)*
 - Check these late orders in database:
   - claims_count column should be set to 1,
   - claimed_date should be today

Testing claimed-for parameter:
 - change the claimed_date column in database for all orders (i.e 2 days
   back),
 - launch "perl misc/cronjobs/ordersClaim.pl" again with the following
   parameters:
   - delay: Same than for the first test,
   - claimed-for: 3 (if set claimed_date 2 days back),
   - max-claims: at least 2
 - check that there is no new entry in message_queue
 - launch "perl misc/cronjobs/ordersClaim.pl" again by moving only
   claimed-for to 1
 - There should be new entry in message_queue
 - claims_count column should be set to 2
 - claimed_date should be today

Testing max-claims parameter:
 - change the claimed_date column in database for all orders (i.e 2 days
   back),
 - launch "perl misc/cronjobs/ordersClaim.pl" again with the following
   parameters:
   - delay: Same than for the first and second test,
   - claimed-for: 1 (if set claimed_date 2 days back),
   - max-claims: 1 and 2 (test the two options),
 - There should be new entry in message_queue
 - launch "perl misc/cronjobs/ordersClaim.pl" again by moving only
   max-claims to 3
 - There should be new entry in message_queue
 - claims_count column should be set to 3
 - claimed_date should be today

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Created attachment 74839 [details] [review]
Bug 20125: QA followup   - fix a QA script warning   - rename script description   - add letter-code parameter to ordersClaim script   - use, in order, library email or admin email as from address

perlcritic -1 ... WOW! It's so clean compared to most Koha things.

It may be missing "=cut" in misc/cronjobs/ordersClaim.pl

Created attachment 112977 [details] [review]
Bug 20125: Take into account new table aqorders_claims

Created attachment 112981 [details] [review]
Bug 20125: Make Koha::Acquisition::Orders->filter_by_lates usable in command line scripts

+    my $basket = $schema->resultset('Aqbookseller')->find(
+        { id => $basketno },
+        { result_class => 'DBIx::Class::ResultClass::HashRefInflator' })

We can now use Koha::Acquisition::Bookseller no ?

+use C4::Letters;
+use C4::Acquisition;


Needs to be :
+ use C4::Letters qw( GetPreparedLetter EnqueueLetter );

C4::Acquisition seems not used anymore

Is this a duplicate of Bug 18783? Have both been abandoned?

(In reply to Owen Leonard from comment #19)
> Is this a duplicate of Bug 18783? Have both been abandoned?

Not a duplicate as one of about orders and the other about serial issues. 
I'd still love to both features in Koha.

> my @lateorders = Koha::Acquisition::Orders->filter_by_lates({ delay => $delay });
Now needs "->as_list"

Maybe squash patches.