Bug 21975

Summary: Unnecessary substitutions in automatic item modification by age
Product: Koha Reporter: Fridolin Somers <fridolin.somers>
Component: Command-line UtilitiesAssignee: Fridolin Somers <fridolin.somers>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: minor    
Priority: P5 - low CC: katrin.fischer, lucas, martin.renvoize, nick, robin
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.05
Bug Depends on: 11023, 21206    
Bug Blocks:    
Attachments: Bug 21975: get item field in substitutions
Bug 21975: add Unit Test
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age
Bug 21975: get item field in substitutions
Bug 21975: add Unit Test
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age
Bug 21975 : (followup) get item datas from previous SQL query
Bug 21975 : (followup 2) avoid undef in eq
Bug 21975: get item field in substitutions
Bug 21975: add Unit Test
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age
Bug 21975 : (followup) get item datas from previous SQL query
Bug 21975 : (followup 2) avoid undef in eq
Bug 21975: get item field in substitutions
Bug 21975: add Unit Test
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age
Bug 21975: (follow-up) get item datas from previous SQL query
Bug 21975: (follow-up) avoid undef in eq

Description Fridolin Somers 2018-12-07 14:18:32 UTC
Automatic item modification by age cronjob is based on rules with conditions and substitutions.
When substitution value is equal to actual item value, the code should not call C4::Items::ModItem. It adds unnecessary action log and entry in zebraqueue.
With a rule than can impact all catalogue you can explode your database with action logs.
Comment 1 Fridolin Somers 2018-12-07 15:41:50 UTC
Created attachment 82983 [details] [review]
Bug 21975: get item field in substitutions

substitutions field stores the item field with 'items.' prefix.
Looks like call of C4::Items::ModItem() works but it will not if ORM object is used in the futur.
So I prefer fix it now and it is needed for main patch.

Test plan :
Run t/db_dependent/Items/AutomaticItemModificationByAge.t
Comment 2 Fridolin Somers 2018-12-07 15:41:54 UTC
Created attachment 82984 [details] [review]
Bug 21975: add Unit Test

Added missing transaction rollback
Comment 3 Fridolin Somers 2018-12-07 15:41:58 UTC
Created attachment 82985 [details] [review]
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age

Automatic item modification by age cronjob is based on rules with conditions and substitutions.
When substitution value is equal to actual item value, the code should not call C4::Items::ModItem. It adds unnecessary action log and entry in zebraqueue.
With a rule than can impact all catalogue you can explode your database with action logs.

Test plan :
1) Run prove t/db_dependent/Items/AutomaticItemModificationByAge.t
2) Define a item modification by age with no condition
3) Run several times misc/cronjobs/automatic_item_modification_by_age.pl
4) Check it creates only one entry in action_logs and zebraqueue
Comment 4 Martin Renvoize 2019-04-09 06:27:59 UTC
Created attachment 87575 [details] [review]
Bug 21975: get item field in substitutions

substitutions field stores the item field with 'items.' prefix.
Looks like call of C4::Items::ModItem() works but it will not if ORM object is used in the futur.
So I prefer fix it now and it is needed for main patch.

Test plan :
Run t/db_dependent/Items/AutomaticItemModificationByAge.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2019-04-09 06:28:01 UTC
Created attachment 87576 [details] [review]
Bug 21975: add Unit Test

Added missing transaction rollback

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2019-04-09 06:28:04 UTC
Created attachment 87577 [details] [review]
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age

Automatic item modification by age cronjob is based on rules with conditions and substitutions.
When substitution value is equal to actual item value, the code should not call C4::Items::ModItem. It adds unnecessary action log and entry in zebraqueue.
With a rule than can impact all catalogue you can explode your database with action logs.

Test plan :
1) Run prove t/db_dependent/Items/AutomaticItemModificationByAge.t
2) Define a item modification by age with no condition
3) Run several times misc/cronjobs/automatic_item_modification_by_age.pl
4) Check it creates only one entry in action_logs and zebraqueue

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2019-04-09 06:28:42 UTC
Well spotted, good tests added.. works as expected.. Signing off
Comment 8 Katrin Fischer 2019-04-09 09:27:08 UTC
Fails QA script, can you please check? 

 FAIL	C4/Items.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   FAIL	  pod coverage
		POD coverage was greater before, try perl -MPod::Coverage=PackageName -e666
   OK	  spelling
   FAIL	  valid
		BEGIN failed--compilation aborted 
		Compilation failed in require 
		Global symbol "$item" requires explicit package name (did you forget to declare "my $item"?) 

 FAIL	t/db_dependent/Items/AutomaticItemModificationByAge.t
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  spelling
   FAIL	  valid
		Global symbol "$item" requires explicit package name (did you forget to declare "my $item"?) 
		Compilation failed in require
