Bug 33363 - More specific permissions for purchase suggestions
Summary: More specific permissions for purchase suggestions
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Assignee: Kyle M Hall
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-03-29 19:25 UTC by Donna
Modified: 2024-07-12 08:22 UTC (History)
13 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00


Attachments
Bug 33363: Add separate permissions for creating and deleting suggestions (47.77 KB, patch)
2023-04-26 15:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add separate permissions for creating and deleting suggestions (47.77 KB, patch)
2023-05-01 10:21 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review
Bug 33363: Add separate permissions for creating and deleting suggestions (47.98 KB, patch)
2023-07-07 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add separate permissions for creating and deleting suggestions (48.75 KB, patch)
2024-01-11 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add unit tests (3.31 KB, patch)
2024-01-11 13:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add separate permissions for creating and deleting suggestions (49.00 KB, patch)
2024-01-12 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33363: Add unit tests (3.35 KB, patch)
2024-01-12 09:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33363: Add separate permissions for creating and deleting suggestions (44.14 KB, patch)
2024-04-18 18:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add unit tests (3.31 KB, patch)
2024-04-18 18:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add separate permissions for creating and deleting suggestions (33.04 KB, patch)
2024-06-25 11:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33363: Add unit tests (3.35 KB, patch)
2024-06-25 11:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33363: Add separate permissions for creating and deleting suggestions (33.07 KB, patch)
2024-06-25 12:02 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 33363: Add unit tests (3.41 KB, patch)
2024-06-25 12:02 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 33363: Add separate permissions for creating and deleting suggestions (33.09 KB, patch)
2024-07-02 12:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add unit tests (3.41 KB, patch)
2024-07-02 12:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: (QA Follow-up) Use say_success in atomic update (1.90 KB, patch)
2024-07-02 12:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: (QA Follow-up) Fix bug preventing Suggestions from showing in Acquistions menu (1.74 KB, patch)
2024-07-02 12:11 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: (QA Follow-up) Update manage permission description (1.03 KB, patch)
2024-07-11 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add separate permissions for creating and deleting suggestions (32.97 KB, patch)
2024-07-11 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add unit tests (3.41 KB, patch)
2024-07-11 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: (QA Follow-up) Use say_success in atomic update (1.90 KB, patch)
2024-07-11 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: (QA Follow-up) Fix bug preventing Suggestions from showing in Acquistions menu (1.74 KB, patch)
2024-07-11 12:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: (QA Follow-up) Update manage permission description (1.03 KB, patch)
2024-07-11 12:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add separate permissions for creating and deleting suggestions (32.96 KB, patch)
2024-07-11 21:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: Add unit tests (3.45 KB, patch)
2024-07-11 21:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: (QA Follow-up) Use say_success in atomic update (1.90 KB, patch)
2024-07-11 21:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: (QA Follow-up) Fix bug preventing Suggestions from showing in Acquistions menu (1.74 KB, patch)
2024-07-11 21:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33363: (QA Follow-up) Update manage permission description (1.03 KB, patch)
2024-07-11 21:46 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Donna 2023-03-29 19:25:36 UTC
There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.  

Currently if staff are given Suggestions Management/Manage Purchase Suggestions, and no Acq permissions, they are able to not only submit/view from the borrower record, but also access the purchase suggestion module and update/approve/delete purchase suggestions.  If Suggestions Management is not turned on, then staff cannot enter a purchase suggestion from a borrower record, nor can they see them from the patron record to let the patron know what the status is.

It would be helpful to have an addition permission level that would allow staff to enter a purchase suggestion from the borrower record, and view the existing purchase suggestions on a patron account.
Comment 1 Kristi 2023-04-03 21:08:00 UTC
Thank you for submitting a bug report for this, Donna!
Comment 2 Kyle M Hall 2023-04-26 15:26:19 UTC
Created attachment 150247 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )
Comment 3 PTFS Europe Sandboxes 2023-05-01 10:21:05 UTC
Created attachment 150450 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 4 Kyle M Hall 2023-07-07 12:56:34 UTC
Created attachment 153177 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 5 Jonathan Druart 2023-10-04 14:00:27 UTC
Please rebase and add REST API tests.
Comment 6 Kyle M Hall 2024-01-11 13:59:10 UTC
Created attachment 160846 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 7 Kyle M Hall 2024-01-11 13:59:17 UTC
Created attachment 160847 [details] [review]
Bug 33363: Add unit tests
Comment 8 Martin Renvoize 2024-01-12 09:33:46 UTC
Created attachment 160902 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2024-01-12 09:33:49 UTC
Created attachment 160903 [details] [review]
Bug 33363: Add unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2024-01-12 09:34:27 UTC
Tidied and squash said tidies.

