Bug 22827 - Automatic item modifications by age: add age depencency on other field(s) than dateaccessioned
Summary: Automatic item modifications by age: add age depencency on other field(s) tha...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords: Academy
Depends on:
Blocks: 33979
  Show dependency treegraph
 
Reported: 2019-05-02 11:27 UTC by Michal Denar
Modified: 2023-06-14 09:18 UTC (History)
11 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement enables librarians to automatically modify items based on date fields other than items.dateaccessioned. The 'Automatic item modifications by age' tool can now key rules off any one the following: items.dateaccessioned, items.replacementpricedate, items.datelastborrowed, items.datelastseen, items.damaged_on, items.itemlost_on, items.withdrawn_on. Existing rules will continue to key off the items.dateaccessioned field.
Version(s) released in:
22.05.00


Attachments
Bug 22827: Add age dependency on other fields than dateaccessioned (11.16 KB, patch)
2022-01-13 05:42 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 22827: Add age dependency on other fields than dateaccessioned (12.27 KB, patch)
2022-01-14 03:08 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 22827: Add age dependency on other fields than dateaccessioned (12.31 KB, patch)
2022-01-14 13:28 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 22827: Add age dependency on other fields than dateaccessioned (12.37 KB, patch)
2022-03-26 23:33 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Michal Denar 2019-05-02 11:27:04 UTC
Some libraries needs automatic modification based on other date that dateaccessioned. For example: copyright rules allow start borrow some kind of materials x months after release date. In this case must be age modification based on other field than dateaccessioned.
Comment 1 Josef Moravec 2019-05-02 11:30:45 UTC
Current date columns of items table which could apply for this enhancement:

replacementpricedate
datelastborrowed
datelastseen
damaged_on
itemlost_on
withdrawn_on
Comment 2 Katrin Fischer 2020-05-10 10:55:45 UTC
I was about to file a new bug for this as we discussed the use of this tool in context of 'quarantining' books. I think having the option to use datelastseeen (updated on checkin) would be very helpful to enable us to make the necessary status change.

But the other columns listed also create ideas for use cases.
Comment 3 Andrew Fuerste-Henry 2020-07-07 16:48:43 UTC
+1, we definitely have libraries that would make use of this if it could key off of dates other than dateaccessioned.
Comment 4 Alex Buckley 2022-01-13 05:42:50 UTC
Created attachment 129371 [details] [review]
Bug 22827: Add age dependency on other fields than dateaccessioned

A new agefield has been added to the 'Automatic item modifications by
age' tool. The options for the agefield are: replacementpricedate, datelastborrowed,
datelastseen, damaged_on, itemlost_on, withdrawn_on

If no option is selected then Koha will default to using
dateaccessioned.

Test plan:

1. Go to: Tools > Catalog > Automatic item modifications by age
2. Observe there is a new 'Age field' dropdown in the rule form.
3. Create a rule, set the values:
- 'Age in days' = 20
- 'Substitutions': 'items.barcode' = 'test'
fields
- 'Age field' = 'items.datelastseen'
- Save the rule
4. Confirm the 'List of rules' page displays 'items.datelastseen' in the 'Age field' column
5. Add another rule:
- 'Age in days' = 1
- 'Substitutions': 'items.barcode' = 'test2'
- Leave 'Age field' = 'Choose an age field'
- Save the rule
6. Confirm the 'List of rules' page displays 'items.dateaccesioned' in
the 'Age field' column for that second rule
7. Confirm you can delete rules
8. Edit a record:
- Make the items.dateaccessioned less than 20 days ago (so rule 1 is false)
- Make the items.datelastseen more than 1 day ago (so rule 2 is true)
9. Run the automatic_items_modification_by_age.pl:
- sudo koha-shell <instance>
- cd misc/cronjobs
- ./automatic_item_modification_by_age.pl -v
10. Confirm the item has it's barcode set to 'test2'
11. Run unit tests:
- sudo koha-shell <instance>
- cd t
- prove db_dependent/Item/AutomaticItemModificationByAge.t -v

Sponsored-By: Catalyst IT
Comment 5 Alex Buckley 2022-01-13 05:44:11 UTC
Ready for testing. 

In my patchset the fields in comment #1 are hardcoded. Please let me know if there is a way to fetch datetime items columns without having to hardcode them?
Comment 6 Martin Renvoize 2022-01-13 09:58:34 UTC
I've not read the patch here yet.. but to answer your question.. you can introspect the Koha::Object.. see https://git.koha-community.org/Koha-community/Koha/src/branch/master/Koha/Object.pm#L129 for an example.

