Bug 13380 - Auto fill order cancellation reasons from authorised values
Summary: Auto fill order cancellation reasons from authorised values
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 7162 13369 13379
Blocks:
  Show dependency treegraph
 
Reported: 2014-12-03 12:10 UTC by Jonathan Druart
Modified: 2015-12-03 22:11 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13380: Fill order cancellation reasons from AV (4.59 KB, patch)
2014-12-03 12:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13380: Add the AV in the install files (1.44 KB, patch)
2014-12-03 12:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13380: Fill order cancellation reasons from AV (4.62 KB, patch)
2014-12-05 13:56 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13380: Add the AV in the install files (1.49 KB, patch)
2014-12-28 06:17 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 13380: Fill order cancellation reasons from AV (4.67 KB, patch)
2014-12-28 06:18 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 13380: Add the AV in the install files (1.58 KB, patch)
2015-02-06 12:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13380: Fill order cancellation reasons from AV (4.75 KB, patch)
2015-02-06 12:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13380: Wording changes (2.13 KB, patch)
2015-02-10 08:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13380: Add ORDER_CANCELLATION_REASON AV to all language files (7.10 KB, patch)
2015-02-10 08:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13380: Wording changes (2.13 KB, patch)
2015-02-10 09:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13380: 3.18.x Fix previous DB entry (1.28 KB, patch)
2015-03-23 16:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
[master] Bug 13380: Follow-up - only run database update, when it hasn't been run already (4.34 KB, patch)
2015-03-23 20:51 UTC, Katrin Fischer
Details | Diff | Splinter Review
[master] Bug 13380: Follow-up - only run database update, when it hasn't been run already (4.39 KB, patch)
2015-03-24 13:52 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 13380: Follow-up - only run database update, when it hasn't been run already (4.46 KB, patch)
2015-03-30 11:20 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 Jonathan Druart 2014-12-03 12:10:19 UTC
Since bug 7162, it's possible to give a cancellation reason on deleting an order.
This would be better to fill an authorised values category with the different possible values.
Like that we will avoid to have duplicate or similar reasons.
Comment 1 Jonathan Druart 2014-12-03 12:25:04 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2014-12-03 12:25:08 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2014-12-05 13:56:21 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2014-12-08 21:15:21 UTC
I am not sure about enforcing an authorized value here - maybe we treat it similar to the bsort authorized values? The statistic fields are normal inputs until you add values for the authorized value.
Comment 5 Jonathan Druart 2014-12-09 08:21:53 UTC
(In reply to Katrin Fischer from comment #4)
> I am not sure about enforcing an authorized value here - maybe we treat it
> similar to the bsort authorized values? The statistic fields are normal
> inputs until you add values for the authorized value.

Yes maybe, we could. But how to deal with existing values?
I worked on the assumption that there won't be many and many reasons.
Comment 6 Katrin Fischer 2014-12-09 08:30:34 UTC
I am not sure, but I think as we introduced it as a free text field people might have left unstructured comments and could end up with quite a long list for the authorized value - which might also not very welcome.

Maybe put the SQL for the update on the wiki and leave as is? 
I would make the value show up in the list of authorized values but leave it empty I think.
Comment 7 Jonathan Druart 2014-12-10 10:11:46 UTC
(In reply to Katrin Fischer from comment #6)
> I am not sure, but I think as we introduced it as a free text field people
> might have left unstructured comments and could end up with quite a long
> list for the authorized value - which might also not very welcome.

I am not sure a lot of people already used it, it has only been pushed in 3.18.
200 characters is quite long for an order cancellation reason :)

> Maybe put the SQL for the update on the wiki and leave as is? 
> I would make the value show up in the list of authorized values but leave it
> empty I think.

I don't understand what you meant here, sorry :-/

What about a warning on updating the DB : "the field XXX for order YYY with value ZZZ has been truncated"?

Or we could extend authorised_values.lib to a TEXT? But I don't like this solution.
Comment 8 Katrin Fischer 2014-12-10 11:16:49 UTC
I think the question is if this is going to be in 3.18.x or as a new enhancement in 3.20. And by then... libraries will have used it. I think currently as a input field it invites for longer unstructured comments.

What I meant is that I would not transform the existing reasons into an authorized value for existing installations but leave things as they are and have the auth_value as a non-enforced option. That would also avoid the truncation problem.

Maybe we should ask for opinions on the list? Some good feedback there recently.
Comment 9 Chris Cormack 2014-12-28 06:11:30 UTC
(In reply to Katrin Fischer from comment #8)
> I think the question is if this is going to be in 3.18.x or as a new
> enhancement in 3.20. And by then... libraries will have used it. I think
> currently as a input field it invites for longer unstructured comments.
> 
> What I meant is that I would not transform the existing reasons into an
> authorized value for existing installations but leave things as they are and
> have the auth_value as a non-enforced option. That would also avoid the
> truncation problem.
> 
> Maybe we should ask for opinions on the list? Some good feedback there
> recently.

I'm happy to classify this as a bugfix and push it for 3.18.3 there shouldn't have been too much usage of it by then.
Comment 10 Chris Cormack 2014-12-28 06:17:59 UTC Comment hidden (obsolete)
Comment 11 Chris Cormack 2014-12-28 06:18:17 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2015-01-02 18:14:12 UTC
QA Issue: The new authorized values need to be added to each translation.
Comment 13 Jonathan Druart 2015-01-05 09:47:20 UTC
(In reply to Kyle M Hall from comment #12)
> QA Issue: The new authorized values need to be added to each translation.

Kyle,
As you know, this change is very boring to do.
I would prefer to wait for the QA step passes and the wording is validated before to add the changes to all languages.
I hope it makes sense for you.
Comment 14 Katrin Fischer 2015-01-28 21:58:48 UTC
Hi Kyle, could you give Jonathan feedback on the wording?
Comment 15 Kyle M Hall 2015-02-06 12:55:44 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2015-02-06 12:56:25 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2015-02-06 12:59:07 UTC
(In reply to Jonathan Druart from comment #13)
> (In reply to Kyle M Hall from comment #12)
> > QA Issue: The new authorized values need to be added to each translation.
> 
> Kyle,
> As you know, this change is very boring to do.
> I would prefer to wait for the QA step passes and the wording is validated
> before to add the changes to all languages.
> I hope it makes sense for you.

In that case, I would suggest

No reason => No reason provided
Sold out => Out of stock
Restocking => Already ordered

Does that make sense?
Comment 18 Katrin Fischer 2015-02-06 13:09:23 UTC
Wondering, what is meant here?
Restocking => Already ordered
Comment 19 Jonathan Druart 2015-02-10 08:40:46 UTC
(In reply to Katrin Fischer from comment #18)
> Wondering, what is meant here?
> Restocking => Already ordered

Yes I am not sure it's what I meant.
Restocking means the supplier does not have the item and you want to cancel it.
Comment 20 Jonathan Druart 2015-02-10 08:48:16 UTC Comment hidden (obsolete)
Comment 21 Jonathan Druart 2015-02-10 08:50:05 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2015-02-10 08:55:02 UTC
Maybe we could just leave it out and go with 2 status?
Restocking and out of stock seem very similar.
Comment 23 Katrin Fischer 2015-02-10 09:00:13 UTC
Hi Joubu, 
I think there is a little typo:
('ORDER_CANCELLATION_REASON', 1, 'Old of stock'),
it's in a/installer/data/mysql/updatedatabase.pl
Comment 24 Jonathan Druart 2015-02-10 09:33:16 UTC Comment hidden (obsolete)
Comment 25 Katrin Fischer 2015-03-09 13:22:45 UTC
The plan was to have this in the maintenance releases (see comment #9) - Tomas, could you please push soon? Thx!
Comment 26 Tomás Cohen Arazi 2015-03-15 12:12:13 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 27 Chris Cormack 2015-03-17 08:56:51 UTC
Pushed to 3.18.x will be in 3.18.5
Comment 28 Katrin Fischer 2015-03-23 14:49:00 UTC
Sorry to say this, but we missed a dependency :(
Bug 13379 - changes the category to varchar(16).
This means that on updating to 3.18.05 the value is too short and the code ends up being ORDER_CANCELLATI
Comment 29 Katrin Fischer 2015-03-23 15:18:56 UTC
I am afraid we need a follow up patch with another database update:
- changing the size of the 2 colums like in patch 13379
- changing the shortened codes the correct full length codes

Not sure if there is a better solution, but I don't see one right now.
Comment 30 Katrin Fischer 2015-03-23 15:24:38 UTC
In the GUI the problem manifests as follows:

- the cancellation reason input field shows "0"
- an entered cancellation reason (free text) saves correctly to the database 
- the entered reason is not shown on the cancelled order line

So basically we are back to as before, with the change that the display is affected.
Comment 31 Jonathan Druart 2015-03-23 16:03:43 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2015-03-23 16:17:15 UTC
Comment on attachment 37144 [details] [review]
Bug 13380: 3.18.x Fix previous DB entry

Review of attachment 37144 [details] [review]:
-----------------------------------------------------------------

::: installer/data/mysql/updatedatabase.pl
@@ +9733,5 @@
>  
> +# IMPORTANT
> +# Here should be DB entry from 13379
> +
> +$DBversion = "3.18.05.001";

Should be 002 of course.
Comment 33 Katrin Fischer 2015-03-23 20:51:53 UTC Comment hidden (obsolete)
Comment 34 Jonathan Druart 2015-03-24 13:52:40 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2015-03-25 08:21:49 UTC
Obsoleted the already pushed patches - the last one for master needs QA now.
Comment 36 Kyle M Hall 2015-03-30 11:20:05 UTC
Created attachment 37363 [details] [review]
[PASSED QA] Bug 13380: Follow-up - only run database update, when it hasn't been run already

As the ORDER_CANCELLATION_REASON authorised value has been
backported to 3.18.5, we need to make sure that the database
update is only run once.

This patch adds a check for the existance of the authorised
value in order to decide if the databaes update can be run.

To test:
- Create a branch 3.19.00.016
- Make sure you have a few orders where the cancellationreason
  is set (cancel an order, enter a reason)
- Update your branch to master + this patch
- Run the web installer
- ORDER_CANCELLATION_REASON authorised value category is created
- Reset your system preference Version to 3.19.00.015
- Run database update again - no changes are made, all is fine

Or:
- Create a branch 3.18.04 with cancelled orders
- Update to latest 3.18.x
- Make sure ORDER_CANCELLATION_REASON has been created
- Dump database
- Switch to master without this patch
- Run update - verify more O_C_R were created (bad)
- Load database
- Update to master with this patch
- All should be fine

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 37 Tomás Cohen Arazi 2015-04-08 14:37:10 UTC
Followup pushed to master.

Thanks Katrina!