Bug 30933 - Add a designated owner for shared and public lists at patron deletion
Summary: Add a designated owner for shared and public lists at patron deletion
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Lists (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on: 25498
Blocks: 31288
  Show dependency treegraph
 
Reported: 2022-06-09 09:58 UTC by Marcel de Rooy
Modified: 2023-12-28 20:42 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact: Caroline Cyr La Rose
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/658
Text to go in the release notes:
This enhancement enables the transfer of public list ownership when a patron is deleted (from the staff interface, and scripts or cron jobs such as misc/cronjobs/delete_patrons.pl). A new system preference ListOwnershipUponPatronDeletion sets the action to take when a patron with public lists is deleted (options: delete the lists or change owner of these lists). If set to change the owner, the lists are transferred to the borrower number set in the new ListOwnerDesignated system preference.
Version(s) released in:
22.11.00


Attachments
Bug 30933: Database revision (new pref) (2.98 KB, patch)
2022-06-22 14:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: Change for patron->delete (4.92 KB, patch)
2022-06-22 14:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: Adjust pref description, include ListOwnerDesignated (1.84 KB, patch)
2022-06-22 14:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: (follow-up) Consolidate transfer checks (6.80 KB, patch)
2022-06-22 14:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: (follow-up) Use cannot_be_transferred in shelves scripts (5.12 KB, patch)
2022-06-22 14:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: Database revision (new pref) (2.99 KB, patch)
2022-07-06 19:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 30933: Change for patron->delete (4.93 KB, patch)
2022-07-06 19:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 30933: Adjust pref description, include ListOwnerDesignated (1.85 KB, patch)
2022-07-06 19:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 30933: (follow-up) Consolidate transfer checks (6.81 KB, patch)
2022-07-06 19:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 30933: (follow-up) Use cannot_be_transferred in shelves scripts (5.13 KB, patch)
2022-07-06 19:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 30933: Database revision (new pref) (3.05 KB, patch)
2022-07-08 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30933: Change for patron->delete (4.99 KB, patch)
2022-07-08 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30933: Adjust pref description, include ListOwnerDesignated (1.91 KB, patch)
2022-07-08 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30933: (follow-up) Consolidate transfer checks (6.87 KB, patch)
2022-07-08 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30933: (follow-up) Use cannot_be_transferred in shelves scripts (5.20 KB, patch)
2022-07-08 14:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30933: Unit tests (11.13 KB, patch)
2022-07-19 18:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30933: Add lists methods for disowning (5.30 KB, patch)
2022-07-19 18:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 30933: Unit tests (11.23 KB, patch)
2022-07-20 11:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: Add lists methods for disowning (5.39 KB, patch)
2022-07-20 11:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: Database revision (new pref) (3.09 KB, patch)
2022-07-20 11:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: Change for patron->delete (5.03 KB, patch)
2022-07-20 11:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: Adjust pref description, include ListOwnerDesignated (1.95 KB, patch)
2022-07-20 11:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: (follow-up) Consolidate transfer checks (6.91 KB, patch)
2022-07-20 11:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: (follow-up) Use cannot_be_transferred in shelves scripts (5.16 KB, patch)
2022-07-20 11:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: Unit tests (11.23 KB, patch)
2022-07-20 11:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: Add lists methods for disowning (5.39 KB, patch)
2022-07-20 11:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 30933: Unit tests (11.25 KB, patch)
2022-07-20 12:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 30933: Add lists methods for disowning (5.42 KB, patch)
2022-07-20 12:01 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-06-09 09:58:52 UTC
Follow-up of 11889
Comment 1 Marcel de Rooy 2022-06-22 14:02:39 UTC
Created attachment 136420 [details] [review]
Bug 30933: Database revision (new pref)

Adds pref ListOwnerDesignated.
Generalize description of ListOwnershipUponPatronDeletion a bit.

Test plan:
Run updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marcel de Rooy 2022-06-22 14:02:42 UTC
Created attachment 136421 [details] [review]
Bug 30933: Change for patron->delete

Test plan:
Run t/db_dependent/Koha/Patrons.t
Set pref ListOwnershipUponPatronDeletion to transfer.
Set pref ListOwnerDesignated to some valid borrowernumber.
Pick a user with public or shared list, delete from interface.
Pick a user with public or shared list, delete by script. (*)
Verify in both cases that new list owner is the designated one.

(*) Tip: Create another branch. Move the patron to be deleted to that
branch. And delete by script with:
    misc/cronjobs/delete_patons.pl -c -v -library YOUR_CODE

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Marcel de Rooy 2022-06-22 14:02:46 UTC
Created attachment 136422 [details] [review]
Bug 30933: Adjust pref description, include ListOwnerDesignated

Test plan:
View the pref on the System preferences, Patrons tab.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 4 Marcel de Rooy 2022-06-22 14:02:50 UTC
Created attachment 136423 [details] [review]
Bug 30933: (follow-up) Consolidate transfer checks

Adding shelf->cannot_be_transferred with unit tests.

Note: This follow-up actually refers to comment42 on the
preceding report 25498.
Furthermore, we could still improve later on using error code
more effectively (adding codes). Here we concentrate on moving
the checks to module level.

Test plan:
Run t/db_dependent/Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2022-06-22 14:02:54 UTC
Created attachment 136424 [details] [review]
Bug 30933: (follow-up) Use cannot_be_transferred in shelves scripts

Test plan:
Verify if transfer shared list on OPAC still works as expected.
Same for intranet counterpart for public lists.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2022-06-22 14:10:29 UTC
Martin, the two follow-up patches cover comment42 on the preceding report.
It only reduces the code in the scripts slightly, but offers much more testing opportunities.
Comment 7 David Nind 2022-07-06 19:42:34 UTC
Created attachment 137241 [details] [review]
Bug 30933: Database revision (new pref)

Adds pref ListOwnerDesignated.
Generalize description of ListOwnershipUponPatronDeletion a bit.

Test plan:
Run updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2022-07-06 19:42:39 UTC
Created attachment 137242 [details] [review]
Bug 30933: Change for patron->delete

Test plan:
Run t/db_dependent/Koha/Patrons.t
Set pref ListOwnershipUponPatronDeletion to transfer.
Set pref ListOwnerDesignated to some valid borrowernumber.
Pick a user with public or shared list, delete from interface.
Pick a user with public or shared list, delete by script. (*)
Verify in both cases that new list owner is the designated one.

(*) Tip: Create another branch. Move the patron to be deleted to that
branch. And delete by script with:
    misc/cronjobs/delete_patons.pl -c -v -library YOUR_CODE

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2022-07-06 19:42:43 UTC
Created attachment 137243 [details] [review]
Bug 30933: Adjust pref description, include ListOwnerDesignated

Test plan:
View the pref on the System preferences, Patrons tab.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2022-07-06 19:42:48 UTC
Created attachment 137244 [details] [review]
Bug 30933: (follow-up) Consolidate transfer checks

Adding shelf->cannot_be_transferred with unit tests.

Note: This follow-up actually refers to comment42 on the
preceding report 25498.
Furthermore, we could still improve later on using error code
more effectively (adding codes). Here we concentrate on moving
the checks to module level.

Test plan:
Run t/db_dependent/Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 David Nind 2022-07-06 19:42:52 UTC
Created attachment 137245 [details] [review]
Bug 30933: (follow-up) Use cannot_be_transferred in shelves scripts

Test plan:
Verify if transfer shared list on OPAC still works as expected.
Same for intranet counterpart for public lists.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: David Nind <david@davidnind.com>
Comment 12 David Nind 2022-07-06 20:05:59 UTC
Testing notes (using koha-testing-docker):

1. Test plan correction for patch Bug 30933: Change for patron->delete, the script name at the end to delete patrons is not spelt correctly - this should be ..patrons.pl.. not patons.pl: 

   misc/cronjobs/delete_patons.pl -c -v -library YOUR_CODE

2. Testing that lists continue to work after transfer from the staff interface:
   . Can delete public lists as the super librarian (koha)
   . Can transfer
   . Can edit
   . Can't add or remove items if 'Allow changes to contents from:' is set to 'Owner only', but can change this then add or remove items

2. Testing that lists continue to work as expected after transfer from the OPAC:
   . If changes can only be made by the owner, another OPAC user can't make any changes (as expected)
   . If changes can be made by anyone seeing the list, another OPAC user can add and remove items from the list (as expected), can't delete or edit the list (as expected)
   . If changes can be made by staff only, then they can only delete, edit, etc., if they have the correct permissions (as expected)
Comment 13 Marcel de Rooy 2022-07-07 06:06:55 UTC
(In reply to David Nind from comment #11)
> Signed-off-by: David Nind <david@davidnind.com>

Thx David.
Comment 14 Martin Renvoize 2022-07-08 14:05:37 UTC
Created attachment 137421 [details] [review]
Bug 30933: Database revision (new pref)

Adds pref ListOwnerDesignated.
Generalize description of ListOwnershipUponPatronDeletion a bit.

Test plan:
Run updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2022-07-08 14:05:41 UTC
Created attachment 137422 [details] [review]
Bug 30933: Change for patron->delete

Test plan:
Run t/db_dependent/Koha/Patrons.t
Set pref ListOwnershipUponPatronDeletion to transfer.
Set pref ListOwnerDesignated to some valid borrowernumber.
Pick a user with public or shared list, delete from interface.
Pick a user with public or shared list, delete by script. (*)
Verify in both cases that new list owner is the designated one.

(*) Tip: Create another branch. Move the patron to be deleted to that
branch. And delete by script with:
    misc/cronjobs/delete_patons.pl -c -v -library YOUR_CODE

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2022-07-08 14:05:45 UTC
Created attachment 137423 [details] [review]
Bug 30933: Adjust pref description, include ListOwnerDesignated

Test plan:
View the pref on the System preferences, Patrons tab.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2022-07-08 14:05:49 UTC
Created attachment 137424 [details] [review]
Bug 30933: (follow-up) Consolidate transfer checks

Adding shelf->cannot_be_transferred with unit tests.

Note: This follow-up actually refers to comment42 on the
preceding report 25498.
Furthermore, we could still improve later on using error code
more effectively (adding codes). Here we concentrate on moving
the checks to module level.

Test plan:
Run t/db_dependent/Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2022-07-08 14:05:54 UTC
Created attachment 137425 [details] [review]
Bug 30933: (follow-up) Use cannot_be_transferred in shelves scripts

Test plan:
Verify if transfer shared list on OPAC still works as expected.
Same for intranet counterpart for public lists.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2022-07-08 14:06:52 UTC
I did wonder if we could save ourselves a syspref and just use the anonymous user.. but I decided the clarity here was probably better.

All works as expected, QA scripts are happy and tests are passing

Passing QA
Comment 20 Tomás Cohen Arazi 2022-07-19 18:02:23 UTC
Created attachment 137879 [details] [review]
Bug 30933: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 21 Tomás Cohen Arazi 2022-07-19 18:02:31 UTC
Created attachment 137880 [details] [review]
Bug 30933: Add lists methods for disowning

This patch aims to tidy things a bit by doing the following changes:

* Koha::Virtualshelf gets the 'transfer_ownership' method
* Koha::Virtualshelves gets the 'disown_or_delete' method
* Koha::Patron->delete gets rewritten/simplified by using
  disown_or_delete

The idea is to capture the current behavior in more fine grained
methods. So current tests should be passing as they do now.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patrons.t \
           t/db_dependent/Virtualshelves.t
=> SUCCESS: Tests pass
2. Apply this patches
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Run:
  k$ prove t/db_dependent/Koha/Virtualshelf.t \
           t/db_dependent/Koha/Virtualshelves.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2022-07-19 18:03:59 UTC
@Marcel and @Martin. Please review my follow-ups. I'll push the tree if we agree with them. Would like your signature, though.
Comment 23 Marcel de Rooy 2022-07-20 11:36:03 UTC
Created attachment 137906 [details] [review]
Bug 30933: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2022-07-20 11:36:07 UTC
Created attachment 137907 [details] [review]
Bug 30933: Add lists methods for disowning

This patch aims to tidy things a bit by doing the following changes:

* Koha::Virtualshelf gets the 'transfer_ownership' method
* Koha::Virtualshelves gets the 'disown_or_delete' method
* Koha::Patron->delete gets rewritten/simplified by using
  disown_or_delete

The idea is to capture the current behavior in more fine grained
methods. So current tests should be passing as they do now.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patrons.t \
           t/db_dependent/Virtualshelves.t
=> SUCCESS: Tests pass
2. Apply this patches
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Run:
  k$ prove t/db_dependent/Koha/Virtualshelf.t \
           t/db_dependent/Koha/Virtualshelves.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2022-07-20 11:36:59 UTC
(In reply to Tomás Cohen Arazi from comment #22)
> @Marcel and @Martin. Please review my follow-ups. I'll push the tree if we
> agree with them. Would like your signature, though.

They are fine with me. Please wait a bit still, looking at a small interface issue in the middle patch set.
Comment 26 Marcel de Rooy 2022-07-20 11:52:15 UTC
Created attachment 137909 [details] [review]
Bug 30933: Database revision (new pref)

Adds pref ListOwnerDesignated.
Generalize description of ListOwnershipUponPatronDeletion a bit.

Test plan:
Run updatedatabase.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Marcel de Rooy 2022-07-20 11:52:20 UTC
Created attachment 137910 [details] [review]
Bug 30933: Change for patron->delete

Test plan:
Run t/db_dependent/Koha/Patrons.t
Set pref ListOwnershipUponPatronDeletion to transfer.
Set pref ListOwnerDesignated to some valid borrowernumber.
Pick a user with public or shared list, delete from interface.
Pick a user with public or shared list, delete by script. (*)
Verify in both cases that new list owner is the designated one.

(*) Tip: Create another branch. Move the patron to be deleted to that
branch. And delete by script with:
    misc/cronjobs/delete_patons.pl -c -v -library YOUR_CODE

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Marcel de Rooy 2022-07-20 11:52:24 UTC
Created attachment 137911 [details] [review]
Bug 30933: Adjust pref description, include ListOwnerDesignated

Test plan:
View the pref on the System preferences, Patrons tab.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Marcel de Rooy 2022-07-20 11:52:28 UTC
Created attachment 137912 [details] [review]
Bug 30933: (follow-up) Consolidate transfer checks

Adding shelf->cannot_be_transferred with unit tests.

Note: This follow-up actually refers to comment42 on the
preceding report 25498.
Furthermore, we could still improve later on using error code
more effectively (adding codes). Here we concentrate on moving
the checks to module level.

Test plan:
Run t/db_dependent/Virtualshelves.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Marcel de Rooy 2022-07-20 11:52:36 UTC
Created attachment 137913 [details] [review]
Bug 30933: (follow-up) Use cannot_be_transferred in shelves scripts

Test plan:
Verify if transfer shared list on OPAC still works as expected.
Same for intranet counterpart for public lists.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Marcel de Rooy 2022-07-20 11:52:41 UTC
Created attachment 137914 [details] [review]
Bug 30933: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 32 Marcel de Rooy 2022-07-20 11:52:46 UTC
Created attachment 137915 [details] [review]
Bug 30933: Add lists methods for disowning

This patch aims to tidy things a bit by doing the following changes:

* Koha::Virtualshelf gets the 'transfer_ownership' method
* Koha::Virtualshelves gets the 'disown_or_delete' method
* Koha::Patron->delete gets rewritten/simplified by using
  disown_or_delete

The idea is to capture the current behavior in more fine grained
methods. So current tests should be passing as they do now.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patrons.t \
           t/db_dependent/Virtualshelves.t
=> SUCCESS: Tests pass
2. Apply this patches
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Run:
  k$ prove t/db_dependent/Koha/Virtualshelf.t \
           t/db_dependent/Koha/Virtualshelves.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2022-07-20 11:53:27 UTC
Rebased Bug 30933: (follow-up) Use cannot_be_transferred in shelves scripts
for change on 25498
Comment 34 Martin Renvoize 2022-07-20 11:57:23 UTC
Looks like a great improvement to the code readability to me..
Comment 35 Martin Renvoize 2022-07-20 12:00:56 UTC
Created attachment 137916 [details] [review]
Bug 30933: Unit tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2022-07-20 12:01:00 UTC
Created attachment 137917 [details] [review]
Bug 30933: Add lists methods for disowning

This patch aims to tidy things a bit by doing the following changes:

* Koha::Virtualshelf gets the 'transfer_ownership' method
* Koha::Virtualshelves gets the 'disown_or_delete' method
* Koha::Patron->delete gets rewritten/simplified by using
  disown_or_delete

The idea is to capture the current behavior in more fine grained
methods. So current tests should be passing as they do now.

To test:
1. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patrons.t \
           t/db_dependent/Virtualshelves.t
=> SUCCESS: Tests pass
2. Apply this patches
3. Repeat 1
=> SUCCESS: Tests still pass!
4. Run:
  k$ prove t/db_dependent/Koha/Virtualshelf.t \
           t/db_dependent/Koha/Virtualshelves.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Tomás Cohen Arazi 2022-07-20 12:10:02 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 38 Marcel de Rooy 2022-08-03 11:16:33 UTC
-        my $userenv = C4::Context->userenv;
-        return $userenv->{'number'} if $userenv;
Replaced by:
+                    $new_owner = C4::Context->userenv->{number};

Will open a new bug. Saw this already:
perl t/db_dependent/Koha/AdditionalContents.t
1..5
# Subtest: ->author
    1..3
    ok 1 - Koha::AdditionalContent->author returns a Koha::Patron object
    # Looks like you planned 3 tests but ran 1.
not ok 4 - ->author
#   Failed test '->author'
#   at t/db_dependent/Koha/AdditionalContents.t line 156.
{UNKNOWN}: Can't use an undefined value as a HASH reference at /usr/share/koha/Koha/Virtualshelves.pm line 59. at /usr/share/koha/Koha/Virtualshelves.pm line 73
# Looks like your test exited with 255 just after 4.
Comment 39 Lucas Gass 2022-08-23 19:48:10 UTC
Enhancement will not be backported to 22.05.x series