Bug 18399

Summary: Add reasons on edit suggestion page
Product: Koha Reporter: Koha Team Lyon 3 <koha>
Component: AcquisitionsAssignee: Alex Arnaud <alex.arnaud>
Status: Pushed to Master --- QA Contact: Nick Clemens <nick>
Severity: enhancement    
Priority: P5 - low CC: alex.arnaud, jonathan.druart, katrin.fischer, koha, nick, sandboxes
Version: master   
Hardware: All   
OS: All   
Whiteboard:
Change sponsored?: --- Patch complexity: Trivial patch
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Attachments: Bug 18399 - Reason is missing in suggestion management part
Bug 18399 - Add reasons in edit suggestion page
Bug 18399 - Add reasons in edit suggestion page
Bug 18399 - Add reasons in edit suggestion page
Bug 18399 - Add reasons in edit suggestion page
Bug 18399 - Add reasons in edit suggestion page
Bug 18399 - Add reasons in edit suggestion page
Bug 18399 - Followup - escape reason input

Description Koha Team Lyon 3 2017-04-07 13:20:39 UTC
When we want to deal a suggestion by editing it (suggestion.pl?suggestionid=XX&op=edit), we can change the status but we can't add a reason.

It's a pitty because changing the status would trigger the workflow and mail sending and in email we provide the reason if the suggestion is rejected...
So patron will not have the reason if we change the status here.
Comment 1 Owen Leonard 2017-05-11 14:14:33 UTC Comment hidden (obsolete)
Comment 2 Alex Arnaud 2017-10-04 15:10:13 UTC Comment hidden (obsolete)
Comment 3 Alex Arnaud 2017-10-04 15:15:40 UTC Comment hidden (obsolete)
Comment 4 Koha Team Lyon 3 2017-10-05 09:40:20 UTC
Hello,
I've followed the test plan. It works as described except when we re-edit a suggestion rejected with an "other reason". We can see the reason typed when editing the suggestion but it's lost when saving the suggestion.

Sonia
Comment 5 Alex Arnaud 2017-10-05 12:28:12 UTC
Created attachment 67633 [details] [review]
Bug 18399 - Add reasons in edit suggestion page

Test plan:

 - Fill the authorised values's category 'SUGGEST' with reason
 - Edit a pending suggestion. Set the status to rejected
   and select a reason
 - Check the reason has been saved (i.e in the suggestions
   table of rejected tab)
 - Check in the corresponding message_queue that the reason
   is here
 - Re-edit the same suggestion
 - Check that the correct reason is selected by defaut
 - Edit an other pending suggetion. Set the status to rejected,
   select other reason and fill the input text
 - Re-edit the previews suggestion and make sure the input
   "other" is displayed by default with the correct value
