Bug 24232 - Fix permissions for deleting a bib record after attaching the last item to another bib
Summary: Fix permissions for deleting a bib record after attaching the last item to an...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 15496
Blocks:
  Show dependency treegraph
 
Reported: 2019-12-12 18:17 UTC by Kelly McElligott
Modified: 2020-01-15 23:16 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.02


Attachments
Bug 24232: Regression tests (2.06 KB, patch)
2019-12-17 17:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24232: Fix required permissions in DELETE /biblios/:biblio_id (1.08 KB, patch)
2019-12-17 17:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24232: Fix required permissions in DELETE /biblios/:biblio_id (1.14 KB, patch)
2019-12-17 20:16 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24232: Hide the "Delete biblio" if permission edit_catalogue not set (1.44 KB, patch)
2019-12-18 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24232: Regression tests (2.12 KB, patch)
2019-12-18 17:32 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24232: Fix required permissions in DELETE /biblios/:biblio_id (1.20 KB, patch)
2019-12-18 17:32 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24232: Hide the "Delete biblio" if permission edit_catalogue not set (1.50 KB, patch)
2019-12-18 17:32 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 24232: Regression tests (2.19 KB, patch)
2019-12-18 18:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24232: Fix required permissions in DELETE /biblios/:biblio_id (1.27 KB, patch)
2019-12-18 18:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24232: Hide the "Delete biblio" if permission edit_catalogue not set (1.57 KB, patch)
2019-12-18 18:34 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly McElligott 2019-12-12 18:17:53 UTC
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.
Comment 1 Tomás Cohen Arazi 2019-12-17 17:46:15 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
Comment 2 Tomás Cohen Arazi 2019-12-17 17:46:18 UTC
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
Comment 3 Tomás Cohen Arazi 2019-12-17 17:50:05 UTC
(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!
Comment 4 Kelly McElligott 2019-12-17 19:56:09 UTC
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.
Comment 5 ByWater Sandboxes 2019-12-17 20:16:08 UTC
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>
Comment 6 Jonathan Druart 2019-12-18 08:44:19 UTC
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.
Comment 7 Jonathan Druart 2019-12-18 08:48:01 UTC
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.
Comment 8 Jonathan Druart 2019-12-18 08:53:10 UTC
Note that the message is meaningless: "The record has X attached items" is only displayed when X=0.
Comment 9 Tomás Cohen Arazi 2019-12-18 11:44:47 UTC
(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!
Comment 10 Tomás Cohen Arazi 2019-12-18 11:46:14 UTC
(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!
Comment 11 Tomás Cohen Arazi 2019-12-18 11:46:51 UTC
(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!
Comment 12 ByWater Sandboxes 2019-12-18 17:32:54 UTC
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>
Comment 13 ByWater Sandboxes 2019-12-18 17:32:56 UTC
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>
Comment 14 ByWater Sandboxes 2019-12-18 17:32:58 UTC
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>
Comment 15 Kelly McElligott 2019-12-18 17:35:27 UTC
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.
Comment 16 Jonathan Druart 2019-12-18 18:33:59 UTC
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>
Comment 17 Jonathan Druart 2019-12-18 18:34:02 UTC
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>
Comment 18 Jonathan Druart 2019-12-18 18:34:06 UTC
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>
Comment 19 Martin Renvoize 2019-12-20 12:10:00 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 20 Joy Nelson 2020-01-03 22:38:12 UTC
Pushed to 19.11.x branch for 19.11.02
Comment 21 Tomás Cohen Arazi 2020-01-04 00:07:31 UTC
We should rename the bug for something like 'Wrong permissions...'
Comment 22 Lucas Gass 2020-01-13 22:13:42 UTC
backported to 19.05.x for 19.05.07