Bug 20125 - Add a cron script to claim late orders
Summary: Add a cron script to claim late orders
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Arnaud
QA Contact: Alex Arnaud
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-02 09:14 UTC by Alex Arnaud
Modified: 2023-09-01 21:32 UTC (History)
6 users (show)

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


Attachments
Bug 20125 - Add a script to claim late orders (ordersClaim.pl) (7.01 KB, patch)
2018-02-02 10:01 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20125 - Add a script to claim late orders (ordersClaim.pl) (7.01 KB, patch)
2018-02-07 14:06 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20125 - Add a script to claim late orders (ordersClaim.pl) (7.16 KB, patch)
2018-04-23 20:29 UTC, Séverine Queune
Details | Diff | Splinter 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 (3.23 KB, patch)
2018-04-25 09:09 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20125: Add a script to claim late orders (ordersClaim.pl) (7.15 KB, patch)
2018-04-25 09:11 UTC, Alex Arnaud
Details | Diff | Splinter 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 (3.23 KB, patch)
2018-04-25 09:11 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20125: Take into account new table aqorders_claims (2.85 KB, patch)
2020-11-04 08:57 UTC, Alex Arnaud
Details | Diff | Splinter Review
Bug 20125: Make Koha::Acquisition::Orders->filter_by_lates usable in command line scripts (923 bytes, patch)
2020-11-04 10:34 UTC, Alex Arnaud
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Arnaud 2018-02-02 09:14:23 UTC
We need a script that automatically send order claims by mail to the booksellers. The script should take 3 important paramters:

delay: Number of days from which an orders is considered as late
claimed-for: Minimum number of days since the last claim
max-claim: Maximum number of claims
Comment 1 Alex Arnaud 2018-02-02 10:01:43 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2018-02-03 08:27:08 UTC
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?
Comment 3 Katrin Fischer 2018-02-03 08:30:32 UTC
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.
Comment 4 Alex Arnaud 2018-02-07 14:06:12 UTC
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
Comment 5 Alex Arnaud 2018-02-07 14:10:50 UTC
(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)
Comment 6 Katrin Fischer 2018-02-07 18:37:55 UTC
Ah, so it's optional? Great :)
Comment 7 Séverine Queune 2018-04-23 20:29:02 UTC
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>
Comment 8 Séverine Queune 2018-04-23 20:39:58 UTC
Works as described, thanks for this very clear test plan !
Comment 9 Katrin Fischer 2018-04-24 06:01:58 UTC
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?
Comment 10 Alex Arnaud 2018-04-25 09:09:59 UTC Comment hidden (obsolete)
Comment 11 Alex Arnaud 2018-04-25 09:11:01 UTC
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>
Comment 12 Alex Arnaud 2018-04-25 09:11:08 UTC
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
Comment 13 Mark Tompsett 2018-10-03 19:26:43 UTC
perlcritic -1 ... WOW! It's so clean compared to most Koha things.
Comment 14 Fridolin Somers 2020-08-26 13:55:40 UTC
It may be missing "=cut" in misc/cronjobs/ordersClaim.pl
Comment 15 Alex Arnaud 2020-11-04 08:57:55 UTC
Created attachment 112977 [details] [review]
Bug 20125: Take into account new table aqorders_claims
Comment 16 Alex Arnaud 2020-11-04 10:34:56 UTC
Created attachment 112981 [details] [review]
Bug 20125: Make Koha::Acquisition::Orders->filter_by_lates usable in command line scripts
Comment 17 Fridolin Somers 2023-09-01 21:26:11 UTC
+    my $basket = $schema->resultset('Aqbookseller')->find(
+        { id => $basketno },
+        { result_class => 'DBIx::Class::ResultClass::HashRefInflator' })

We can now use Koha::Acquisition::Bookseller no ?
Comment 18 Fridolin Somers 2023-09-01 21:32:17 UTC
+use C4::Letters;
+use C4::Acquisition;


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

C4::Acquisition seems not used anymore