Summary: | Add reasons on edit suggestion page | ||
---|---|---|---|
Product: | Koha | Reporter: | Koha Team University Lyon 3 <koha> |
Component: | Acquisitions | Assignee: | Alex Arnaud <alex.arnaud> |
Status: | CLOSED FIXED | QA Contact: | Nick Clemens (kidclamp) <nick> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | alex.arnaud, jonathan.druart, koha, nick, sandboxes |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | Trivial patch | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: | Circulation function: | ||
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 University Lyon 3
2017-04-07 13:20:39 UTC
Created attachment 63384 [details] [review] Bug 18399 - Reason is missing in suggestion management part This patch adds a form field for suggestion status "reason" to the suggestion edit form. To test, edit an existing suggestion and try entering a suggestion status explanation in the new form field. Your changes should be saved correctly. Created attachment 67608 [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 Created attachment 67609 [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 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 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 (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. 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 Patch tested with a sandbox, by Sonia BOUIS <sonia.bouis@univ-lyon3.fr> 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> 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 (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 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 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> Added Sonia's sign off back to patch (one line change) Works well, 'Cancel' does seem awkward but like the feature, not a blocker You need to escape the string somewhere, try to enter "a reason" (with the quote) Created attachment 67935 [details] [review] Bug 18399 - Followup - escape reason input Pushed to master for 17.11, thanks to everybody involved! |