QA scripts now happy, tests make sense and are passing.. controllers updated appropriately.

Passing QA
Comment 11 Katrin Fischer 2024-03-22 09:08:16 UTC
I am sorry, this no longer applies. Can I please get a rebase? It will jump right up back to the front of the list after.

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 33363: Add separate permissions for creating and deleting suggestions
Using index info to reconstruct a base tree...
M	installer/data/mysql/mandatory/userpermissions.sql
M	koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc
M	koha-tmpl/intranet-tmpl/prog/en/includes/header.inc
M	koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc
M	koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
M	koha-tmpl/intranet-tmpl/prog/en/modules/members/purchase-suggestions.tt
M	koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt
M	members/purchase-suggestions.pl
M	suggestion/suggestion.pl
M	t/Koha/Auth/Permissions.t
Falling back to patching base and 3-way merge...
Auto-merging t/Koha/Auth/Permissions.t
Auto-merging suggestion/suggestion.pl
CONFLICT (content): Merge conflict in suggestion/suggestion.pl
Auto-merging members/purchase-suggestions.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/members/purchase-suggestions.tt
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/patron-search.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/header.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/includes/circ-menu.inc
Auto-merging installer/data/mysql/mandatory/userpermissions.sql
error: Failed to merge in the changes.
Patch failed at 0001 Bug 33363: Add separate permissions for creating and deleting suggestions
hint: Use 'git am --show-current-patch=diff' 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-33363-Add-separate-permissions-for-creating-an-4fjafpom.patch
Comment 12 Kyle M Hall 2024-04-18 18:10:42 UTC
Created attachment 165142 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 13 Kyle M Hall 2024-04-18 18:10:49 UTC
Created attachment 165143 [details] [review]
Bug 33363: Add unit tests
Comment 14 Martin Renvoize 2024-06-25 11:04:35 UTC
Created attachment 168052 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2024-06-25 11:04:38 UTC
Created attachment 168053 [details] [review]
Bug 33363: Add unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize 2024-06-25 11:05:08 UTC
That was a fun rebase.. back to PQA
Comment 17 ByWater Sandboxes 2024-06-25 12:02:08 UTC
Created attachment 168055 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 18 ByWater Sandboxes 2024-06-25 12:02:11 UTC
Created attachment 168056 [details] [review]
Bug 33363: Add unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 19 Katrin Fischer 2024-06-25 15:44:22 UTC
Some small stuff, please follow-up:

1) Database update

It's not using say_success, but it's also not giving us any success message :)

2) What is actually meant by "manage"? 

I assume that "manage" actually means "edit". While any of the permissions allow to "view".

If that is the case, I feel it would be good to adjust the permission descriptions accordingly.

There is a slight discrepancy tho:
With only create permission, I have the entry to suggestions in the "more" menu, but it's missing from the navigation in the acquisition module.
I can also not see the pending suggestions on the start page.

I think we should adjust the navigation in the acq module at least.

Should the pending suggestions on acq and staff interface home page be visible with manage_suggestions only or also with any suggestion permission? 

3) Question

+        <div class="note">Only staff with superlibrarian or full suggestions permissions are returned in the search results</div>

I think this is in line with the previous behavior, but I wonder if we should not change to allow "any" instead of "full" here. 

I assume this is for setting the manager of a suggestion - editing might be required, but not sure if delete and create would be as well.
Comment 20 Kyle M Hall 2024-07-02 11:57:37 UTC
(In reply to Katrin Fischer from comment #19)
> Some small stuff, please follow-up:
> 
> 1) Database update
> 
> It's not using say_success, but it's also not giving us any success message
> :)

Done

> 
> 2) What is actually meant by "manage"?
> 
> I assume that "manage" actually means "edit". While any of the permissions
> allow to "view".
> 
> If that is the case, I feel it would be good to adjust the permission
> descriptions accordingly.