But.. I'm not sure if it's worth the overhead/extra code lines here if I'm honest?
Comment 7 Andrew Fuerste-Henry 2022-01-13 21:49:38 UTC
I followed the test plan successfully except:
- your directions are inconsistent. To make step 8 work, the rule in step 3 should use dateaccessioned and the rule in step 5 should use datelastseen
- I get a messy error on the test at the end. I don't have a clue what this means, so it could be my error:

Cannot detect source of 't/db_dependent/Item/AutomaticItemModificationByAge'! at /usr/share/perl/5.32/TAP/Parser/IteratorFactory.pm line 256.
	TAP::Parser::IteratorFactory::detect_source(TAP::Parser::IteratorFactory=HASH(0x55d2dc3c5c48), TAP::Parser::Source=HASH(0x55d2dc292438)) called at /usr/share/perl/5.32/TAP/Parser/IteratorFactory.pm line 211
	TAP::Parser::IteratorFactory::make_iterator(TAP::Parser::IteratorFactory=HASH(0x55d2dc3c5c48), TAP::Parser::Source=HASH(0x55d2dc292438)) called at /usr/share/perl/5.32/TAP/Parser.pm line 472
	TAP::Parser::_initialize(TAP::Parser=HASH(0x55d2dc030148), HASH(0x55d2dc03fee0)) called at /usr/share/perl/5.32/TAP/Object.pm line 55
	TAP::Object::new("TAP::Parser", HASH(0x55d2dc03fee0)) called at /usr/share/perl/5.32/TAP/Object.pm line 130
	TAP::Object::_construct(TAP::Harness=HASH(0x55d2dbad1678), "TAP::Parser", HASH(0x55d2dc03fee0)) called at /usr/share/perl/5.32/TAP/Harness.pm line 852
	TAP::Harness::make_parser(TAP::Harness=HASH(0x55d2dbad1678), TAP::Parser::Scheduler::Job=HASH(0x55d2dc268728)) called at /usr/share/perl/5.32/TAP/Harness.pm line 651
	TAP::Harness::_aggregate_single(TAP::Harness=HASH(0x55d2dbad1678), TAP::Parser::Aggregator=HASH(0x55d2dbad2128), TAP::Parser::Scheduler=HASH(0x55d2dc2686f8)) called at /usr/share/perl/5.32/TAP/Harness.pm line 743
	TAP::Harness::aggregate_tests(TAP::Harness=HASH(0x55d2dbad1678), TAP::Parser::Aggregator=HASH(0x55d2dbad2128), "t/db_dependent/Item/AutomaticItemModificationByAge.t") called at /usr/share/perl/5.32/TAP/Harness.pm line 558
	TAP::Harness::__ANON__() called at /usr/share/perl/5.32/TAP/Harness.pm line 571
	TAP::Harness::runtests(TAP::Harness=HASH(0x55d2dbad1678), "t/db_dependent/Item/AutomaticItemModificationByAge.t") called at /usr/share/perl/5.32/App/Prove.pm line 548
	App::Prove::_runtests(App::Prove=HASH(0x55d2dba9c538), HASH(0x55d2dbf17978), "t/db_dependent/Item/AutomaticItemModificationByAge.t") called at /usr/share/perl/5.32/App/Prove.pm line 506
	App::Prove::run(App::Prove=HASH(0x55d2dba9c538)) called at /usr/bin/prove line 13
