Duplicate of the see also ?

(In reply to Marcel de Rooy from comment #1)
> Duplicate of the see also ?

I don't think so. The see also only seems to be concerned with what to do when deleting a patron. 

In this case, I'm just targeting the inability/ability to manually change the owner of a list.

Personally, I would think this issue would make Bug 11889 redundant, but that bug seems to be lost in a swamp of conflicting ideas and expectations. So I opened this one with a very simple goal.

What we will encounter here too, is the "privacy" issue. You should not be able to take ownership of arbitrary private or shared lists.
I am still thinking in terms of moving owner to NULL for a public list when you remove its owner. And allowing superlibrarians to take over such lists.

(In reply to Marcel de Rooy from comment #3)
> What we will encounter here too, is the "privacy" issue. You should not be
> able to take ownership of arbitrary private or shared lists.
> I am still thinking in terms of moving owner to NULL for a public list when
> you remove its owner. And allowing superlibrarians to take over such lists.

+1

(In reply to Marcel de Rooy from comment #3)
> What we will encounter here too, is the "privacy" issue. You should not be
> able to take ownership of arbitrary private or shared lists.
> I am still thinking in terms of moving owner to NULL for a public list when
> you remove its owner. And allowing superlibrarians to take over such lists.

I think that the solution is to allow only superlibrarians to change ownership of lists. Since they have full access to the SQL reporting, they already have full access to that data, so the privacy issue is a bit moot. 

In the use cases described by kholt@roseville.ca.us and myself, we're most concerned about public lists. It really should be possible for a superlibrarian to transfer ownership (ie responsibility) for a list to a different staff person.

I'd be willing to work on a feature where a superlibrarian can only change the ownership of a public list. That would reduce the chance of a superlibrarian accidentally changing the owner of a private list, and thus leaking personal information between private individuals.

(In reply to David Cook from comment #5)
> Since they have full access to the SQL reporting, they
> already have full access to that data, so the privacy issue is a bit moot. 

Agreed. But it may be a matter of Does the system facilitate crossing such privacy borders? The reporting backdoor is a subject on itself ;)

(In reply to Marcel de Rooy from comment #6)
> (In reply to David Cook from comment #5)
> > Since they have full access to the SQL reporting, they
> > already have full access to that data, so the privacy issue is a bit moot. 
> 
> Agreed. But it may be a matter of Does the system facilitate crossing such
> privacy borders? The reporting backdoor is a subject on itself ;)

Those are good points too heh.

I think in that case maybe private lists should be absolutely private, but public lists should be able to have owners changed by superlibrarians. 

Shared lists... well I don't quite understand the concept of shared lists yet (neither from the user or technical perspective), although I think I get what you'd mean by disowning a list in that context.

I've been talking to a library that wants to create custom lists for patrons -- the patron fills out a readers advisory form and the library prepares a list of recommendations. It'd be great if this would allow the library to create a private list and make the patron the owner.

Are you still intending on working on this David?  We'd love to see such functionality too.

(In reply to Martin Renvoize from comment #9)
> Are you still intending on working on this David?  We'd love to see such
> functionality too.

No, I have no plans to work on this, unfortunately.

We would love this feature, too! Mainly so a librarian can continue building on existing public lists when the original list owner is no longer involved.

I would love for there to be a way to change ownership of a list or to transfer ownership to another staff member when the original list owner is no longer involved.

(In reply to Andrew Fuerste-Henry from comment #8)
> I've been talking to a library that wants to create custom lists for patrons
> -- the patron fills out a readers advisory form and the library prepares a
> list of recommendations. It'd be great if this would allow the library to
> create a private list and make the patron the owner.

Andrew's suggestion could be a useful tool too!

But more than an anything, an ability to transfer ownership of public lists would be very helpful.

A staff member doesn't even need to leave an institution for this to be helpful. A change in position within the same institution might mean that someone new needs to maintain an old list, and it'd be very nice not to have to scrap that old list and start from scratch.

This feature would be very helpful! As someone who makes lists frequently and collaborates with other coworkers in making lists, it'd be helpful to transfer ownership to update a list that was started by a coworker (or to give ownership to someone else).

This functionality would be very useful at our library.

Will be proposing something here shortly in terms of patches

Created attachment 135889 [details] [review]
Bug 25498: Add transfer button on intranet

Note: Adjusting shelves_results template too for not allowing delete
when you do not have delete_public_lists (or higher). If you are the
owner, you are allowed to edit or delete. But transfer specifically
needs edit_public_lists (or higher).

Test plan:
Check if you see the transfer button on public lists only when having
permission edit_public_lists (or higher). And never on private lists.

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

By far not ready :)

I've never really investigated this area of Koha much.. I was surprised to see the 'share' options only work from the OPAC.. I thought you'd be able to share a private list with other staff via the staff client... that belief made me think perhaps the transfer of ownership stuff would be tied to who you shared a list with (and what capacity the share was.. read only, edit, owner.. somewhat like google docs and similar friends).

Just some meandering thoughts.. I'll be interested to see where you take this Marcel.

(In reply to Martin Renvoize from comment #19)
> I've never really investigated this area of Koha much.. I was surprised to
> see the 'share' options only work from the OPAC.. I thought you'd be able to
> share a private list with other staff via the staff client... that belief
> made me think perhaps the transfer of ownership stuff would be tied to who
> you shared a list with (and what capacity the share was.. read only, edit,
> owner.. somewhat like google docs and similar friends).
> 
> Just some meandering thoughts.. I'll be interested to see where you take
> this Marcel.

Thinking along the following lines:
Allow transfer ownership of public list on staff interface. Staff member with permission can transfer to any user.
Allow the owner of a shared list to transfer ownership to another member of the list like you wrote above.
Still not completely sure about handling deletion of a shared list. Since the owner will have the choice between transfer [and remove share] or delete, a deliberate delete should probably be performed as now. On the other hand, bug 11889 may transfer the shared list to a staff member when deleting the owner as patron. Easy to defend: you shared that list..
Private lists should not be transferred.

Created attachment 135921 [details] [review]
Bug 25498: Add a transfer form for public lists

Test plan:
[1] Valid transfer:
    Log in with staff patron C having edit_public_lists.
    Pick a public list. Choose Transfer. Select a patron.
    Submit and check if transfer was successful.
[2] Permission problem:
    Pick a staff user D. Temporary enable edit_public_lists.
    Add another private tab. Login with D.
    Pick a public list. Transfer. Choose another patron. Wait.
    Switch tab: remove added permission from D.
    Switch tab again: Submit transfer form. Error? Close tab.
[3] Bonus test - Shelf disappeared:
    Pick a public list. Transfer. Choose another patron. Wait.
    Open another tab. Delete the selected public list.
    Close tab. Submit the transfer. Not exist error?
[4] Bonus test - Patron not found:
    Pick a public list. Transfer. Choose another patron. Wait.
    Open another tab. Delete selected patron.
    Close tab. Submit transfer. Patron not found error?

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

(In reply to Marcel de Rooy from comment #20)
> Allow the owner of a shared list to transfer ownership to another member of
> the list like you wrote above.

Does anyone already have some thoughts about the sensitivity of this OPAC operation? I'm thinking of a simple select containing the names only of the members that accepted a share of this list. But actually we only mailed an invitation via the Share button; very strictly speaking this would reveal more information. On the other hand, the shared list owner should be entitled somehow to see which people are using the list.

Note: Instead of a name, we could list an email address with a borrowernumber behind it in the select option?

Created attachment 136008 [details] [review]
Bug 25498: Add transfer button on intranet

Note: Adjusting shelves_results template too for not allowing delete
when you do not have delete_public_lists (or higher). If you are the
owner, you are allowed to edit or delete. But transfer specifically
needs edit_public_lists (or higher).

Note: Removed a few useless POD lines to make qa tools happy.

Test plan:
Check if you see the transfer button on public lists only when having
permission edit_public_lists (or higher). And never on private lists.

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

Created attachment 136009 [details] [review]
Bug 25498: Add a transfer form for public lists

Test plan:
[1] Valid transfer:
    Log in with staff patron C having edit_public_lists.
    Pick a public list. Choose Transfer. Select a patron.
    Submit and check if transfer was successful.
[2] Permission problem:
    Pick a staff user D. Temporary enable edit_public_lists.
    Add another private tab. Login with D.
    Pick a public list. Transfer. Choose another patron. Wait.
    Switch tab: remove added permission from D.
    Switch tab again: Submit transfer form. Error? Close tab.
[3] Bonus test - Shelf disappeared:
    Pick a public list. Transfer. Choose another patron. Wait.
    Open another tab. Delete the selected public list.
    Close tab. Submit the transfer. Not exist error?
[4] Bonus test - Patron not found:
    Pick a public list. Transfer. Choose another patron. Wait.
    Open another tab. Delete selected patron.
    Close tab. Submit transfer. Patron not found error?

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

Created attachment 136010 [details] [review]
Bug 25498: Show transfer button for shared lists on OPAC

Test plan:
Pick or create a shared list.
Verify that OPAC only shows the Transfer button when you are the
owner of a shared list.

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

Created attachment 136011 [details] [review]
Bug 25498: Transfer ownership shared list on OPAC

Test plan:
Pick or create a shared list. Login as owner.
Transfer to one of the other members.
Verify that you do no longer have Transfer, but see Remove share.
Login as the other member. Transfer it back.

Bonus: Working with two tabs, you can open the Transfer form and
in the meantime delete the list, the patron or remove the share.
Or change permissions. Submit after that and check the error message.

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

Current stand:
[1] Allow transfer ownership of public list on staff interface. Staff member with permission can transfer to any user.
[2] Allow the owner of a shared list to transfer ownership to another member of the list (currently only via OPAC form).
[3] Private lists should not be transferred.

(In reply to Marcel de Rooy from comment #22)
> Does anyone already have some thoughts about the sensitivity of this OPAC
> operation? 
Not yet. But this implementation only copies a few borrowernumbers (not directly visible) and email addresses to the new user combo. The owner is imo entitled to that information; we can assume that he already knows them in general too. Other patron data is not revealed.

QA team:

 FAIL   Koha/Virtualshelfshare.pm
   FAIL   valid
                Constant subroutine Koha::Virtualshelfshare::SHARE_INVITATION_EXPIRY_DAYS redefined

This is a false negative. Nothing wrong with that constant. It wasnt changed either.

Why does this report not make it possible to change the owner of a shared list on staff?
In short, share a list is an OPAC feature.
Note that I opened bug 30953 to add an intranet counterpart. It could be addressed there.

Also opened bug 30955 for an additional notice for a new owner of a shared list.

Created attachment 136033 [details] [review]
Bug 25498: Add transfer button on intranet

Note: Adjusting shelves_results template too for not allowing delete
when you do not have delete_public_lists (or higher). If you are the
owner, you are allowed to edit or delete. But transfer specifically
needs edit_public_lists (or higher).

Note: Removed a few useless POD lines to make qa tools happy.

Test plan:
Check if you see the transfer button on public lists only when having
permission edit_public_lists (or higher). And never on private lists.

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 136034 [details] [review]
Bug 25498: Add a transfer form for public lists

Test plan:
[1] Valid transfer:
    Log in with staff patron C having edit_public_lists.
    Pick a public list. Choose Transfer. Select a patron.
    Submit and check if transfer was successful.
[2] Permission problem:
    Pick a staff user D. Temporary enable edit_public_lists.
    Add another private tab. Login with D.
    Pick a public list. Transfer. Choose another patron. Wait.
    Switch tab: remove added permission from D.
    Switch tab again: Submit transfer form. Error? Close tab.
[3] Bonus test - Shelf disappeared:
    Pick a public list. Transfer. Choose another patron. Wait.
    Open another tab. Delete the selected public list.
    Close tab. Submit the transfer. Not exist error?
[4] Bonus test - Patron not found:
    Pick a public list. Transfer. Choose another patron. Wait.
    Open another tab. Delete selected patron.
    Close tab. Submit transfer. Patron not found error?

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 136035 [details] [review]
Bug 25498: Show transfer button for shared lists on OPAC

Test plan:
Pick or create a shared list.
Verify that OPAC only shows the Transfer button when you are the
owner of a shared list.

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 136036 [details] [review]
Bug 25498: Transfer ownership shared list on OPAC

Test plan:
Pick or create a shared list. Login as owner.
Transfer to one of the other members.
Verify that you do no longer have Transfer, but see Remove share.
Login as the other member. Transfer it back.

Bonus: Working with two tabs, you can open the Transfer form and
in the meantime delete the list, the patron or remove the share.
Or change permissions. Submit after that and check the error message.

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

This is working well, but I think we ought to wait and rework the autocomplete parts of it after Bug 30578 makes progress.

(In reply to Owen Leonard from comment #36)
> This is working well, but I think we ought to wait and rework the
> autocomplete parts of it after Bug 30578 makes progress.

Thanks Owen.
It should not be that hard to add a tiny follow-up changing the ysearch call to REST API at either side. Without already committing ourselves to dependency order now?

Created attachment 136166 [details] [review]
Bug 25498: Add transfer button on intranet

Note: Adjusting shelves_results template too for not allowing delete
when you do not have delete_public_lists (or higher). If you are the
owner, you are allowed to edit or delete. But transfer specifically
needs edit_public_lists (or higher).

Note: Removed a few useless POD lines to make qa tools happy.

Test plan:
Check if you see the transfer button on public lists only when having
permission edit_public_lists (or higher). And never on private lists.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 136167 [details] [review]
Bug 25498: Add a transfer form for public lists

Test plan:
[1] Valid transfer:
    Log in with staff patron C having edit_public_lists.
    Pick a public list. Choose Transfer. Select a patron.
    Submit and check if transfer was successful.
[2] Permission problem:
    Pick a staff user D. Temporary enable edit_public_lists.
    Add another private tab. Login with D.
    Pick a public list. Transfer. Choose another patron. Wait.
    Switch tab: remove added permission from D.
    Switch tab again: Submit transfer form. Error? Close tab.
[3] Bonus test - Shelf disappeared:
    Pick a public list. Transfer. Choose another patron. Wait.
    Open another tab. Delete the selected public list.
    Close tab. Submit the transfer. Not exist error?
[4] Bonus test - Patron not found:
    Pick a public list. Transfer. Choose another patron. Wait.
    Open another tab. Delete selected patron.
    Close tab. Submit transfer. Patron not found error?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 136168 [details] [review]
Bug 25498: Show transfer button for shared lists on OPAC

Test plan:
Pick or create a shared list.
Verify that OPAC only shows the Transfer button when you are the
owner of a shared list.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 136169 [details] [review]
Bug 25498: Transfer ownership shared list on OPAC

Test plan:
Pick or create a shared list. Login as owner.
Transfer to one of the other members.
Verify that you do no longer have Transfer, but see Remove share.
Login as the other member. Transfer it back.

Bonus: Working with two tabs, you can open the Transfer form and
in the meantime delete the list, the patron or remove the share.
Or change permissions. Submit after that and check the error message.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

This works well, tests pass and QA script is happy.

Minor wonderment, should the permissions check be moved up in the the module level rather than being in the controllers.. so we can have it unit tested consistently?

Otherwise, this is all good and gets my PQA stamp, great work Marcel

Isn't Koha::Virtualshelfshare->patron confusing?

(In reply to Martin Renvoize from comment #42)
> This works well, tests pass and QA script is happy.
> 
> Minor wonderment, should the permissions check be moved up in the the module
> level rather than being in the controllers.. so we can have it unit tested
> consistently?
> 
> Otherwise, this is all good and gets my PQA stamp, great work Marcel

Thanks Martin

(In reply to Jonathan Druart from comment #43)
> Isn't Koha::Virtualshelfshare->patron confusing?

Hmm Good question. Do you have a suggestion already?

(In reply to Martin Renvoize from comment #42)
> Minor wonderment, should the permissions check be moved up in the the module
> level rather than being in the controllers.. so we can have it unit tested
> consistently?

Could do that. Will move it to the next report. 30933

(In reply to Marcel de Rooy from comment #45)
> (In reply to Jonathan Druart from comment #43)
> > Isn't Koha::Virtualshelfshare->patron confusing?
> 
> Hmm Good question. Do you have a suggestion already?

Not really, "contributor" maybe?

Do they contribute or just get access to view?

'Recipient' or 'Beneficiary'


or 'Sharer' and 'Sharee'.. those feel funky but might actually be the clearest two quasi-terms.?

> 
> or 'Sharer' and 'Sharee'.. those feel funky but might actually be the
> clearest two quasi-terms.?

I like those best from the suggested. user I guess is too general in context here?

(In reply to Katrin Fischer from comment #49)
> > 
> > or 'Sharer' and 'Sharee'.. those feel funky but might actually be the
> > clearest two quasi-terms.?
> 
> I like those best from the suggested. user I guess is too general in context
> here?

Thx.
Lets go with Sharee here, since this patron received the share and is not the owner who shared (which would be the sharer).

Altough I must admit that $share->sharee is a bit funky indeed :)

Created attachment 136221 [details] [review]
Bug 25498: (QA follow-up) Rename virtualshelfshare->patron

This patron is a 'sharee' (not the sharer).

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

Created attachment 137862 [details] [review]
Bug 25498: Add transfer button on intranet

Note: Adjusting shelves_results template too for not allowing delete
when you do not have delete_public_lists (or higher). If you are the
owner, you are allowed to edit or delete. But transfer specifically
needs edit_public_lists (or higher).

Note: Removed a few useless POD lines to make qa tools happy.

Test plan:
Check if you see the transfer button on public lists only when having
permission edit_public_lists (or higher). And never on private lists.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 137863 [details] [review]
Bug 25498: Add a transfer form for public lists

Test plan:
[1] Valid transfer:
    Log in with staff patron C having edit_public_lists.
    Pick a public list. Choose Transfer. Select a patron.
    Submit and check if transfer was successful.
[2] Permission problem:
    Pick a staff user D. Temporary enable edit_public_lists.
    Add another private tab. Login with D.
    Pick a public list. Transfer. Choose another patron. Wait.
    Switch tab: remove added permission from D.
    Switch tab again: Submit transfer form. Error? Close tab.
[3] Bonus test - Shelf disappeared:
    Pick a public list. Transfer. Choose another patron. Wait.
    Open another tab. Delete the selected public list.
    Close tab. Submit the transfer. Not exist error?
[4] Bonus test - Patron not found:
    Pick a public list. Transfer. Choose another patron. Wait.
    Open another tab. Delete selected patron.
    Close tab. Submit transfer. Patron not found error?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 137864 [details] [review]
Bug 25498: Show transfer button for shared lists on OPAC

Test plan:
Pick or create a shared list.
Verify that OPAC only shows the Transfer button when you are the
owner of a shared list.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 137865 [details] [review]
Bug 25498: Transfer ownership shared list on OPAC

Test plan:
Pick or create a shared list. Login as owner.
Transfer to one of the other members.
Verify that you do no longer have Transfer, but see Remove share.
Login as the other member. Transfer it back.

Bonus: Working with two tabs, you can open the Transfer form and
in the meantime delete the list, the patron or remove the share.
Or change permissions. Submit after that and check the error message.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 137866 [details] [review]
Bug 25498: (QA follow-up) Rename virtualshelfshare->patron

This patron is a 'sharee' (not the sharer).

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

Created attachment 137867 [details] [review]
Bug 25498: (follow-up) Change for autocomplete change of 30578

Test plan:
Check if the patron search still works on the intranet shelves form
when transferring ownership.

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

Created attachment 137871 [details] [review]
Bug 25498: (follow-up) Change for autocomplete change of 30578

Test plan:
Check if the patron search still works on the intranet shelves form
when transferring ownership.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Just wanted to test the last patch. All good.

A quick remark when testing: I think we need the "transfer" action available from the show list view (/virtualshelves/shelves.pl?op=view&shelfnumber=1). Not considering blocker however.

Created attachment 137908 [details] [review]
Bug 25498: (follow-up) Show error message when no sharee has email

Test plan:
Share a list with a patron that has no email address.
Try to transfer ownership. Verify that you get the error message.

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

Pushed to master for 22.11.

Nice work everyone, thanks!

Enhancement will not be backported to 22.05.x series

Added to manual https://gitlab.com/koha-community/koha-manual/-/merge_requests/658