Created attachment 131090 [details] [review]
[SOLVED] Every librarian can edit every item with  IndependentBranches on

Test plan:
=========
1. Have (at least) two branches defined.
2. Have IndependentBranches set.
3. Have a biblio record with items belonging to different branches.
4. Be a librarian without superlibrarian rights.
5. Go to Edit -> Edit items view (cataloguing/additems.pl).
6. You will be able to edit every item, also ones not from the branch
   you are from.
7. Apply the patch.
8. Repeat 5.
9. You should be able to edit only the items from your branch.

Thanks for this patch.

You also need to fix :
koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc

Please have a look at how we write commit/patch messages :
https://wiki.koha-community.org/wiki/Commit_messages

Created attachment 131123 [details] [review]
Bug 30178: (bugs 27526 and 28445 follow-up) Every librarian can edit every item with IndependentBranches on

Additionally, problems caused by koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc are resolved.  

Fridolin, thank you for pointing that out.

This changes sub GetLoggedInBranchcode to GetLoggedInBranchname, but doesn't this mean that GetLoggedInBranchcode is wrong?

But this doesn't feel right as we compare to item.homebranch, which should be the code? Something here is for sure confusing.

Katrin, you are right, it is confusing.  I wish it was a branch code, but it is not--it is the name.  Have a look at columns_to_str here (cataloguing/additem.pl):

my @items;
for my $item ( $biblio->items->as_list, $biblio->host_items->as_list ) {
    push @items, $item->columns_to_str;
}

It took me some time to figure this out.  The values of item.homebranch are then put directly into the table for users, so--I guess--it has been decided to pass this like this...

(In reply to Janusz Kaczmarek from comment #5)
> Katrin, you are right, it is confusing.  I wish it was a branch code, but it
> is not--it is the name.  Have a look at columns_to_str here
> (cataloguing/additem.pl):
> 
> my @items;
> for my $item ( $biblio->items->as_list, $biblio->host_items->as_list ) {
>     push @items, $item->columns_to_str;
> }
> 
> It took me some time to figure this out.  The values of item.homebranch are
> then put directly into the table for users, so--I guess--it has been decided
> to pass this like this...

We just arrived at the same spot :) I first confirmed it by adding the line to the templates:

[% item.homebranch %] | [% Branches.GetLoggedInBranchname() %]

So the fix is correct, but I feel we might want to revisit this confusing variable at some point. Changing code to name is an easy template task, so passing it like this appears not necessary.

And great commit message now btw!

One could add for testing a line like this:

[% Koha.Preference('IndependentBranches') %] // [% logged_in_user.is_superlibrarian %] // [% item.homebranch %] // [% Branches.GetLoggedInBranchname() ) %] // [% Branches.GetLoggedInBranchcode() ) %] </br>

next to "SET can_be_edited" to see the actual values on the web page...

Created attachment 131137 [details] [review]
Bug 30178: (bugs 27526 and 28445 follow-up) Every librarian can edit every item with IndependentBranches on

Problem arises after applying bugfix 27526 and 28445: with
IndependentBranches on, a librarian without superlibrarian privileges,
can edit (and potentially delete) every item (i.e. also from a foreign
branch).  This is because can_be_edited calculation is buggy (in two spots).

Test plan:
1. Have (at least) two branches defined.
2. Have IndependentBranches set.
3. Have a biblio record with items belonging to different branches.
4. Be a librarian without superlibrarian rights, with editcatalogue
   and tool permissions set.

Scenario A (Edit items):
1. Go to Edit -> Edit items view (cataloguing/additems.pl).
2. You will be able to edit every item, also ones not from the branch
   you are from (cf. the button 'Actions').
3. Apply the patch.
4. Repeat 1.
5. You should be able to edit only the items from your branch.

Scenario B (Edit items in batch):
1. From Normal view go to Edit -> Edit items in batch.
2. You will be able to batch edit every item, also ones not from the branch
   you are from.
3. Apply the patch.
4. Repeat 1.
5. You should be able to edit only the items from your branch (and
   see 'Cannot edit' for others.

Scenario C (Delete items in batch):
1. From Normal view go to Edit -> Delete items in batch.
2. You will not see the string 'Cannot delete' and only by chance
   will not be able to activate the checkboxes next to foreign items.
3. Apply the patch.
4. Repeat 1.
5. You should be able to delete only the items from your branch (and
   see 'Cannot delete' for others.

Scenario D and E:
Analogous steps can be executed from Tools -> Batch item modification
and Tools -> Batch item deletion

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

> We just arrived at the same spot :) I first confirmed it by adding the line
> to the templates:
> 
> [% item.homebranch %] | [% Branches.GetLoggedInBranchname() %]
> 
> So the fix is correct, but I feel we might want to revisit this confusing
> variable at some point. Changing code to name is an easy template task, so
> passing it like this appears not necessary.

The more that the use of GetLoggedInBranchname creates a possible security issue in a malignant case of two different branches with the same name.

Maybe we should create a Koha::Patron method can_edit_item(), like can_log_into()

(In reply to Fridolin Somers from comment #11)
> Maybe we should create a Koha::Patron method can_edit_item(), like
> can_log_into()

I had similar thoughts - but I think we could treat this as an urgent bug fix maybe and I'll file another bug for centralizing the code? I believe it would make sense, but might create a bigger/more conflict-prone patch set if done right.

Sure, another bug report for long-term fix is good.

Created attachment 131182 [details] [review]
Bug 30178: (bugs 27526 and 28445 follow-up) Every librarian can edit every item with IndependentBranches on

Problem arises after applying bugfix 27526 and 28445: with
IndependentBranches on, a librarian without superlibrarian privileges,
can edit (and potentially delete) every item (i.e. also from a foreign
branch).  This is because can_be_edited calculation is buggy (in two spots).

Test plan:
1. Have (at least) two branches defined.
2. Have IndependentBranches set.
3. Have a biblio record with items belonging to different branches.
4. Be a librarian without superlibrarian rights, with editcatalogue
   and tool permissions set.

Scenario A (Edit items):
1. Go to Edit -> Edit items view (cataloguing/additems.pl).
2. You will be able to edit every item, also ones not from the branch
   you are from (cf. the button 'Actions').
3. Apply the patch.
4. Repeat 1.
5. You should be able to edit only the items from your branch.

Scenario B (Edit items in batch):
1. From Normal view go to Edit -> Edit items in batch.
2. You will be able to batch edit every item, also ones not from the branch
   you are from.
3. Apply the patch.
4. Repeat 1.
5. You should be able to edit only the items from your branch (and
   see 'Cannot edit' for others.

Scenario C (Delete items in batch):
1. From Normal view go to Edit -> Delete items in batch.
2. You will not see the string 'Cannot delete' and only by chance
   will not be able to activate the checkboxes next to foreign items.
3. Apply the patch.
4. Repeat 1.
5. You should be able to delete only the items from your branch (and
   see 'Cannot delete' for others.

Scenario D and E:
Analogous steps can be executed from Tools -> Batch item modification
and Tools -> Batch item deletion

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Well sleuthed, this was hard to trace.

I've confirmed the fix works and I can't find any regressions. Lets get this one pushed as a matter of urgency and continue tidying up in a further bug.. I'll leave Katrin and Fridolin to report that new bug.

So.. Passing QA, thanks for the patch Janusz

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

I see this is in 21.11.
Missing dependency, not backported to 21.05