Bug 22140 - More use of EasyAnalyticalRecords pref
Summary: More use of EasyAnalyticalRecords pref
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin SOMERS
QA Contact: Testopia
URL:
Keywords:
Depends on: 20702 21971
Blocks: 22416
  Show dependency treegraph
 
Reported: 2019-01-16 12:31 UTC by Fridolin SOMERS
Modified: 2019-05-27 12:24 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22140: More use of EasyAnalyticalRecords pref (5.47 KB, patch)
2019-01-16 12:39 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 22140: Use of EasyAnalyticalRecords pref in search (5.48 KB, patch)
2019-02-06 14:30 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 22140: Use of EasyAnalyticalRecords pref is get_hostitemnumbers_of (1.30 KB, patch)
2019-02-06 14:31 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 22140: Larger unit test for get_hostitemnumbers_of (2.66 KB, patch)
2019-02-06 15:11 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 22140: Use of EasyAnalyticalRecords pref in search (5.54 KB, patch)
2019-02-11 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22140: Use of EasyAnalyticalRecords pref is get_hostitemnumbers_of (1.37 KB, patch)
2019-02-11 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22140: Larger unit test for get_hostitemnumbers_of (2.72 KB, patch)
2019-02-11 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22140: Use of EasyAnalyticalRecords pref in search (5.60 KB, patch)
2019-02-15 07:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22140: Use of EasyAnalyticalRecords pref is get_hostitemnumbers_of (1.43 KB, patch)
2019-02-15 07:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 22140: Larger unit test for get_hostitemnumbers_of (2.78 KB, patch)
2019-02-15 07:10 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 2019-01-16 12:31:07 UTC
Like Bug 20702 defined GetHostItemsInfo does nothing if EasyAnalyticalRecords pref is disabled, there are other places where code must be dependant on this pref.
Comment 1 Fridolin SOMERS 2019-01-16 12:39:30 UTC
Created attachment 84060 [details] [review]
Bug 22140: More use of EasyAnalyticalRecords pref

Like Bug 20702 defined GetHostItemsInfo does nothing if EasyAnalyticalRecords pref is disabled, there are other places where code must be dependant on this pref.

Test plan :
1) Build an analitical record with 773$0 and $9
2) Enable EasyAnalyticalRecords
3) Don't apply patch
4) Go to OPAC
5) Perform a search that displays the record, check there is the linked item
6) Open record detail page, check there is the linked item
7) Apply patch and redo 5) and 6)
8) Disable EasyAnalyticalRecords
9) redo 5) and 6), you should not see the linked item
Comment 2 helene 2019-01-17 22:04:23 UTC
We restarted memcached and plack and the link still did not disappear.
Comment 3 Fridolin SOMERS 2019-01-22 07:27:33 UTC
(In reply to helene from comment #2)
> We restarted memcached and plack and the link still did not disappear.

Sorry could you describe more please ? Where does the test plan fail ?
Comment 4 Martin Renvoize 2019-01-23 11:47:40 UTC
If memory serves there's some fun to be had regarding the EasyAnalytics and the UseControlNumber preferences around this code.. you may (or may not) want to investiage that preference a little further too before making this change.

I do however like the general idea here.. tidying up a little.
Comment 5 Martin Renvoize 2019-01-23 11:48:54 UTC
Also.. looking at the original diff (https://github.com/Koha-Community/Koha/commit/f09e2ca27ee89860dbdd0ca512735f37fbfaf8a6) of the EasyAnalytics submission, it looks like this could go futher.
Comment 6 Fridolin SOMERS 2019-02-06 13:35:25 UTC
I raise to major importance because its impact on display is very strange and difficult to debug.

@Martin you mean your OK with the patch ?
Comment 7 Fridolin SOMERS 2019-02-06 14:30:37 UTC
Created attachment 84820 [details] [review]
Bug 22140: Use of EasyAnalyticalRecords pref in search

Like Bug 20702 defined GetHostItemsInfo does nothing if EasyAnalyticalRecords pref is disabled, there are other places where code must be dependant on this pref.

Test plan :
1) Build an analitical record with 773$0 and $9
2) Enable EasyAnalyticalRecords
3) Don't apply patch
4) Go to OPAC
5) Perform a search that displays the record, check there is the linked item
6) Open record detail page, check there is the linked item
7) Apply patch and redo 5) and 6)
8) Disable EasyAnalyticalRecords
9) redo 5) and 6), you should not see the linked item
Comment 8 Fridolin SOMERS 2019-02-06 14:31:05 UTC
Created attachment 84822 [details] [review]
Bug 22140: Use of EasyAnalyticalRecords pref is get_hostitemnumbers_of

Like Bug 20702 defined GetHostItemsInfo does nothing if EasyAnalyticalRecords pref is disabled, get_hostitemnumbers_of must be dependant on this pref too.

