Bug 26346

Having an additional option so that all users could see the list but only library staff could add or remove items would be WONDERFUL!  Our Youth Services department would be most grateful.

This would be fantastic! When a staff member leaves we have to recreate their lists so we can update the list as needed.

This I believe is exactly what I am looking for.... A member of our public services team created a list and currently, I ( in tech services) am unable when new titles are added to edit this list.  I would like this to be linked to not only staff but the staff in my organization as we are in a multi-library group.  Several libraries have created a local list of their new selections of DVDs, etc. and we would want to make sure that permission to change the list was limited to the institution who created the lists -- further modified by those on our staff that have the permission to add, edit, or delete lists.

Thanks Alex, this is a real improvement! I followed the test plan and everything worked as you said it would.

Option to Remove items from list?
-----------------------
This patch allows other library staff to add items to the list but it doesn't give them the ability to remove items. Is this functionality you are planning to add as a subsequent patch or do you see that as a separate bug? Without the ability to remove items, the interface is slightly confusing because both the OPAC and staff interface still give library staff the option to remove items - but if they try then there's an error, "no record was removed".


Update Edit list hint message
-----------------------
On the staff interface when creating or editing a list, if you select "Allow changes to contents from: Staff Only" while the category is still private, you get this hint: "The Anyone permission has no actual effect while this list is strictly private." Can you tweak this to reflect the new staff permission options?

No staff only option in OPAC
---------------------
The staff only option doesn't appear in the OPAC, so if the list owner edits a list from the OPAC - e.g. to edit the name - the list permissions will change. I don't know if this is easily fixable because you wouldn't want the staff only option appearing for all users but I thought I'd mention it.

We would use this 100% of the time. Most of our 'public' lists are only public to share with staff and work on collaboratively.

(In reply to Henry Bolshaw from comment #5)
> Thanks Alex, this is a real improvement! I followed the test plan and
> everything worked as you said it would.
> 
Thanks Henry! Apologies for the delay getting back to you I've been moving house.

I've attached patches addressing two of your points below, and a third patch refactoring the code - creating a new subroutine in Koha/Patron.pm to remove code duplication.
>
> Option to Remove items from list?
> -----------------------
> This patch allows other library staff to add items to the list but it
> doesn't give them the ability to remove items. Is this functionality you are
> planning to add as a subsequent patch or do you see that as a separate bug?
> Without the ability to remove items, the interface is slightly confusing
> because both the OPAC and staff interface still give library staff the
> option to remove items - but if they try then there's an error, "no record
> was removed".
> 
> 
This should have been added by one of the patches I have attached.
>
> Update Edit list hint message
> -----------------------
> On the staff interface when creating or editing a list, if you select "Allow
> changes to contents from: Staff Only" while the category is still private,
> you get this hint: "The Anyone permission has no actual effect while this
> list is strictly private." Can you tweak this to reflect the new staff
> permission options?
> 
This should also have been added in one of my new patches.
>
> No staff only option in OPAC
> ---------------------
> The staff only option doesn't appear in the OPAC, so if the list owner edits
> a list from the OPAC - e.g. to edit the name - the list permissions will
> change. I don't know if this is easily fixable because you wouldn't want the
> staff only option appearing for all users but I thought I'd mention it.
> 
I completely agree with you. I'm not 100% sure how to go about fixing this. Perhaps it could be a follow-up fix?

I am part way through another patch for this bug report adding unit test coverage, that will hopefully be finished sometime next week. So I'll keep the status as 'Assigned' for now.

Hi Henry, 

I've rebased all my commits into a single patch to reduce down to one test plan. I've also added unit test coverage in t/db_dependent/Virtualshelves.t and t/db_dependent/Koha/Patron.t

The fixes I mentioned in comment #10 are in my rebased patch. 

Could you please re-test now?

Many thanks,
Alex

This works great up until step 23 of your test plan. I'm not getting the "permission has no actual effect while this list is strictly private" message on the OPAC when I pick Staff or Anyone for a private list.

Hi Andrew and Henry, 

Thanks for testing Andrew. I found you weren't getting the "permission has no actual effect while this list is strictly private" message in the OPAC due to a JavaScript error. I think that should be fixed up in the patch I've just attached, 

Henry, I think I've also addressed your "No staff only option in OPAC" point (from  comment #5), and have added a step 25 to the test plan on my new patch to cover testing that.

Can you please re-test now?

Many thanks,
Alex

Thanks for the quick fix! I hated to fail it on something so small right at the end. This will be a really great change, I have librarians ask about it all the time!

(In reply to Andrew Fuerste-Henry from comment #18)
> Thanks for the quick fix! I hated to fail it on something so small right at
> the end. This will be a really great change, I have librarians ask about it
> all the time!

No problem at all Andrew. Thanks for testing!

> Henry, I think I've also addressed your "No staff only option in OPAC" point
> (from  comment #5), and have added a step 25 to the test plan on my new
> patch to cover testing that.
> 
> Can you please re-test now?
> 
> Many thanks,
> Alex

Yes, this all works well on the OPAC now. Thanks!

Hi Andrew and Henry, 

Thank you both for your testing of this enhancement so far!

We spotted a potential problem with the first patch: If your library has a public 'owner only' list and you want to switch it to being a 'staff only' list then you have to ask the list owner to make this switch.

If the list owner has left, or is unavailable, then it's not possible to make this switch. 

So I've just written a follow-up second patch. It adds a new patron sub-permission: 'edit_public_lists'. When enabled a patron can edit a public 'owner only' list, switching it to a 'staff only' list.

Could you please test my second patch (which has it's own test plan)?

Many thanks,
Alex

Sorry, just had to make a quick correction to the commit text of the second patch. The second patch is all ready for testing now :)

Hi Henry and Andrew, 

Just wondering if either of you have had time to re-test the patchset with the follow-up patch in place now?

Thanks,
Alex

Sorry Alex, I've not had a chance to look yet but I'll try and do some testing this week. The new functionality sound great though!

Thanks Henry and Andrew for your continued interest in this patchset, I really appreciate it!

Thanks for the test and signoff Andrew!

QAing

 FAIL   opac/opac-shelves.pl
   FAIL   critic
                # Variables::ProhibitConditionalDeclarations: Got 1 violation(s).

 FAIL   t/db_dependent/Virtualshelves.t
   OK     critic
   FAIL   forbidden patterns
                forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 384)
                forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 389)
                forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 394)
                forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 399)
                forbidden pattern: Do not assume male gender, use they/them instead (bug 18432) (line 404)

Anonymous OPAC user

Can't call method "userid" on an undefined value at /usr/share/koha/Koha/Virtualshelf.pm line 253

Surely, I do not like that :)

diff --git a/Koha/Schema/Result/Letter.pm b/Koha/Schema/Result/Letter.pm
index 613c4d9b27..b5627b9674 100644
--- a/Koha/Schema/Result/Letter.pm
+++ b/Koha/Schema/Result/Letter.pm
@@ -29,8 +29,6 @@ __PACKAGE__->table("letter");
   is_auto_increment: 1
   is_nullable: 0

-primary key identifier
-
 =head2 module

   data_type: 'varchar'
@@ -217,8 +215,8 @@ __PACKAGE__->belongs_to(
 );


-# Created by DBIx::Class::Schema::Loader v0.07046 @ 2021-02-11 12:33:50
-# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:qqdTVEicMu5rHppY5qsEuA
+# Created by DBIx::Class::Schema::Loader v0.07046 @ 2021-04-09 22:58:34
+# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:2QkUwMWgZ9WfOk9Ngf7yFA

