Bug 14849 - Owner should be FK in uploaded_files
Summary: Owner should be FK in uploaded_files
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 5334
  Show dependency treegraph
 
Reported: 2015-09-18 12:17 UTC by Marcel de Rooy
Modified: 2016-03-18 13:05 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14849: Add foreign key on uploaded_files.owner (2.05 KB, patch)
2016-01-13 13:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14849: Add foreign key on uploaded_files.owner (2.10 KB, patch)
2016-01-17 07:07 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 14849: Add foreign key on uploaded_files.owner (2.26 KB, patch)
2016-01-22 07:36 UTC, Marcel de Rooy
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 2015-09-18 12:17:10 UTC
QA comment from 6874/14321
Will get attention soon.
Comment 1 Jonathan Druart 2016-01-13 13:52:46 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2016-01-17 07:07:12 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2016-01-22 07:36:38 UTC
Created attachment 47157 [details] [review]
Bug 14849: Add foreign key on uploaded_files.owner

uploaded_files.owner refers to a borrowernumber, the DB structure should
reflect this constraint.

Test plan:
0/ Do not execute the update DB entry
1/ Add a new patron
2/ Use this patron to upload a new file
3/ In the uploaded_files you should see that the owner value is the
borrowernumber of the newly created patron
4/ Execute the DB entry
5/ The uploaded_files.owner value should have been set to NULL
6/ Repeat 1, 2 and 3
7/ Delete the patron
5/ The uploaded_files.owner value should have been set to NULL

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
The test plan seems to miss one step: 3b Delete the patron.
Comment 4 Jonathan Druart 2016-02-02 17:20:51 UTC
Need to think about it (merge deletedborrowers with borrowers table).
Comment 5 Marcel de Rooy 2016-03-17 13:32:04 UTC
(In reply to Jonathan Druart from comment #4)
> Need to think about it (merge deletedborrowers with borrowers table).

You have been thinking for some time now :)
Separate from what we will do with deletedborrowers, I think this could be pushed in the meantime.
Can we switch the status back ?
Comment 6 Jonathan Druart 2016-03-18 10:16:59 UTC
(In reply to Marcel de Rooy from comment #5)
> (In reply to Jonathan Druart from comment #4)
> > Need to think about it (merge deletedborrowers with borrowers table).
> 
> You have been thinking for some time now :)
> Separate from what we will do with deletedborrowers, I think this could be
> pushed in the meantime.
> Can we switch the status back ?

Not really: we have not discussed this topic during the last 2 dev meetings.
The choice we will make can have an impact:
If we decide not to care about the "history" (who did what [when]), we can move forward with this patch.
Otherwise (deleted and active patrons in the same table, with an active/deleted flag) we can keep this information. In this case, if this patch is pushed, we will loose the info we want to keep (uploaded_files.owner will have been set to NULL, no way to retrieve it).
Comment 7 Marcel de Rooy 2016-03-18 10:34:36 UTC
(In reply to Jonathan Druart from comment #6)

> If we decide not to care about the "history" (who did what [when]), we can
> move forward with this patch.
> Otherwise (deleted and active patrons in the same table, with an
> active/deleted flag) we can keep this information. In this case, if this
> patch is pushed, we will loose the info we want to keep
> (uploaded_files.owner will have been set to NULL, no way to retrieve it).

Not sure if I understand what you mean.
If you have all patrons in the same table, this means that you do not delete the patron but just set a flag. In that case the FK constraint does nothing. So, why do you say that it is set to NULL?
Comment 8 Jonathan Druart 2016-03-18 12:21:14 UTC
This patch will set it to NULL
Comment 9 Marcel de Rooy 2016-03-18 13:05:33 UTC
(In reply to Jonathan Druart from comment #8)
> This patch will set it to NULL

See what you mean now. The period between pushing both patch sets.
Note that we will probably already loose information in some tables now.