The report is broken
Created attachment 92403 [details] [review] Bug 23484: Update pendingreserves.pl to use circulation_rules
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
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,
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?
(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
(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
Created attachment 93728 [details] [review] Bug 23484: Update pendingreserves.pl to use circulation_rules Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Created attachment 93731 [details] [review] Bug 23484: (follow-up) add parenthesis in JOIN
My followup is basic so I keep bug in SO status, if its OK with QA.
QAing
QA Comment: [1] Nick, please tell me if we strictly need the rule. If a hold is already placed (it passed the rules of that moment), shouldn't we allow it? The rule should be used when placing the hold, not pulling it. If we dont pull it, this hold will be in the system indefinitely. [2] Secondly, the rule applies to item level holds only. You are listing biblio level holds, but hey the items of that book could all be 'forbidden'? Note that this says more about circulation rules than the pull report btw. [3] If we follow the rule, shouldnt we make a distinction between From any library and From home library. In other words: why dont you check the branchcode here? My recommendation would be cf [1] remove this condition. Keep it simple when you pull holds. Changing status for need of feedback.
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.
(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?
Forget my former QA comment. I tested biblio level hold and item level hold now. Probably misread the query ;)
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.
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>
Created attachment 93986 [details] [review] Bug 23484: (follow-up) add parenthesis in JOIN Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Nice work! Pushed to master for 19.11.00
(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.
Pushed to 19.05.x for 19.05.05
missing dependency for 18.11.x, no backport
Oups sorry Bug 18928 is not in 19.05.x, default_branch_item_rules still exists. so I revert