Dont ever do these things in a regular patch !
Please move such changes to separate patches.
In this case I wonder what you are doing..

Line 190
 Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists )
Line 213
Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists

Wonder if we could have a crash here too?

@@ -267,4 +270,3 @@ sub _type {
     return 'Virtualshelve';
 }

-1;

Never never do this!

Virtualshelves

+        if ( Koha::Patrons->find( $borrowernumber )->can_patron_change_staff_only_lists ) {

Could this crash?

+++ b/Koha/Virtualshelves.pm

+use C4::Auth;
+

Out of the blue? Why are you adding it ??

Sorry but there is really too much to fix on this small patch set to get it further now.
Including serious errors like removing a true value, adding DBIx changes in a patch, using modules unrelated.

Please carefully adjust !

And still forgot:

The Staff only permission has no actual effect while this list is strictly private.
=> Should we not allow that change then ?


CHECK Handling 0 and NULL in column allow_change_from_staff
I saw in my table a NULL and a zero there. Probably existing/new rec.


This is a bit teasing: But I cannot add an item to a public list from another staff member. But I can edit the list, change the 'Allow', add or delete items, and undo Allow. Just noting.

Created attachment 122535 [details] [review]
Bug 26346: Database changes

- Adds allow_change_from_staff to virtualshelves table

Sponsored-By: Catalyst IT

Created attachment 122536 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT

(In reply to Marcel de Rooy from comment #40 and comment #41)

Hi Marcel,

Thank you for QAing!

- Comments #31, #32, #35, #36, #37, #38 (which was an accident- apologies!) should hopefully be addressed by the 'Bug 26346: (follow-up) Fixes in response to QA' patch.

- Comment #34 should hopefully have been addressed by my splitting the database change + DBIx change from the 'Bug 26346: Add option to make public lists editable by all staff' patch out into two separate patches.
Note: The change to Letter.pm you noted in comment #34 was also incorrectly committed and is not in the new 'Bug 26346: Updated schema file' patch.

Regarding comment #41:

1. I can certainly add that, however, my thoughts on this are: If a list is private I would have thought it was expected that other staff members couldn't see/edit/delete it. Thereby making it justified that the Staff only permission has not effect while the list is private.

What do you think?

2. Existing lists will have allow_change_from_staff = 0 due to that being the default value of the new column - https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122535&action=diff

The use of both 0 and NULL in allow_change_from_staff also happens in allow_change_from_others. This is what I observed when creating and adding items to lists:

* When creating a public 'anyone seeing this list' list allow_change_from_staff = NULL. When I add an item to the list then allow_change_from_staff = 0

* When creating a public 'staff only' list allow_change_from_others = NULL. When I add an item to the list then allow_change_from_others = 0

Similar behaviour exists if you test on master too:

* Create a public 'owner only' list allow_change_from_others = NULL. When I add an item to the list then allow_change_from_others = 0

3. Would you mind elaborating on if this is in the staff client and/or OPAC? I haven't been able to replicate so not sure if I'm doing something wrong.

Putting this in 'Needs signoff', apologies if that's the incorrect status for the bug report at this point!

Alex

Good to see these changes.
We still need some attention here:

sub get_some_shelves {
    my ( $self, $params ) = @_;
    my $borrowernumber = $params->{borrowernumber} || 0;
    my $category = $params->{category} || 1;
    my $add_allowed = $params->{add_allowed};

    my @conditions;
    my $patron = Koha::Patrons->find( $borrowernumber ) or return 0;
    if ( $add_allowed ) {

At this point we prevented a crash but now the side-effect is that we wont see any public lists at all when not logged in..

(In reply to Marcel de Rooy from comment #49)
> Good to see these changes.
> We still need some attention here:
> 
> sub get_some_shelves {
>     my ( $self, $params ) = @_;
>     my $borrowernumber = $params->{borrowernumber} || 0;
>     my $category = $params->{category} || 1;
>     my $add_allowed = $params->{add_allowed};
> 
>     my @conditions;
>     my $patron = Koha::Patrons->find( $borrowernumber ) or return 0;
>     if ( $add_allowed ) {
> 
> At this point we prevented a crash but now the side-effect is that we wont
> see any public lists at all when not logged in..

Thanks Marcel!

Working on this now.

Created attachment 122685 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create a public list and select the new 'Staff only' option under
'Allow changes to contents from'

5. Log out of the staff client and log back in as a different user with
the 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled.

6. Navigate to the Lists module, and click on the name of the list you
created in #5

7. Select 'Add items' and enter an item barcode and submit

8. Notice the item has been added to the list

9. Search the catalogue in the staff client. Confirm you can add to the
list you created in #5 from both the search result page and biblio
record detail page

10. Confirm you can remove items from the list in the OPAC and staff
client

11. Login to the OPAC as the same user as #5

12. Perform an OPAC search, Select 'Save to lists' under a biblio
record, choose the list from #4 in 'Select a list' and save

13. Confirm in the staff client that the record from #11 has been added to the list

14. Logout of OPAC and login again as a user which does not have 'Staff
access, allows viewing of catalogue in staff interface (catalogue)' or
superlibrarian enabled

15. Do a OPAC search, hit 'Save to lists' and notice you cannot add the
record to the list from #4

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

17. Login to the staff client as the same user as #5

18. Create a new list in the staff client, select 'Category'='Private',
    'Allow changes to contents from'='Staff only'

19. Notice 'The Staff only permission has no actual effect while this
list is strictly private' message hint is displayed

20. Change 'Category'='Public' and notice the hint is removed

21. Switch back to 'Category'='Private' and 'Allow changes to contents
from'='Anyone'

22. Notice the 'The Anyone permission has no actual effect while this
list is strictly private.' is displayed

23. Repeat steps 18-22 in the OPAC

24. Create a 'staff only' list in the OPAC and then login as a different
user with 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled. Confirm you are able to add items to
this new 'staff only' list.

25. Confirm that the 'Staff only' option is not available to
patrons with no special permissions when creating lists in the OPAC

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 122686 [details] [review]
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists

The first patch on bug 26346 only allowed owners of existing 'owner only'
public lists switch those lists to be 'Staff only'. This patch will allow other patrons with the 'edit_public_lists' sub-permission to also make this change.

Test plan:
1. Apply the first 26346 patch and follow it's test plan

2. Apply this patch

3. Apply database changes:
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

4. Restart memcached and plack

5. Login to the staff client. Create an 'owner only' public list

6. Create another patron record. Tick the following two permissions:

* "Staff access, allows viewing of catalogue in staff interface
 (catalogue) Required for staff login." permission

* Lists > "Edit public lists (edit_public_lists)" sub-permission

7. Logout of the staff client, and login as the user from step #6

8. Navigate to the Lists module, click 'Edit' on the 'owner only' list

9. Confirm you can change the 'Allow changes to contents from:' option
to 'Staff only'

10. Confirm you can now add items to this 'staff only' list

11. Login to the OPAC as the patron from step #5

12. Create a 'owner only' public list

13. Logout of OPAC, and login as the patron from step #6

14. Navigate to public lists in the OPAC and edit the OPAC created
'owner only' public list from step #

15. Confirm you can change the 'Allow changes to contents from:" option
to 'Staff only'

16. Confirm you can now add items to this 'staff only' list

17. Run Virtualshelves.t unit test:
sudo koha-shell <instance>
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Catalyst IT

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 122687 [details] [review]
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user

Seriously!
Can't call method "userid" on an undefined value at /usr/share/koha/Koha/Virtualshelf.pm line 253
Trivial fix.

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

Created attachment 122688 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49

Sponsored-By: Catalyst IT

Created attachment 122689 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49

Test plan for comment #49 fix:

1. Create a 'staff only' public list and 'Anyone seeing this list'
public list

2. Visit the OPAC. Do not login. Confirm you are able to see both lists
created in comment #1 in the OPAC

3. Perform a search, select a search result checkbox and
view the dropdown options in the 'Select titles to:' dropdown

3. Confirm both lists in step #1 are displayed in the dropdown

4. Try adding an item to the 'staff only' list. Observe you're prompted
to login

5. Login as a user with staff client permissions and confirm you can add
the item to the 'staff only' list

6. Logout, and repeat step 4. Login as a user with no permissionsi.
Confirm you cannot add an item to a 'staff only' list

7. Confirm both staff users and users with no permissions can add items to the 'Anyone seeing the list' public list in the OPAC

Sponsored-By: Catalyst IT

I haven't quite finished this yet. 
Have re-thought the get_some_shelves() change. Amended patch to come shortly.

Created attachment 122691 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client). Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the dropdown

Sponsored-By: Catalyst IT

(In reply to Marcel de Rooy from comment #49)

Hi Marcel, 

1. I've rebased the third patch 'Bug 26346: Add option to make public lists editable by all staff'. It conflicted when applied on master.

2. Have amended the sixth patch 'Bug 26346: (follow-up) Fixes in response to QA' to address comment #49 - along with a test plan for the comment #49 fix in case that's something you wanted.

- All types of public lists are now displayed in the 'Lists' dropdown in the OPAC header to both un-authenticated and authenticated users

- When an un-authenticated user (i.e. not logged in) selects an item and tries to see what lists they can add said item to then only "Anyone seeing this list" public lists show as available.

- 'Owner only' and staff only' lists are only displayed as lists which can be added to if the OPAC user is A) Logged in and B) Permitted to add to those lists.

This is essentially the same behaviour as happens on master with regards to 'owner only' public lists.

Ready for review.

Alex

sub can_be_deleted {
    my ( $self, $borrowernumber ) = @_;

    return 0 unless $borrowernumber;
    return 1 if $self->owner == $borrowernumber;

    my $patron = Koha::Patrons->find( $borrowernumber );

    return 1 if $self->is_public and haspermission( $patron->userid, { lists => 'delete_public_lists' } );

    return 0;
}

This is still different from surrounding code as to testing Patrons::find.

can_biblios_be_added

if $borrowernumber
      and ( ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) or ( $self->allow_change_from_staff && $patron->can_patron_change_staff_only_lists ) or $self->allow_change_from_others );

Isnt it actually strange that you could be the owner of a list, being a staff member, but without the staff_only_lists perm, and you cannot even add or remove one book to your own list ?

Note that as owner you still can manage or delete the whole list!
Do we need a more clear definition of what "staff only" actually means?
I would think more of an extension to staff users..

add_biblio

 my $patron = Koha::Patrons->find( $borrowernumber ) or return 0;
    return unless ( $self->owner == $borrowernumber && $self->allow_change_from_owner ) || ( $self->allow_change_from_staff && $patron->can_patron_change_staff_only_lists ) || $self->allow_change_from_others;

Why return 0 on the first line and return undef on the second?

(In reply to Alex Buckley from comment #58)
> - When an un-authenticated user (i.e. not logged in) selects an item and
> tries to see what lists they can add said item to then only "Anyone seeing
> this list" public lists show as available.

Anyone seeing this list is not really true.
Anonymous cant add or delete entries.

Hmm. I do fear that we still need another QA iteration step. Sorry for that.

(In reply to Marcel de Rooy from comment #63)
> Hmm. I do fear that we still need another QA iteration step. Sorry for that.

Thanks for your QA work Marcel. I will attempt to keep my responses fast, but will probably not be able to attach amended patches till the end of this week/early next week.

Created attachment 124010 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

* Fix addressing comment #59 - Making can_be_deleted() consistent to
testing Patrons::find

* Fix addressing comment comment #61 - Have made the first and second
lines consistent, both now return 0

Sponsord-By: Catalyst IT

Created attachment 124011 [details]
Public list with "Allow changes to contents from=Anyone seeing this list" shows to anonymous patron in Add to a list page

(In reply to Marcel de Rooy from comment #63)

Hi Marcel, 

* Follow-up patch attached containing fixes for comments #59 and #61. Could you please take a look?

* Regarding comment #60: I've been testing this today. 

When a 'staff only' list is created both the virtualshelves.allow_change_from_owner and virtualshelves.allow_change_from_staff values are set to 1 (see below):

| allow_change_from_owner | allow_change_from_others | allow_change_from_staff |
+-------------------------+--------------------------+-------------------------+
|                       1 |                        0 |                       1 |
+-------------------------+--------------------------+-------------------------+

So the first condition ($self->owner == $borrowernumber &&
> $self->allow_change_from_owner) is true when a staff member who owns a 'staff only' list  and doesn't have the staff_only_lists permission tries to add/delete an item from their list. i.e. they can add/remove an item in their cart.

Am I perhaps missing something?

* Regarding comment #62: When you refer to anonymous user do you mean logging in as the borrower set in the  AnonymousPatron  system preference?

When I log into the OPAC as borrower 53 (AnonymousPatron = 53), do a search, click 'Save to lists' under a biblio, then public lists set to "Allow changes to contents from=Anyone seeing this list" show in the "Select a list" dropdown (I've attached a screenshot showing 'public' displaying in the Add to a list page), I can then add the item to that list.

Thanks,
Alex

Will follow-up on this report after it has been signed off again.

Shouldn't we have a single 'allowed_to_change' column as an ENUM('owner','everyone','staff') ?

(In reply to Tomás Cohen Arazi from comment #69)
> Shouldn't we have a single 'allowed_to_change' column as an
> ENUM('owner','everyone','staff') ?

Hi Tomas, 

Yes, I can certainly implement that. 

Thanks for testing Caroline, I will also rebase the patches against master, and re-attach them to this bug report.

Created attachment 125436 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

* Fix addressing comment #59 - Making can_be_deleted() consistent to
testing Patrons::find

* Fix addressing comment comment #61 - Have made the first and second
lines consistent, both now return 0

Sponsord-By: Catalyst IT

Rebased the last patch against master. Whole patchset applies cleanly now. 

I'm working on adding a single 'allowed_to_change' column as suggested by Tomás.

I think the 'allowed_to_change' column should have four values ENUM('nobody', 'owner','everyone','staff')?

This is because 'Nobody' is an option available in the 'Allow changes to contents from' dropdown when creating a list.

(In reply to Tomás Cohen Arazi from comment #69)
> Shouldn't we have a single 'allowed_to_change' column as an
> ENUM('owner','everyone','staff') ?

Hi Tomás,

I've had more of a think about this, and discussed with Chris C and Aleisha. Here are our thoughts:

1. Introducing a single 'allowed_to_change' column should be done on a different bug report. This bug report is adding an enhancement. Introducing a single 'allowed_to_change' column would require existing permission logic for 'owners' and 'everyone' to be altered which is outside the scope of this enhancement. 

2. As we understand it currently a list can have a combination of permissions, hence why multiple columns were originally introduced. See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18228#c2

Furthermore in comment #67 I noted a 'staff only' list has both allow_change_from_owner=1 and allow_change_from_staff=1. 

Merging these separate columns into a single 'allowed_to_change' column would eliminate the granularity that's currently available, so we wonder if it's not the best solution.

All of that being the case, I'm marking this patchset as 'Needs signoff', as it's been rebased. Happy to hear your thoughts though!

Many thanks
Alex

(In reply to Alex Buckley from comment #73)
> (In reply to Tomás Cohen Arazi from comment #69)
> > Shouldn't we have a single 'allowed_to_change' column as an
> > ENUM('owner','everyone','staff') ?
> 
> Hi Tomás,
> 
> I've had more of a think about this, and discussed with Chris C and Aleisha.
> Here are our thoughts:
> 
> 1. Introducing a single 'allowed_to_change' column should be done on a
> different bug report. This bug report is adding an enhancement. Introducing
> a single 'allowed_to_change' column would require existing permission logic
> for 'owners' and 'everyone' to be altered which is outside the scope of this
> enhancement. 
> 
> 2. As we understand it currently a list can have a combination of
> permissions, hence why multiple columns were originally introduced. See
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18228#c2
> 
> Furthermore in comment #67 I noted a 'staff only' list has both
> allow_change_from_owner=1 and allow_change_from_staff=1. 
> 
> Merging these separate columns into a single 'allowed_to_change' column
> would eliminate the granularity that's currently available, so we wonder if
> it's not the best solution.
> 
> All of that being the case, I'm marking this patchset as 'Needs signoff', as
> it's been rebased. Happy to hear your thoughts though!

Sounds like a good review of the current status of the feature. And makes sense. I will poke at it tomorrow. My 'fear' is we could be adding some technical debt.
As I said, I'll be happy to review this.

Created attachment 125472 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49
8. Comment #59
9. Comment #61

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client).
Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the
dropdown

Sponsored-By: Catalyst IT

(In reply to Tomás Cohen Arazi from comment #74)

> Sounds like a good review of the current status of the feature. And makes
> sense. I will poke at it tomorrow. My 'fear' is we could be adding some
> technical debt.
> As I said, I'll be happy to review this.

Thanks Tomás. Let me know how you go with your investigation tomorrow.

I've just replaced the last patch on this bug report as I realised it was missing some earlier fixes.

Ready for testing again.

(In reply to Tomás Cohen Arazi from comment #74)
> Sounds like a good review of the current status of the feature. And makes
> sense. I will poke at it tomorrow. My 'fear' is we could be adding some
> technical debt.
> As I said, I'll be happy to review this.

Go ahead and sign off. I will resume QA after that.

*** Bug 28791 has been marked as a duplicate of this bug. ***

I waited too long to test it... I'm sorry Alex! :(

(In reply to Caroline Cyr La Rose from comment #79)
> I waited too long to test it... I'm sorry Alex! :(

Hi Caroline,

Not a problem! I've rebased and will re-attach the patches.

Alex

Created attachment 125914 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create a public list and select the new 'Staff only' option under
'Allow changes to contents from'

5. Log out of the staff client and log back in as a different user with
the 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled.

6. Navigate to the Lists module, and click on the name of the list you
created in #5

7. Select 'Add items' and enter an item barcode and submit

8. Notice the item has been added to the list

9. Search the catalogue in the staff client. Confirm you can add to the
list you created in #5 from both the search result page and biblio
record detail page

10. Confirm you can remove items from the list in the OPAC and staff
client

11. Login to the OPAC as the same user as #5

12. Perform an OPAC search, Select 'Save to lists' under a biblio
record, choose the list from #4 in 'Select a list' and save

13. Confirm in the staff client that the record from #11 has been added to the list

14. Logout of OPAC and login again as a user which does not have 'Staff
access, allows viewing of catalogue in staff interface (catalogue)' or
superlibrarian enabled

15. Do a OPAC search, hit 'Save to lists' and notice you cannot add the
record to the list from #4

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

17. Login to the staff client as the same user as #5

18. Create a new list in the staff client, select 'Category'='Private',
    'Allow changes to contents from'='Staff only'

19. Notice 'The Staff only permission has no actual effect while this
list is strictly private' message hint is displayed

20. Change 'Category'='Public' and notice the hint is removed

21. Switch back to 'Category'='Private' and 'Allow changes to contents
from'='Anyone'

22. Notice the 'The Anyone permission has no actual effect while this
list is strictly private.' is displayed

23. Repeat steps 18-22 in the OPAC

24. Create a 'staff only' list in the OPAC and then login as a different
user with 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled. Confirm you are able to add items to
this new 'staff only' list.

25. Confirm that the 'Staff only' option is not available to
patrons with no special permissions when creating lists in the OPAC

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 125915 [details] [review]
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists

The first patch on bug 26346 only allowed owners of existing 'owner only'
public lists switch those lists to be 'Staff only'. This patch will allow other patrons with the 'edit_public_lists' sub-permission to also make this change.

Test plan:
1. Apply the first 26346 patch and follow it's test plan

2. Apply this patch

3. Apply database changes:
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

4. Restart memcached and plack

5. Login to the staff client. Create an 'owner only' public list

6. Create another patron record. Tick the following two permissions:

* "Staff access, allows viewing of catalogue in staff interface
 (catalogue) Required for staff login." permission

* Lists > "Edit public lists (edit_public_lists)" sub-permission

7. Logout of the staff client, and login as the user from step #6

8. Navigate to the Lists module, click 'Edit' on the 'owner only' list

9. Confirm you can change the 'Allow changes to contents from:' option
to 'Staff only'

10. Confirm you can now add items to this 'staff only' list

11. Login to the OPAC as the patron from step #5

12. Create a 'owner only' public list

13. Logout of OPAC, and login as the patron from step #6

14. Navigate to public lists in the OPAC and edit the OPAC created
'owner only' public list from step #

15. Confirm you can change the 'Allow changes to contents from:" option
to 'Staff only'

16. Confirm you can now add items to this 'staff only' list

17. Run Virtualshelves.t unit test:
sudo koha-shell <instance>
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Catalyst IT

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 125916 [details] [review]
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user

Seriously!
Can't call method "userid" on an undefined value at /usr/share/koha/Koha/Virtualshelf.pm line 253
Trivial fix.

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

Created attachment 125917 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49
8. Comment #59
9. Comment #61

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client).
Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the
dropdown

Sponsored-By: Catalyst IT

(In reply to Alex Buckley from comment #80)
> (In reply to Caroline Cyr La Rose from comment #79)
> > I waited too long to test it... I'm sorry Alex! :(
> 
> Hi Caroline,
> 
> Not a problem! I've rebased and will re-attach the patches.
> 
> Alex

Ready to test again Caroline :)

Hi Alex,

Patches apply well! I'm having problems with the test plan, however.

I'm stuck at step 6 from Comment #81.

When I log in with a user who did not create the list (whatever their permissions, whatever the 'changes allowed from' value (owner only or staff only)), I can't see public lists.

Here's what I tried :

1. With superlibrarian, I created a public list set to 'owner only'
   --> Public lists appear OK
2. With staff account who has 'catalog' permission, I went to Lists > Public lists
   --> Public lists appear OK
3. I applied 3 first patches, updated db (I don't have plack/memcached installed)
4. With superlibrarian, I went to Lists > Public lists
   --> Public lists appear OK
5. With staff account who has 'catalog' permission, I went to Lists > Public lists
   --> 'Processing' appears and lists never load
6. With staff account who has 'catalog' permission, I created a public list set to 'owner only'
   --> Public lists still don't appear
7. With superlibrarian, I went to Lists > Public lists
   --> Public lists don't appear anymore for this user

I also tried by applying all patches and with a staff account who has 'lists' permission and with public lists with different 'changed allowed from' values. When patches are applied, public lists appear only to the owner, only if they are all created by that same owner. If there are public lists by multiple owners, nobody can see public lists.

Is this because I don't have plack/memcached? I have a git install on my computer that I constantly update (I don't use devbox or ktd).

Caroline

(In reply to Caroline Cyr La Rose from comment #86)
> Hi Alex,
> 
> Patches apply well! I'm having problems with the test plan, however.
> 
> I'm stuck at step 6 from Comment #81.
> 
> When I log in with a user who did not create the list (whatever their
> permissions, whatever the 'changes allowed from' value (owner only or staff
> only)), I can't see public lists.
> 
> Here's what I tried :
> 
> 1. With superlibrarian, I created a public list set to 'owner only'
>    --> Public lists appear OK
> 2. With staff account who has 'catalog' permission, I went to Lists > Public
> lists
>    --> Public lists appear OK
> 3. I applied 3 first patches, updated db (I don't have plack/memcached
> installed)
> 4. With superlibrarian, I went to Lists > Public lists
>    --> Public lists appear OK
> 5. With staff account who has 'catalog' permission, I went to Lists > Public
> lists
>    --> 'Processing' appears and lists never load
> 6. With staff account who has 'catalog' permission, I created a public list
> set to 'owner only'
>    --> Public lists still don't appear
> 7. With superlibrarian, I went to Lists > Public lists
>    --> Public lists don't appear anymore for this user
> 
> I also tried by applying all patches and with a staff account who has
> 'lists' permission and with public lists with different 'changed allowed
> from' values. When patches are applied, public lists appear only to the
> owner, only if they are all created by that same owner. If there are public
> lists by multiple owners, nobody can see public lists.
> 
> Is this because I don't have plack/memcached? I have a git install on my
> computer that I constantly update (I don't use devbox or ktd).
> 
> Caroline

Hi Caroline, 

Thank you for testing. I'll look into what might be happening here and get back to you.

Thanks!
Alex

(In reply to Caroline Cyr La Rose from comment #86)
> Hi Alex,
> 
> Patches apply well! I'm having problems with the test plan, however.
> 
> I'm stuck at step 6 from Comment #81.
> 
> When I log in with a user who did not create the list (whatever their
> permissions, whatever the 'changes allowed from' value (owner only or staff
> only)), I can't see public lists.
> 
> Here's what I tried :
> 
> 1. With superlibrarian, I created a public list set to 'owner only'
>    --> Public lists appear OK
> 2. With staff account who has 'catalog' permission, I went to Lists > Public
> lists
>    --> Public lists appear OK
> 3. I applied 3 first patches, updated db (I don't have plack/memcached
> installed)
> 4. With superlibrarian, I went to Lists > Public lists
>    --> Public lists appear OK
> 5. With staff account who has 'catalog' permission, I went to Lists > Public
> lists
>    --> 'Processing' appears and lists never load
> 6. With staff account who has 'catalog' permission, I created a public list
> set to 'owner only'
>    --> Public lists still don't appear
> 7. With superlibrarian, I went to Lists > Public lists
>    --> Public lists don't appear anymore for this user
> 
> I also tried by applying all patches and with a staff account who has
> 'lists' permission and with public lists with different 'changed allowed
> from' values. When patches are applied, public lists appear only to the
> owner, only if they are all created by that same owner. If there are public
> lists by multiple owners, nobody can see public lists.
> 
> Is this because I don't have plack/memcached? I have a git install on my
> computer that I constantly update (I don't use devbox or ktd).
> 
> Caroline

Hi Caroline, 

Thanks very much for testing. I've been trying to replicate (I use kohadevbox) but unfortunately I haven't been able to so I have a few questions:

Q1. Could you please let me know are the tests you did above in the staff client or the OPAC?

Q2. In step 5,6, and 7 if you right click, select 'Inspect element' then look in the 'Console' tab do you see any errors?

Q3. If you try applying the patches in a sandbox (https://wiki.koha-community.org/wiki/Sandboxes) do you also still encounter this problem?

Thanks
Alex

This patch would be very helpful to some of our customers.  But what would it take to extend the concept to private lists to be viewable by staff ?

There's Bug 19501 opened for that, but it seems to me this patch here is almost there already.

Created attachment 127556 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT

Created attachment 127557 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create a public list and select the new 'Staff only' option under
'Allow changes to contents from'

5. Log out of the staff client and log back in as a different user with
the 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled.

6. Navigate to the Lists module, and click on the name of the list you
created in #5

7. Select 'Add items' and enter an item barcode and submit

8. Notice the item has been added to the list

9. Search the catalogue in the staff client. Confirm you can add to the
list you created in #5 from both the search result page and biblio
record detail page

10. Confirm you can remove items from the list in the OPAC and staff
client

11. Login to the OPAC as the same user as #5

12. Perform an OPAC search, Select 'Save to lists' under a biblio
record, choose the list from #4 in 'Select a list' and save

13. Confirm in the staff client that the record from #11 has been added to the list

14. Logout of OPAC and login again as a user which does not have 'Staff
access, allows viewing of catalogue in staff interface (catalogue)' or
superlibrarian enabled

15. Do a OPAC search, hit 'Save to lists' and notice you cannot add the
record to the list from #4

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

17. Login to the staff client as the same user as #5

18. Create a new list in the staff client, select 'Category'='Private',
    'Allow changes to contents from'='Staff only'

19. Notice 'The Staff only permission has no actual effect while this
list is strictly private' message hint is displayed

20. Change 'Category'='Public' and notice the hint is removed

21. Switch back to 'Category'='Private' and 'Allow changes to contents
from'='Anyone'

22. Notice the 'The Anyone permission has no actual effect while this
list is strictly private.' is displayed

23. Repeat steps 18-22 in the OPAC

24. Create a 'staff only' list in the OPAC and then login as a different
user with 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled. Confirm you are able to add items to
this new 'staff only' list.

25. Confirm that the 'Staff only' option is not available to
patrons with no special permissions when creating lists in the OPAC

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 127558 [details] [review]
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists

The first patch on bug 26346 only allowed owners of existing 'owner only'
public lists switch those lists to be 'Staff only'. This patch will allow other patrons with the 'edit_public_lists' sub-permission to also make this change.

Test plan:
1. Apply the first 26346 patch and follow it's test plan

2. Apply this patch

3. Apply database changes:
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

4. Restart memcached and plack

5. Login to the staff client. Create an 'owner only' public list

6. Create another patron record. Tick the following two permissions:

* "Staff access, allows viewing of catalogue in staff interface
 (catalogue) Required for staff login." permission

* Lists > "Edit public lists (edit_public_lists)" sub-permission

7. Logout of the staff client, and login as the user from step #6

8. Navigate to the Lists module, click 'Edit' on the 'owner only' list

9. Confirm you can change the 'Allow changes to contents from:' option
to 'Staff only'

10. Confirm you can now add items to this 'staff only' list

11. Login to the OPAC as the patron from step #5

12. Create a 'owner only' public list

13. Logout of OPAC, and login as the patron from step #6

14. Navigate to public lists in the OPAC and edit the OPAC created
'owner only' public list from step #

15. Confirm you can change the 'Allow changes to contents from:" option
to 'Staff only'

16. Confirm you can now add items to this 'staff only' list

17. Run Virtualshelves.t unit test:
sudo koha-shell <instance>
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Catalyst IT

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 127559 [details] [review]
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user

Seriously!
Can't call method "userid" on an undefined value at /usr/share/koha/Koha/Virtualshelf.pm line 253
Trivial fix.

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

Created attachment 127560 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49
8. Comment #59
9. Comment #61

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client).
Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the
dropdown

Sponsored-By: Catalyst IT

Hi Caroline,

I've rebased this patchset against master. Would you mind testing this patchset in a sandbox (https://wiki.koha-community.org/wiki/Sandboxes) to see if you still encounter the problems in comment #86?

---

(In reply to Blou from comment #89)
> This patch would be very helpful to some of our customers.  But what would
> it take to extend the concept to private lists to be viewable by staff ?
> 
> There's Bug 19501 opened for that, but it seems to me this patch here is
> almost there already.

Hi Blou, 

Apologies for my slow response! My team has been flat out with client work recently.

I'll have a think about this and try to get back to you as soon as possible.

Thanks,
Alex

Created attachment 128017 [details] [review]
Bug 26346: Database changes

- Adds allow_change_from_staff to virtualshelves table

Sponsored-By: Catalyst IT

Created attachment 128018 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT

Created attachment 128019 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create a public list and select the new 'Staff only' option under
'Allow changes to contents from'

5. Log out of the staff client and log back in as a different user with
the 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled.

6. Navigate to the Lists module, and click on the name of the list you
created in #5

7. Select 'Add items' and enter an item barcode and submit

8. Notice the item has been added to the list

9. Search the catalogue in the staff client. Confirm you can add to the
list you created in #5 from both the search result page and biblio
record detail page

10. Confirm you can remove items from the list in the OPAC and staff
client

11. Login to the OPAC as the same user as #5

12. Perform an OPAC search, Select 'Save to lists' under a biblio
record, choose the list from #4 in 'Select a list' and save

13. Confirm in the staff client that the record from #11 has been added to the list

14. Logout of OPAC and login again as a user which does not have 'Staff
access, allows viewing of catalogue in staff interface (catalogue)' or
superlibrarian enabled

15. Do a OPAC search, hit 'Save to lists' and notice you cannot add the
record to the list from #4

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

17. Login to the staff client as the same user as #5

18. Create a new list in the staff client, select 'Category'='Private',
    'Allow changes to contents from'='Staff only'

19. Notice 'The Staff only permission has no actual effect while this
list is strictly private' message hint is displayed

20. Change 'Category'='Public' and notice the hint is removed

21. Switch back to 'Category'='Private' and 'Allow changes to contents
from'='Anyone'

22. Notice the 'The Anyone permission has no actual effect while this
list is strictly private.' is displayed

23. Repeat steps 18-22 in the OPAC

24. Create a 'staff only' list in the OPAC and then login as a different
user with 'Staff access, allows viewing of catalogue in staff interface
(catalogue)' permission enabled. Confirm you are able to add items to
this new 'staff only' list.

25. Confirm that the 'Staff only' option is not available to
patrons with no special permissions when creating lists in the OPAC

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 128020 [details] [review]
Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists

The first patch on bug 26346 only allowed owners of existing 'owner only'
public lists switch those lists to be 'Staff only'. This patch will allow other patrons with the 'edit_public_lists' sub-permission to also make this change.

Test plan:
1. Apply the first 26346 patch and follow it's test plan

2. Apply this patch

3. Apply database changes:
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

4. Restart memcached and plack

5. Login to the staff client. Create an 'owner only' public list

6. Create another patron record. Tick the following two permissions:

* "Staff access, allows viewing of catalogue in staff interface
 (catalogue) Required for staff login." permission

* Lists > "Edit public lists (edit_public_lists)" sub-permission

7. Logout of the staff client, and login as the user from step #6

8. Navigate to the Lists module, click 'Edit' on the 'owner only' list

9. Confirm you can change the 'Allow changes to contents from:' option
to 'Staff only'

10. Confirm you can now add items to this 'staff only' list

11. Login to the OPAC as the patron from step #5

12. Create a 'owner only' public list

13. Logout of OPAC, and login as the patron from step #6

14. Navigate to public lists in the OPAC and edit the OPAC created
'owner only' public list from step #

15. Confirm you can change the 'Allow changes to contents from:" option
to 'Staff only'

16. Confirm you can now add items to this 'staff only' list

17. Run Virtualshelves.t unit test:
sudo koha-shell <instance>
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Catalyst IT

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 128021 [details] [review]
Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user

Seriously!
Can't call method "userid" on an undefined value at /usr/share/koha/Koha/Virtualshelf.pm line 253
Trivial fix.

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

Created attachment 128022 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49
8. Comment #59
9. Comment #61

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client).
Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the
dropdown

Sponsored-By: Catalyst IT

Created attachment 128023 [details] [review]
Bug 26346: (follow-up) Fixes in response to QA

Fixes for the following comments on bug report 26346:

1. Comment #31
2. Comment #32
3. Comment #35
4. Comment #36
5. Comment #37
6. Comment #38
7. Comment #49
8. Comment #59
9. Comment #61

Test plan for comment #49 fix:

1. Log into the staff client as User A. Create one of each of the
following types of public lists:
- 'Owner only'
- 'staff only'
- 'Anyone seeing this list'

2. Visit the OPAC. Do not login. Confirm you are able to see all three
public lists under the 'Lists' dropdown in the OPAC header

3. Perform a search. Select a search result checkbox and view the
dropdown options in the 'Select titles to:' dropdown

4. Confirm only the 'Anyone seeing this list' is displayed as an option

5. Log into the OPAC as a user B (a user with no permissions). Confirm
you can see all three public lists under the header 'Lists' dropdown

6. Repeat steps 3, and 4 as User B with the same results as
un-authenticated user

6. Log into the OPAC as user C (a user with access to the staff client).
Confirm
you can see all three public lists under the header 'Lists' dropdown

7. Repeat steps 3, and 4 but this time you should see the 'staff only'
and 'Anyone seeing this list' public lists in the dropdown

8. Log into the OPAC as user A. Confirm you can see all three poublic
lists under the header 'Lists' dropdown

9. Repeat steps 3, and 4 but this time you should see the 'owner only',
'staff only' and 'Anyone seeing this list' public lists in the
dropdown

Sponsored-By: Catalyst IT

I've uploaded a fixed second patch on this bug report which was causing the t/db_dependent/Virtualshelves.t unit test to fail.

(In reply to Alex Buckley from comment #95)
> Hi Caroline,
> 
> I've rebased this patchset against master. Would you mind testing this
> patchset in a sandbox (https://wiki.koha-community.org/wiki/Sandboxes) to
> see if you still encounter the problems in comment #86?
> 
> ---
> 
> (In reply to Blou from comment #89)
> > This patch would be very helpful to some of our customers.  But what would
> > it take to extend the concept to private lists to be viewable by staff ?
> > 
> > There's Bug 19501 opened for that, but it seems to me this patch here is
> > almost there already.
> 
> Hi Blou, 
> 
> Apologies for my slow response! My team has been flat out with client work
> recently.
> 
> I'll have a think about this and try to get back to you as soon as possible.
> 
> Thanks,
> Alex

Hi Blou, 

Yes, extending this patchset to implement functionality requested in Bug 19501 would be straightforward except for the fact that the virtualshelves.category has been changed to a boolean virtualshelves.public field in bug 28959. 
Meaning adding a third option of 'Restricted', or 'Staff client only' in that field, would be regressing 28959.

However, the way I would approach the functionality you're after is:

* Change virtualshelves.public back to virtualshelves.category
* Add a new option to virtualshelves.category of 3 (Staff client only)
* Add two new patron lists sub-permissions:
- 'Delete staff client lists (delete_staff_client_lists)'
- 'Edit staff client lists (edit_staff_client_lists)'

When a 'staff client only' list is created it would be viewable by all logged-in users on the staff client. It would not be visible to anyone in the OPAC. 

Only users with the 'edit_staff_client_lists' sub-permission would be able to edit it. Similarly, it would only be deletable by users with the 'delete_staff_client_lists' sub-permission.

Thanks!
Alex

Hi all, 

I've rebased this patchset, added a missing change to kohastructure.sql, and merged the following 4 patches into 1 patch:

-  Bug 26346: Add option to make public lists editable by all staff
-  Bug 26346: Add edit_public_lists sub-permission enabling patrons to switch 'owner only' public lists (which they're not the owners of) to 'staff only' lists
-  Bug 26364: (QA follow-up) Do not crash on anonymous OPAC user
-  Bug 26346: (follow-up) Fixes in response to QA

I have also reworked the test plan to hopefully make it more succinct, and easier to follow.

Alex

Created attachment 128383 [details] [review]
Bug 26346: Database changes

- Adds allow_change_from_staff to virtualshelves table
- Add edit_public_lists sub-permission

Sponsored-By: Catalyst IT

Created attachment 128384 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT

Created attachment 128385 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Ready to test again!

Here is the result after I tried to apply the changes with `git bz apply 26346`:


Bug 26346 - Add option to make a public list editable by library staff only

128383 - Bug 26346: Database changes
128384 - Bug 26346: Updated schema file
128385 - Bug 26346: Add option to make public lists editable by all staff

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 26346: Database changes
Applying: Bug 26346: Updated schema file
Applying: Bug 26346: Add option to make public lists editable by all staff
Using index info to reconstruct a base tree...
M	Koha/Patron.pm
M	koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
M	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
M	opac/opac-shelves.pl
M	t/db_dependent/Koha/Patron.t
M	virtualshelves/shelves.pl
Falling back to patching base and 3-way merge...
Auto-merging virtualshelves/shelves.pl
Auto-merging t/db_dependent/Koha/Patron.t
CONFLICT (content): Merge conflict in t/db_dependent/Koha/Patron.t
Auto-merging opac/opac-shelves.pl
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
Auto-merging Koha/Patron.pm
CONFLICT (content): Merge conflict in Koha/Patron.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 26346: Add option to make public lists editable by all staff
hint: Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-26346-Add-option-to-make-public-lists-editable-HTuW5n.patch

(In reply to shi-yao.wang from comment #110)

Thanks, Shi-Yao I will rebase my patches against master and re-attach them to this ticket to re-test.

Thanks,
Alex

Created attachment 131699 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Created attachment 131700 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Rebased and ready to test again!

Patch doesn't apply.



Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 26346: Database changes
Applying: Bug 26346: Updated schema file
Applying: Bug 26346: Add option to make public lists editable by all staff
Using index info to reconstruct a base tree...
M	Koha/Patron.pm
M	koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
M	t/db_dependent/Koha/Patron.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Patron.t
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
Auto-merging Koha/Patron.pm
CONFLICT (content): Merge conflict in Koha/Patron.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 26346: Add option to make public lists editable by all staff

Created attachment 131757 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

I fixed the patch.

At step 6, when logged in the staff client as User B, if I go to Lists -> Public lists it loads the page forever so I cannot access the individual public list page.

Created attachment 131764 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Thanks for rebasing Blou! I've just attached the final patch again as the t/db_dependent/Koha/Patron.t unit test was failing. 

Shi-yao, I've gone through the test plan on my master branch with the patches applied, but unfortunately I am not encountering what you saw in step 6. 

Can you please paste what you see in the plack-error.log file?
tail -f /var/log/koha/<instancename>/plack-error.log

Blou or Andrew, could either of you please see if you're able to replicate what Shi-yao is seeing on step 6?

(In reply to Alex Buckley from comment #120)

> Blou or Andrew, could either of you please see if you're able to replicate
> what Shi-yao is seeing on step 6?

I can't reproduce that problem. The page loads correctly for me under those circumstances. However, I see a couple of complaints from the QA tool that should be checked.

I have no output from my plack file, but since nobody has this problem it should only be an issue on my own setup.

(In reply to Owen Leonard from comment #121)
> (In reply to Alex Buckley from comment #120)
> 
> > Blou or Andrew, could either of you please see if you're able to replicate
> > what Shi-yao is seeing on step 6?
> 
> I can't reproduce that problem. The page loads correctly for me under those
> circumstances. However, I see a couple of complaints from the QA tool that
> should be checked.

Hi Owen, 

Thanks very much for testing!

Could you please paste the output from your QA tool? 

When I run the QA tool the output I get looks fine (I think): https://paste.koha-community.org/8282

Thanks,
Alex

(In reply to Alex Buckley from comment #123)
> (In reply to Owen Leonard from comment #121)
> > (In reply to Alex Buckley from comment #120)
> > 
> > > Blou or Andrew, could either of you please see if you're able to replicate
> > > what Shi-yao is seeing on step 6?
> > 
> > I can't reproduce that problem. The page loads correctly for me under those
> > circumstances. However, I see a couple of complaints from the QA tool that
> > should be checked.
> 
> Hi Owen, 
> 
> Thanks very much for testing!
> 
> Could you please paste the output from your QA tool? 
> 
> When I run the QA tool the output I get looks fine (I think):
> https://paste.koha-community.org/8282
> 
> Thanks,
> Alex

Hi Marcel and all, 

Would you mind pasting the content of your QA test tool as when I run my tool I don't get any failures?

Many thanks,
Alex

(In reply to Alex Buckley from comment #124)
> (In reply to Alex Buckley from comment #123)
> > (In reply to Owen Leonard from comment #121)
> > > (In reply to Alex Buckley from comment #120)
> > > 
> > > > Blou or Andrew, could either of you please see if you're able to replicate
> > > > what Shi-yao is seeing on step 6?
> > > 
> > > I can't reproduce that problem. The page loads correctly for me under those
> > > circumstances. However, I see a couple of complaints from the QA tool that
> > > should be checked.
> > 
> > Hi Owen, 
> > 
> > Thanks very much for testing!
> > 
> > Could you please paste the output from your QA tool? 
> > 
> > When I run the QA tool the output I get looks fine (I think):
> > https://paste.koha-community.org/8282
> > 
> > Thanks,
> > Alex
> 
> Hi Marcel and all, 
> 
> Would you mind pasting the content of your QA test tool as when I run my
> tool I don't get any failures?
> 
> Many thanks,
> Alex

Correction: Sorry this should be: Hi Owen and all

Hi Alex, these are the results when I run the QA tools:

Processing files before patches
|========================>| 13 / 13 (100.00%)
Processing files after patches
|========================>| 13 / 13 (100.00%)

 OK	Koha/Patron.pm
 OK	Koha/Schema/Result/Virtualshelve.pm
 OK	Koha/Virtualshelf.pm
 OK	Koha/Virtualshelves.pm
 FAIL	installer/data/mysql/kohastructure.sql
   FAIL	  tinyint_has_boolean_flag
		The new column virtualshelves.public is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/Virtualshelve.pm), see the SQL12 coding guideline
		The new column virtualshelves.allow_change_from_staff is using TINYINT(1) as type but is not defined as boolean in the schema file (Koha/Schema/Result/Virtualshelve.pm), see the SQL12 coding guideline

 OK	koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
 OK	koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
 OK	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
 OK	opac/opac-addbybiblionumber.pl
 OK	opac/opac-shelves.pl
 OK	t/db_dependent/Koha/Patron.t
 FAIL	t/db_dependent/Virtualshelves.t
   FAIL	  spelling
		 thier ==> their

 OK	virtualshelves/shelves.pl

Hi Katrin, 

Thank you very much, I'll get onto fixing this right away!

Thanks,
Alex

Created attachment 132373 [details] [review]
Bug 26346: (follow-up) Fixing for QA tool

Adding is_boolean to virtualshelves.public and
virtualshelves.allow_change_from_staff in Virtualshelve.pm DBIC schema file.

Fixing typo in Virtualshelves.t

Sponsored-by: Catalyst IT, New Zealand

(In reply to Katrin Fischer from comment #126)
> Hi Alex, these are the results when I run the QA tools:
> 
> Processing files before patches
> |========================>| 13 / 13 (100.00%)
> Processing files after patches
> |========================>| 13 / 13 (100.00%)
> 
>  OK	Koha/Patron.pm
>  OK	Koha/Schema/Result/Virtualshelve.pm
>  OK	Koha/Virtualshelf.pm
>  OK	Koha/Virtualshelves.pm
>  FAIL	installer/data/mysql/kohastructure.sql
>    FAIL	  tinyint_has_boolean_flag
> 		The new column virtualshelves.public is using TINYINT(1) as type but is
> not defined as boolean in the schema file
> (Koha/Schema/Result/Virtualshelve.pm), see the SQL12 coding guideline
> 		The new column virtualshelves.allow_change_from_staff is using TINYINT(1)
> as type but is not defined as boolean in the schema file
> (Koha/Schema/Result/Virtualshelve.pm), see the SQL12 coding guideline
> 
>  OK	koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
>  OK	koha-tmpl/intranet-tmpl/prog/en/modules/virtualshelves/shelves.tt
>  OK	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt
>  OK	opac/opac-addbybiblionumber.pl
>  OK	opac/opac-shelves.pl
>  OK	t/db_dependent/Koha/Patron.t
>  FAIL	t/db_dependent/Virtualshelves.t
>    FAIL	  spelling
> 		 thier ==> their
> 
>  OK	virtualshelves/shelves.pl

Hi Katrin, 

Could you please re-run the QA test tool now after applying the fourth patch on this bug report? 
Hopefully, that will fix up those QA errors. 

Many thanks,
Alex

Created attachment 132475 [details] [review]
Bug 26346: Database changes

- Adds allow_change_from_staff to virtualshelves table
- Add edit_public_lists sub-permission

Sponsored-By: Catalyst IT

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Created attachment 132476 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Created attachment 132477 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Created attachment 132478 [details] [review]
Bug 26346: (follow-up) Fixing for QA tool

Adding is_boolean to virtualshelves.public and
virtualshelves.allow_change_from_staff in Virtualshelve.pm DBIC schema file.

Fixing typo in Virtualshelves.t

Sponsored-by: Catalyst IT, New Zealand

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Created attachment 132479 [details] [review]
Bug 26346: (follow-up) Fixing for QA tool

Adding is_boolean to virtualshelves.public and
virtualshelves.allow_change_from_staff in Virtualshelve.pm DBIC schema file.

Fixing typo in Virtualshelves.t

Sponsored-by: Catalyst IT, New Zealand

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

This all seems to work great! Followed the test plan and everything worked as described. 

Tests passed and QA script seems to be happy now! This is a great addition.

(In reply to Lucas Gass from comment #135)
> This all seems to work great! Followed the test plan and everything worked
> as described. 
> 
> Tests passed and QA script seems to be happy now! This is a great addition.

Thank you Lucas!

Created attachment 132751 [details] [review]
Bug 26346: Database changes

- Adds allow_change_from_staff to virtualshelves table
- Add edit_public_lists sub-permission

Sponsored-By: Catalyst IT

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 132752 [details] [review]
Bug 26346: Updated schema file

Sponsored-By: Catalyst IT

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 132753 [details] [review]
Bug 26346: Add option to make public lists editable by all staff

If a staff member has access to the staff client (either because
'catalogue' permission is enabled or they're a superlibrarian
then that user can add items (from OPAC or staff client) to a list
marked 'Staff only'

Test plan:
1. In the staff client go to: Lists > 'New list'. Notice under 'Allow changes to contents
from' there are three options: Nobody, Owner only, Anyone seeing this
list

2. Apply first 3 patches and run updatedatabase.pl
cd installer/data/mysql
sudo koha-shell <instance>
./updatedatabase.pl

3. Restart memcached and plack

4. Create 4 patron accounts:
- User A : Superlibrarian permissions
- User B : 'Staff access, allows viewing of catalogue in staff interface
(catalogue)'
- User C : No permissions
- User D : 'Staff access, allows viewing of catalogue in staff
interface' and 'Lists' > Edit public lists (edit_public_lists)' sub-permission

5. Login to staff client as User A.
Create a public list and select the new 'Staff only' option under 'Allow changes to contents from'

6. Log into the staff client as User B.
Confirm you can add items to the list from the following staff client pages:
- Individual list page using the 'Add items' button
- Staff client search result page
- Staff client biblio detail page

7. Confirm you can remove items from the list

8. Confirm you can perform an OPAC search when not logged in

9. Log into the OPAC as User B. Confirm you can add items to the list
from the following OPAC pages:
- OPAC search result page
- OPAC biblio detail page

10. Log into the OPAC as User C. Do an OPAC search and confirm you
can view the list, but not add items to it

11. Login to the staff client as User B. Create a new list with the
following settings:
- 'Category'='Private',
- 'Allow changes to contents from'='Staff only'

Notice a red hint message is displayed.
Change 'Category'='Public' and notice the hint is removed

12. Log into the OPAC as User C. Notice the 'Staff only' option is not
available when creating a list

13. Log into the OPAC as User B. Repeat step 11. Confirm the same
outcome

14. Log into the staff client as User A. Create a list with the
following settings:

- Public = 'Public'
- Allow changes to contents from = 'owner only'

15. Log into the staff client as User D. Edit the list from step 14
confirm you can edit the list to have 'Allow changes to contents from' =
'Staff only'

16. Run Patron.t and Virtualshelves.t unit tests:
sudo koha-shell <instance>
prove t/db_dependent/Koha/Patron.t
prove t/db_dependent/Virtualshelves.t

Sponsored-by: Horowhenua District Council, New Zealand

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 132754 [details] [review]
Bug 26346: (follow-up) Fixing for QA tool

Adding is_boolean to virtualshelves.public and
virtualshelves.allow_change_from_staff in Virtualshelve.pm DBIC schema file.

Fixing typo in Virtualshelves.t

Sponsored-by: Catalyst IT, New Zealand

Signed-off-by: Lucas Gass <lucas@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 132755 [details] [review]
Bug 26346: (QA follow-up) Clarify permission

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Tested, this works as described and passes tests, great work Alex.

I admit to being confused at first on what the permission did, so I added a follow-up to clarify, let me know if it makes sense.

I also filed bug 30418 for the functionality I thought the permission added.

(In reply to Nick Clemens from comment #142)
> Tested, this works as described and passes tests, great work Alex.
> 
> I admit to being confused at first on what the permission did, so I added a
> follow-up to clarify, let me know if it makes sense.
> 
> I also filed bug 30418 for the functionality I thought the permission added.

Hi Nick, 

Yay, thanks so much! 

Yes, I think your follow-up clarifies the edit_public_lists permission nicely.

Many thanks,
Alex

Pushed to master for 22.05, thanks to everybody involved ﷐[U+1F984]﷑

Not sure if it is coming from here, but it should be the first report to blame :)

If I am adding an entry to a new list on OPAC, and I select Public list, it adds the entry to a private list.

This is rather confusing?

Opened bug 30925

(In reply to Marcel de Rooy from comment #145)
> Not sure if it is coming from here, but it should be the first report to
> blame :)

Might be bug 28959 btw