Bug 26738 - Unable to change manager of purchase suggestion
Summary: Unable to change manager of purchase suggestion
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 23590 24819
Blocks:
  Show dependency treegraph
 
Reported: 2020-10-19 19:25 UTC by Andrew Fuerste-Henry
Modified: 2020-11-17 20:29 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06


Attachments
Bug 26738: (bug 24819 follow-up) Fix manager selection (14.87 KB, patch)
2020-11-02 14:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26738: (bug 24819 follow-up) Fix manager selection (14.98 KB, patch)
2020-11-02 14:41 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 26738: (bug 24819 follow-up) Fix manager selection (15.01 KB, patch)
2020-11-03 13:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26738: (QA follow-up) Add missing filter (1.10 KB, patch)
2020-11-03 13:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26738: [20.05.x] (bug 24819 follow-up) Fix manager selection (15.11 KB, patch)
2020-11-17 17:11 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2020-10-19 19:25:34 UTC
Bug 23590 added new features around tracking the manager of a suggestion. I'm unable to edit those values, however.
To recreate:
- create a new purchase suggestion from within acquisitions (suggestion.pl?op=add)
- click "select manager," search for user, click Select
- see that the user you just selected shows under "Created by,"
- see that "Managed by" still says "You"
- save your suggestion
- query the database, see that suggestion.suggestedby is the user you selected, suggestion.managedby is your own borrowernumber

I see that same behavior when editing an existing suggestion rather than making one from scratch.
Comment 1 Katrin Fischer 2020-10-19 21:45:02 UTC
I just tested this and you are right, there is definitely a bug there.

Set patron should change the suggestor, but now the suggestor is also changed when using "select manager". 

I assume it could be a Javascript issue targetting the wrong field?
Comment 2 Andrew Fuerste-Henry 2020-10-20 12:36:13 UTC
There are also controls to change the manager in bulk from the main purchase suggestion page. I can't get that to work, either.
Comment 3 Jonathan Druart 2020-11-02 14:14:49 UTC
This is caused by bug 24819. It has been pushed 1 month after bug 23590.
Comment 4 Jonathan Druart 2020-11-02 14:19:39 UTC
Created attachment 112836 [details] [review]
Bug 26738: (bug 24819 follow-up) Fix manager selection

Bug 23590 added a new feature to select the manager of a suggestion.
One month later bug 24819 added the ability to pick the suggester.

This second patchset broke the manager selection.

This patch simplifies the way the suggester is selected, using the
generic way and mimicking what is done for the manager.

Test plan:
- create a new purchase suggestion from within acquisitions (suggestion.pl?op=add)
- click "select manager," search for user, click Select
- see that the user you just selected shows under "Created by,"
- see that "Managed by" still says "You"
- modify the suggester
- save your suggestion
=> Everything is saved correctly

QA will test the permission alert:
Edit suggestion.tt and remove "&permissions=suggestions.suggestions_manage"
Edit the suggestion, select a manager, pick a patron in the list who
does not have sufficient permissions, save
=> you get the alert
Comment 5 Jonathan Druart 2020-11-02 14:20:13 UTC
New feature advertised in 20.05 is broken, upping severity.
Comment 6 Andrew Fuerste-Henry 2020-11-02 14:38:15 UTC
This does allow me to save changes to the manager of a purchase suggestion.

I've not seen any notices generated as I do so. My two demo users do both have email addresses and the notice "NOTIFY_MANAGER" exists. Am I missing something there?

Is the manager of a purchase suggestion meant to always default to whoever last edited it? If I edit an existing suggestion with another user as the manager, Koha will set me as the manger unless I remember to click to tell it not to.
Comment 7 Andrew Fuerste-Henry 2020-11-02 14:41:55 UTC
Created attachment 112842 [details] [review]
Bug 26738: (bug 24819 follow-up) Fix manager selection

Bug 23590 added a new feature to select the manager of a suggestion.
One month later bug 24819 added the ability to pick the suggester.

This second patchset broke the manager selection.

This patch simplifies the way the suggester is selected, using the
generic way and mimicking what is done for the manager.

Test plan:
- create a new purchase suggestion from within acquisitions (suggestion.pl?op=add)
- click "select manager," search for user, click Select
- see that the user you just selected shows under "Created by,"
- see that "Managed by" still says "You"
- modify the suggester
- save your suggestion
=> Everything is saved correctly

QA will test the permission alert:
Edit suggestion.tt and remove "&permissions=suggestions.suggestions_manage"
Edit the suggestion, select a manager, pick a patron in the list who
does not have sufficient permissions, save
=> you get the alert

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 8 Andrew Fuerste-Henry 2020-11-02 14:43:59 UTC
I *was* missing something! there's a checkbox!

