Bug 28565 - Adding a new syspref for sharing through HEA should be simpler
Summary: Adding a new syspref for sharing through HEA should be simpler
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-06-14 20:43 UTC by Tomás Cohen Arazi
Modified: 2022-06-06 20:25 UTC (History)
5 users (show)

See Also:
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 (2.92 KB, patch)
2021-06-14 20:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28565: Simplify tests (7.75 KB, patch)
2021-06-14 20:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28565: Move the sysprefs to a reusable sub (2.97 KB, patch)
2021-06-15 11:11 UTC, David Nind
Details | Diff | Splinter Review
Bug 28565: Simplify tests (7.80 KB, patch)
2021-06-15 11:11 UTC, David Nind
Details | Diff | Splinter Review
Bug 28565: Refactor tests (26.90 KB, patch)
2021-06-22 14:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 28565: Move the sysprefs to a reusable sub (3.02 KB, patch)
2021-06-23 22:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 28565: Refactor tests (26.95 KB, patch)
2021-06-23 22:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 28565: Move the sysprefs to a reusable sub (3.08 KB, patch)
2021-07-24 23:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 28565: Refactor tests (27.11 KB, patch)
2021-07-24 23:25 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 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