Bug 36068 - Add maintenance script acq_cancel_obsolete_orders.pl
Summary: Add maintenance script acq_cancel_obsolete_orders.pl
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 27893 35994
  Show dependency treegraph
 
Reported: 2024-02-12 10:24 UTC by Marcel de Rooy
Modified: 2024-04-19 16:07 UTC (History)
3 users (show)

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


Attachments
Bug 36068: Add maint script acq_cancel_obsolete_orders.pl (9.15 KB, patch)
2024-02-19 08:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36068: Do not overwrite cancellation date in ->cancel (2.41 KB, patch)
2024-02-19 08:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36068: Add maint script acq_cancel_obsolete_orders.pl (9.21 KB, patch)
2024-02-19 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36068: Do not overwrite cancellation date in ->cancel (2.41 KB, patch)
2024-02-19 08:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36068: Add maint script acq_cancel_obsolete_orders.pl (9.22 KB, patch)
2024-03-22 16:48 UTC, David Nind
Details | Diff | Splinter Review
Bug 36068: Do not overwrite cancellation date in ->cancel (2.42 KB, patch)
2024-03-22 16:48 UTC, David Nind
Details | Diff | Splinter Review
Bug 36068: Add maint script acq_cancel_obsolete_orders.pl (9.29 KB, patch)
2024-04-17 12:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36068: Do not overwrite cancellation date in ->cancel (2.49 KB, patch)
2024-04-17 12:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36068: (QA follow-up) Add OPTIONS section in POD (1.29 KB, patch)
2024-04-18 14:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2024-02-12 10:24:26 UTC
i.e. Cancel order lines that have no biblio anymore but are still considered open (received < ordered, not cancelled).