That was the pre-existing term and in Koha we've always used it as a catch-all for anything not covered by more specific permissions. It covers more than just viewing, such as approvals.

> There is a slight discrepancy tho:
> With only create permission, I have the entry to suggestions in the "more"
> menu, but it's missing from the navigation in the acquisition module.

Can you add a screenshot or be more specific about where this pulldown is?

> I can also not see the pending suggestions on the start page.

That is intended. A librarian that can only create suggestions has no need to see the suggestions that need managed.

> I think we should adjust the navigation in the acq module at least.
> 
> Should the pending suggestions on acq and staff interface home page be
> visible with manage_suggestions only or also with any suggestion permission? 

They should be visible only for manage_suggestions.

> 
> 3) Question
> 
> +        <div class="note">Only staff with superlibrarian or full
> suggestions permissions are returned in the search results</div>
> 
> I think this is in line with the previous behavior, but I wonder if we
> should not change to allow "any" instead of "full" here. 

I disagree. This is still a limit for just librarians that manage permissions, just like all the other manage permissions we can limit patron searches too.
 
> 
> I assume this is for setting the manager of a suggestion - editing might be
> required, but not sure if delete and create would be as well.

They would not be.
Comment 21 Kyle M Hall 2024-07-02 12:11:19 UTC
Created attachment 168356 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 22 Kyle M Hall 2024-07-02 12:11:28 UTC
Created attachment 168357 [details] [review]
Bug 33363: Add unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 23 Kyle M Hall 2024-07-02 12:11:32 UTC
Created attachment 168358 [details] [review]
Bug 33363: (QA Follow-up) Use say_success in atomic update
Comment 24 Kyle M Hall 2024-07-02 12:11:35 UTC
Created attachment 168359 [details] [review]
Bug 33363: (QA Follow-up) Fix bug preventing Suggestions from showing in Acquistions menu
Comment 25 Katrin Fischer 2024-07-02 12:23:33 UTC
Hi Kyle, some quick explanations to keep this going:

(In reply to Kyle M Hall from comment #20)
> (In reply to Katrin Fischer from comment #19)
> > Some small stuff, please follow-up:
> > 
> > 1) Database update
> > 
> > It's not using say_success, but it's also not giving us any success message
> > :)
> 
> Done

Thanks!

> 
> > 
> > 2) What is actually meant by "manage"?
> > 
> > I assume that "manage" actually means "edit". While any of the permissions
> > allow to "view".
> > 
> > If that is the case, I feel it would be good to adjust the permission
> > descriptions accordingly.
> 
> That was the pre-existing term and in Koha we've always used it as a
> catch-all for anything not covered by more specific permissions. It covers
> more than just viewing, such as approvals.

We have used it as a catch all, I agree, it meant: CRUD.
But now that we are starting to split permissions up, I think we should try to be as precise in our descriptions as possible.

For example: manage so far included deleting, but it does no longer.

What do you think about "View and edit ..." as a description to replace "Manage..." 

(not renaming permission code, but permission description)


> > There is a slight discrepancy tho:
> > With only create permission, I have the entry to suggestions in the "more"
> > menu, but it's missing from the navigation in the acquisition module.
> 
> Can you add a screenshot or be more specific about where this pulldown is?

The "More" in the main navigation of Koha on top of every page :)

I think it should probably be visible with "any" suggestion permission, what do you think?

The same for the acquisition module navigation on the left.

> > I can also not see the pending suggestions on the start page.
> 
> That is intended. A librarian that can only create suggestions has no need
> to see the suggestions that need managed.
> 
> > I think we should adjust the navigation in the acq module at least.

see above

> > Should the pending suggestions on acq and staff interface home page be
> > visible with manage_suggestions only or also with any suggestion permission? 
> 
> They should be visible only for manage_suggestions.

OK, so we leave the pending suggestions display for the "manage" permission.

> 
> > 
> > 3) Question
> > 
> > +        <div class="note">Only staff with superlibrarian or full
> > suggestions permissions are returned in the search results</div>
> > 
> > I think this is in line with the previous behavior, but I wonder if we
> > should not change to allow "any" instead of "full" here. 
> 
> I disagree. This is still a limit for just librarians that manage
> permissions, just like all the other manage permissions we can limit patron
> searches too.
>  
> > 
> > I assume this is for setting the manager of a suggestion - editing might be
> > required, but not sure if delete and create would be as well.
> 
> They would not be.