Comment 6 Alex Arnaud 2017-10-05 12:29:43 UTC
(In reply to Koha Team Lyon 3 from comment #4)
> Hello,
> I've followed the test plan. It works as described except when we re-edit a
> suggestion rejected with an "other reason". We can see the reason typed when
> editing the suggestion but it's lost when saving the suggestion.
> 
> Sonia

Good catch Sonia. Fixed. You can test again.
Comment 7 Koha Team Lyon 3 2017-10-05 13:33:48 UTC
I tested it again following the test plan and everything is OK.
When editing a suggestion, we can choose a reason for reject (in authorized value or free reason with other). This reason is correctly displayed in message (verified in message_queue).
We can re-edit and save the suggestion, reasons are kept.

Thank you Alex
Comment 8 sandboxes@biblibre.com 2017-10-05 13:35:12 UTC
Patch tested with a sandbox, by Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Comment 9 sandboxes@biblibre.com 2017-10-05 13:35:33 UTC
Created attachment 67635 [details] [review]
Bug 18399 - Add reasons in edit suggestion page

Test plan:

 - Fill the authorised values's category 'SUGGEST' with reason
 - Edit a pending suggestion. Set the status to rejected
   and select a reason
 - Check the reason has been saved (i.e in the suggestions
   table of rejected tab)
 - Check in the corresponding message_queue that the reason
   is here
 - Re-edit the same suggestion
 - Check that the correct reason is selected by defaut
 - Edit an other pending suggetion. Set the status to rejected,
   select other reason and fill the input text
 - Re-edit the previews suggestion and make sure the input
   "other" is displayed by default with the correct value

Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Comment 10 Nick Clemens 2017-10-07 01:04:35 UTC
Hi Alex,

Why not use Koha::Template::Plugin::AuthorisedValues rather than processing the category in the pl and passing it?

$(this).next("#other_reason").show();
should be the same as
$("#other_reason").show();

I like the way the 'Other' suggestions work but 'Cancel' is a confusing wording (if I am editing an existing suggestion I am not 'cancelling' but 'changing') I would suggest 'Choose an standard value' or something similar

Nice and needed feature.

-Nick
Comment 11 Alex Arnaud 2017-10-09 08:34:45 UTC
(In reply to Nick Clemens from comment #10)
> Hi Alex,
> 
> Why not use Koha::Template::Plugin::AuthorisedValues rather than processing
> the category in the pl and passing it?
GetAuthorisedValues("SUGGEST") was already there in the pl. I just moved it to make it available everywhere. Also i need these authorised values to check if a suggestion has a common or other reason before displaying it. I think this peace of code is more easily "writable" in the pl than in the template.
> 
> $(this).next("#other_reason").show();
> should be the same as
> $("#other_reason").show();
Right!
> 
> I like the way the 'Other' suggestions work but 'Cancel' is a confusing
> wording (if I am editing an existing suggestion I am not 'cancelling' but
> 'changing') I would suggest 'Choose an standard value' or something similar
I totaly agree that the word is not relevant (i wondered the same while making the patch). But i thought the best was to keep consitency between the 2 pages (list of suggestions / editing one suggestion). Right?
> 
> Nice and needed feature.
> 
> -Nick
Comment 12 Alex Arnaud 2017-10-09 08:41:07 UTC
Created attachment 67832 [details] [review]
Bug 18399 - Add reasons in edit suggestion page

Test plan:

 - Fill the authorised values's category 'SUGGEST' with reason
 - Edit a pending suggestion. Set the status to rejected
   and select a reason
 - Check the reason has been saved (i.e in the suggestions
   table of rejected tab)
 - Check in the corresponding message_queue that the reason
   is here
 - Re-edit the same suggestion
 - Check that the correct reason is selected by defaut
 - Edit an other pending suggetion. Set the status to rejected,
   select other reason and fill the input text
 - Re-edit the previews suggestion and make sure the input
   "other" is displayed by default with the correct value
Comment 13 Nick Clemens 2017-10-10 16:21:15 UTC
Created attachment 67885 [details] [review]
Bug 18399 - Add reasons in edit suggestion page

Test plan:

 - Fill the authorised values's category 'SUGGEST' with reason
 - Edit a pending suggestion. Set the status to rejected
   and select a reason
 - Check the reason has been saved (i.e in the suggestions
   table of rejected tab)
 - Check in the corresponding message_queue that the reason
   is here
 - Re-edit the same suggestion
 - Check that the correct reason is selected by defaut
 - Edit an other pending suggetion. Set the status to rejected,
   select other reason and fill the input text
 - Re-edit the previews suggestion and make sure the input
   "other" is displayed by default with the correct value

Signed-off-by: Sonia BOUIS <sonia.bouis@univ-lyon3.fr>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2017-10-10 16:22:09 UTC
Added Sonia's sign off back to patch (one line change)

Works well, 'Cancel' does seem awkward but like the feature, not a blocker
Comment 15 Jonathan Druart 2017-10-10 17:10:47 UTC
You need to escape the string somewhere, try to enter "a reason" (with the quote)
Comment 16 Alex Arnaud 2017-10-11 10:17:05 UTC
Created attachment 67935 [details] [review]
Bug 18399 - Followup - escape reason input
Comment 17 Jonathan Druart 2017-10-11 16:53:41 UTC
Pushed to master for 17.11, thanks to everybody involved!