Manager changed successfully through both edit and batch interfaces.
Notice sent successfully.
I'm still a little torn about defaulting the changing the manager each time you edit, but will wait to see what users think.
Comment 9 Martin Renvoize 2020-11-03 08:03:01 UTC
(In reply to Andrew Fuerste-Henry from comment #8)
> I *was* missing something! there's a checkbox!
> 
> Manager changed successfully through both edit and batch interfaces.
> Notice sent successfully.
> I'm still a little torn about defaulting the changing the manager each time
> you edit, but will wait to see what users think.

I'm sure I've seen another bug regarding always setting the manager to the last person to update the suggestion.. I can't remember which way the bug was pushing for and can't find the bug now though :(.
Comment 10 Tomás Cohen Arazi 2020-11-03 12:51:28 UTC
I don't manage to change the manager to another user than the current one. Followed Andrew's steps.

Jonathan's are about changing the 'suggester' so I'm confused.
Comment 11 Jonathan Druart 2020-11-03 13:12:47 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> I don't manage to change the manager to another user than the current one.
> Followed Andrew's steps.
> 
> Jonathan's are about changing the 'suggester' so I'm confused.

Code is affecting both, manager and suggester, so both searches must be tested.

The manager search will only return patrons with suggestions_manage.

"Only staff with superlibrarian or suggestions_manage permissions are returned in the search results"
Comment 12 Andrew Fuerste-Henry 2020-11-03 13:13:11 UTC
Just tested this again, successfully. Updated test plan:
To test:
1- make sure you have at least 2 patrons with purchase suggestion permissions and email addresses
2- open a new or existing purchase suggestion
3- in the Suggestion Management section, click the Select Manager link
4- in the patron search popup, search for and select a staff member with suggestion permissions
5- on the suggestion, click the "Notifiy Manager" checkbox
6- save your suggestion
7- on suggestion.pl, confirm your suggestion shows your selected staff member for Managed By
8- confirm your staff member received a notice
9- on suggestion.pl, use the checkboxes to select your suggestion and any number of additional suggestions
10- click Select Manager in the Update Manger box underneath the suggestions table
11- again, use the patron search popup to pick someone
12- click Submit to make your change
13- confirm the suggestions table reflects your change
(there is not yet a way to send a notice for batch changes)
Comment 13 Tomás Cohen Arazi 2020-11-03 13:19:33 UTC
Both search+choose work correctly. I just found saving didn't store the chosen manager.

RETESTING.
Comment 14 Tomás Cohen Arazi 2020-11-03 13:43:55 UTC
Created attachment 112920 [details] [review]
Bug 26738: (bug 24819 follow-up) Fix manager selection

Bug 23590 added a new feature to select the manager of a suggestion.
One month later bug 24819 added the ability to pick the suggester.

This second patchset broke the manager selection.

This patch simplifies the way the suggester is selected, using the
generic way and mimicking what is done for the manager.

Test plan:
- create a new purchase suggestion from within acquisitions (suggestion.pl?op=add)
- click "select manager," search for user, click Select
- see that the user you just selected shows under "Created by,"
- see that "Managed by" still says "You"
- modify the suggester
- save your suggestion
=> Everything is saved correctly

QA will test the permission alert:
Edit suggestion.tt and remove "&permissions=suggestions.suggestions_manage"
Edit the suggestion, select a manager, pick a patron in the list who
does not have sufficient permissions, save
=> you get the alert

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2020-11-03 13:43:59 UTC
Created attachment 112921 [details] [review]
Bug 26738: (QA follow-up) Add missing filter

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Jonathan Druart 2020-11-04 12:04:14 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 17 Lucas Gass 2020-11-16 21:04:28 UTC
this patchset does not clean apply to 20.05.x, no backport

please rebase if needed
Comment 18 Jonathan Druart 2020-11-17 17:11:25 UTC
Created attachment 113746 [details] [review]
Bug 26738: [20.05.x] (bug 24819 follow-up) Fix manager selection

Bug 23590 added a new feature to select the manager of a suggestion.
One month later bug 24819 added the ability to pick the suggester.

This second patchset broke the manager selection.

This patch simplifies the way the suggester is selected, using the
generic way and mimicking what is done for the manager.

Test plan:
- create a new purchase suggestion from within acquisitions (suggestion.pl?op=add)
- click "select manager," search for user, click Select
- see that the user you just selected shows under "Created by,"
- see that "Managed by" still says "You"
- modify the suggester
- save your suggestion
=> Everything is saved correctly

QA will test the permission alert:
Edit suggestion.tt and remove "&permissions=suggestions.suggestions_manage"
Edit the suggestion, select a manager, pick a patron in the list who
does not have sufficient permissions, save
=> you get the alert

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Bug 26738: (QA follow-up) Add missing filter

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 19 Lucas Gass 2020-11-17 17:52:58 UTC
Thanks Jonathan! 

backported to 20.05.x for 20.05.06
Comment 20 Aleisha Amohia 2020-11-17 20:29:50 UTC
missing dependencies, not backported to 19.11.x