Bug 29341

Summary: If OpacRenewalBranch = opacrenew, pseudonymization process leads to "internal server error" when patrons renew the loans at OPAC
Product: Koha Reporter: Koha Team University Lyon 3 <koha>
Component: PatronsAssignee: Jonathan Druart <jonathan.druart+koha>
Status: RESOLVED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: critical    
Priority: P5 - low CC: david, fridolin.somers, gmcharlt, jonathan.druart+koha, joonas.kylmala, koha, kyle.m.hall, kyle, m.de.rooy, nick, pierre.genty, sonia.bouis, victor
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.05,20.11.12
Bug Depends on: 24151    
Bug Blocks:    
Attachments: Bug 29341: Prevent OPAC renew to crash if Pseudonymization is set
Bug 29341: Prevent OPAC renew to crash if Pseudonymization is set
Bug 29341: Don't modify the branchcode for stats
Bug 29341: Prevent OPAC renew to crash if Pseudonymization is set
Bug 29341: Remove foreign keys on pseudonymized_transactions
Bug 29341: Remove foreign keys on pseudonymized_transactions

Description Koha Team University Lyon 3 2021-10-28 09:12:19 UTC
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.
Comment 1 Jonathan Druart 2021-10-28 09:31:47 UTC Comment hidden (obsolete)
Comment 2 David Nind 2021-10-28 19:29:52 UTC
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>
Comment 3 David Nind 2021-10-28 19:38:42 UTC
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.
Comment 4 David Nind 2021-10-28 19:43:39 UTC
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
Comment 5 Joonas Kylmälä 2021-10-30 17:30:28 UTC
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.
Comment 6 Jonathan Druart 2021-11-02 12:02:29 UTC
Created attachment 127213 [details] [review]
Bug 29341: Don't modify the branchcode for stats
Comment 7 Jonathan Druart 2021-11-02 12:02:41 UTC
(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?
Comment 8 Marcel de Rooy 2021-11-05 09:38:34 UTC
QA: Looking here
Comment 9 Marcel de Rooy 2021-11-05 10:08:46 UTC
Maybe those people using OPACRenewalBranch, should add that branchcode too in their system ;)
Comment 10 Marcel de Rooy 2021-11-05 10:17:06 UTC
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.
Comment 11 Marcel de Rooy 2021-11-05 10:18:05 UTC
There is still time for another QA stamp. But if there would not be, I am fine with passing QA too.
Comment 12 Koha Team University Lyon 3 2021-11-08 14:49:17 UTC
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
Comment 13 Jonathan Druart 2021-11-09 08:05:02 UTC
(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.
Comment 14 Joonas Kylmälä 2021-11-13 14:32:14 UTC
(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.
Comment 15 Katrin Fischer 2021-11-13 21:11:55 UTC
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 :) )
Comment 16 Marcel de Rooy 2021-11-14 13:43:38 UTC
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.
Comment 17 Jonathan Druart 2021-11-15 08:54:50 UTC
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.
Comment 18 Jonathan Druart 2021-11-15 08:56:10 UTC
(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.
Comment 19 Marcel de Rooy 2021-11-15 09:30:10 UTC
(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.
Comment 20 Sonia Bouis 2021-11-15 16:26:13 UTC
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
Comment 21 Nick Clemens 2021-11-16 12:41:02 UTC
(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
Comment 22 Jonathan Druart 2021-11-16 12:58:02 UTC
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.
Comment 23 Jonathan Druart 2021-11-16 15:48:16 UTC
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.
Comment 24 Jonathan Druart 2021-11-17 11:27:15 UTC
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.
Comment 25 Katrin Fischer 2021-11-17 11:44:45 UTC
(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.
Comment 26 Marcel de Rooy 2021-11-19 09:07:35 UTC
(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
Comment 27 Marcel de Rooy 2021-11-19 09:08:09 UTC
  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?
Comment 28 Marcel de Rooy 2021-11-19 09:08:56 UTC
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>
Comment 29 Marcel de Rooy 2021-11-19 09:10:33 UTC
OK We took the other turn :)
Apart from comment27 no further questions. Trivial enough to combine SO and QA at this late time.
Comment 30 Jonathan Druart 2021-11-19 14:15:37 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 31 Kyle M Hall 2021-11-19 16:49:54 UTC
Pushed to 21.05.x for 21.05.05
Comment 32 Fridolin Somers 2021-11-20 00:25:57 UTC
(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
Comment 33 Fridolin Somers 2021-11-20 00:37:02 UTC
/!\ 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"
Comment 34 Fridolin Somers 2021-11-20 02:08:28 UTC
Pushed to 20.11.x for 20.11.12
Comment 35 Victor Grousset/tuxayo 2021-11-22 01:45:53 UTC
Missing dependencies for 20.05.x, it shouldn't be affected, no backport.