|
Description
Kyle M Hall (khall)
2017-06-05 15:08:30 UTC
(In reply to Kyle M Hall from comment #0) > This is do to the fact that on those two pages the API is used and it used > the PUT for hold to update all hold fields, which requires this permission. > Another endpoint for updating only the pickup location should be added that > does not require the "modify_holds_priority" permission. Kyle, could you specify what exact permission should be needed? (In reply to Tomás Cohen Arazi from comment #1) > (In reply to Kyle M Hall from comment #0) > > This is do to the fact that on those two pages the API is used and it used > > the PUT for hold to update all hold fields, which requires this permission. > > Another endpoint for updating only the pickup location should be added that > > does not require the "modify_holds_priority" permission. > > Kyle, could you specify what exact permission should be needed? A permission shouldn't be needed, as that is not needed to set the pickup branch on any of the other pages. If the staff user only has 'place_holds' the holds tab in the patron account is broken and gives an error: DataTables warning: table id=holds-table - Ajax error. For more information about this error, please see http://datatables.net/tn/7 Investigating. (In reply to Katrin Fischer from comment #3) > If the staff user only has 'place_holds' the holds tab in the patron account > is broken and gives an error: > > DataTables warning: table id=holds-table - Ajax error. For more information > about this error, please see http://datatables.net/tn/7 > > Investigating. Forget above, the problem is the hols tab not working if staff only has borrowers, but not circulate_remaining_permissions. I opened a separate bug for this issue: bug 20794 Tomas, could you take another look here? I am not sure how to solve this bug. I think the problem is that we require reserveforothers for updates to holds, but in some cases we only want circulate_remaining_permissions. This is still valid on master. It'd be a great addition to Koha if it could be resolved. We need all of our librarians to be able to make location modifications, but we don't want them to be able to adjust the priority of the holds. Patrons have a valid reason to say to staff, "I want to pick my hold up from ABC instead XYZ" - but they don't have one to say, "I want to be moved from 174 to 9 in the queue." We'd love to be able to take the "modify_holds_priority" feature away from some librarians, but we can't justify losing the location modification feature in the process. I've just seen: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23517 - do those changes resolve this bug? Tomas, are you still aiming to get to this one, or should we unassign it? (In reply to Sally from comment #7) > I've just seen: > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23517 - do those > changes resolve this bug? That one only deals with priority. Question: what permissions would you require for changing the pickup location? Don't think in terms of the current UI, maybe we can add modify_holds_pickup_location for example. (In reply to Tomás Cohen Arazi from comment #9) > (In reply to Sally from comment #7) > > I've just seen: > > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23517 - do those > > changes resolve this bug? > > That one only deals with priority. Question: what permissions would you > require for changing the pickup location? Don't think in terms of the > current UI, maybe we can add modify_holds_pickup_location for example. Ah, thanks Tomas - yes, a new modify_holds_pickup_location would work for us. The key part for us is separating the permissions between modifying the pickup location and modifying the hold priority, so that suggestion works. (In reply to Sally from comment #10) > (In reply to Tomás Cohen Arazi from comment #9) > > (In reply to Sally from comment #7) > > > I've just seen: > > > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23517 - do those > > > changes resolve this bug? > > > > That one only deals with priority. Question: what permissions would you > > require for changing the pickup location? Don't think in terms of the > > current UI, maybe we can add modify_holds_pickup_location for example. > > Ah, thanks Tomas - yes, a new modify_holds_pickup_location would work for > us. > > The key part for us is separating the permissions between modifying the > pickup location and modifying the hold priority, so that suggestion works. Thinking of an upgrade path, would you agree users with modify_holds_priority should inherit the new modify_holds_pickup_location permission? > Thinking of an upgrade path, would you agree users with > modify_holds_priority should inherit the new modify_holds_pickup_location > permission? I think I would even make it simpler and use reserve_for_others or place_holds. As Kyle wrote it's already possible to change the pickup location without the modify_holds_priority permission - just from another page. The API way to do things was added later - so people have probably been doing the other way for a long time with the lower permission. Short: I'd match what the holds page for the record has. And if we add a new permission we should probably change it there as well? comment#0 >A librarian can update a hold's pickup library from the record details page, >but not from the patron details or checkout pages unless he or she has the >"modify_holds_priority". If the librarian does not, an error is displayed >instead. Tomas, this one seems an important one to have for 20.05 (In reply to Jonathan Druart from comment #13) > Tomas, this one seems an important one to have for 20.05 Hum, I thought it was a regression caused by bug 22284, but looking at the low bug number it should not be related actually. Created attachment 114067 [details] [review] Bug 18729: Add PUT /holds/{hold_id}/pickup_priority Tomas, can you have a look at this patch please? I think I am almost there, but the test is failing (500 instead of 200) and the UI is getting a 400. Created attachment 114311 [details] [review] Bug 18729: Add PUT /holds/{hold_id}/pickup_priority Bug 18729: Fix return type Thanks Tomas! :) Hi, I'd like to mention that: - The idea of having a route to change the pickup location *only*, was meant so we could have a more granular permission set. So I expect this route to require something like "reserveforothers": "modify_pickup_location". That permission should be set on an atomicupdate to all users having place_holds, as per bug 27117. - I liked the idea of calling them 'pickup locations' instead of the already used 'pickup_library' (see the hold objects definition). I like the fact that we prepare for some future in which there could be other pickup locations (shelves? special lockers for sanitized materials on this new world?). With this in mind, I have two other comments to make: - The PUT verb is accepting (and returning) a string, that actually represents what we already call pickup_library_id, but named generically 'pickup location'. I suggest we drop this and in turn expect a JSON object, with a 'pickup_library_id' attribute, replicating the already used terminology. It will also allow us to add new options for the future. - I regret not proposing this before, but we should really have a class abstracting pickup locations, that could initially just be a wrapper for libraries, but with a ->to_api method that makes it return the pickup_library_id and the name. If we need more things, we can solve that through embedding (e.g. embed the library object altogether). This will allow us to have simpler routes for dropdowns (right now we should take care of 'private' attributes for such dropdowns in the OPAC, for example). About this last comment, I haven't polished the idea. But the main message is 'do not put any design constraint ourselves at this point'. And I'm willing to have the full discussion when appropriate. Created attachment 114337 [details] [review] Bug 18729: Add more tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 114388 [details] [review] Bug 18729: Add PUT /holds/{hold_id}/pickup_priority Bug 18729: Fix return type Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 114389 [details] [review] Bug 18729: Add more tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 114390 [details] [review] Bug 18729: (follow-up) Adjust API to new spec The previous patch introduces some behavioural changes to the API, as well as the data types that need to be passed; all happens in the tests. This patch adapts the route so it complies with those changes: - JSON object containing pickup_library_id attribute is now passed back and forth. - The controller should take care of checking the pickup location is valid, using the available tools. To test: 1. Apply the patches, up to the tests 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => ERROR: Tests fail because the controller doesn't implement the desired behavior 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 114391 [details] [review] Bug 18729: Adapt holds.js Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 114393 [details] [review] Bug 18729: Add PUT /holds/{hold_id}/pickup_location This patch adds a route to overwrite the current pickup location. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 114394 [details] [review] Bug 18729: Add more tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 114395 [details] [review] Bug 18729: (follow-up) Adjust API to new spec The previous patch introduces some behavioural changes to the API, as well as the data types that need to be passed; all happens in the tests. This patch adapts the route so it complies with those changes: - JSON object containing pickup_library_id attribute is now passed back and forth. - The controller should take care of checking the pickup location is valid, using the available tools. To test: 1. Apply the patches, up to the tests 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => ERROR: Tests fail because the controller doesn't implement the desired behavior 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 114396 [details] [review] Bug 18729: Adapt holds.js Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 117656 [details] [review] Bug 18729: Add PUT /holds/{hold_id}/pickup_location This patch adds a route to overwrite the current pickup location. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 117657 [details] [review] Bug 18729: Add more tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 117658 [details] [review] Bug 18729: (follow-up) Adjust API to new spec The previous patch introduces some behavioural changes to the API, as well as the data types that need to be passed; all happens in the tests. This patch adapts the route so it complies with those changes: - JSON object containing pickup_library_id attribute is now passed back and forth. - The controller should take care of checking the pickup location is valid, using the available tools. To test: 1. Apply the patches, up to the tests 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => ERROR: Tests fail because the controller doesn't implement the desired behavior 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 117659 [details] [review] Bug 18729: Adapt holds.js Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Created attachment 119711 [details] [review] Bug 18729: Add PUT /holds/{hold_id}/pickup_location This patch adds a route to overwrite the current pickup location. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119712 [details] [review] Bug 18729: Add more tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119713 [details] [review] Bug 18729: (follow-up) Adjust API to new spec The previous patch introduces some behavioural changes to the API, as well as the data types that need to be passed; all happens in the tests. This patch adapts the route so it complies with those changes: - JSON object containing pickup_library_id attribute is now passed back and forth. - The controller should take care of checking the pickup location is valid, using the available tools. To test: 1. Apply the patches, up to the tests 2. Run: $ kshell k$ prove t/db_dependent/api/v1/holds.t => ERROR: Tests fail because the controller doesn't implement the desired behavior 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 119714 [details] [review] Bug 18729: Adapt holds.js Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Works well, is a good enhancement/bugfix and passes the QA scripts. Passing QA Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.05 Pushed to 20.05.x for 20.05.11 Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed. |