|
Description
Nick Clemens (kidclamp)
2019-05-13 20:06:54 UTC
Created attachment 89706 [details] [review] Bug 22899: Add pending_hold method to Koha::Item To test: 1 - Enable course reserves 2 - Create a course 3 - Add an item to the course 4 - Attempt to view the course on the OPAC 5 - Internal Server Error 6 - Apply patch 7 - Add an item to the holds queue by placing a hold and running holds queue builder or: INSERT INTO tmp_holdsqueue (itemnumber) VALUES (###); 8 - View the course page, note item appears 'Pending hold' 9 - Remove the holdsqueue line 10 - View the course page, note item appears 'Available' 11 - prove -v t/db_dependent/Koha/Item.t Created attachment 89707 [details] [review] Bug 22899: Database update Created attachment 89708 [details] [review] Bug 22899: [DO NOT PUSH] Schema updates Created attachment 89709 [details] [review] Bug 22899: Add pending_hold method to Koha::Item To test: 1 - Enable course reserves 2 - Create a course 3 - Add an item to the course 4 - Attempt to view the course on the OPAC 5 - Internal Server Error 6 - Apply patch 7 - Add an item to the holds queue by placing a hold and running holds queue builder or: INSERT INTO tmp_holdsqueue (itemnumber) VALUES (###); 8 - View the course page, note item appears 'Pending hold' 9 - Remove the holdsqueue line 10 - View the course page, note item appears 'Available' 11 - prove -v t/db_dependent/Koha/Item.t A couple of small things: 1. The new constraint will not be added if there are itemnumbers in the tmp_holdsqueue that do not exist in items. I know it's unlikely to happen but... who knows? 2. I am expecting ->pending_hold to return a Koha::Hold, maybe ->has_pending_hold could be better? Created attachment 89725 [details] [review] Bug 22899: Add pending_hold method to Koha::Item To test: 1 - Enable course reserves 2 - Create a course 3 - Add an item to the course 4 - Attempt to view the course on the OPAC 5 - Internal Server Error 6 - Apply patch 7 - Add an item to the holds queue by placing a hold and running holds queue builder or: INSERT INTO tmp_holdsqueue (itemnumber) VALUES (###); 8 - View the course page, note item appears 'Pending hold' 9 - Remove the holdsqueue line 10 - View the course page, note item appears 'Available' 11 - prove -v t/db_dependent/Koha/Item.t Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> I see Jonathan's comments about small improvements, but will sign off as everything works as expected here. Grr.. bit annoyed I missed this whilst QAing bug 15505 :( This patch doesn't feel quit right yet, I agree with Jonathan than the accessor name is misleading especially as it's not only acting as a check on the tmp_holdsqueue but also the system preference. I'm not familiar enough with the logic in this area now I'm afraid... but I do wonder a few things: 1) Should it really be an ON DELETE CASCADE.. or should we be setting to NULL or something to allow the hold to be reassigned to another item perhaps? 2) I don't like the accessor name as above 3) Should be trivial to correct the DB update to catch bad rows and clean them up before adding the key.. but it kind of depends what the answer to point 1 is to know what route to take (set to null or delete row) (In reply to Martin Renvoize from comment #8) > I'm not familiar enough with the logic in this area now I'm afraid... but I > do wonder a few things: > > 1) Should it really be an ON DELETE CASCADE.. or should we be setting to > NULL or something to allow the hold to be reassigned to another item perhaps? Reminding myself now, looks like the whole table is cleared down as part of every run of the cronjob. > 2) I don't like the accessor name as above Still outstanding > 3) Should be trivial to correct the DB update to catch bad rows and clean > them up before adding the key.. but it kind of depends what the answer to > point 1 is to know what route to take (set to null or delete row) Lets go ahead and Delete Created attachment 89736 [details] [review] Bug 22899: Database update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89737 [details] [review] Bug 22899: [DO NOT PUSH] Schema updates Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89738 [details] [review] Bug 22899: Add pending_hold method to Koha::Item To test: 1 - Enable course reserves 2 - Create a course 3 - Add an item to the course 4 - Attempt to view the course on the OPAC 5 - Internal Server Error 6 - Apply patch 7 - Add an item to the holds queue by placing a hold and running holds queue builder or: INSERT INTO tmp_holdsqueue (itemnumber) VALUES (###); 8 - View the course page, note item appears 'Pending hold' 9 - Remove the holdsqueue line 10 - View the course page, note item appears 'Available' 11 - prove -v t/db_dependent/Koha/Item.t Signed-off-by: Hayley Mapley <hayleymapley@catalyst.net.nz> I see Jonathan's comments about small improvements, but will sign off as everything works as expected here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89739 [details] [review] Bug 22899: (QA follow-up) Ensure constraint is created Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 89740 [details] [review] Bug 22899: (QA follow-up) Change accessor name This patch changes pending_hold to has_pending_hold to signify that we're returning a boolean and not a Koha::Hold object. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Right, with these two followups I think I'm happy to add my PQA stamp. Awesome work all! Pushed to master for 19.05 As this is also a regression in 18.11.05, I request that this be backported to 18.11.x or that the patch for bug 15505 be reverted from 18.11.x. Pushed to 18.11.x for 18.11.06 didn't backport 15505 to 18.05.x, wont backport this |