Bug 22619

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

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?

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>

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>

Tests all pass for me here.. joint QA effort.

Passing QA

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>

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.

Awesome work all!

Pushed to master for 19.05

Bug with enhancement 18925 not in 18.11.x series

(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.