Summary: | Holds to pull (pendingreserves.pl) uses removed default_branch_item_rules table | ||
---|---|---|---|
Product: | Koha | Reporter: | Nick Clemens <nick> |
Component: | Hold requests | Assignee: | Nick Clemens <nick> |
Status: | CLOSED FIXED | QA Contact: | Marcel de Rooy <m.de.rooy> |
Severity: | major | ||
Priority: | P5 - low | CC: | fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer, lucas, m.de.rooy, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22431 | ||
Change sponsored?: | --- | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
19.11.00
|
|
Bug Depends on: | 18928 | ||
Bug Blocks: | 23485 | ||
Attachments: |
Bug 23484: Update pendingreserves.pl to use circulation_rules
Bug 23484: Update pendingreserves.pl to use circulation_rules Bug 23484: (follow-up) add parenthesis in JOIN Bug 23484: Update pendingreserves.pl to use circulation_rules Bug 23484: (follow-up) add parenthesis in JOIN |
Description
Nick Clemens
2019-08-21 13:38:48 UTC
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 |