When you activate the pseudonymization process and the system preference OpacRenewalBranch = opacrenew, there's an "internal server error" when patrons renew there loans at the OPAC. In logs, you can find : DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`pseudonymized_trans actions`, CONSTRAINT `pseudonymized_transactions_borrowers_ibfk_3` FOREIGN KEY (`transaction_branchcode`) REFERENCES `branches` (`branchcode`)) [for Stat ement "INSERT INTO `pseudonymized_transactions` ( `branchcode`, `categorycode`, `ccode`, `dateenrolled`, `datetime`, `has_cardnumber`, `hashed_borrowernu mber`, `holdingbranch`, `homebranch`, `itemcallnumber`, `itemnumber`, `itemtype`, `location`, `sort1`, `sort2`, `transaction_branchcode`, `transaction_ty pe`) VALUES ( ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='MAN', 1='ETUL3', 2=undef, 3='2021-01-11', 4='2021-10-27 06:28:28' , 5=1, 6="$xxxxxxxxxxxxxxxxxxxxxxxxx", 7='MAN', 8='MAN', 9='345 PIN', 10="728028", 11='PRET', 12='MANDG3', 13='ELdro', 14='3101000IL', 15='OPACRenew', 16='renew'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836.
Created attachment 127034 [details] [review] Bug 29341: Prevent OPAC renew to crash if Pseudonymization is set If OpacRenewalBranch = opacrenew then we try to insert a wrong branchcode into pseudonymized_transactions.transaction_branchcode We are trying to insert 'OPACRenew' Test plan: Setup Pseudonymization Set OpacRenewalBranch = opacrenew Renew an item from the OPAC Notice that pseudonymized_transactions.transaction_branchcode is set to NULL
Created attachment 127078 [details] [review] Bug 29341: Prevent OPAC renew to crash if Pseudonymization is set If OpacRenewalBranch = opacrenew then we try to insert a wrong branchcode into pseudonymized_transactions.transaction_branchcode We are trying to insert 'OPACRenew' Test plan: Setup Pseudonymization Set OpacRenewalBranch = opacrenew Renew an item from the OPAC Notice that pseudonymized_transactions.transaction_branchcode is set to NULL Signed-off-by: David Nind <david@davidnind.com>
Testing notes (koha-testing-docker): - To set up Pseudonymization: . generate a bcrypt_settings code from the command line: htpasswd -bnBC 10 "" password | tr -d ':\n' | sed 's/$2y/$2a/' . add entry to <bcrypt_settings>generated code</bcrypt_settings> in koha-conf.xml somewhere within the <config></config> (this setting was not in the koha-conf.xml file and I added before the closing </config>) . example: <bcrypt_settings>$2a$10$Jpln8Gg9Towzux6D4iZVaucXtxum.hf.lnbHIw1RXxZSzy9.mLjo6</bcrypt_settings> . flush_memcached and restart_all . You can then enable the Pseudonymization system preference and set values for PseudonymizationPatronFields and PseudonymizationTransactionFields (I selected all) . I initially had trouble generating the error, but was probably related to refreshing the browser/cached data.
More testing notes (koha-tetsing-docker): - Database query: . access the database: koha-mysql kohadev . select * from pseudonymized_transactions; - View the log where the error appears: vi /var/log/koha/kohadev/plack-opac-error.log - Run tests before and after - they should pass: . prove t/db_dependent/Koha/Pseudonymization.t
This messes up the statistic table, people are expecting there to be 'OPACRenew' as the value and not NULL. If you don't want to make this a huge patch I would suggest to just drop the foreign key for the 'transaction_branchcode' in pseudonymized_transactions table. I also assume people don't want there to be foreign key relation in order to be able to delete branches and still keep the stats in place. However, if you want to go the original patch's route, which I think not, then the change should be done in Koha::Item::renewal_branchcode instead. The syspref documentation and database upgrade would need to be provided as well.
Created attachment 127213 [details] [review] Bug 29341: Don't modify the branchcode for stats
(In reply to Joonas Kylmälä from comment #5) > This messes up the statistic table, people are expecting there to be > 'OPACRenew' as the value and not NULL. If you don't want to make this a huge > patch I would suggest to just drop the foreign key for the > 'transaction_branchcode' in pseudonymized_transactions table. I also assume > people don't want there to be foreign key relation in order to be able to > delete branches and still keep the stats in place. > > However, if you want to go the original patch's route, which I think not, > then the change should be done in Koha::Item::renewal_branchcode instead. > The syspref documentation and database upgrade would need to be provided as > well. Oops, my bad! Thanks! I think we want to keep the FK, this patch (can be squashed) is modifying the branchcode AFTER the stat entry has been created. Sonia, Lyon 3, can you confirm the expected behaviour please?
QA: Looking here
Maybe those people using OPACRenewalBranch, should add that branchcode too in their system ;)
Created attachment 127350 [details] [review] Bug 29341: Prevent OPAC renew to crash if Pseudonymization is set If OpacRenewalBranch = opacrenew then we try to insert a wrong branchcode into pseudonymized_transactions.transaction_branchcode We are trying to insert 'OPACRenew' Test plan: Setup Pseudonymization Set OpacRenewalBranch = opacrenew Renew an item from the OPAC Notice that pseudonymized_transactions.transaction_branchcode is set to NULL Signed-off-by: David Nind <david@davidnind.com> [SQUASHED] Bug 29341: Don't modify the branchcode for stats Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Amended test for 'as is' and a reference to OPACRenew in test script.
There is still time for another QA stamp. But if there would not be, I am fine with passing QA too.
Hi, I haven't tested it yet because I'm still fighting with my kohadevbox. But, in theory, It's a pity to store NULL value in pseudonymized_transactions instead of a value that gives us the information the it's a renew done on the OPAC. It's not possible to store the same value that the one in the statistics table? Sonia
(In reply to Koha Team University Lyon 3 from comment #12) > Hi, > I haven't tested it yet because I'm still fighting with my kohadevbox. > > But, in theory, It's a pity to store NULL value in > pseudonymized_transactions instead of a value that gives us the information > the it's a renew done on the OPAC. > It's not possible to store the same value that the one in the statistics > table? > > Sonia I wasn't aware of this behaviour, but it does not sound correct. "Branch" must contain the branchcode, and be a foreign key. If we want to keep this additional information it should be stored into a separate column. Note that Marcel's workaround should work, if you create a branch with branchcode="OPACRenew" that should be inserted instead of NULL.
(In reply to Jonathan Druart from comment #13) > I wasn't aware of this behaviour, but it does not sound correct. "Branch" > must contain the branchcode, and be a foreign key. If we want to keep this > additional information it should be stored into a separate column. The change should be then done for the statistics table as well, in this same bug report, since there is only one syspref, OpacRenewalBranch, for both the tables (statistics and pseudonymized_transactions). In addition to that the OpacRenewalBranch syspref needs to be updated to described that if opacrenewal is used then it is stored into another column. Another thing we might wanna fix here is that if the user actually selects "NULL" as the OpacRenewalBranch syspref value then we store empty string and not null really (I wonder if this NULL/empty string is actually a regression?). > Note that Marcel's workaround should work, if you create a branch with > branchcode="OPACRenew" that should be inserted instead of NULL. Not suitable for production.
I agree that knowing if a renewal was done from the OPAC is important and we need to maintain that information in some way. We could argue that in statistics the column doesn't need to be an FK, as you might want to keep statistics for deleted branches still. Same for itemtypes, categories etc. I think we could treat statistics like a log that is not altered by changes happening around it where we can avoid it. Adding a branch for that purpose is no solution (as Joonas already said :) )
I am getting the feeling here that this fix is blamed for the weird OpacRenewalBranch stuff already in Koha. Obviously, this should have been developed differently.
I disagree with removing the FK. Statistics should not keep a wrong/obsolete code, they are kept (ON DELETE SET NULL) however. There are different things, and different fixes: Statistics table is missing the FK (or does not have it on purpose), and the OpacRenewalBranch behaviour is badly designed. I don't think removing the FK is going into the correct direction, adding a separate column is definitely out of the scope here. We need a quick bugfix for the ugly 500 (that is blocker), and this patch is the way to go in my opinion.
(In reply to Katrin Fischer from comment #15) > Adding a branch for that purpose is no solution (as Joonas already said :) ) I called it a "workaround", it's the only way to remove the 500 in stable branches right now.
(In reply to Jonathan Druart from comment #18) > (In reply to Katrin Fischer from comment #15) > > Adding a branch for that purpose is no solution (as Joonas already said :) ) > > I called it a "workaround", it's the only way to remove the 500 in stable > branches right now. I would call it a legitimate workaround given the lack of quality of this so-called feature.
Hello, I'm OK that this is not statisfying but unless someone has a better way to fix it rapidly, it's perhaps better to fix it than to keep an internal servor error ? I'm not sur to really understand the technical part with the foreign key. But, as Katrin said, in a library, the branches can change (closure, opening of a new building) and we may want to delete the branch but we need to keep the statistics and the pseudonymized information. It would be nice if this BZ doesn't stay too long in discussion. Sonia
(In reply to Jonathan Druart from comment #17) > I disagree with removing the FK. Statistics should not keep a wrong/obsolete > code, they are kept (ON DELETE SET NULL) however. > > There are different things, and different fixes: Statistics table is missing > the FK (or does not have it on purpose), and the OpacRenewalBranch behaviour > is badly designed. > > I don't think removing the FK is going into the correct direction, adding a > separate column is definitely out of the scope here. We need a quick bugfix > for the ugly 500 (that is blocker), and this patch is the way to go in my > opinion. I disagree here, I think we should remove the FK to branches as the quick fix. While we may have opinions about how that feature was implemented, it has been implemented. Statistics is a ledger of transactions - foreign keys are going to remove/nullify data after the fact and I think that would be incorrect. If Pseudonymized transactions are meant to mirror the statistics table then I feel we should remove the FK for now, and fix the larger issue with OPACRenew etc on a new bug. Adding a new branch is going to add the branch in many places throughout Koha and will impact workflows
If you delete the library then recreate another one with the same branchcode, do we want the stats to be linked to the new library? I don't think so.
Sonia confirmed me that it's ok to keep the branchcode and so remove the FK. I personally still don't think it's a good idea, but I understand the use case. This is also a bad use case: Create branch CPL, for testing purpose (hey you just installed the software and want to try it) Do some stuffs, it will create stats entries Remove CPL to start from scratch and clean state Reuse CPL as branchcode Stats are accumulating using the test data I will provide an alternate patch tomorrow if nobody beats me to it.
Created attachment 127727 [details] [review] Bug 29341: Remove foreign keys on pseudonymized_transactions Behave like the statistics table and don't remove the code even if the branch or patron's category is removed.
(In reply to Jonathan Druart from comment #23) > Sonia confirmed me that it's ok to keep the branchcode and so remove the FK. > > I personally still don't think it's a good idea, but I understand the use > case. > > This is also a bad use case: > Create branch CPL, for testing purpose (hey you just installed the software > and want to try it) > Do some stuffs, it will create stats entries > Remove CPL to start from scratch and clean state > Reuse CPL as branchcode > Stats are accumulating using the test data > > I will provide an alternate patch tomorrow if nobody beats me to it. I feel like most people might have a test system or would be able to differentiate by timestamp in that case. My feeling is, that this would not be a big issue.
(In reply to Nick Clemens from comment #21) > I disagree here, I think we should remove the FK to branches as the quick > fix. > > While we may have opinions about how that feature was implemented, it has > been implemented. Statistics is a ledger of transactions - foreign keys are > going to remove/nullify data after the fact and I think that would be > incorrect. > > If Pseudonymized transactions are meant to mirror the statistics table then > I feel we should remove the FK for now, and fix the larger issue with > OPACRenew etc on a new bug. > > Adding a new branch is going to add the branch in many places throughout > Koha and will impact workflows Fair enough
KEY `pseudonymized_transactions_ibfk_1` (`categorycode`), KEY `pseudonymized_transactions_borrowers_ibfk_2` (`branchcode`), KEY `pseudonymized_transactions_borrowers_ibfk_3` (`transaction_branchcode`) When removing the FKs, the question is: Do we still need those indexes?
Created attachment 127848 [details] [review] Bug 29341: Remove foreign keys on pseudonymized_transactions Behave like the statistics table and don't remove the code even if the branch or patron's category is removed. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
OK We took the other turn :) Apart from comment27 no further questions. Trivial enough to combine SO and QA at this late time.
Pushed to master for 21.11, thanks to everybody involved!
Pushed to 21.05.x for 21.05.05
(In reply to Marcel de Rooy from comment #27) > KEY `pseudonymized_transactions_ibfk_1` (`categorycode`), > KEY `pseudonymized_transactions_borrowers_ibfk_2` (`branchcode`), > KEY `pseudonymized_transactions_borrowers_ibfk_3` > (`transaction_branchcode`) > > When removing the FKs, the question is: Do we still need those indexes? I bet yes, for SQL join performance
/!\ Arg, Koha.pm change is missing in 21.05.x : It is still in "21.05.05.003" when updatedatabase is "21.05.05.005"
Pushed to 20.11.x for 20.11.12
Missing dependencies for 20.05.x, it shouldn't be affected, no backport.