Test plan :
1) Build an analytical record with 773$0 and $9
2) Enable EasyAnalyticalRecords
4) Place an hold on the record
5) => You see the linked item
6) Disable EasyAnalyticalRecords
7) Place an hold on the record
8) => You dont see the linked item
Comment 9 Fridolin SOMERS 2019-02-06 14:48:19 UTC
Found a new method missing the syspref check.

I try to create a UT on get_hostitemnumbers_of but its hard since it uses db biblio and items.
Comment 10 Fridolin SOMERS 2019-02-06 15:11:26 UTC
Created attachment 84828 [details] [review]
Bug 22140: Larger unit test for get_hostitemnumbers_of

Add syspref EasyAnalyticalRecords impact in unit test for get_hostitemnumbers_of

Test plan :
Run t/db_dependent/Items.t
Comment 11 Fridolin SOMERS 2019-02-06 15:13:12 UTC
Piouf I did it in the end in Items.t in addition to existing test.
Comment 12 Martin Renvoize 2019-02-11 10:07:23 UTC
Created attachment 84958 [details] [review]
Bug 22140: Use of EasyAnalyticalRecords pref in search

Like Bug 20702 defined GetHostItemsInfo does nothing if EasyAnalyticalRecords pref is disabled, there are other places where code must be dependant on this pref.

Test plan :
1) Build an analitical record with 773$0 and $9
2) Enable EasyAnalyticalRecords
3) Don't apply patch
4) Go to OPAC
5) Perform a search that displays the record, check there is the linked item
6) Open record detail page, check there is the linked item
7) Apply patch and redo 5) and 6)
8) Disable EasyAnalyticalRecords
9) redo 5) and 6), you should not see the linked item

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-02-11 10:07:27 UTC
Created attachment 84959 [details] [review]
Bug 22140: Use of EasyAnalyticalRecords pref is get_hostitemnumbers_of

Like Bug 20702 defined GetHostItemsInfo does nothing if EasyAnalyticalRecords pref is disabled, get_hostitemnumbers_of must be dependant on this pref too.

Test plan :
1) Build an analytical record with 773$0 and $9
2) Enable EasyAnalyticalRecords
4) Place an hold on the record
5) => You see the linked item
6) Disable EasyAnalyticalRecords
7) Place an hold on the record
8) => You dont see the linked item

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2019-02-11 10:07:31 UTC
Created attachment 84960 [details] [review]
Bug 22140: Larger unit test for get_hostitemnumbers_of

Add syspref EasyAnalyticalRecords impact in unit test for get_hostitemnumbers_of

Test plan :
Run t/db_dependent/Items.t

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2019-02-11 10:08:47 UTC
Sorry it took me a while to test this after you mentioned it, I wanted to give it my due attention.

Looks good and works as expected for me with no regressions that I can see.  Signing Off.
Comment 16 Katrin Fischer 2019-02-15 07:10:23 UTC
Created attachment 85143 [details] [review]
Bug 22140: Use of EasyAnalyticalRecords pref in search

Like Bug 20702 defined GetHostItemsInfo does nothing if EasyAnalyticalRecords pref is disabled, there are other places where code must be dependant on this pref.

Test plan :
1) Build an analitical record with 773$0 and $9
2) Enable EasyAnalyticalRecords
3) Don't apply patch
4) Go to OPAC
5) Perform a search that displays the record, check there is the linked item
6) Open record detail page, check there is the linked item
7) Apply patch and redo 5) and 6)
8) Disable EasyAnalyticalRecords
9) redo 5) and 6), you should not see the linked item

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 17 Katrin Fischer 2019-02-15 07:10:27 UTC
Created attachment 85144 [details] [review]
Bug 22140: Use of EasyAnalyticalRecords pref is get_hostitemnumbers_of

Like Bug 20702 defined GetHostItemsInfo does nothing if EasyAnalyticalRecords pref is disabled, get_hostitemnumbers_of must be dependant on this pref too.

Test plan :
1) Build an analytical record with 773$0 and $9
2) Enable EasyAnalyticalRecords
4) Place an hold on the record
5) => You see the linked item
6) Disable EasyAnalyticalRecords
7) Place an hold on the record
8) => You dont see the linked item

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Katrin Fischer 2019-02-15 07:10:31 UTC
Created attachment 85145 [details] [review]
Bug 22140: Larger unit test for get_hostitemnumbers_of

Add syspref EasyAnalyticalRecords impact in unit test for get_hostitemnumbers_of

Test plan :
Run t/db_dependent/Items.t

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 19 Nick Clemens 2019-02-15 18:50:26 UTC
Awesome work all!

Pushed to master for 19.05
Comment 20 Martin Renvoize 2019-02-25 13:25:40 UTC
Pushed to 18.11.x for 18.11.04
Comment 21 Martin Renvoize 2019-02-25 15:12:11 UTC
This was a "fun" backport.. had to backport the build_sample_* tree for t/lib/TestBuilder too... shout if you need a had with that subsequent rmaints.. hopefully I've taken the edge off it if you do decide to backport.
Comment 22 Lucas Gass 2019-03-04 22:33:04 UTC
cant cleanly apply this to 18.05.x. If needed please rebase.