Bug 22619 - Adding a new circ rule with unlimited checkouts is broken
Summary: Adding a new circ rule with unlimited checkouts is broken
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Nick Clemens
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 18925
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-02 00:33 UTC by Nick Clemens
Modified: 2019-04-27 20:25 UTC (History)
5 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:
19.05.00


Attachments
Bug 22619: Fix null empty behaviour for new rules (3.75 KB, patch)
2019-04-02 00:38 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22619: Fix null empty behaviour for new rules (3.79 KB, patch)
2019-04-02 19:40 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22619: Fix null empty behaviour for new rules (3.85 KB, patch)
2019-04-03 15:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22619: Fix null empty behaviour for new rules (3.91 KB, patch)
2019-04-03 15:09 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22619: Fix null empty behaviour for new rules (3.99 KB, patch)
2019-04-03 15:16 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 Nick Clemens 2019-04-02 00:33:58 UTC
On bug 18925 we missed a case of fixing null/empty
Comment 1 Nick Clemens 2019-04-02 00:38:00 UTC
Created attachment 87305 [details] [review]
Bug 22619: Fix null empty behaviour for new rules

To test:
 1 - Go to Admin->Circulation and fines rules
 2 - Set All/All rules ot have a limit for Current checkouts allowed (and onsite checkouts allowed)
 3 - Select an itemtype to add a new rule
 4 - Save without entering any values
 5 - Note that new rule displays with the same numebr as the all/all rule
 6 - Check the DB, not no lines were added to circulation_rules table
 7 - Delete the rule from the staff client
 8 - Apply patch
 9 - Repeat, note the new rule is unlimited
10 - Run selenium tests
11 - They pass!
12 - Check no rules/itemtypes are added to your system by the tests
Comment 2 Nick Clemens 2019-04-02 00:39:21 UTC
Selenium tests may be messy, I have not written them in some time, and the circ rules table has few identifying classes etc.

We don't execute in a transaction, but I didn't know if this is intentional?
Comment 3 Liz Rea 2019-04-02 19:40:08 UTC
Created attachment 87331 [details] [review]
Bug 22619: Fix null empty behaviour for new rules

To test:
 1 - Go to Admin->Circulation and fines rules
 2 - Set All/All rules ot have a limit for Current checkouts allowed (and onsite checkouts allowed)
 3 - Select an itemtype to add a new rule
 4 - Save without entering any values
 5 - Note that new rule displays with the same numebr as the all/all rule
 6 - Check the DB, not no lines were added to circulation_rules table
 7 - Delete the rule from the staff client
 8 - Apply patch
 9 - Repeat, note the new rule is unlimited
10 - Run selenium tests
11 - They pass!
12 - Check no rules/itemtypes are added to your system by the tests

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 4 Martin Renvoize 2019-04-03 15:07:57 UTC
Created attachment 87364 [details] [review]
Bug 22619: Fix null empty behaviour for new rules

To test:
 1 - Go to Admin->Circulation and fines rules
 2 - Set All/All rules ot have a limit for Current checkouts allowed (and onsite checkouts allowed)
 3 - Select an itemtype to add a new rule
 4 - Save without entering any values
 5 - Note that new rule displays with the same numebr as the all/all rule
 6 - Check the DB, not no lines were added to circulation_rules table
 7 - Delete the rule from the staff client
 8 - Apply patch
 9 - Repeat, note the new rule is unlimited
10 - Run selenium tests
11 - They pass!
12 - Check no rules/itemtypes are added to your system by the tests

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2019-04-03 15:08:19 UTC
Tests all pass for me here.. joint QA effort.

Passing QA
Comment 6 Martin Renvoize 2019-04-03 15:09:09 UTC
Created attachment 87365 [details] [review]
Bug 22619: Fix null empty behaviour for new rules

To test:
 1 - Go to Admin->Circulation and fines rules
 2 - Set All/All rules ot have a limit for Current checkouts allowed (and onsite checkouts allowed)
 3 - Select an itemtype to add a new rule
 4 - Save without entering any values
 5 - Note that new rule displays with the same numebr as the all/all rule
 6 - Check the DB, not no lines were added to circulation_rules table
 7 - Delete the rule from the staff client
 8 - Apply patch
 9 - Repeat, note the new rule is unlimited
10 - Run selenium tests
11 - They pass!
12 - Check no rules/itemtypes are added to your system by the tests

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Signed-off-by: Katrin Fischer <katrin.fischer@bsz-bw.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Katrin Fischer 2019-04-03 15:16:02 UTC
Created attachment 87366 [details] [review]
Bug 22619: Fix null empty behaviour for new rules

To test:
 1 - Go to Admin->Circulation and fines rules
 2 - Set All/All rules ot have a limit for Current checkouts allowed (and onsite checkouts allowed)
 3 - Select an itemtype to add a new rule
 4 - Save without entering any values
 5 - Note that new rule displays with the same numebr as the all/all rule
 6 - Check the DB, not no lines were added to circulation_rules table
 7 - Delete the rule from the staff client
 8 - Apply patch
 9 - Repeat, note the new rule is unlimited
10 - Run selenium tests
11 - They pass!
12 - Check no rules/itemtypes are added to your system by the tests

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Verified that the patch works, but failed to got the Selenium
tests to pass before and after applying the patch.
Leaving that to the next tester.
Comment 8 Nick Clemens 2019-04-05 13:12:20 UTC
Awesome work all!

Pushed to master for 19.05
Comment 9 Martin Renvoize 2019-04-15 10:33:12 UTC
Bug with enhancement 18925 not in 18.11.x series
Comment 10 Jonathan Druart 2019-04-27 20:25:56 UTC
(In reply to Nick Clemens from comment #2)
> We don't execute in a transaction, but I didn't know if this is intentional?

It will not help, they are simulating hits on the interface, so outside the transaction you would create.
So, if the data is created using the interface, you need to delete them in the cleanup sub.
And if the data is created from the test file, inside a transaction, the interface will have "see" it.

For intance:
1. $itemtype is created using the admin form (click /admin/itemtypes.pl?op=add_form&itemtype=)
=> Need to be deleted in the cleanup sub

2. $itype is created using TestBuilder, inside a transaction
=> No need to delete it in the cleanup, but you will not be able to use it. It is useless

3. $itype is created using TestBuilder, without transaction (our case here)
=> You need to delete it, otherwise it will still exist after the test is ran.
*And* you must delete it in the cleanup (called from END), to make sure the data will be deleted even if something goes wrong. A good example how to handle those data to delete is regression.t (push to @cleanup what we want to delete)

Hope it makes sense.