Bug 22658 - aqbasket.authorisedby is missing a foreign key constraint
Summary: aqbasket.authorisedby is missing a foreign key constraint
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Martin Renvoize (ashimema)
QA Contact: Testopia
URL:
Keywords:
Depends on: 22642
Blocks: 5334
  Show dependency tree / graph
 
Reported: 2019-04-08 16:35 UTC by Martin Renvoize (ashimema)
Modified: 2020-01-21 17:19 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
Circulation function:

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize (ashimema) 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 (tcohen) 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 (tcohen) 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 (ashimema) 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 (tcohen) 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.