But if the search is limited to must have "all module" permissions, I cannot edit someone that has only manage or only delete or only create to be a manager. 
I think the "only manage" could maybe be a use case?
Comment 26 Kyle M Hall 2024-07-02 13:58:37 UTC
> > > 2) What is actually meant by "manage"?
> > > 
> > > I assume that "manage" actually means "edit". While any of the permissions
> > > allow to "view".
> > > 
> > > If that is the case, I feel it would be good to adjust the permission
> > > descriptions accordingly.
> > 
> > That was the pre-existing term and in Koha we've always used it as a
> > catch-all for anything not covered by more specific permissions. It covers
> > more than just viewing, such as approvals.
> 
> We have used it as a catch all, I agree, it meant: CRUD.
> But now that we are starting to split permissions up, I think we should try
> to be as precise in our descriptions as possible.

I disagree with that interpretation, but I'm open to other possibilities. I think we've hit something philosophical that it far outside the scope of this development. Maybe we should discuss this at the next dev meeting?


> > Can you add a screenshot or be more specific about where this pulldown is?
> 
> The "More" in the main navigation of Koha on top of every page :)
> 
> I think it should probably be visible with "any" suggestion permission, what
> do you think?
> 
> The same for the acquisition module navigation on the left.

You found a bug! Fixed!
 
> > > 3) Question
> > > 
> > > +        <div class="note">Only staff with superlibrarian or full
> > > suggestions permissions are returned in the search results</div>
> > > 
> > > I think this is in line with the previous behavior, but I wonder if we
> > > should not change to allow "any" instead of "full" here. 
> > 
> > I disagree. This is still a limit for just librarians that manage
> > permissions, just like all the other manage permissions we can limit patron
> > searches too.
> >  
> > > 
> > > I assume this is for setting the manager of a suggestion - editing might be
> > > required, but not sure if delete and create would be as well.
> > 
> > They would not be.
> 
> But if the search is limited to must have "all module" permissions, I cannot
> edit someone that has only manage or only delete or only create to be a
> manager. 
> I think the "only manage" could maybe be a use case?

I'm not sure I understand. Those searches are already filtering for the "suggestions_manage". Those searches are for setting a manager for the suggestion. Everything works as is, right? I did not make any changes to this code.
Comment 27 Katrin Fischer 2024-07-02 14:11:07 UTC
(In reply to Kyle M Hall from comment #26)
> > > > 2) What is actually meant by "manage"?
> > > > 
> > > > I assume that "manage" actually means "edit". While any of the permissions
> > > > allow to "view".
> > > > 
> > > > If that is the case, I feel it would be good to adjust the permission
> > > > descriptions accordingly.
> > > 
> > > That was the pre-existing term and in Koha we've always used it as a
> > > catch-all for anything not covered by more specific permissions. It covers
> > > more than just viewing, such as approvals.
> > 
> > We have used it as a catch all, I agree, it meant: CRUD.
> > But now that we are starting to split permissions up, I think we should try
> > to be as precise in our descriptions as possible.
> 
> I disagree with that interpretation, but I'm open to other possibilities. I
> think we've hit something philosophical that it far outside the scope of
> this development. Maybe we should discuss this at the next dev meeting?

Maybe I am misunderstanding... but with manage_suggestions I can no longer delete/create now. So it feels absolutely necessary to me to adapt the permission description?

