|
Description
Kelly McElligott
2019-12-12 18:17:53 UTC
Created attachment 96388 [details] [review] Bug 24232: Regression tests This patch enhances the tests for the DELETE /biblios/:biblio_id endpoint so more fine grained permissions are tested. It highlights the fact that the required permissions defined on the spec are not correcttly defined. To test: 1. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail because the user (that now has subpermissions only) does not have enough permissions for a non-existent subpermission Created attachment 96389 [details] [review] Bug 24232: Fix required permissions in DELETE /biblios/:biblio_id This patch fixes the required permissions defined on the spec for the mentioned endpoint. To test: 1. Apply the tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail due to badly defined permissions 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Notice parameters: edit_catelogue didn't exist 5. Sign off :-D (In reply to Kelly McElligott from comment #0) > Bug 15496 > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15496 > Now gives the ability to delete the bib if the item you are attaching to > another bib is the last item on the record. The code states that the > permission needed is Edit Catalogue, however, when this process is done, the > API is making a call and not getting the right signal (or something). > > Upon working on this, it seems that a librarian would need to have the > Overarching Permission in the Admin module to perform this action. Thanks for reporting it Kelly, it wasn't straightforward to detect the error! Can the test plan be more simplistic for users? 1. Have a staff member with Edit Catalog permissions (Edit catalog (Modify bibliographic/holdings data) (edit_catalogue)) and within the Edit Catalog permissions, Edit Items. 2. Find a bib record with 1 item. 3. Attach that 1 item to another bib record. 4. Koha gives an error when staff tries to delete the record. 5. Apply patch 6. Go through steps 1-3. 7. Success. Staff can attach an item to a record and successfully delete the itemless bib in the same step. Created attachment 96393 [details] [review] Bug 24232: Fix required permissions in DELETE /biblios/:biblio_id This patch fixes the required permissions defined on the spec for the mentioned endpoint. To test: 1. Apply the tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail due to badly defined permissions 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Notice parameters: edit_catelogue didn't exist 5. Sign off :-D Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Tomas, Kelly, The script moveitem.pl requires editcatalogue => 'edit_items', the deletion of a biblio editcatalogue => edit_catalogue. Even if weird I guess we could have a librarian with edit_items but edit_catalogue. I think we should show delete-biblio-btn if CAN_user_editcatalogue_edit_catalogue. Created attachment 96398 [details] [review] Bug 24232: Hide the "Delete biblio" if permission edit_catalogue not set The script moveitem.pl requires editcatalogue => 'edit_items', the deletion of a biblio editcatalogue => edit_catalogue. Even if weird I guess we could have a librarian with edit_items but edit_catalogue. Note that the message is meaningless: "The record has X attached items" is only displayed when X=0. (In reply to Jonathan Druart from comment #6) > Tomas, Kelly, > > The script moveitem.pl requires editcatalogue => 'edit_items', the deletion > of a biblio editcatalogue => edit_catalogue. Even if weird I guess we could > have a librarian with edit_items but edit_catalogue. It is not that weird. It happens in academic institutions where there's central cataloguing and the rest of the branches just add their items. > I think we should show delete-biblio-btn if > CAN_user_editcatalogue_edit_catalogue. Good idea! (In reply to Kelly McElligott from comment #4) > Can the test plan be more simplistic for users? > > 1. Have a staff member with Edit Catalog permissions (Edit catalog (Modify > bibliographic/holdings data) (edit_catalogue)) and within the Edit Catalog > permissions, Edit Items. > > 2. Find a bib record with 1 item. > > 3. Attach that 1 item to another bib record. > > 4. Koha gives an error when staff tries to delete the record. > > 5. Apply patch > > 6. Go through steps 1-3. > > 7. Success. Staff can attach an item to a record and successfully delete > the itemless bib in the same step. Thanks for that, Kelly. Can you retest with Jonathan's follow-up? When submitting the signed patches to Bugzilla, please specify that it is 3 patches! (see the attachments block here) and don't forget to mark it Signed Off. Thanks! (In reply to Jonathan Druart from comment #8) > Note that the message is meaningless: "The record has X attached items" is > only displayed when X=0. Can you file a separate bug with that? I was dealing with wrong API permissions here! Created attachment 96434 [details] [review] Bug 24232: Regression tests This patch enhances the tests for the DELETE /biblios/:biblio_id endpoint so more fine grained permissions are tested. It highlights the fact that the required permissions defined on the spec are not correcttly defined. To test: 1. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail because the user (that now has subpermissions only) does not have enough permissions for a non-existent subpermission Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 96435 [details] [review] Bug 24232: Fix required permissions in DELETE /biblios/:biblio_id This patch fixes the required permissions defined on the spec for the mentioned endpoint. To test: 1. Apply the tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail due to badly defined permissions 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Notice parameters: edit_catelogue didn't exist 5. Sign off :-D Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Created attachment 96436 [details] [review] Bug 24232: Hide the "Delete biblio" if permission edit_catalogue not set The script moveitem.pl requires editcatalogue => 'edit_items', the deletion of a biblio editcatalogue => edit_catalogue. Even if weird I guess we could have a librarian with edit_items but edit_catalogue. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Permissions: If staff member has Edit Catalog and Edit Items, this process can be done. If a staff member has Edit Items but not Edit Catalog, they can still attach the item, however the option to delete is not given. If a staff member has only Edit Catalog (and not Edit Items), the option is not there to Attach an Item. Created attachment 96455 [details] [review] Bug 24232: Regression tests This patch enhances the tests for the DELETE /biblios/:biblio_id endpoint so more fine grained permissions are tested. It highlights the fact that the required permissions defined on the spec are not correcttly defined. To test: 1. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail because the user (that now has subpermissions only) does not have enough permissions for a non-existent subpermission Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 96456 [details] [review] Bug 24232: Fix required permissions in DELETE /biblios/:biblio_id This patch fixes the required permissions defined on the spec for the mentioned endpoint. To test: 1. Apply the tests patch 2. Run: $ kshell k$ prove t/db_dependent/api/v1/biblios.t => FAIL: Tests fail due to badly defined permissions 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! Notice parameters: edit_catelogue didn't exist 5. Sign off :-D Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 96457 [details] [review] Bug 24232: Hide the "Delete biblio" if permission edit_catalogue not set The script moveitem.pl requires editcatalogue => 'edit_items', the deletion of a biblio editcatalogue => edit_catalogue. Even if weird I guess we could have a librarian with edit_items but edit_catalogue. Signed-off-by: Kelly McElligott <kelly@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Nice work everyone! Pushed to master for 20.05 Pushed to 19.11.x branch for 19.11.02 We should rename the bug for something like 'Wrong permissions...' backported to 19.05.x for 19.05.07 |