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
Created attachment 71139 [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
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 74837 [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
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.
I managed to rebase the patch but I get an error when I try to execute the script : -------- kohadev-koha@kohadevbox:koha(bug20125)$ misc/cronjobs/ordersClaim.pl -d 0 {UNKNOWN}: DBI Exception: DBD::mysql::db selectall_arrayref failed: Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause at /usr/share/perl5/DBIx/Class/Schema.pm line 1118. DBIx::Class::Schema::throw_exception(Koha::Schema=HASH(0x5630cbcc1000), "DBI Exception: DBD::mysql::db selectall_arrayref failed: Mixi"...) called at /usr/share/perl5/DBIx/Class/Storage.pm line 113 DBIx::Class::Storage::throw_exception(DBIx::Class::Storage::DBI::mysql=HASH(0x5630d59f10d0), "DBI Exception: DBD::mysql::db selectall_arrayref failed: Mixi"...) called at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1623 DBIx::Class::Storage::DBI::__ANON__("DBD::mysql::db selectall_arrayref failed: Mixing of GROUP col"..., DBI::db=HASH(0x5630d5e39220), undef) called at misc/cronjobs/ordersClaim.pl line 11 -------- If I run the same query with koha-mysql it does return results though : -------- MariaDB [koha_kohadev]> select aqorders.*, aqbasket.*, biblio.*, count(aqorders_claims.claimed_on) as claims_count, max(aqorders_claims.claimed_on) as last_claim FROM aqorders LEFT JOIN biblio ON aqorders.biblionumber = biblio.biblionumber LEFT JOIN aqbasket ON aqorders.basketno = aqbasket.basketno LEFT JOIN aqorders_claims ON aqorders.or dernumber = aqorders_claims.ordernumber having claims_count<=2 AND (last_claim IS NULL or last_claim <= DATE_SUB(CAST(now() AS date),INTERVAL 0 day)); 4 rows in set (0.019 sec) --------
I got rid of the error after setting this in koha-conf.xml : <strict_sql_modes>0</strict_sql_modes>
Created attachment 185807 [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>
Applied all diff suggested by Frido, rebased and squashed!
1. We should avoid such raw SQL queries. Cannot you do something like - loop on orders - use ->claims->filter_by_late (or use ->claimed_date) And use Koha::Objects everywhere? 2. We need to tests (and so move the code to methods) 3. We need try-catch and better error handling.