> > > Can you add a screenshot or be more specific about where this pulldown is?
> > 
> > The "More" in the main navigation of Koha on top of every page :)
> > 
> > I think it should probably be visible with "any" suggestion permission, what
> > do you think?
> > 
> > The same for the acquisition module navigation on the left.
> 
> You found a bug! Fixed!
>  
> > > > 3) Question
> > > > 
> > > > +        <div class="note">Only staff with superlibrarian or full
> > > > suggestions permissions are returned in the search results</div>
> > > > 
> > > > I think this is in line with the previous behavior, but I wonder if we
> > > > should not change to allow "any" instead of "full" here. 
> > > 
> > > I disagree. This is still a limit for just librarians that manage
> > > permissions, just like all the other manage permissions we can limit patron
> > > searches too.
> > >  
> > > > 
> > > > I assume this is for setting the manager of a suggestion - editing might be
> > > > required, but not sure if delete and create would be as well.
> > > 
> > > They would not be.
> > 
> > But if the search is limited to must have "all module" permissions, I cannot
> > edit someone that has only manage or only delete or only create to be a
> > manager. 
> > I think the "only manage" could maybe be a use case?
> 
> I'm not sure I understand. Those searches are already filtering for the
> "suggestions_manage". Those searches are for setting a manager for the
> suggestion. Everything works as is, right? I did not make any changes to
> this code.

I was referring to these lines here:
-        <div class="note">Only staff with superlibrarian or suggestions_manage permissions are returned in the search results</div>
+        <div class="note">Only staff with superlibrarian or full suggestions permissions are returned in the search results</div>
Comment 28 Katie Bliss 2024-07-02 15:37:04 UTC
Glad to see this one seems to be moving along! We get about 250-300 suggestions a month, so we're very interested in bugs like this.

Question about the staff with create-only permissions: when you say that the intention is for those staff to NOT be able to view pending suggestions, does that mean they can't see them anywhere? Would they be able to see them on a patron's account? The comments above seem to primarily be about the link on the homepage that lets you know there are pending suggestions to be managed, but I wasn't sure. 

For us, we have two staff who are in charge of managing/making decisions on purchase suggestions, while pretty much everyone else in our system would only need the ability to create them. That said, the "everyone else" are our front-line staff who need to be able to provide information to our patrons, such as the status of their suggestions. So, it would make sense that they would be able to see a patron's suggestions, regardless of status, even if they can't edit/delete them.
Comment 29 Katrin Fischer 2024-07-02 16:38:35 UTC
(In reply to Katie Bliss from comment #28)
> Glad to see this one seems to be moving along! We get about 250-300
> suggestions a month, so we're very interested in bugs like this.
> 
> Question about the staff with create-only permissions: when you say that the
> intention is for those staff to NOT be able to view pending suggestions,
> does that mean they can't see them anywhere? Would they be able to see them
> on a patron's account? The comments above seem to primarily be about the
> link on the homepage that lets you know there are pending suggestions to be
> managed, but I wasn't sure. 

The create and delete permissions 'include' the view permission.

I hope it came across that I have nothing against the feature - but I'd like us to really clearly define what each permission does/includes and describe the permissions accordingly. I think this will help to avoid misunderstandings and inconsistencies along the line when we are going to split up other manage permissions as well.

> For us, we have two staff who are in charge of managing/making decisions on
> purchase suggestions, while pretty much everyone else in our system would
> only need the ability to create them. That said, the "everyone else" are our
> front-line staff who need to be able to provide information to our patrons,
> such as the status of their suggestions. So, it would make sense that they
> would be able to see a patron's suggestions, regardless of status, even if
> they can't edit/delete them.

The purchase suggestions tab in the patron account in the staff interface will be accessible with any of the suggestions permissions (delete, create and manage/edit permission) with this patch set.

+        flagsrequired   => { suggestions => '*' },
Comment 30 Katie Bliss 2024-07-02 16:41:38 UTC
Thank you for clarifying! I just wanted to make sure I understood what you and Kyle were discussing.
Comment 31 Katrin Fischer 2024-07-02 16:55:47 UTC
(In reply to Katie Bliss from comment #30)
> Thank you for clarifying! I just wanted to make sure I understood what you
> and Kyle were discussing.

Thanks for asking! I hope the explanation made sense!

I have also added this to the agenda for the meeting tomorrow as Kyle suggested, so maybe we can move this forward there:
https://wiki.koha-community.org/wiki/Development_IRC_meeting_3_July_2024
Comment 32 Martin Renvoize 2024-07-08 09:39:30 UTC
What's the state of this right now?  It was PQA but is now back to SO...
Comment 33 Katrin Fischer 2024-07-08 11:00:04 UTC
Another small follow-up is needed to update the permission descriptions as discussed at the dev meeting.
Comment 34 Kyle M Hall 2024-07-11 12:49:08 UTC
Created attachment 168803 [details] [review]
Bug 33363: (QA Follow-up) Update manage permission description
Comment 35 Kyle M Hall 2024-07-11 12:49:36 UTC
Created attachment 168804 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 36 Kyle M Hall 2024-07-11 12:49:52 UTC
Created attachment 168805 [details] [review]
Bug 33363: Add unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 37 Kyle M Hall 2024-07-11 12:49:54 UTC
Created attachment 168806 [details] [review]
Bug 33363: (QA Follow-up) Use say_success in atomic update
Comment 38 Kyle M Hall 2024-07-11 12:49:57 UTC
Created attachment 168807 [details] [review]
Bug 33363: (QA Follow-up) Fix bug preventing Suggestions from showing in Acquistions menu
Comment 39 Kyle M Hall 2024-07-11 12:50:00 UTC
Created attachment 168808 [details] [review]
Bug 33363: (QA Follow-up) Update manage permission description
Comment 40 Kyle M Hall 2024-07-11 12:52:27 UTC
(In reply to Katrin Fischer from comment #33)
> Another small follow-up is needed to update the permission descriptions as
> discussed at the dev meeting.

Katrin, did I get you request right? 🤞
Comment 41 Katrin Fischer 2024-07-11 13:14:57 UTC
(In reply to Kyle M Hall from comment #40)
> (In reply to Katrin Fischer from comment #33)
> > Another small follow-up is needed to update the permission descriptions as
> > discussed at the dev meeting.
> 
> Katrin, did I get you request right? 🤞

Yep, I think we can go with that :)

Do you want another sign-off?
Comment 42 Kyle M Hall 2024-07-11 21:46:16 UTC
Created attachment 168865 [details] [review]
Bug 33363: Add separate permissions for creating and deleting suggestions

There are a number of libraries that would like for their staff to be able to submit (and view existing) purchase suggestions from the borrower record, but not give the staff the ability to edit/manage/delete purchase suggestions.

Test Plan:
1) Apply this patch
2) Run restart all the things!
3) Run updatedatabase
4) Verify anyone with the suggestions manage permissions now has the create and delete permissions as well
5) Verify anyone without the suggestions manage permission has not recieved the new permissions
6) Enable only the create permission for a librarian
7) Verify that librarian can only create new suggestions ( not manage or delete )
8) Enable only the manage permission for a librarian
9) Verify that librarian can only manage existing suggestions ( not create or delete )
10) Enable only the delete permission for a librarian
11) Verify that librarian can only delete suggestions ( not create or manage )

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Sponsored-by: Cuyahoga County Public Library
Comment 43 Kyle M Hall 2024-07-11 21:46:23 UTC
Created attachment 168866 [details] [review]
Bug 33363: Add unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>