Note that Koha still is a bit hybrid to determine if an order has been cancelled: we have orderstatus and we have datecancellationprinted..
Comment 1 Marcel de Rooy 2024-02-14 15:32:50 UTC
(In reply to Marcel de Rooy from comment #0)
> Note that Koha still is a bit hybrid to determine if an order has been
> cancelled: we have orderstatus and we have datecancellationprinted..

And in production data I come across both flavors:
status=ordered and date cancellation filled
status=cancelled and date is not filled.
Comment 2 Katrin Fischer 2024-02-14 18:57:33 UTC
Hm, I would assume this should not happen with "today's" code, but could be from earlier versions.

 Can you maybe tell form the dates/ordernumbers if this is older or more recnt? 
Or could it be migration/imported data?
Comment 3 Marcel de Rooy 2024-02-15 07:18:28 UTC
(In reply to Katrin Fischer from comment #2)
> Hm, I would assume this should not happen with "today's" code, but could be
> from earlier versions.
> 
>  Can you maybe tell form the dates/ordernumbers if this is older or more
> recnt? 
> Or could it be migration/imported data?

No migrated data.
Yes, it seems that by far most occurrences are from 5-10 years ago. But I think that I had at least a few more recent.
Many installs will probably have such historical cases and this script lets you cleanup.
Comment 4 Marcel de Rooy 2024-02-19 08:24:42 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2024-02-19 08:24:44 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2024-02-19 08:26:04 UTC
Created attachment 162261 [details] [review]
Bug 36068: Add maint script acq_cancel_obsolete_orders.pl

Introducing $orders->filter_by_obsolete and $orders->cancel.

Test plan:
Run t/db_dependent/Koha/Acquisition/Orders.t
Create basket with few orders. Remove some biblio records.
Run acq_cancel_obsolete_orders.pl -c

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2024-02-19 08:26:06 UTC
Created attachment 162262 [details] [review]
Bug 36068: Do not overwrite cancellation date in ->cancel

Test plan:
Run t/db_dependent/Koha/Acquisition/Order.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 David Nind 2024-03-22 16:48:05 UTC
Created attachment 163721 [details] [review]
Bug 36068: Add maint script acq_cancel_obsolete_orders.pl

Introducing $orders->filter_by_obsolete and $orders->cancel.

Test plan:
Run t/db_dependent/Koha/Acquisition/Orders.t
Create basket with few orders. Remove some biblio records.
Run acq_cancel_obsolete_orders.pl -c

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2024-03-22 16:48:08 UTC
Created attachment 163722 [details] [review]
Bug 36068: Do not overwrite cancellation date in ->cancel

Test plan:
Run t/db_dependent/Koha/Acquisition/Order.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Comment 10 Martin Renvoize 2024-04-17 12:24:47 UTC
Created attachment 165000 [details] [review]
Bug 36068: Add maint script acq_cancel_obsolete_orders.pl

Introducing $orders->filter_by_obsolete and $orders->cancel.

Test plan:
Run t/db_dependent/Koha/Acquisition/Orders.t
Create basket with few orders. Remove some biblio records.
Run acq_cancel_obsolete_orders.pl -c

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2024-04-17 12:24:49 UTC
Created attachment 165001 [details] [review]
Bug 36068: Do not overwrite cancellation date in ->cancel

Test plan:
Run t/db_dependent/Koha/Acquisition/Order.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2024-04-17 12:25:38 UTC
All works as described.. QA scripts are happy and new methods are tested.

Passing QA
Comment 13 Katrin Fischer 2024-04-18 14:01:42 UTC
Hi Marcel, trying to understand the logic here I stumbled on this line:

+    [1] Order lines that have no biblio anymore but are still considered open
+        (received < ordered, not cancelled).

Can you please explain especially the second part?

Similar here:

+    [1] Biblionumber is null but received < ordered and not cancelled.

Thanks for writing a lot of documentation on these scripts.

A note: -c is logical, but often reserves for "confirm". Should we maybe switch it?

./misc/maintenance/acq_cancel_obsolete_orders.pl -h

Gives me the documentation, but it doesn't contain the typical list of parameter the script accepts. I think it would be nice to be consistent here with the other scripts.
Comment 14 Marcel de Rooy 2024-04-18 14:16:35 UTC
(In reply to Katrin Fischer from comment #13)
> Hi Marcel, trying to understand the logic here I stumbled on this line:
> 
> +    [1] Order lines that have no biblio anymore but are still considered
> open
> +        (received < ordered, not cancelled).
> 
> Can you please explain especially the second part?

The biblio has been already deleted. But somehow we had order lines that were still 'open'. They were not cancelled. (Note that Koha is confused about that status because we have two fields: datecancellationprinted and status.)
What also was possible (perhaps by older bugs) is quantity undefined or received < quantity.
Comment 15 Marcel de Rooy 2024-04-18 14:18:41 UTC
(In reply to Katrin Fischer from comment #13)
> Similar here:
> 
> +    [1] Biblionumber is null but received < ordered and not cancelled.

Thats referring to the same. The script calls the module.
Comment 16 Marcel de Rooy 2024-04-18 14:21:03 UTC
(In reply to Katrin Fischer from comment #13)
> A note: -c is logical, but often reserves for "confirm". Should we maybe
> switch it?

+GetOptions(
+    'confirm' => \$params->{confirm}, 'help' => \$params->{help}, 'age:i' => \$params->{age},
+);

The c stands for confirm but it is confirming a cancellation. Script name is helpful too: acq_cancel_obsolete_orders.pl.
Comment 17 Marcel de Rooy 2024-04-18 14:22:23 UTC
(In reply to Katrin Fischer from comment #13)
> ./misc/maintenance/acq_cancel_obsolete_orders.pl -h
> 
> Gives me the documentation, but it doesn't contain the typical list of
> parameter the script accepts. I think it would be nice to be consistent here
> with the other scripts.

Will have a look.
Comment 18 Marcel de Rooy 2024-04-18 14:33:56 UTC
Created attachment 165113 [details] [review]
Bug 36068: (QA follow-up) Add OPTIONS section in POD

Test plan:
Run script with --help.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Katrin Fischer 2024-04-18 14:54:44 UTC
I think my issue is, I don't understand your syntax here:

(received < ordered, not cancelled).

How do you read the < ?
Comment 20 Marcel de Rooy 2024-04-19 06:43:22 UTC
(In reply to Katrin Fischer from comment #19)
> I think my issue is, I don't understand your syntax here:
> 
> (received < ordered, not cancelled).
> 
> How do you read the < ?

Simply as lower than. The number of received items is lower than the number ordered :)
Comment 21 Katrin Fischer 2024-04-19 16:07:36 UTC
Pushed for 24.05!

Well done everyone, thank you!