Bug 22899 - Cannot view course details
Summary: Cannot view course details
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Course reserves (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords: regression
Depends on: 15505
Blocks:
  Show dependency treegraph
 
Reported: 2019-05-13 20:06 UTC by Nick Clemens
Modified: 2019-10-14 20:02 UTC (History)
4 users (show)

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


Attachments
Bug 22899: Add pending_hold method to Koha::Item (2.87 KB, patch)
2019-05-13 20:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22899: Database update (2.12 KB, patch)
2019-05-13 20:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22899: [DO NOT PUSH] Schema updates (2.89 KB, patch)
2019-05-13 20:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22899: Add pending_hold method to Koha::Item (2.87 KB, patch)
2019-05-13 20:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 22899: Add pending_hold method to Koha::Item (3.03 KB, patch)
2019-05-14 05:29 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22899: Database update (2.17 KB, patch)
2019-05-14 12:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22899: [DO NOT PUSH] Schema updates (2.95 KB, patch)
2019-05-14 12:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22899: Add pending_hold method to Koha::Item (3.10 KB, patch)
2019-05-14 12:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22899: (QA follow-up) Ensure constraint is created (1.34 KB, patch)
2019-05-14 12:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22899: (QA follow-up) Change accessor name (6.25 KB, patch)
2019-05-14 12:25 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-05-13 20:06:54 UTC
Bug 15505 added a 'pending_hold' column to item-status.inc

Course reserves, however, passes item objects to the template and there is no object method for pending_hold

To recreate:
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
Comment 1 Nick Clemens 2019-05-13 20:23:40 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
Comment 2 Nick Clemens 2019-05-13 20:28:44 UTC
Created attachment 89707 [details] [review]
Bug 22899: Database update
Comment 3 Nick Clemens 2019-05-13 20:28:46 UTC
Created attachment 89708 [details] [review]
Bug 22899: [DO NOT PUSH] Schema updates
Comment 4 Nick Clemens 2019-05-13 20:28:49 UTC
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
Comment 5 Jonathan Druart 2019-05-13 23:50:21 UTC
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?
Comment 6 Hayley Mapley 2019-05-14 05:29:34 UTC
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.
Comment 7 Martin Renvoize 2019-05-14 07:31:06 UTC
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.
Comment 8 Martin Renvoize 2019-05-14 07:38:32 UTC
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)
Comment 9 Martin Renvoize 2019-05-14 07:52:29 UTC
(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
Comment 10 Martin Renvoize 2019-05-14 12:24:53 UTC
Created attachment 89736 [details] [review]
Bug 22899: Database update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2019-05-14 12:24:55 UTC
Created attachment 89737 [details] [review]
Bug 22899: [DO NOT PUSH] Schema updates

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-05-14 12:24:58 UTC
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>
Comment 13 Martin Renvoize 2019-05-14 12:25:01 UTC
Created attachment 89739 [details] [review]
Bug 22899: (QA follow-up) Ensure constraint is created

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2019-05-14 12:25:04 UTC
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>
Comment 15 Martin Renvoize 2019-05-14 12:25:39 UTC
Right, with these two followups I think I'm happy to add my PQA stamp.
Comment 16 Nick Clemens 2019-05-14 18:22:44 UTC
Awesome work all!

Pushed to master for 19.05
Comment 17 Galen Charlton 2019-05-22 14:09:51 UTC
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.
Comment 18 Martin Renvoize 2019-05-24 13:41:22 UTC
Pushed to 18.11.x for 18.11.06
Comment 19 Lucas Gass 2019-05-30 15:48:56 UTC
didn't backport 15505 to 18.05.x, wont backport this