Sponsored-by: Cuyahoga County Public Library
Comment 44 Kyle M Hall 2024-07-11 21:46:26 UTC
Created attachment 168867 [details] [review]
Bug 33363: (QA Follow-up) Use say_success in atomic update
Comment 45 Kyle M Hall 2024-07-11 21:46:28 UTC
Created attachment 168868 [details] [review]
Bug 33363: (QA Follow-up) Fix bug preventing Suggestions from showing in Acquistions menu
Comment 46 Kyle M Hall 2024-07-11 21:46:31 UTC
Created attachment 168869 [details] [review]
Bug 33363: (QA Follow-up) Update manage permission description
Comment 47 Katrin Fischer 2024-07-12 07:52:10 UTC
Last changes were just text and by my request, we got another sign-off - picking this out of the QA queue. 

Fixed inline:

Processing additional checks

	* Commit title does not contain 'follow-up' correctly spelt - 66da72c23e4
	* Commit title does not contain '(QA follow-up)' correctly spelt - 66da72c23e4
	* Commit title does not contain 'follow-up' correctly spelt - f93263600af
	* Commit title does not contain '(QA follow-up)' correctly spelt - f93263600af
	* Commit title does not contain 'follow-up' correctly spelt - 42bd595ac80
	* Commit title does not contain '(QA follow-up)' correctly spelt - 42bd595ac80
Comment 48 Katrin Fischer 2024-07-12 08:22:07 UTC
Pushed for 24.11!

Well done everyone, thank you!