Bug 23484 - Holds to pull (pendingreserves.pl) uses removed default_branch_item_rules table
Summary: Holds to pull (pendingreserves.pl) uses removed default_branch_item_rules table
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 18928
Blocks: 23485
  Show dependency treegraph
 
Reported: 2019-08-21 13:38 UTC by Nick Clemens
Modified: 2019-11-15 07:33 UTC (History)
7 users (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:
19.11.00


Attachments
Bug 23484: Update pendingreserves.pl to use circulation_rules (1.58 KB, patch)
2019-08-21 13:40 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23484: Update pendingreserves.pl to use circulation_rules (1.64 KB, patch)
2019-10-04 10:00 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 23484: (follow-up) add parenthesis in JOIN (1.16 KB, patch)
2019-10-04 10:04 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 23484: Update pendingreserves.pl to use circulation_rules (1.73 KB, patch)
2019-10-11 08:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23484: (follow-up) add parenthesis in JOIN (1.25 KB, patch)
2019-10-11 08:21 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 Nick Clemens 2019-08-21 13:38:48 UTC
The report is broken
Comment 1 Nick Clemens 2019-08-21 13:40:01 UTC
Created attachment 92403 [details] [review]
Bug 23484: Update pendingreserves.pl to use circulation_rules
Comment 2 Nick Clemens 2019-08-21 13:42:06 UTC
To test:
1 - Disable strict mysql modes
2 - Place some reserves on a biblio with some items
3 - Browse to Circulation->Holds to pull
4 - Note there are no results in report
5 - Check logs - MySQL error
6 - Apply patch
7 - Refresh page, should load!
8 - Mark an itemtype as nothholdable in default circ rules
9 - Those items no longer show in report
Comment 3 Jonathan Druart 2019-09-01 19:39:37 UTC
The error is:
DBD::mysql::st execute failed: Table 'koha_kohadev.default_branch_item_rules' doesn't exist [for Statement "SELECT min(reservedate) as l_reservedate,
Comment 4 Jonathan Druart 2019-09-01 19:44:59 UTC
I am wondering if we should not retrieve the list of itemtypes for which the holds are allowed, then use them in the WHERE clause.
It will make the query more readable and we will remove a LEFT JOIN. What do you think?
Comment 5 Nick Clemens 2019-09-04 12:00:38 UTC
(In reply to Jonathan Druart from comment #4)
> I am wondering if we should not retrieve the list of itemtypes for which the
> holds are allowed, then use them in the WHERE clause.
> It will make the query more readable and we will remove a LEFT JOIN. What do
> you think?

That makes some sense, but will be two queries instead of one? In either case I think we should fix this now and re-engineer this report on another bug
Comment 6 Katrin Fischer 2019-09-04 12:07:50 UTC
(In reply to Nick Clemens from comment #5)
> (In reply to Jonathan Druart from comment #4)
> > I am wondering if we should not retrieve the list of itemtypes for which the
> > holds are allowed, then use them in the WHERE clause.
> > It will make the query more readable and we will remove a LEFT JOIN. What do
> > you think?
> 
> That makes some sense, but will be two queries instead of one? In either
> case I think we should fix this now and re-engineer this report on another
> bug

+1
Comment 7 Fridolin SOMERS 2019-10-04 10:00:06 UTC
Created attachment 93728 [details] [review]
Bug 23484: Update pendingreserves.pl to use circulation_rules

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 8 Fridolin SOMERS 2019-10-04 10:04:31 UTC
Created attachment 93731 [details] [review]
Bug 23484: (follow-up) add parenthesis in JOIN
Comment 9 Fridolin SOMERS 2019-10-04 10:05:43 UTC
My followup is basic so I keep bug in SO status, if its OK with QA.
Comment 10 Marcel de Rooy 2019-10-11 06:07:30 UTC
QAing
Comment 11 Marcel de Rooy 2019-10-11 06:33:21 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2019-10-11 06:57:30 UTC
I am not sure I agree about 1): If you have it show up on the holds to pull report, it should be made sure that the items you pull can trigger the hold on return. Hiding the itemtypes that are not holdable makes sense to me.

But more important, as this is a bug fix, the page should behave the same as it behaved before we broke it for now and changes should be discussed separately.
Comment 13 Marcel de Rooy 2019-10-11 08:00:36 UTC
(In reply to Katrin Fischer from comment #12)
> I am not sure I agree about 1): If you have it show up on the holds to pull
> report, it should be made sure that the items you pull can trigger the hold
> on return. Hiding the itemtypes that are not holdable makes sense to me.

Changing status again. Still rethinking a bit about what you say. Thx for feedback.


> But more important, as this is a bug fix, the page should behave the same as
> it behaved before we broke it for now and changes should be discussed
> separately.

Hmm. A fix should not be a new bug?
Comment 14 Marcel de Rooy 2019-10-11 08:17:28 UTC
Forget my former QA comment. I tested biblio level hold and item level hold now. Probably misread the query ;)
Comment 15 Marcel de Rooy 2019-10-11 08:19:40 UTC
So, I will pass QA. The only thing raised about a hold that no longer should be a hold since it does no longer pass circ rules, should not block this report indeed.
Comment 16 Marcel de Rooy 2019-10-11 08:21:27 UTC
Created attachment 93985 [details] [review]
Bug 23484: Update pendingreserves.pl to use circulation_rules

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2019-10-11 08:21:32 UTC
Created attachment 93986 [details] [review]
Bug 23484: (follow-up) add parenthesis in JOIN

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Martin Renvoize 2019-10-11 09:56:10 UTC
Nice work!

Pushed to master for 19.11.00
Comment 19 Jonathan Druart 2019-10-15 11:12:12 UTC
(In reply to Nick Clemens from comment #5)
> (In reply to Jonathan Druart from comment #4)
> > I am wondering if we should not retrieve the list of itemtypes for which the
> > holds are allowed, then use them in the WHERE clause.
> > It will make the query more readable and we will remove a LEFT JOIN. What do
> > you think?
> 
> That makes some sense, but will be two queries instead of one? In either
> case I think we should fix this now and re-engineer this report on another
> bug

2 queries but 1 LEFT JOIN removed.
Comment 20 Fridolin SOMERS 2019-11-08 13:55:26 UTC
Pushed to 19.05.x for 19.05.05
Comment 21 Lucas Gass 2019-11-13 23:07:49 UTC
missing dependency for 18.11.x, no backport
Comment 22 Fridolin SOMERS 2019-11-15 07:33:56 UTC
Oups sorry Bug 18928 is not in 19.05.x, default_branch_item_rules still exists.
so I revert