Description
Alex Sassmannshausen
2017-03-03 07:43:01 UTC
Created attachment 60805 [details] [review] Add per borrower category create ILL privilege. * admin/categories.pl: Add validation & update for `canplaceill`. * koha-tmpl/intranet-tmpl/prog/en/modules/admin/categories.tt: Add `canplaceill` handling. * ill/ill-requests.pl: Pass `canplaceill` to template. * koha-tmpl/intranet-tmpl/prog/en/includes/ill-toolbar.inc: Check for `canplaceill`. * opac/opac-illrequests.pl: Pass `canplaceill` to template. * koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-illrequests.tt: Check for `canplaceill`. * installer/data/mysql/kohastructure.sql: Create `canplaceill` column. * installer/data/mysql/atomicupdate/ill_cat_permission.sql: New file. Created attachment 60806 [details] [review] DO NOT PUSH: Schema changes. * Koha/Schema/Result/Category.pm: Add `canplaceill`. Test Plan: - Have ILLModule working - With any non-database user verify you can place new ILL requests in OPAC and Intranet - Apply patches here - Run dbupdate - Verify you can no longer place new ILL requests in OPAC and Intranet - In borrower categories, set 'Can place ILL' to yes. - Verify you can place new ILL requests in OPAC and Intranet. I think this would still be very useful to have as I see no other option to limit ILL requests for patrons. By patron category should work for most libraries. Has there been any update on this since the DO NOT PUSH? Thanks, Niamh *** Bug 23976 has been marked as a duplicate of this bug. *** Alex is no longer working on Koha so I am not sure why this was never set to 'needs sign-off'. The DO NOT PUSH for schema changes is normal as this file is only used for testing and the RM will not use it when pushing the patch set. So this doesn't tell us much. hm. I am updating the status to 'Patch doesn't apply' and adding Andrew and Martin who may know what happened here. I believe this would still be a nice to have feature. Created attachment 145331 [details] [review] Bug 18203: Add per borrower category restrictions on ILL placement Test Plan: - Enable ILLModule sys pref and install any backend - Verify you can place new ILL requests in OPAC and Intranet - Apply patch - Run dbic - Verify you can no longer place new ILL requests in OPAC and Intranet - In borrower categories, set 'Can place ILL' to yes. - Verify you can place new ILL requests in OPAC and Intranet. Created attachment 145349 [details] [review] Bug 18203: Add per borrower category restrictions on ILL placement Test Plan: - Enable ILLModule sys pref and install any backend - Verify you can place new ILL requests in OPAC and Intranet - Apply patch - Run dbic - Verify you can no longer place new ILL requests in OPAC and Intranet - In borrower categories, set 'Can place ILL' to yes. - Verify you can place new ILL requests in OPAC and Intranet. Signed-off-by: David Nind <david@davidnind.com> Created attachment 145354 [details] [review] Bug 18203: Add per borrower category restrictions on ILL placement Test Plan: - Enable ILLModule sys pref and install any backend - Verify you can place new ILL requests in OPAC and Intranet - Apply patch - Run dbic - Verify you can no longer place new ILL requests in OPAC and Intranet - In borrower categories, set 'Can place ILL' to yes. - Verify you can place new ILL requests in OPAC and Intranet. Signed-off-by: David Nind <david@davidnind.com> Fixed commit author, had originally been submitted as John Doe. 1) There is a fail in the QA scripts: FAIL installer/data/mysql/kohastructure.sql FAIL tinyint_has_boolean_flag The new column categories.canplaceill is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/Category.pm), see the SQL12 coding guideline 2) Behavior in staff interface I think, we should not use the staff person's patron category for the check on the 'new request' button in the staff interface. If a staff user has permissions for the interlibrary loans module, they should be able to place ILL requests for the users of the library. They are usually not placing ILL requests for themselves, but for others. Similar to hold requests. The check on the category should be implemented in the patron search/the user they try to place the request for. But I am not sure if this would have to be done in the backends then? 3) Changed behaviour for existing installations As all categories are allowed to place ILL requests now, we should set "Can place ILL requests?" to yes for all categories if the ILLModule is activated. This way we won't break things for libraries and they can implement the new feature if they chose to do so. But overall this looks quite good already! Created attachment 148707 [details] [review] Bug 18203: (QA follow-up) * Add boolean flag in Category.pm > 2) Behavior in staff interface > > I think, we should not use the staff person's patron category for the check > on the 'new request' button in the staff interface. > > If a staff user has permissions for the interlibrary loans module, they > should be able to place ILL requests for the users of the library. They are > usually not placing ILL requests for themselves, but for others. Similar to > hold requests. > > The check on the category should be implemented in the patron search/the > user they try to place the request for. But I am not sure if this would have > to be done in the backends then? Yes, I agree with this idea. But it's the backend who is responsible for attaching the borrowernumber to the illrequest. However, this does seem a bit redundant for the staff UI, i.e. if a staff member already has the ill permission, this check should not even be required (since ill permission needs to be assigned on a per user in the first place anyway). I suggest we restructure this bug's functionality to just apply to OPAC, since I also believe the original problem it's trying to solve is only on the OPAC anyway (prevent certain patron categories from being able to place requests to avoid costs/noise) > 3) Changed behaviour for existing installations > > As all categories are allowed to place ILL requests now, we should set "Can > place ILL requests?" to yes for all categories if the ILLModule is > activated. This way we won't break things for libraries and they can > implement the new feature if they chose to do so. Should we: 1) keep canplaceill default 0, and in atomicupdate set to 1 for every existing installation if ILLModule sys pref is enabled, or 2) change canplaceill default to 1, which means upon enabling ILLModule it just works for everyone and then let staff members untick the canplaceill checkbox for any given category. Please Katrin let me know what you think of the above, or if some of what I said doesn't make sense or is wrong. Thanks! Current patches may or may not be causing issues with the table rendered in patron categories /cgi-bin/koha/admin/categories.pl Need to double-check that. Created attachment 148719 [details] [review] WIP: Fix categories table presentation But now there is an error in the console if ILL is disabled Squash this later Created attachment 149010 [details] [review] Bug 18203: Add per borrower category restrictions on placing ILL requests Default value for canplaceillopac is 1 Patron in a category that cannot place ill can still visit illlist and illview Created attachment 153345 [details] [review] Bug 18203: DB specific and atomicupdate Created attachment 153346 [details] [review] Bug 18203: dbic specific [DONT PUSH] Created attachment 153347 [details] [review] Bug 18203: Add per borrower category restrictions on placing ILL requests Test Plan: - Enable ILLModule sys pref and install any backend, or run bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) - Verify you can place new ILL requests in OPAC and Intranet - Apply patch - In borrower categories, verify there's a new column for 'can place ILL in opac' and is set to 'yes' by default - Edit your borrower's patron category and set 'can place ILL in opac' to 'No' - Verify you can no longer place new ILL requests in OPAC - Verify you also cannot place new ILL requests through URL: :8080/cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm Rebased this after bug 33702. Updated test plan. Split submission to multiple patches (db, dbic and feature). Would someone be able to take a look at this? It's a quite small patch! (easy sign-off). Created attachment 153370 [details] [review] Bug 18203: DB specific and atomicupdate Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153371 [details] [review] Bug 18203: dbic specific [DONT PUSH] Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 153372 [details] [review] Bug 18203: Add per borrower category restrictions on placing ILL requests Test Plan: - Enable ILLModule sys pref and install any backend, or run bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) - Verify you can place new ILL requests in OPAC and Intranet - Apply patch - In borrower categories, verify there's a new column for 'can place ILL in opac' and is set to 'yes' by default - Edit your borrower's patron category and set 'can place ILL in opac' to 'No' - Verify you can no longer place new ILL requests in OPAC - Verify you also cannot place new ILL requests through URL: :8080/cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm Signed-off-by: Sam Lau <samalau@gmail.com> Needs a rebase on top of bug 34282 Needs a rebase on top of bug 34382 Created attachment 155316 [details] [review] Bug 18203: DB specific and atomicupdate Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 155317 [details] [review] Bug 18203: dbic specific [DONT PUSH] Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 155318 [details] [review] Bug 18203: Add per borrower category restrictions on placing ILL requests Test Plan: - Enable ILLModule sys pref and install any backend, or run bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) - Verify you can place new ILL requests in OPAC and Intranet - Apply patch - In borrower categories, verify there's a new column for 'can place ILL in opac' and is set to 'yes' by default - Edit your borrower's patron category and set 'can place ILL in opac' to 'No' - Verify you can no longer place new ILL requests in OPAC - Verify you also cannot place new ILL requests through URL: :8080/cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 158126 [details] [review] Bug 18203: DB specific and atomicupdate Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 158127 [details] [review] Bug 18203: dbic specific [DONT PUSH] Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 158128 [details] [review] Bug 18203: Add per borrower category restrictions on placing ILL requests Test Plan: - Enable ILLModule sys pref and install any backend, or run bash <(curl -s https://raw.githubusercontent.com/ammopt/koha-ill-dev/master/start-ill-dev.sh) - Verify you can place new ILL requests in OPAC and Intranet - Apply patch - In borrower categories, verify there's a new column for 'can place ILL in opac' and is set to 'yes' by default - Edit your borrower's patron category and set 'can place ILL in opac' to 'No' - Verify you can no longer place new ILL requests in OPAC - Verify you also cannot place new ILL requests through URL: :8080/cgi-bin/koha/opac-illrequests.pl?method=create&backend=FreeForm Signed-off-by: Sam Lau <samalau@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Pushed to master for 23.11. Nice work everyone, thanks! I made a last minute change on the column name. I hope you agree. That's awesome Tomas thank you. (In reply to Pedro Amorim from comment #36) > That's awesome Tomas thank you. We have a problem here, see bug 35263 - cannot edit patron categories Enhancement not pushed to 23.05.x |