Comment 9 Fridolin Somers 2019-04-12 09:26:36 UTC
Ah OK, caused by Bug 21206
Comment 10 Fridolin Somers 2019-04-12 09:45:54 UTC
Created attachment 87876 [details] [review]
Bug 21975 : (followup) get item datas from previous SQL query

Forced by removal of C4::Items::GetItem from Bug 21206
Comment 11 Fridolin Somers 2019-04-12 09:52:35 UTC
Created attachment 87877 [details] [review]
Bug 21975 : (followup 2) avoid undef in eq
Comment 12 Martin Renvoize 2019-04-12 11:56:58 UTC
Created attachment 87879 [details] [review]
Bug 21975: get item field in substitutions

substitutions field stores the item field with 'items.' prefix.
Looks like call of C4::Items::ModItem() works but it will not if ORM object is used in the futur.
So I prefer fix it now and it is needed for main patch.

Test plan :
Run t/db_dependent/Items/AutomaticItemModificationByAge.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-04-12 11:57:01 UTC
Created attachment 87880 [details] [review]
Bug 21975: add Unit Test

Added missing transaction rollback

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2019-04-12 11:57:04 UTC
Created attachment 87881 [details] [review]
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age

Automatic item modification by age cronjob is based on rules with conditions and substitutions.
When substitution value is equal to actual item value, the code should not call C4::Items::ModItem. It adds unnecessary action log and entry in zebraqueue.
With a rule than can impact all catalogue you can explode your database with action logs.

Test plan :
1) Run prove t/db_dependent/Items/AutomaticItemModificationByAge.t
2) Define a item modification by age with no condition
3) Run several times misc/cronjobs/automatic_item_modification_by_age.pl
4) Check it creates only one entry in action_logs and zebraqueue

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2019-04-12 11:57:07 UTC
Created attachment 87882 [details] [review]
Bug 21975 : (followup) get item datas from previous SQL query

Forced by removal of C4::Items::GetItem from Bug 21206

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2019-04-12 11:57:09 UTC
Created attachment 87883 [details] [review]
Bug 21975 : (followup 2) avoid undef in eq

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2019-04-12 11:57:47 UTC
Back to SO.. works for me.
Comment 18 Katrin Fischer 2019-04-12 12:26:24 UTC
Created attachment 87889 [details] [review]
Bug 21975: get item field in substitutions

substitutions field stores the item field with 'items.' prefix.
Looks like call of C4::Items::ModItem() works but it will not if ORM object is used in the futur.
So I prefer fix it now and it is needed for main patch.

Test plan :
Run t/db_dependent/Items/AutomaticItemModificationByAge.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Katrin Fischer 2019-04-12 12:26:27 UTC
Created attachment 87890 [details] [review]
Bug 21975: add Unit Test

Added missing transaction rollback

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 20 Katrin Fischer 2019-04-12 12:26:31 UTC
Created attachment 87891 [details] [review]
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age

Automatic item modification by age cronjob is based on rules with conditions and substitutions.
When substitution value is equal to actual item value, the code should not call C4::Items::ModItem. It adds unnecessary action log and entry in zebraqueue.
With a rule than can impact all catalogue you can explode your database with action logs.

Test plan :
1) Run prove t/db_dependent/Items/AutomaticItemModificationByAge.t
2) Define a item modification by age with no condition
3) Run several times misc/cronjobs/automatic_item_modification_by_age.pl
4) Check it creates only one entry in action_logs and zebraqueue

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2019-04-12 12:26:35 UTC
Created attachment 87892 [details] [review]
Bug 21975: (follow-up) get item datas from previous SQL query

Forced by removal of C4::Items::GetItem from Bug 21206

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 22 Katrin Fischer 2019-04-12 12:26:38 UTC
Created attachment 87893 [details] [review]
Bug 21975: (follow-up) avoid undef in eq

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Nick Clemens (kidclamp) 2019-04-16 13:24:39 UTC
Awesome work all!

Pushed to master for 19.05
Comment 24 Martin Renvoize 2019-04-25 14:19:31 UTC
Pushed to 18.11.x for 18.11.05
Comment 25 Lucas Gass 2019-05-10 20:18:34 UTC
missing dependencies for 18.05.x, no backport