Bug 28565

Summary: Adding a new syspref for sharing through HEA should be simpler
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: david, jonathan.druart, katrin.fischer, kyle, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28563
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This enhancement simplifies the way new system preferences are added to Hea for statistical reporting. Before this enhancement the tests (t/db_dependent/UsageStats.t) required adjusting every time a new system preference was added. Now when a new system preference is added to Hea they are automatically picked up for the tests.
Version(s) released in:
21.11.00
Attachments: Bug 28565: Move the sysprefs to a reusable sub
Bug 28565: Simplify tests
Bug 28565: Move the sysprefs to a reusable sub
Bug 28565: Simplify tests
Bug 28565: Refactor tests
Bug 28565: Move the sysprefs to a reusable sub
Bug 28565: Refactor tests
Bug 28565: Move the sysprefs to a reusable sub
Bug 28565: Refactor tests

Description Tomás Cohen Arazi 2021-06-14 20:43:08 UTC
Right now, we need to add the syspref both in C4::UsageStats and in the UsageStats.t file.

The tests should use the list from the lib.

To test:
1. Add some syspref to the C4::UsageStats::BuildReport loop
2. Run:
   $ kshell
  k$ prove t/db_dependent/UsageStats.t
=> FAIL: The new syspref is not in the tests, things break
Comment 1 Tomás Cohen Arazi 2021-06-14 20:48:14 UTC
Created attachment 121938 [details] [review]
Bug 28565: Move the sysprefs to a reusable sub

This patch moves the big sysprefs list to a sub. Behavior shouldn't
change.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/UsageStats.t
=> SUCCESS: Things work as expected, no changes.
2. Sign off :-D
Comment 2 Tomás Cohen Arazi 2021-06-14 20:48:18 UTC
Created attachment 121939 [details] [review]
Bug 28565: Simplify tests

This patch makes the tests for UsageStats. This way the tests don't need
to be adjusted everytime a syspref is added.

To test:
1. Apply the first patch
2. Add some random syspref to C4::UsageStats
3. Run:
   $ kshell
  k$ prove t/db_dependent/UsageStats.t
=> FAIL: Things explode
4. Apply this patch
5. Repeat 3
=> SUCCESS: It doesn't explode anymore!
6. Sign off :-D
Comment 3 David Nind 2021-06-15 11:11:44 UTC
Created attachment 121986 [details] [review]
Bug 28565: Move the sysprefs to a reusable sub

This patch moves the big sysprefs list to a sub. Behavior shouldn't
change.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/UsageStats.t
=> SUCCESS: Things work as expected, no changes.
2. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2021-06-15 11:11:48 UTC
Created attachment 121987 [details] [review]
Bug 28565: Simplify tests

This patch makes the tests for UsageStats. This way the tests don't need
to be adjusted everytime a syspref is added.

To test:
1. Apply the first patch
2. Add some random syspref to C4::UsageStats
3. Run:
   $ kshell
  k$ prove t/db_dependent/UsageStats.t
=> FAIL: Things explode
4. Apply this patch
5. Repeat 3
=> SUCCESS: It doesn't explode anymore!
6. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Jonathan Druart 2021-06-22 12:43:11 UTC
What's the point of this test? :D
Comment 6 Tomás Cohen Arazi 2021-06-22 12:49:49 UTC
(In reply to Jonathan Druart from comment #5)
> What's the point of this test? :D

I was wondering :-D I guess we want the test if we are not leaking things (bug 23849)
Comment 7 Jonathan Druart 2021-06-22 12:58:15 UTC
But it's not the test that will catch it, even less if the list is in a single place now.
Comment 8 Tomás Cohen Arazi 2021-06-22 13:00:51 UTC
(In reply to Jonathan Druart from comment #7)
> But it's not the test that will catch it, even less if the list is in a
> single place now.

I'm fine with dropping the test instead of my patches here. Let me know if you want me to do that. My first instinct was to keep what we have, but make it simpler to maintain.
Comment 9 Jonathan Druart 2021-06-22 13:18:48 UTC
I would keep the first patch but remove the tests in a second patch.
Comment 10 Tomás Cohen Arazi 2021-06-22 14:52:29 UTC
Created attachment 122289 [details] [review]
Bug 28565: Refactor tests

Tests were too complex with no gain. They even deleted the whole DB to
run (inside a transaction). I refactored to test the same thing, and
make it rely (mostly) on existing data.

_count behavior is tested implicitly by comparing with Koha::*->count.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/UsageStats.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 11 David Nind 2021-06-23 22:41:17 UTC
Created attachment 122353 [details] [review]
Bug 28565: Move the sysprefs to a reusable sub

This patch moves the big sysprefs list to a sub. Behavior shouldn't
change.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/UsageStats.t
=> SUCCESS: Things work as expected, no changes.
2. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2021-06-23 22:41:21 UTC
Created attachment 122354 [details] [review]
Bug 28565: Refactor tests

Tests were too complex with no gain. They even deleted the whole DB to
run (inside a transaction). I refactored to test the same thing, and
make it rely (mostly) on existing data.

_count behavior is tested implicitly by comparing with Koha::*->count.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/UsageStats.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 13 Katrin Fischer 2021-07-24 23:25:08 UTC
Created attachment 123167 [details] [review]
Bug 28565: Move the sysprefs to a reusable sub

This patch moves the big sysprefs list to a sub. Behavior shouldn't
change.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/UsageStats.t
=> SUCCESS: Things work as expected, no changes.
2. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2021-07-24 23:25:13 UTC
Created attachment 123168 [details] [review]
Bug 28565: Refactor tests

Tests were too complex with no gain. They even deleted the whole DB to
run (inside a transaction). I refactored to test the same thing, and
make it rely (mostly) on existing data.

_count behavior is tested implicitly by comparing with Koha::*->count.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/UsageStats.t
=> SUCCESS: Tests pass
2. Apply this patch
3. Repeat 1
=> SUCCESS: Tests still pass
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Jonathan Druart 2021-07-26 10:48:06 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 16 Kyle M Hall 2021-07-30 16:16:54 UTC
Enhancement not backported for 21.05