When a staff member modifies a suggestion, it is automatically attributed to the logged librarian. It would be very useful to have the option to select another user that will be the one in charge of this suggestion. This other user could also be notified by an email after the suggestion is modified. This feature probably means to create new fields in the ‘suggestions’ table to separate ‘managedby’ / ‘manageddate’ from the ‘lastmodificationby’ / ‘‘lastmodificationdate’ informations, but I think it will ensure a better follow up on the suggestions workflow. The BULAC (http://www.bulac.fr/) would like to sponsor this development. As long as “Change sponsored?” is set on “Seeking developer”, anyone wanting to quote us for the development of such a feature or just ask for more information is welcome to write to us: <sigb@bulac.fr>.
Created attachment 95162 [details] [review] Bug 23590: Add ability to edit the manager of a suggestion Prior to this patch there was an hidden behavior that set the manager to the logged in user when a suggestion was edited. This patch proposes to let the librarian pick another manager. Other small adjustements will be added to polish this new behavior: * Create 2 new DB columns: suggestions.lastmodificationby and suggestion.lastmodificationdate * Choose a manager when editing a suggestion * Batch modify suggestions and set a manager for them * Let notify the new manager using the TO_PROCESS letter * Display the manageddate, lastmodificationby and lastmodificationdate info where appropriate This first patch adds a new "Select manager" link when editing a suggestion. Test plan for the whole patchset: 0/ a. Execute the update DB entry, generate the new DBIC file and restart all b. Create at least 2 patrons with the suggestions_manage permission 1/ Submit a new suggestion (OPAC or staff, not important) 2/ Accept it 3/ Edit it => "Last modification by" is empty => You see that you are the manager of this suggestion 4/ Click "Select manager" and search for a new manager => The patron search will only display patrons with the suggestions_manage permission 5/ Save 6/ Edit again => The manager is set to you, but there is a note saying that previously it was the patron you picked => The "Last modification by" is set to you 7/ Click "Keep existing manager" => The manager is now set to the previous manager 8/ Select another manager (which has a valid email address defined) 9/ Click the "notify" checkbox 10/ Save 11/ Confirm that a TO_PROCESS notice has been generated into the message_queue table 12/ Create at least one other suggestion 13/ List the suggestions => There is a 4th action column to assign a manager to several suggestions in one go. 14/ Use this new button and confirm that it works as expected 15/ Go to your purchase suggestion list (OPAC and staff) => You see the "managed date" displayed in a new column
Created attachment 95163 [details] [review] Bug 23590: Clean TT variables for patrons Just a bit of cleaning
Created attachment 95164 [details] [review] Bug 23590: Cleaning - Remove op_* vars
Created attachment 95165 [details] [review] Bug 23590: Batch update manager for suggestions
Created attachment 95166 [details] [review] Bug 23590: Only return patrons that have the suggestions_manage permission
Created attachment 95167 [details] [review] Bug 23590: Add the ability to notify a new manager
Created attachment 95168 [details] [review] Bug 23590: Display managed date in 'Your purchase suggestions'
Created attachment 95169 [details] [review] Bug 23590: Add lastmodification by and date columns No tests are provided for the changes made to SearchSuggestion. It is going to be remove very soon as it is super ugly...
Patch failed on both sandbox and devbox. Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 23590: Add ability to edit the manager of a suggestion fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 23590: Add ability to edit the manager of a suggestion
The patches need to be applied in the following (reverse) order: Bug 23590: Add lastmodification by and date columns Bug 23590: Display managed date in 'Your purchase suggestions' Bug 23590: Add the ability to notify a new manager Bug 23590: Only return patrons that have the suggestions_manage permission Bug 23590: Batch update manager for suggestions Bug 23590: Cleaning - Remove op_* vars Bug 23590: Clean TT variables for patrons Bug 23590: Add ability to edit the manager of a suggestion Bug 14963: Set status=ORDERED for suggestions created from existing record Bug 14963: Add the ability to suggest purchase on existing title Bug 23854: Fix failure on dates when editing a suggestion Bug 23594: Restore previous view after delete or update itemtype Bug 23594: Fix [un]checkall links Bug 23594: Batch modification for itemtypes on suggestions page Bug 14973: Add missing filters in templates Bug 14973: Remove double declaration of suggestions_loop Bug 14973: Remove double textarea for notes Bug 14973: Do not skip validation step when submiting a suggestion - OPAC Bug 14973: Check existing biblio when submitting a purchase suggestion (staff side) Bug 14973: Check existing biblio when submitting a purchase suggestion (opac side) If it can help, I have pushed a branch with all the patches applied: https://gitlab.com/joubu/Koha/commits/bug_23590
Created attachment 95317 [details] [review] Bug 23590: Add ability to edit the manager of a suggestion Prior to this patch there was an hidden behavior that set the manager to the logged in user when a suggestion was edited. This patch proposes to let the librarian pick another manager. Other small adjustements will be added to polish this new behavior: * Create 2 new DB columns: suggestions.lastmodificationby and suggestion.lastmodificationdate * Choose a manager when editing a suggestion * Batch modify suggestions and set a manager for them * Let notify the new manager using the TO_PROCESS letter * Display the manageddate, lastmodificationby and lastmodificationdate info where appropriate This first patch adds a new "Select manager" link when editing a suggestion. Test plan for the whole patchset: 0/ a. Execute the update DB entry, generate the new DBIC file and restart all b. Create at least 2 patrons with the suggestions_manage permission 1/ Submit a new suggestion (OPAC or staff, not important) 2/ Accept it 3/ Edit it => "Last modification by" is empty => You see that you are the manager of this suggestion 4/ Click "Select manager" and search for a new manager => The patron search will only display patrons with the suggestions_manage permission 5/ Save 6/ Edit again => The manager is set to you, but there is a note saying that previously it was the patron you picked => The "Last modification by" is set to you 7/ Click "Keep existing manager" => The manager is now set to the previous manager 8/ Select another manager (which has a valid email address defined) 9/ Click the "notify" checkbox 10/ Save 11/ Confirm that a TO_PROCESS notice has been generated into the message_queue table 12/ Create at least one other suggestion 13/ List the suggestions => There is a 4th action column to assign a manager to several suggestions in one go. 14/ Use this new button and confirm that it works as expected 15/ Go to your purchase suggestion list (OPAC and staff) => You see the "managed date" displayed in a new column
Created attachment 95318 [details] [review] Bug 23590: Clean TT variables for patrons Just a bit of cleaning
Created attachment 95319 [details] [review] Bug 23590: Cleaning - Remove op_* vars
Created attachment 95320 [details] [review] Bug 23590: Batch update manager for suggestions
Created attachment 95321 [details] [review] Bug 23590: Only return patrons that have the suggestions_manage permission
Created attachment 95322 [details] [review] Bug 23590: Add the ability to notify a new manager
Created attachment 95323 [details] [review] Bug 23590: Display managed date in 'Your purchase suggestions'
Created attachment 95324 [details] [review] Bug 23590: Add lastmodification by and date columns No tests are provided for the changes made to SearchSuggestion. It is going to be remove very soon as it is super ugly...
Hi Jonathan, After I apply the patches, none of my suggestions are displayed. Do you know where it can come from ?
Created attachment 95398 [details] [review] Bug 23590: Add atomic update file
Created attachment 95399 [details] [review] Bug 23590: Remove JS error on the suggestion list The notify option is only available from the edit view
On step 8-9 : when I select a new manager, the checkbox to notify stays disabled even if there is a valid email (I didn't notice that point on my first tests and I correctly received an email at that moment, but I can't reproduce it on my second tests) On step 14 : the pop-up to select the new manager for several suggestions only works on the first "Accepted" tab On step 15 : I don't see the "managed date" on OPAC
Created attachment 95407 [details] [review] Bug 23590: Remove JS error on the suggestion list The notify option is only available from the edit view
Created attachment 95408 [details] [review] Bug 23590: Fix select manager on list for all tabs
(In reply to Séverine Queune from comment #22) > On step 8-9 : when I select a new manager, the checkbox to notify stays > disabled even if there is a valid email (I didn't notice that point on my > first tests and I correctly received an email at that moment, but I can't > reproduce it on my second tests) Fixed > On step 14 : the pop-up to select the new manager for several suggestions > only works on the first "Accepted" tab And fixed! > On step 15 : I don't see the "managed date" on OPAC Looks ok for me: https://snipboard.io/KZ9bQS.jpg
Created attachment 95420 [details] [review] Bug 23590: DBIC Changes
Created attachment 95421 [details] [review] Bug 23590: Add ability to edit the manager of a suggestion Prior to this patch there was an hidden behavior that set the manager to the logged in user when a suggestion was edited. This patch proposes to let the librarian pick another manager. Other small adjustements will be added to polish this new behavior: * Create 2 new DB columns: suggestions.lastmodificationby and suggestion.lastmodificationdate * Choose a manager when editing a suggestion * Batch modify suggestions and set a manager for them * Let notify the new manager using the TO_PROCESS letter * Display the manageddate, lastmodificationby and lastmodificationdate info where appropriate This first patch adds a new "Select manager" link when editing a suggestion. Test plan for the whole patchset: 0/ a. Execute the update DB entry, generate the new DBIC file and restart all b. Create at least 2 patrons with the suggestions_manage permission 1/ Submit a new suggestion (OPAC or staff, not important) 2/ Accept it 3/ Edit it => "Last modification by" is empty => You see that you are the manager of this suggestion 4/ Click "Select manager" and search for a new manager => The patron search will only display patrons with the suggestions_manage permission 5/ Save 6/ Edit again => The manager is set to you, but there is a note saying that previously it was the patron you picked => The "Last modification by" is set to you 7/ Click "Keep existing manager" => The manager is now set to the previous manager 8/ Select another manager (which has a valid email address defined) 9/ Click the "notify" checkbox 10/ Save 11/ Confirm that a TO_PROCESS notice has been generated into the message_queue table 12/ Create at least one other suggestion 13/ List the suggestions => There is a 4th action column to assign a manager to several suggestions in one go. 14/ Use this new button and confirm that it works as expected 15/ Go to your purchase suggestion list (OPAC and staff) => You see the "managed date" displayed in a new column Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95422 [details] [review] Bug 23590: Clean TT variables for patrons Just a bit of cleaning Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95423 [details] [review] Bug 23590: Cleaning - Remove op_* vars Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95424 [details] [review] Bug 23590: Batch update manager for suggestions Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95425 [details] [review] Bug 23590: Only return patrons that have the suggestions_manage permission Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95426 [details] [review] Bug 23590: Add the ability to notify a new manager Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95427 [details] [review] Bug 23590: Display managed date in 'Your purchase suggestions' Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95428 [details] [review] Bug 23590: Add lastmodification by and date columns No tests are provided for the changes made to SearchSuggestion. It is going to be remove very soon as it is super ugly... Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95429 [details] [review] Bug 23590: Remove JS error on the suggestion list The notify option is only available from the edit view Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95430 [details] [review] Bug 23590: Fix select manager on list for all tabs Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95431 [details] [review] Bug 23590: DBIC Changes Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Great development, thanks a lot Jonathan ! (In reply to Jonathan Druart from comment #25) > > On step 15 : I don't see the "managed date" on OPAC > > Looks ok for me: https://snipboard.io/KZ9bQS.jpg My bad, OpacSuggestionManagedBy needs to be enabled.
Created attachment 95487 [details] [review] Bug 23590: Add ability to edit the manager of a suggestion Prior to this patch there was an hidden behavior that set the manager to the logged in user when a suggestion was edited. This patch proposes to let the librarian pick another manager. Other small adjustements will be added to polish this new behavior: * Create 2 new DB columns: suggestions.lastmodificationby and suggestion.lastmodificationdate * Choose a manager when editing a suggestion * Batch modify suggestions and set a manager for them * Let notify the new manager using the TO_PROCESS letter * Display the manageddate, lastmodificationby and lastmodificationdate info where appropriate This first patch adds a new "Select manager" link when editing a suggestion. Test plan for the whole patchset: 0/ a. Execute the update DB entry, generate the new DBIC file and restart all b. Create at least 2 patrons with the suggestions_manage permission 1/ Submit a new suggestion (OPAC or staff, not important) 2/ Accept it 3/ Edit it => "Last modification by" is empty => You see that you are the manager of this suggestion 4/ Click "Select manager" and search for a new manager => The patron search will only display patrons with the suggestions_manage permission 5/ Save 6/ Edit again => The manager is set to you, but there is a note saying that previously it was the patron you picked => The "Last modification by" is set to you 7/ Click "Keep existing manager" => The manager is now set to the previous manager 8/ Select another manager (which has a valid email address defined) 9/ Click the "notify" checkbox 10/ Save 11/ Confirm that a TO_PROCESS notice has been generated into the message_queue table 12/ Create at least one other suggestion 13/ List the suggestions => There is a 4th action column to assign a manager to several suggestions in one go. 14/ Use this new button and confirm that it works as expected 15/ Go to your purchase suggestion list (OPAC and staff) => You see the "managed date" displayed in a new column Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95488 [details] [review] Bug 23590: Clean TT variables for patrons Just a bit of cleaning Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95489 [details] [review] Bug 23590: Cleaning - Remove op_* vars Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95490 [details] [review] Bug 23590: Batch update manager for suggestions Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95491 [details] [review] Bug 23590: Only return patrons that have the suggestions_manage permission Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95492 [details] [review] Bug 23590: Add the ability to notify a new manager Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95493 [details] [review] Bug 23590: Display managed date in 'Your purchase suggestions' Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95494 [details] [review] Bug 23590: Add lastmodification by and date columns No tests are provided for the changes made to SearchSuggestion. It is going to be remove very soon as it is super ugly... Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95495 [details] [review] Bug 23590: Add atomic update file Sponsored-by: BULAC - http://www.bulac.fr/
Created attachment 95496 [details] [review] Bug 23590: Remove JS error on the suggestion list The notify option is only available from the edit view Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95497 [details] [review] Bug 23590: Fix select manager on list for all tabs Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 95498 [details] [review] Bug 23590: DBIC Changes Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
This still applies, QA?
Onto the next one... Jonathan, can you please rebase?
Created attachment 98119 [details] [review] Bug 23590: Add ability to edit the manager of a suggestion Prior to this patch there was an hidden behavior that set the manager to the logged in user when a suggestion was edited. This patch proposes to let the librarian pick another manager. Other small adjustements will be added to polish this new behavior: * Create 2 new DB columns: suggestions.lastmodificationby and suggestion.lastmodificationdate * Choose a manager when editing a suggestion * Batch modify suggestions and set a manager for them * Let notify the new manager using the TO_PROCESS letter * Display the manageddate, lastmodificationby and lastmodificationdate info where appropriate This first patch adds a new "Select manager" link when editing a suggestion. Test plan for the whole patchset: 0/ a. Execute the update DB entry, generate the new DBIC file and restart all b. Create at least 2 patrons with the suggestions_manage permission 1/ Submit a new suggestion (OPAC or staff, not important) 2/ Accept it 3/ Edit it => "Last modification by" is empty => You see that you are the manager of this suggestion 4/ Click "Select manager" and search for a new manager => The patron search will only display patrons with the suggestions_manage permission 5/ Save 6/ Edit again => The manager is set to you, but there is a note saying that previously it was the patron you picked => The "Last modification by" is set to you 7/ Click "Keep existing manager" => The manager is now set to the previous manager 8/ Select another manager (which has a valid email address defined) 9/ Click the "notify" checkbox 10/ Save 11/ Confirm that a TO_PROCESS notice has been generated into the message_queue table 12/ Create at least one other suggestion 13/ List the suggestions => There is a 4th action column to assign a manager to several suggestions in one go. 14/ Use this new button and confirm that it works as expected 15/ Go to your purchase suggestion list (OPAC and staff) => You see the "managed date" displayed in a new column Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98120 [details] [review] Bug 23590: Clean TT variables for patrons Just a bit of cleaning Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98121 [details] [review] Bug 23590: Cleaning - Remove op_* vars Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98122 [details] [review] Bug 23590: Batch update manager for suggestions Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98123 [details] [review] Bug 23590: Only return patrons that have the suggestions_manage permission Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98124 [details] [review] Bug 23590: Add the ability to notify a new manager Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98126 [details] [review] Bug 23590: Display managed date in 'Your purchase suggestions' Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98127 [details] [review] Bug 23590: Add lastmodification by and date columns No tests are provided for the changes made to SearchSuggestion. It is going to be remove very soon as it is super ugly... Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98128 [details] [review] Bug 23590: Add atomic update file Sponsored-by: BULAC - http://www.bulac.fr/
Created attachment 98129 [details] [review] Bug 23590: Remove JS error on the suggestion list The notify option is only available from the edit view Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98130 [details] [review] Bug 23590: Fix select manager on list for all tabs Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98131 [details] [review] Bug 23590: DBIC Changes Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 98240 [details] [review] Bug 23590: (QA follow-up) Add missing filters and drop script type Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Looks like Katrin is on the case here.. so just uploading a followup to silence the warning from the QA scripts and leaving Katrin to finish :)
Created attachment 98267 [details] [review] Bug 23590: (QA follow-up) Fix some problems created by recent changes and updatedatabase statement - Ensure that the sequence of columns will be the same for new and updated installations (add AFTER ...) - Fix permissions (see bug 22868) - Fix column configuration (see 16784)
Created attachment 98268 [details] [review] Bug 23590: (QA follow-up) Fix some problems created by recent changes and updatedatabase statement - Ensure that the sequence of columns will be the same for new and updated installations (add AFTER ...) - Fix permissions (see bug 22868) - Fix column configuration (see 16784) - Remove '- ' displying before the date
Notes & questions 1) We might want to modify the bug title a little - in addition to allow changing the manager, this will also store the last person to change and date. Also gives a checkbox for TO_PROCESS. We really needs some release notes :) 2) lastmodificationdate and lastmodificationby are quite long. It might be nicer to used something more readable like: last_modified_on and last_modified_by. Not blocker for me, but putting the thought out there. 3) Is there a reason why we don't let the db update thelastmodificationdate? 4) Please check my follow-up Blockers 1) When I change the manager, the lastmodification* are not updated. 2) The TO_PROCESS email goes to the fund owner - this patch is reusing it for the manager. I believe this should be handled separately. ( Bug 13014: Notify budget owner on new suggestion - cronjob) 3) + if ( $notify ) { + my $patron = Koha::Patrons->find( $suggestion_only->{managedby} ); + my $email_address = $patron->notice_email_address; + if ($patron->notice_email_address) { + my $budget = C4::Budgets::GetBudget( $suggestion_only->{budgetid} ); $budget appears not to be used - possibly a copy&paste because of 2).
I think it would have been nice to split this: - Add last modification* - Add ability to change manager - Add ability to notify - last bit could still be pulled out maybe
(In reply to Katrin Fischer from comment #69) > Notes & questions > > 1) We might want to modify the bug title a little - in addition to allow > changing the manager, this will also store the last person to change and > date. Also gives a checkbox for TO_PROCESS. We really needs some release > notes :) Any suggestions? :) > 2) lastmodificationdate and lastmodificationby are quite long. It might be > nicer to used something more readable like: last_modified_on and > last_modified_by. Not blocker for me, but putting the thought out there. I followed the existing pattern As there were already 4 of those I thought it was better to keep names consistent. > 3) Is there a reason why we don't let the db update thelastmodificationdate? We can pick the max date of suggesteddate, manageddate, accepteddate, rejecteddate, is that was you have in mind? > 4) Please check my follow-up They look good, thanks! > Blockers > > 1) When I change the manager, the lastmodification* are not updated. That was expected (I think). The field lastmodification* are only updated when the suggestion is edited via the edit form. If we want to update it whenever the table is updated, the change must be moved to ModSuggestion. Séverine, can you confirm the behavior you expect? > 2) The TO_PROCESS email goes to the fund owner - this patch is reusing it > for the manager. I believe this should be handled separately. ( Bug 13014: > Notify budget owner on new suggestion - cronjob) That was part of the original specification. It makes sense to have 2 different notice templates. Something to suggest? UPDATE_MANAGER, SUGGEST_MANAGER, TO_PROCESS_MANAGER? > 3) > + if ( $notify ) { > + my $patron = Koha::Patrons->find( > $suggestion_only->{managedby} ); > + my $email_address = $patron->notice_email_address; > + if ($patron->notice_email_address) { > + my $budget = C4::Budgets::GetBudget( > $suggestion_only->{budgetid} ); > $budget appears not to be used - possibly a copy&paste because of 2). Indeed, follow-up coming.
Created attachment 98279 [details] [review] Bug 23590: remove unused variable
Created attachment 100788 [details] [review] Bug 23590: Add ability to edit the manager of a suggestion Prior to this patch there was an hidden behavior that set the manager to the logged in user when a suggestion was edited. This patch proposes to let the librarian pick another manager. Other small adjustements will be added to polish this new behavior: * Create 2 new DB columns: suggestions.lastmodificationby and suggestion.lastmodificationdate * Choose a manager when editing a suggestion * Batch modify suggestions and set a manager for them * Let notify the new manager using the TO_PROCESS letter * Display the manageddate, lastmodificationby and lastmodificationdate info where appropriate This first patch adds a new "Select manager" link when editing a suggestion. Test plan for the whole patchset: 0/ a. Execute the update DB entry, generate the new DBIC file and restart all b. Create at least 2 patrons with the suggestions_manage permission 1/ Submit a new suggestion (OPAC or staff, not important) 2/ Accept it 3/ Edit it => "Last modification by" is empty => You see that you are the manager of this suggestion 4/ Click "Select manager" and search for a new manager => The patron search will only display patrons with the suggestions_manage permission 5/ Save 6/ Edit again => The manager is set to you, but there is a note saying that previously it was the patron you picked => The "Last modification by" is set to you 7/ Click "Keep existing manager" => The manager is now set to the previous manager 8/ Select another manager (which has a valid email address defined) 9/ Click the "notify" checkbox 10/ Save 11/ Confirm that a TO_PROCESS notice has been generated into the message_queue table 12/ Create at least one other suggestion 13/ List the suggestions => There is a 4th action column to assign a manager to several suggestions in one go. 14/ Use this new button and confirm that it works as expected 15/ Go to your purchase suggestion list (OPAC and staff) => You see the "managed date" displayed in a new column Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100789 [details] [review] Bug 23590: Clean TT variables for patrons Just a bit of cleaning Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100790 [details] [review] Bug 23590: Cleaning - Remove op_* vars Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100791 [details] [review] Bug 23590: Batch update manager for suggestions Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100792 [details] [review] Bug 23590: Only return patrons that have the suggestions_manage permission Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100793 [details] [review] Bug 23590: Add the ability to notify a new manager Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100794 [details] [review] Bug 23590: Add lastmodification by and date columns No tests are provided for the changes made to SearchSuggestion. It is going to be remove very soon as it is super ugly... Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100795 [details] [review] Bug 23590: Add atomic update file Sponsored-by: BULAC - http://www.bulac.fr/
Created attachment 100796 [details] [review] Bug 23590: Remove JS error on the suggestion list The notify option is only available from the edit view Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100797 [details] [review] Bug 23590: Fix select manager on list for all tabs Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100798 [details] [review] Bug 23590: DBIC Changes Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100799 [details] [review] Bug 23590: (QA follow-up) Add missing filters and drop script type Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 100800 [details] [review] Bug 23590: (QA follow-up) Fix some problems created by recent changes and updatedatabase statement - Ensure that the sequence of columns will be the same for new and updated installations (add AFTER ...) - Fix permissions (see bug 22868) - Fix column configuration (see 16784) - Remove '- ' displying before the date
Created attachment 100801 [details] [review] Bug 23590: remove unused variable
Created attachment 100802 [details] [review] Bug 23590: Update new fields with relevant values We are filling lastmodificationdate and lastmodificationby depending on the max dates between manageddate, accepteddate and rejecteddate
Created attachment 100803 [details] [review] Bug 23590: Fix ergonomic issue if last modification by is empty
Created attachment 100804 [details] [review] Bug 23590: Create a separate template notice NOTIFY_MANAGER To separate the two feature we want to create a distinct template notice. A new NOTIFY_MANAGER notice is added. A follow-up patch will be added for other languages, when this one will be approved by QA.
(In reply to Jonathan Druart from comment #71) > (In reply to Katrin Fischer from comment #69) > > Notes & questions > > > > 1) We might want to modify the bug title a little - in addition to allow > > changing the manager, this will also store the last person to change and > > date. Also gives a checkbox for TO_PROCESS. We really needs some release > > notes :) > > Any suggestions? :) > > > 2) lastmodificationdate and lastmodificationby are quite long. It might be > > nicer to used something more readable like: last_modified_on and > > last_modified_by. Not blocker for me, but putting the thought out there. > > I followed the existing pattern As there were already 4 of those I thought > it was better to keep names consistent. > > > 3) Is there a reason why we don't let the db update thelastmodificationdate? > > We can pick the max date of suggesteddate, manageddate, accepteddate, > rejecteddate, is that was you have in mind? > > > 4) Please check my follow-up > > They look good, thanks! > > > Blockers > > > > 1) When I change the manager, the lastmodification* are not updated. > > That was expected (I think). The field lastmodification* are only updated > when the suggestion is edited via the edit form. If we want to update it > whenever the table is updated, the change must be moved to ModSuggestion. > > Séverine, can you confirm the behavior you expect? > > > 2) The TO_PROCESS email goes to the fund owner - this patch is reusing it > > for the manager. I believe this should be handled separately. ( Bug 13014: > > Notify budget owner on new suggestion - cronjob) > > That was part of the original specification. > It makes sense to have 2 different notice templates. Something to suggest? > UPDATE_MANAGER, SUGGEST_MANAGER, TO_PROCESS_MANAGER? > > > 3) > > + if ( $notify ) { > > + my $patron = Koha::Patrons->find( > > $suggestion_only->{managedby} ); > > + my $email_address = $patron->notice_email_address; > > + if ($patron->notice_email_address) { > > + my $budget = C4::Budgets::GetBudget( > > $suggestion_only->{budgetid} ); > > $budget appears not to be used - possibly a copy&paste because of 2). > > Indeed, follow-up coming.
(In reply to Jonathan Druart from comment #71) > (In reply to Katrin Fischer from comment #69) > > Notes & questions > > > > 1) We might want to modify the bug title a little - in addition to allow > > changing the manager, this will also store the last person to change and > > date. Also gives a checkbox for TO_PROCESS. We really needs some release > > notes :) > > Any suggestions? :) Done, hope that makes sense. > > 3) Is there a reason why we don't let the db update thelastmodificationdate? > > We can pick the max date of suggesteddate, manageddate, accepteddate, > rejecteddate, is that was you have in mind? I added a follow-up to update the 2 new columns. > > Blockers > > > > 1) When I change the manager, the lastmodification* are not updated. > > That was expected (I think). The field lastmodification* are only updated > when the suggestion is edited via the edit form. If we want to update it > whenever the table is updated, the change must be moved to ModSuggestion. > > Séverine, can you confirm the behavior you expect? Still waiting for an answer here, but I think we could fix that on a follow-up if needed. The change will be minor compared to the work needed to keep this patchset up-to-date with master. > > 2) The TO_PROCESS email goes to the fund owner - this patch is reusing it > > for the manager. I believe this should be handled separately. ( Bug 13014: > > Notify budget owner on new suggestion - cronjob) > > That was part of the original specification. > It makes sense to have 2 different notice templates. Something to suggest? > UPDATE_MANAGER, SUGGEST_MANAGER, TO_PROCESS_MANAGER? Done, a new template notice "NOTIFY_MANAGER" has been added. Sending back to the QA queue.
Almost there - thx for the work on the notice. 1) I think this slipped in from another feature in the tree (22784): + <label for="archived" style="display: inline;">Include archived:</label> + <input type="checkbox" id="archived" name="filter_archived" value="archived" title="Include archived suggestions in the search" /> + </li> + <li> 2) From comment#91 > > 1) When I change the manager, the lastmodification* are not updated. > > That was expected (I think). The field lastmodification* are only updated > when the suggestion is edited via the edit form. If we want to update it > whenever the table is updated, the change must be moved to ModSuggestion. > > Séverine, can you confirm the behavior you expect? Still waiting for an answer here, but I think we could fix that on a follow-up if needed. The change will be minor compared to the work needed to keep this patchset up-to-date with master. -- This one is confusing to me and I keep wondering if this should have been more than one bug :) So at the moment, in suggestions we got: `date` timestamp NOT NULL default CURRENT_TIMESTAMP on update CURRENT_TIMESTAMP, -- date and time the suggestion was updated lastmodificationby INT(11) default NULL, -- borrowernumber for the librarian who edit the suggestion for the last time lastmodificationdate date default NULL, -- date of the last modification So date is actually always updated and should probably be renamed 'datetime' or similar. What is the use case for the additional lastmodificationdate? From the description it's not clear to me. Do we really need it if we have date? Please feel free to get others to look at this - 1) should be a quick fix, 2) is a bit puzzling to me right now!
Created attachment 100846 [details] [review] Bug 23590: Fix select manager on list for all tabs Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100847 [details] [review] Bug 23590: DBIC Changes Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Created attachment 100848 [details] [review] Bug 23590: (QA follow-up) Add missing filters and drop script type Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 100849 [details] [review] Bug 23590: (QA follow-up) Fix some problems created by recent changes and updatedatabase statement - Ensure that the sequence of columns will be the same for new and updated installations (add AFTER ...) - Fix permissions (see bug 22868) - Fix column configuration (see 16784) - Remove '- ' displying before the date
Created attachment 100850 [details] [review] Bug 23590: remove unused variable
Created attachment 100851 [details] [review] Bug 23590: Update new fields with relevant values We are filling lastmodificationdate and lastmodificationby depending on the max dates between manageddate, accepteddate and rejecteddate
Created attachment 100852 [details] [review] Bug 23590: Fix ergonomic issue if last modification by is empty
Created attachment 100853 [details] [review] Bug 23590: Create a separate template notice NOTIFY_MANAGER To separate the two feature we want to create a distinct template notice. A new NOTIFY_MANAGER notice is added. A follow-up patch will be added for other languages, when this one will be approved by QA.
(In reply to Katrin Fischer from comment #92) > Almost there - thx for the work on the notice. > > 1) I think this slipped in from another feature in the tree (22784): > > + <label for="archived" style="display: > inline;">Include archived:</label> > + <input type="checkbox" id="archived" > name="filter_archived" value="archived" title="Include archived suggestions > in the search" /> > + </li> > + <li> Oops, fixed. > 2) From comment#91 > > > > 1) When I change the manager, the lastmodification* are not updated. > > > > That was expected (I think). The field lastmodification* are only updated > > when the suggestion is edited via the edit form. If we want to update it > > whenever the table is updated, the change must be moved to ModSuggestion. > > > > Séverine, can you confirm the behavior you expect? > > Still waiting for an answer here, but I think we could fix that on a > follow-up if needed. The change will be minor compared to the work needed to > keep this patchset up-to-date with master. > -- > > This one is confusing to me and I keep wondering if this should have been > more than one bug :) So at the moment, in suggestions we got: > > `date` timestamp NOT NULL default CURRENT_TIMESTAMP on update > CURRENT_TIMESTAMP, -- date and time the suggestion was updated > > lastmodificationby INT(11) default NULL, -- borrowernumber for the > librarian who edit the suggestion for the last time > lastmodificationdate date default NULL, -- date of the last modification > > So date is actually always updated and should probably be renamed 'datetime' > or similar. What is the use case for the additional lastmodificationdate? > From the description it's not clear to me. Do we really need it if we have > date? suggestions.date must be renamed timestamp, I agree with that. I can commit on renaming it on a separate bug report. The lastmodificationdate is the date when lastmodificationby has been updated. And they are updated when a suggestion is modified, explicitly from the UI. It's not set when the suggestion is created, but only when modified. Also not updated when the status is set to ORDERED, for instance. As said early, one advantage is also to follow the same pattern of the 4 existing fields.
Lately, we've been sticking to: created_on created_by updated_on updated_by I understand this is not exactly the same as updated_on but... lastmodificationdate? I would stick to updated_on and find something else for the status change (e.g. status_updated_on) My 2 cents
I would like to stick with the existing pattern: `suggestedby` int(11) DEFAULT NULL, `suggesteddate` date NOT NULL, `managedby` int(11) DEFAULT NULL, `manageddate` date DEFAULT NULL, `acceptedby` int(11) DEFAULT NULL, `accepteddate` date DEFAULT NULL, `rejectedby` int(11) DEFAULT NULL, `rejecteddate` date DEFAULT NULL,
If they need to be reworded, they will be reworded on a follow-up bug report. But all at once. I will do if requested. Do not you think it's better than having 2 different patterns for the same table?
(In reply to Jonathan Druart from comment #104) > If they need to be reworded, they will be reworded on a follow-up bug > report. But all at once. I will do if requested. > > Do not you think it's better than having 2 different patterns for the same > table? I'm fine with doing it on a separate bug report.
I have opened bug 24887.
Dear All, very sorry for the delayed answer on this : Séverine is not currently working so we left it unanswered... I will try to answer it, but please don't hesitate to ask for more precisions if needed : 1) When I change the manager, the lastmodification* are not updated. > > That was expected (I think). The field lastmodification* are only updated > when the suggestion is edited via the edit form. If we want to update it > whenever the table is updated, the change must be moved to ModSuggestion. > > Séverine, can you confirm the behavior you expect? Still waiting for an answer here, but I think we could fix that on a follow-up if needed. The change will be minor compared to the work needed to keep this patchset up-to-date with master. > Yes it was the expected behavior. We don't need the last modification to be updated as long as it is not updated with the edit form. We consider that the suggestion has not really begun to be processed as long as the edit form has not beeing used : the relevant librarian to process it was not "manager" of the suggestion. Regarding statistics for delay of suggestion management, it will be more useful not to consider this as a last date of modification. Does it make sense ? Thanks again and sorry for the delay !
One step closer, sorry, found another thing: In the patron search for new manager, I can only find patrons with superlibrarian, but not with manage_suggestions
Created attachment 101539 [details] [review] Bug 23590: Fix 'select manager' when creation a suggestion On creating the TT var managedby_patron does not exist yet
(In reply to Katrin Fischer from comment #108) > One step closer, sorry, found another thing: > > In the patron search for new manager, I can only find patrons with > superlibrarian, but not with manage_suggestions That's a known bug, see bug 24964.
(In reply to Jonathan Druart from comment #109) > Created attachment 101539 [details] [review] [review] > Bug 23590: Fix 'select manager' when creation a suggestion > > On creating the TT var managedby_patron does not exist yet That's for another bug I found when trying again.
Created attachment 101548 [details] [review] Bug 23590: DBIC Changes
Created attachment 101588 [details] [review] Bug 23590: Add ability to edit the manager of a suggestion Prior to this patch there was an hidden behavior that set the manager to the logged in user when a suggestion was edited. This patch proposes to let the librarian pick another manager. Other small adjustements will be added to polish this new behavior: * Create 2 new DB columns: suggestions.lastmodificationby and suggestion.lastmodificationdate * Choose a manager when editing a suggestion * Batch modify suggestions and set a manager for them * Let notify the new manager using the TO_PROCESS letter * Display the manageddate, lastmodificationby and lastmodificationdate info where appropriate This first patch adds a new "Select manager" link when editing a suggestion. Test plan for the whole patchset: 0/ a. Execute the update DB entry, generate the new DBIC file and restart all b. Create at least 2 patrons with the suggestions_manage permission 1/ Submit a new suggestion (OPAC or staff, not important) 2/ Accept it 3/ Edit it => "Last modification by" is empty => You see that you are the manager of this suggestion 4/ Click "Select manager" and search for a new manager => The patron search will only display patrons with the suggestions_manage permission 5/ Save 6/ Edit again => The manager is set to you, but there is a note saying that previously it was the patron you picked => The "Last modification by" is set to you 7/ Click "Keep existing manager" => The manager is now set to the previous manager 8/ Select another manager (which has a valid email address defined) 9/ Click the "notify" checkbox 10/ Save 11/ Confirm that a TO_PROCESS notice has been generated into the message_queue table 12/ Create at least one other suggestion 13/ List the suggestions => There is a 4th action column to assign a manager to several suggestions in one go. 14/ Use this new button and confirm that it works as expected 15/ Go to your purchase suggestion list (OPAC and staff) => You see the "managed date" displayed in a new column Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101589 [details] [review] Bug 23590: Clean TT variables for patrons Just a bit of cleaning Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101590 [details] [review] Bug 23590: Cleaning - Remove op_* vars Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101591 [details] [review] Bug 23590: Batch update manager for suggestions Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101592 [details] [review] Bug 23590: Only return patrons that have the suggestions_manage permission Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101593 [details] [review] Bug 23590: Add the ability to notify a new manager Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101594 [details] [review] Bug 23590: Add lastmodification by and date columns No tests are provided for the changes made to SearchSuggestion. It is going to be remove very soon as it is super ugly... Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101595 [details] [review] Bug 23590: Add atomic update file Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101596 [details] [review] Bug 23590: Remove JS error on the suggestion list The notify option is only available from the edit view Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101597 [details] [review] Bug 23590: Fix select manager on list for all tabs Sponsored-by: BULAC - http://www.bulac.fr/ Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101598 [details] [review] Bug 23590: (QA follow-up) Add missing filters and drop script type Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101599 [details] [review] Bug 23590: (QA follow-up) Fix some problems created by recent changes and updatedatabase statement - Ensure that the sequence of columns will be the same for new and updated installations (add AFTER ...) - Fix permissions (see bug 22868) - Fix column configuration (see 16784) - Remove '- ' displying before the date Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101600 [details] [review] Bug 23590: remove unused variable Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101601 [details] [review] Bug 23590: Update new fields with relevant values We are filling lastmodificationdate and lastmodificationby depending on the max dates between manageddate, accepteddate and rejecteddate Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101602 [details] [review] Bug 23590: Fix ergonomic issue if last modification by is empty Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101603 [details] [review] Bug 23590: Create a separate template notice NOTIFY_MANAGER To separate the two feature we want to create a distinct template notice. A new NOTIFY_MANAGER notice is added. A follow-up patch will be added for other languages, when this one will be approved by QA. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101604 [details] [review] Bug 23590: Fix 'select manager' when creation a suggestion On creating the TT var managedby_patron does not exist yet Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101605 [details] [review] Bug 23590: DBIC Changes Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 101606 [details] [review] Bug 23590: (QA follow-up) Fix links to patron records (TT variable confusion) Fixes links to the patron accounts from the edit and list view of suggestions. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Nice work everyone! Pushed to master for 20.05
Created attachment 101695 [details] [review] Bug 23590: Add notice template for translated installer files
not backported to 19.11.x
Should the notice be named NOTIFY_MANAGER_CHANGE? Just in case we want to add other notices.
(In reply to Tomás Cohen Arazi from comment #135) > Should the notice be named NOTIFY_MANAGER_CHANGE? Just in case we want to > add other notices. This has already been released, so I think better to change when we actually implement another notice.