Bug 22658

Summary: aqbasket.authorisedby is missing a foreign key constraint
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: NEW --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, kyle.m.hall, nick, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 22642    
Bug Blocks: 5334    

Description Martin Renvoize 2019-04-08 16:35:28 UTC
The aqbasket table optionally references a number of tables, but the foreign key fields are unconstrained.
Comment 1 Tomás Cohen Arazi 2020-01-20 19:23:54 UTC
I propose we create a new column: creator_id INT(11) NULL DEFAULT NULL, with FK constraint on the borrowers table, and we move/copy there the values of authorisedby that look like a number. If there's no remaining value on authorisedby, then drop the column. Otherwise, just leave it and print a noticeable message about manually removing the column once inconsistencies are solved.

I haven't found references to the column in contexts in which it is not tested against the borrowers.borrowernumber column, but there might be old/legacy data there, or I'm sure Jonathan will find that spot I missed.
Comment 2 Jonathan Druart 2020-01-21 10:25:43 UTC
I would not care that much about authorisedby, it must contain an integer. However it can contain borrowernumber that do not longer exist in borrowers.
Comment 3 Tomás Cohen Arazi 2020-01-21 12:21:24 UTC
(In reply to Jonathan Druart from comment #2)
> I would not care that much about authorisedby, it must contain an integer.
> However it can contain borrowernumber that do not longer exist in borrowers.

Do you say we should set to NULL if it doesn't exist in borrowers?
Comment 4 Jonathan Druart 2020-01-21 12:27:18 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> (In reply to Jonathan Druart from comment #2)
> > I would not care that much about authorisedby, it must contain an integer.
> > However it can contain borrowernumber that do not longer exist in borrowers.
> 
> Do you say we should set to NULL if it doesn't exist in borrowers?

Yes
Comment 5 Katrin Fischer 2020-01-21 12:32:59 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Tomás Cohen Arazi from comment #3)
> > (In reply to Jonathan Druart from comment #2)
> > > I would not care that much about authorisedby, it must contain an integer.
> > > However it can contain borrowernumber that do not longer exist in borrowers.
> > 
> > Do you say we should set to NULL if it doesn't exist in borrowers?
> 
> Yes

This might be a tricky one. I think we control visibility of baskets by their creator (AcqViewBaskets, independentbranches, permissions etc.). We don't want baskets to appear suddenly for libraries/staff who should not be able to see then. Not sure what the best fix here is, just saying it could lead to unwanted consequences.
Comment 6 Martin Renvoize 2020-01-21 17:09:37 UTC
Looking forward to seeing this one.. did you want me to have a bash at it Tomas or were you intended to adopt it?
Comment 7 Tomás Cohen Arazi 2020-01-21 17:19:53 UTC
(In reply to Martin Renvoize from comment #6)
> Looking forward to seeing this one.. did you want me to have a bash at it
> Tomas or were you intended to adopt it?

I prefer to finish my works first. If it is not done yet, I'll pick it.
I intended to use it for bug 24464, but can adapt it once the relationship is done.