Comment 8 Andrew Fuerste-Henry 2022-01-13 21:50:03 UTC
That said, I'm super excited to see a patch here and am eager to test again!
Comment 9 Alex Buckley 2022-01-13 21:52:03 UTC
(In reply to Andrew Fuerste-Henry from comment #8)
> That said, I'm super excited to see a patch here and am eager to test again!

Thanks Andrew. Hmm I'll fix up those 2 problems and attach a new patch to this ASAP!
Comment 10 Alex Buckley 2022-01-14 03:08:09 UTC
Created attachment 129460 [details] [review]
Bug 22827: Add age dependency on other fields than dateaccessioned

A new agefield has been added to the 'Automatic item modifications by
age' tool. The options for the agefield are: replacementpricedate, datelastborrowed,
datelastseen, damaged_on, itemlost_on, withdrawn_on

If no option is selected then Koha will default to saving 'agefield' =
items.dateaccessioned

Similarly, if a Koha instance has an old item rule without 'agefield'
defined then Koha will default to using 'items.dateaccessioned'.
This is confirmed by the AutomaticItemModificationByAge.t unit test.

Test plan:

1. Go to: Tools > Catalog > Automatic item modifications by age
2. Observe there is a new 'Age field' dropdown in the rule form.
3. Create a rule, set the values:
- 'Age in days' = 20
- Leave 'Age field' = 'Choose an age field'
- 'Substitutions': 'items.barcode' = 'test'
- Save the rule
4. Confirm the 'List of rules' page displays 'items.dateaccessioned in the 'Age field' column
5. Add another rule:
- 'Age in days' = 2
- 'Age field' = 'items.datelastseen'
- 'Substitutions': 'items.barcode' = 'test2'
- Save the rule
6. Confirm the 'List of rules' page displays 'items.datelastseen' in
the 'Age field' column for that second rule
7. Add some more rules and confirm you can delete them
8. Edit a record:
- Make the items.dateaccessioned = 3 day ago (so rule 1 is false)
- Make the items.datelastseen = 3 days ago (so rule 2 is true)
9. Run the automatic_items_modification_by_age.pl:
- sudo koha-shell <instance>
- cd misc/cronjobs
- ./automatic_item_modification_by_age.pl -v -c
10. Confirm the item has it's barcode set to 'test2'
11. Run unit tests:
- sudo koha-shell <instance>
- prove t/db_dependent/Items/AutomaticItemModificationByAge.t -v

Sponsored-By: Catalyst IT
Comment 11 Alex Buckley 2022-01-14 03:19:41 UTC
Hi Andrew and Martin, 

Martin: Thank you for your reply in comment #6. My concerns with hard coding were:

1. Everytime a datetime column is added to the items table then the community would have to remember to update this hardcoded list of columns.

2. If an existing datetime column is removed/renamed then the hardcoded list would have to be updated otherwise my alterations to C4::Items->ToggleNewStatus() would throw an error. As the hardcoded list determines what column is queried.

But as you say doing an introspect of the koha::Object may not be worthwhile. So I have left the hardcoded list for now. I'm happy to add it later though if that is what QA wants.

Andrew: I have corrected the test plan. That error you encountered was because my test plan noted an incorrect unit test filepath. So it was not your error :)

Could you please re-test now?

Many thanks!
Alex
Comment 12 Andrew Fuerste-Henry 2022-01-14 13:28:02 UTC
Created attachment 129481 [details] [review]
Bug 22827: Add age dependency on other fields than dateaccessioned

A new agefield has been added to the 'Automatic item modifications by
age' tool. The options for the agefield are: replacementpricedate, datelastborrowed,
datelastseen, damaged_on, itemlost_on, withdrawn_on

If no option is selected then Koha will default to saving 'agefield' =
items.dateaccessioned

Similarly, if a Koha instance has an old item rule without 'agefield'
defined then Koha will default to using 'items.dateaccessioned'.
This is confirmed by the AutomaticItemModificationByAge.t unit test.

Test plan:

1. Go to: Tools > Catalog > Automatic item modifications by age
2. Observe there is a new 'Age field' dropdown in the rule form.
3. Create a rule, set the values:
- 'Age in days' = 20
- Leave 'Age field' = 'Choose an age field'
- 'Substitutions': 'items.barcode' = 'test'
- Save the rule
4. Confirm the 'List of rules' page displays 'items.dateaccessioned in the 'Age field' column
5. Add another rule:
- 'Age in days' = 2
- 'Age field' = 'items.datelastseen'
- 'Substitutions': 'items.barcode' = 'test2'
- Save the rule
6. Confirm the 'List of rules' page displays 'items.datelastseen' in
the 'Age field' column for that second rule
7. Add some more rules and confirm you can delete them
8. Edit a record:
- Make the items.dateaccessioned = 3 day ago (so rule 1 is false)
- Make the items.datelastseen = 3 days ago (so rule 2 is true)
9. Run the automatic_items_modification_by_age.pl:
- sudo koha-shell <instance>
- cd misc/cronjobs
- ./automatic_item_modification_by_age.pl -v -c
10. Confirm the item has it's barcode set to 'test2'
11. Run unit tests:
- sudo koha-shell <instance>
- prove t/db_dependent/Items/AutomaticItemModificationByAge.t -v

Sponsored-By: Catalyst IT

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 13 Katrin Fischer 2022-03-26 23:32:00 UTC
I was wondering about the fallback instead of updating the rules, until I realized that the rules are stored in a system preference: automatic_item_modification_by_age_configuration 

