Description
Marcel de Rooy
2022-06-09 09:58:52 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> 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> 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> 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> 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> 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. 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> 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> 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> 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> 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> 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) (In reply to David Nind from comment #11) > Signed-off-by: David Nind <david@davidnind.com> Thx David. 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> 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> 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> 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> 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> 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 Created attachment 137879 [details] [review] Bug 30933: Unit tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> 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> @Marcel and @Martin. Please review my follow-ups. I'll push the tree if we agree with them. Would like your signature, though. 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> 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> (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. 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> 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> 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> 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> 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> 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> 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> Rebased Bug 30933: (follow-up) Use cannot_be_transferred in shelves scripts for change on 25498 Looks like a great improvement to the code readability to me.. 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> 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> Pushed to master for 22.11. Nice work everyone, thanks! - 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. Enhancement will not be backported to 22.05.x series |