QA comment from 6874/14321 Will get attention soon.
Created attachment 46589 [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
Created attachment 46759 [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>
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.
Need to think about it (merge deletedborrowers with borrowers table).
(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 ?
(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).
(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?
This patch will set it to NULL
(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.
In discussion from 2016? Did pass QA at the time. Stopped because we were going to merge borrowers and deletedborrowers.. Nice.
(In reply to Marcel de Rooy from comment #10) > In discussion from 2016? Did pass QA at the time. Stopped because we were > going to merge borrowers and deletedborrowers.. > > Nice. At the time it was in my priority list. It made sense to delay and think about it. Feel free to resurrect this patch.