Also filed a new bug for a warning that exists before and after the patches are applied: Bug 30366 - Warn when running automatic_item_modification_by_age.pl
Comment 14 Katrin Fischer 2022-03-26 23:33:15 UTC
Created attachment 132265 [details] [review]
Bug 22827: Add age dependency on other fields than dateaccessioned

A new agefield has been added to the 'Automatic item modifications by
age' tool. The options for the agefield are: replacementpricedate, datelastborrowed,
datelastseen, damaged_on, itemlost_on, withdrawn_on

If no option is selected then Koha will default to saving 'agefield' =
items.dateaccessioned

Similarly, if a Koha instance has an old item rule without 'agefield'
defined then Koha will default to using 'items.dateaccessioned'.
This is confirmed by the AutomaticItemModificationByAge.t unit test.

Test plan:

1. Go to: Tools > Catalog > Automatic item modifications by age
2. Observe there is a new 'Age field' dropdown in the rule form.
3. Create a rule, set the values:
- 'Age in days' = 20
- Leave 'Age field' = 'Choose an age field'
- 'Substitutions': 'items.barcode' = 'test'
- Save the rule
4. Confirm the 'List of rules' page displays 'items.dateaccessioned in the 'Age field' column
5. Add another rule:
- 'Age in days' = 2
- 'Age field' = 'items.datelastseen'
- 'Substitutions': 'items.barcode' = 'test2'
- Save the rule
6. Confirm the 'List of rules' page displays 'items.datelastseen' in
the 'Age field' column for that second rule
7. Add some more rules and confirm you can delete them
8. Edit a record:
- Make the items.dateaccessioned = 3 day ago (so rule 1 is false)
- Make the items.datelastseen = 3 days ago (so rule 2 is true)
9. Run the automatic_items_modification_by_age.pl:
- sudo koha-shell <instance>
- cd misc/cronjobs
- ./automatic_item_modification_by_age.pl -v -c
10. Confirm the item has it's barcode set to 'test2'
11. Run unit tests:
- sudo koha-shell <instance>
- prove t/db_dependent/Items/AutomaticItemModificationByAge.t -v

Sponsored-By: Catalyst IT

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Alex Buckley 2022-03-28 21:30:28 UTC
Thanks for testing and QA Andrew and Katrin!
Comment 16 Fridolin Somers 2022-04-08 10:05:03 UTC
Nice enhancement.
Please add a release notes to explain this new option
Comment 17 Fridolin Somers 2022-04-08 13:52:33 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 18 Alex Buckley 2022-04-09 09:10:48 UTC
(In reply to Fridolin Somers from comment #16)
> Nice enhancement.
> Please add a release notes to explain this new option

Thanks, Fridolin! I've added release notes, I hope they adequately explain this enhancement. 

Cheers,
Alex
Comment 19 Jonathan Druart 2023-05-30 06:54:24 UTC
<span class="hint">If not set then items.dateaccessioned will be used</span>

Instead of this we could just select this value in the dropdown list if none exist, and remove the empty option to make it mandatory.
Comment 20 Katrin Fischer 2023-05-30 09:48:31 UTC
(In reply to Jonathan Druart from comment #19)
> <span class="hint">If not set then items.dateaccessioned will be used</span>
> 
> Instead of this we could just select this value in the dropdown list if none
> exist, and remove the empty option to make it mandatory.

Note: We'd also need a database update for the old entries. I can try to file that later.
Comment 21 Jonathan Druart 2023-05-30 09:58:06 UTC
(In reply to Katrin Fischer from comment #20)
> (In reply to Jonathan Druart from comment #19)
> > <span class="hint">If not set then items.dateaccessioned will be used</span>
> > 
> > Instead of this we could just select this value in the dropdown list if none
> > exist, and remove the empty option to make it mandatory.
> 
> Note: We'd also need a database update for the old entries. I can try to
> file that later.

Not necessarily but yes that will be better ofc.
Comment 22 Katrin Fischer 2023-06-10 19:29:59 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Katrin Fischer from comment #20)
> > (In reply to Jonathan Druart from comment #19)
> > > <span class="hint">If not set then items.dateaccessioned will be used</span>
> > > 
> > > Instead of this we could just select this value in the dropdown list if none
> > > exist, and remove the empty option to make it mandatory.
> > 
> > Note: We'd also need a database update for the old entries. I can try to
> > file that later.
> 
> Not necessarily but yes that will be better ofc.

Filed as:
Bug 33979 - Improve behaviour of source list on 'item modifications by age' tool