Created attachment 154890 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch

Attempted to test using git bz with Koha Testing Docker. Received following error:

Applying: Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
Using index info to reconstruct a base tree...
M       koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt
M       tools/additional-contents.pl
Falling back to patching base and 3-way merge...
Auto-merging tools/additional-contents.pl
CONFLICT (content): Merge conflict in tools/additional-contents.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt
error: Failed to merge in the changes.
Patch failed at 0001 Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

Created attachment 162787 [details] [review]
Bug 34631: Independentbranches - rebase to master

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch

rebase to master

Applied patch and received an error when returning to the HTML Customizations tool in the non-superlibrarian login (additional-contents.pl) A PDF of the full error trace is available here: https://drive.google.com/file/d/1GNaNWN-sMIKQcYvdns5i5406qZ2iGw23/view?usp=sharing

The same error persists across News and Pages for both a superlibrarian login and a non-superlibrarian login.

Created attachment 162788 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch

@Matthias, I'm still getting the same error when applying. Git bz applied both patches that are attached here. Either one of them is still producing the error or I'm doing something wrong with Koha Testing Docker. Please advise.

Matthias, your patch sets now include merge markers. Please use the QA tool!

Created attachment 162856 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch

I think it's good now to test

sorry for the trouble I've caused by trying to rebase

Different error, this time when attempting to apply the patch using git bz:

Applying: Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt).
error: could not build fake ancestor
Patch failed at 0001 Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

Bug 34631 - Independentbranches - Non-superuser should not be able to write Additional contents for other branches

162856 - Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

I have no problem to apply the patch, I don't understand your problem. Your master is up to date ?

Matthias: I don't understand it either. I tried again just now. Fresh boot of Koha Testing Docker, fresh git pull, and an extra git reset just for extra measure. When I did the hard reset it said:
    HEAD is now at fee4368cd8 Bug 35930: Add guards for plugins_enabled

Applying the patch with git bz, I get the same errors as yesterday:
    error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/tools/additional-contents.tt).
    error: could not build fake ancestor

I was able to test and sign off on another patch for another bug yesterday, so I'm fairly certain the issue is with this patch and not my setup. I'm sorry I don't have any more insights than that.

Myka, Bug 35930 has been on the stack for a while.  My grep tells me it's number 486 on the master stack.

It's possible to test plenty of issue with an old master, but when it fails, it fails.  The issue right now seems with your setup, I'm sorry.

Created attachment 162929 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch

Signed-off-by: Myka Kennedy Stephens <mkstephens@fosgail.org>

I've been doing git pulls outside of Koha testing docker, which was making it look like my Koha repository was updating, but I guess it wasn't. I did a git pull from within the ktd shell, applied the patch, and then it worked!

Applying the patch has the desired effect that a non-superuser from another branch has the option to preview HTML content but the edit and delete buttons are hidden from content for other branches and all libraries.

After signing-off, I am having some trouble with steps 9 and 10 of the test plan. After trying to save new content with the limited-access user, I get the following error: Template process failed: undef error - The method Koha::AdditionalContent->idnew is not covered by tests!

This error persists to the superuser login. When I do a hard reset to master, the error goes away and I can see the content that I attempted to add and save before I got the error.

Hoping QA or a second tester can look into this further. I'm leaving it as signed off, but please change if there's a more appropriate status.

Well, I'll fail it myself :-P
Matthias, the code for additional-contents.pl has been all reformated, like you passed it in perltidy.  It makes it very hard to see the actual modifications.

Please resend the patch with no extra formating, please (beside the lines with actual changes).

Created attachment 162972 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch

Sorry for the confusion, I was working on an old master's DB, so I hadn't seen the change of a field in the DB that broke everything. Normally the problem is solved I managed to make the test plan.

Created attachment 162995 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch

Signed-off-by: Myka Kennedy Stephens <mkstephens@fosgail.org>

This patch works beautifully! I completed the test plan and everything worked as expected this time, very smooth. With the patch applied, the non-superlibrarian staff user can preview HTML customizations for all libraries and for other branches -- the edit and delete buttons are suppressed. This user can also edit existing content for their branch. When creating new content, the drop-down to select all libraries or a particular branch is suppressed. The user can create new content successfully and when returned to the table of all HTML customizations, the new content appears under the user's branch and has the edit and delete buttons available.

Great work! Glad all the back-and-forth finally paid off and it sounds like we both learned a little in the process. :-)

Signed-off-by: John Doe <you@example.com>

-    my $branchcode = $cgi->param('branchcode') || undef;
+    my $branchcode = $my_branch ? $my_branch : $cgi->param('branchcode') || undef;

This looks odd at least. I am posting on branchcode and we will write to current branch?

+                            [% UNLESS ( my_branch && my_branch != c.branchcode ) %]
                                 <input type="checkbox" name="ids" value="[% c.id | html %]" />
+                            [% END %]

Hiding an id based on an unclear condition (my_branch does not represent what we intuitively think that it is). 
Note that I found one counterargument:
tools/letter.pl:our $my_branch = C4::Context->preference("IndependentBranches") && !$staffflags->{'superlibrarian'}

This still needs a bit more attention at both server side and client side.

Created attachment 172231 [details] [review]
Bug 34631: (Follow up) Changed variable name

Same test plan as before just changed variable name for clarity.

The logic for the branchcode has changed slightly, and it seems to be the right approach. You can access the staff interface with two roles: superlibrarian or staff. Previously, there were no checks, allowing unrestricted editing and deleting.

The main issue was with variable names, which weren’t intuitive. To clarify, the branchcode is linked to the library field (Tools -> HTML customization -> new entry). Now, superlibrarians have access to all libraries, while staff can only access their own.

The variable my_branch indicates whether the user is a staff, adjusting the interface accordingly. I've renamed it for clarity, but that appears to be the only necessary change at this point.

Regarding the hidden ID of the checkbox, it works similarly: if you're staff and your library is different, there’s no checkbox, as it allows deletion of selected items.

Created attachment 175975 [details] [review]
Bug 34631: Independentbranches - Non-superuser should not be able to write Additional contents for other branches

When syspref Independentbranches is enabled Non-superuser should not be able to write Additional contents for other branches

To reproduce:
1- Connect to staff interface as superlibrarian
2- Enable IndependentBranches preference
3- Go to Tools -> HTML customizations
4- Crete an additional content for all libraries
5- Create or edit a borrower
    5-1- Set permission for the borrower
        - catalogue
        - edit_additional_contents
4- Log in to the staff interface with the non-superlibrarian staff user
5- Go to Tools -> HTML customizations
    --> you can delete or edit the additional contents created by superlibrarian for All libraries
6- Click on New entry
    --> you can add additional content for other branches
7- Apply the patch
8- Repeat step 5
    --> you cannot delete or edit the additional contents create by superlibrarian for All libraries
9- Repeat step 6
    --> you cannot add additional content for other branches
10- submit the form
    --> the branch of the created additional content is the current user branch

Signed-off-by: Myka Kennedy Stephens <mkstephens@fosgail.org>
Signed-off-by: David Nind <david@davidnind.com>

Created attachment 175976 [details] [review]
Bug 34631: (Follow up) Changed variable name

Same test plan as before just changed variable name for clarity.

Signed-off-by: David Nind <david@davidnind.com>

I fixed the original commit message sign off signature (changed John Doe to Myka Kennedy Stephens).