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.
Created attachment 34090 [details] [review] Bug 13380: Fill order cancellation reasons from AV 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. Also, it will be easier to filter or create reports. Test plan: 0/ Don't apply the patch 1/ Cancel some orders and give a cancelletion reason 2/ Apply the patch and execute the updatedb entry 3/ Cancel an order and verify the you have a list with the reason you previously filled + 3 new ones ('No reason', 'Sold out' and 'Restocking'); 4/ Choose one and verify the value is correctly displayed on the basket page 5/ You can also try to add other values from the admin module.
Created attachment 34091 [details] [review] Bug 13380: Add the AV in the install files (This will completely done when the patch will be QAed)
Created attachment 34140 [details] [review] Bug 13380: Fill order cancellation reasons from AV 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. Also, it will be easier to filter or create reports. Test plan: 0/ Don't apply the patch 1/ Cancel some orders and give a cancelletion reason 2/ Apply the patch and execute the updatedb entry 3/ Cancel an order and verify the you have a list with the reason you previously filled + 3 new ones ('No reason', 'Sold out' and 'Restocking'); 4/ Choose one and verify the value is correctly displayed on the basket page 5/ You can also try to add other values from the admin module.
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.
(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.
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.
(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.
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.
(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.
Created attachment 34746 [details] [review] Bug 13380: Add the AV in the install files (This will completely done when the patch will be QAed) Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Created attachment 34747 [details] [review] Bug 13380: Fill order cancellation reasons from AV 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. Also, it will be easier to filter or create reports. Test plan: 0/ Don't apply the patch 1/ Cancel some orders and give a cancelletion reason 2/ Apply the patch and execute the updatedb entry 3/ Cancel an order and verify the you have a list with the reason you previously filled + 3 new ones ('No reason', 'Sold out' and 'Restocking'); 4/ Choose one and verify the value is correctly displayed on the basket page 5/ You can also try to add other values from the admin module. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
QA Issue: The new authorized values need to be added to each translation.
(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.
Hi Kyle, could you give Jonathan feedback on the wording?
Created attachment 35695 [details] [review] [PASSED QA] Bug 13380: Add the AV in the install files (This will completely done when the patch will be QAed) Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 35696 [details] [review] [PASSED QA] Bug 13380: Fill order cancellation reasons from AV 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. Also, it will be easier to filter or create reports. Test plan: 0/ Don't apply the patch 1/ Cancel some orders and give a cancelletion reason 2/ Apply the patch and execute the updatedb entry 3/ Cancel an order and verify the you have a list with the reason you previously filled + 3 new ones ('No reason', 'Sold out' and 'Restocking'); 4/ Choose one and verify the value is correctly displayed on the basket page 5/ You can also try to add other values from the admin module. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(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?
Wondering, what is meant here? Restocking => Already ordered
(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.
Created attachment 35767 [details] [review] Bug 13380: Wording changes
Created attachment 35768 [details] [review] Bug 13380: Add ORDER_CANCELLATION_REASON AV to all language files
Maybe we could just leave it out and go with 2 status? Restocking and out of stock seem very similar.
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
Created attachment 35773 [details] [review] Bug 13380: Wording changes
The plan was to have this in the maintenance releases (see comment #9) - Tomas, could you please push soon? Thx!
Patches pushed to master. Thanks Jonathan!
Pushed to 3.18.x will be in 3.18.5
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
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.
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.
Created attachment 37144 [details] [review] Bug 13380: 3.18.x Fix previous DB entry The DB entry from bug 13379 should have been executed before bug 13380. The AV category now contain truncated values. This patch restores the correct category values. The DB entry from bug 13379 should have been pushed before.
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.
Created attachment 37163 [details] [review] [master] 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
Created attachment 37179 [details] [review] [master] Bug 13380: Follow-up - only run database update, when it hasn't been run already Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Obsoleted the already pushed patches - the last one for master needs QA now.
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>
Followup pushed to master. Thanks Katrina!