Bug 30203 - Prevent data loss when running Circulation.t without prove
Summary: Prevent data loss when running Circulation.t without prove
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-03-01 13:13 UTC by Marcel de Rooy
Modified: 2022-03-27 00:34 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.04,21.05.13


Attachments
Bug 30203: Circulation.t: prevent locking by setting envvar (1.44 KB, patch)
2022-03-03 10:35 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30203: Circulation.t: prevent locking by setting envvar (1.46 KB, patch)
2022-03-03 10:39 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-03-01 13:13:08 UTC
Not sure what happened exactly. But a failing Circulation.t must have removed all users in my devbox.

# Start with a clean slate
$dbh->do('DELETE FROM issues');
$dbh->do('DELETE FROM borrowers');


And my borrowers:
select borrowernumber,userid,lastseen from borrowers;
+----------------+---------------------------------------------------------------------------+---------------------+
| borrowernumber | userid                                                                    | lastseen            |
+----------------+---------------------------------------------------------------------------+---------------------+
|            382 | pCITR3m30mWAv4MTsEhckB0QR1JoJFgBac2mdWx                                   | 2022-03-01 13:01:39 |
|            383 | sazlgFEfa4UmwVgniArAfluzcETJXkru9ktdHKYzSqTXwYckCAXNje3ZpqfRabaz42p       | 2022-03-01 13:01:39 |
|            384 | VkGSPpUO7VFE2it55nZ2pflAmo75HUin4eYVKtThczB7tUF_AZFXuOcg95_Q              | 2022-03-01 13:01:39 |
|            385 | opoi06B6_LWhoOXQtTjYrN01Vm182og8OK6Uaezc_CfNnA6SYicmnWzb2hEeZF_           | 2022-03-01 13:01:39 |
|            386 | john.renewal                                                              | NULL                |
|            387 | katrin.reservation                                                        | NULL                |
|            388 | kyle.reservation                                                          | NULL                |
|            389 | alice.reservation                                                         | NULL                |
|            390 | ca.glisse                                                                 | NULL                |
|            391 | T5MrUI8DftRsa_nou6Q_VO76Wby4HAfcDsNP9WXk3gom8                             | 2022-03-01 13:01:39 |
|            392 | goV_DQYtptbpxYFdUtEcpJqz9X4fq3Zn4Arf336wEvxvc7rF8prBsQRDLq8_8FZhQypuamFi  | 2022-03-01 13:01:39 |
|            393 | fridolyn.somers                                                           | NULL                |
|            394 | kyle.hall                                                                 | NULL                |
|            395 | kyle.hall1                                                                | NULL                |
|            396 | chelsea.hall                                                              | NULL                |
|            397 | fn.dn                                                                     | NULL                |

etc etc

The family Hall are prominently visible now :) But Katrin and Fridolin too..
Comment 1 Marcel de Rooy 2022-03-03 10:33:45 UTC
Hmm. I should have known.

One of the last subtests calls SendCirculationAlert..
And that sub includes one of the worst Koha hacks:

    # LOCK TABLES is not transaction-safe and implicitly commits any active transaction before attempting to lock the tables.
    # If the LOCK/UNLOCK statements are executed from tests, the current transaction will be committed.
    # To avoid that we need to guess if this code is execute from tests or not (yes it is a bit hacky)
    my $do_not_lock = ( exists $ENV{_} && $ENV{_} =~ m|prove| ) || $ENV{KOHA_TESTING};

I will send a patch instead of closing this report btw.
Comment 2 Marcel de Rooy 2022-03-03 10:35:29 UTC
Created attachment 131314 [details] [review]
Bug 30203: Circulation.t: prevent locking by setting envvar

Running this test without prove is disastrous. The LOCK in
the called SendCirculationAlert will ruin your data.
But no longer when you apply this patch.

Test plan:
Run prove Circulation.t
Inspect your data (e.g. borrowers table).
Run perl Circulation.t
Inspect your data (e.g. borrowers table) again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2022-03-03 10:37:16 UTC
This is a trivial edit but really handy to keep you focus on development instead of reinstalling or restoring db.

Very trivial: Self SO
Comment 4 Martin Renvoize 2022-03-03 10:39:33 UTC
Created attachment 131315 [details] [review]
Bug 30203: Circulation.t: prevent locking by setting envvar

Running this test without prove is disastrous. The LOCK in
the called SendCirculationAlert will ruin your data.
But no longer when you apply this patch.

Test plan:
Run prove Circulation.t
Inspect your data (e.g. borrowers table).
Run perl Circulation.t
Inspect your data (e.g. borrowers table) again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2022-03-03 10:40:32 UTC
Fix is clear and test plan works as expected.

PQA
Comment 6 Fridolin Somers 2022-03-04 02:12:46 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 7 Andrew Fuerste-Henry 2022-03-24 17:38:33 UTC
I see this is in 21.11.x for 21.11.4
Pushed to 21.05.x for 21.05.13
Comment 8 Victor Grousset/tuxayo 2022-03-27 00:34:17 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.