Bug 21975 - Unnecessary substitutions in automatic item modification by age
Summary: Unnecessary substitutions in automatic item modification by age
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Fridolin Somers
QA Contact: Testopia
URL:
Keywords:
Depends on: 11023 21206
Blocks:
  Show dependency treegraph
 
Reported: 2018-12-07 14:18 UTC by Fridolin Somers
Modified: 2020-06-04 20:32 UTC (History)
5 users (show)

See Also:
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


Attachments
Bug 21975: get item field in substitutions (1.59 KB, patch)
2018-12-07 15:41 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21975: add Unit Test (1.85 KB, patch)
2018-12-07 15:41 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age (1.56 KB, patch)
2018-12-07 15:41 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21975: get item field in substitutions (1.65 KB, patch)
2019-04-09 06:27 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21975: add Unit Test (1.92 KB, patch)
2019-04-09 06:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age (1.61 KB, patch)
2019-04-09 06:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21975 : (followup) get item datas from previous SQL query (1.40 KB, patch)
2019-04-12 09:45 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21975 : (followup 2) avoid undef in eq (1.08 KB, patch)
2019-04-12 09:52 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 21975: get item field in substitutions (1.65 KB, patch)
2019-04-12 11:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21975: add Unit Test (1.92 KB, patch)
2019-04-12 11:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age (1.61 KB, patch)
2019-04-12 11:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21975 : (followup) get item datas from previous SQL query (1.46 KB, patch)
2019-04-12 11:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21975 : (followup 2) avoid undef in eq (1.14 KB, patch)
2019-04-12 11:57 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 21975: get item field in substitutions (1.71 KB, patch)
2019-04-12 12:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21975: add Unit Test (1.97 KB, patch)
2019-04-12 12:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21975: Avoid unnecessary substitutions in automatic item modification by age (1.67 KB, patch)
2019-04-12 12:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21975: (follow-up) get item datas from previous SQL query (1.52 KB, patch)
2019-04-12 12:26 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21975: (follow-up) avoid undef in eq (1.19 KB, patch)
2019-04-12 12:26 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 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 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