Bug 8832

Summary: can't enter a tax rate on vendor or when ordering
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart+koha>
Status: CLOSED FIXED QA Contact:
Severity: critical    
Priority: P5 - low CC: chris, jcamins, jonathan.druart+koha, katrin.fischer, kyle, mtompset, paul.poulain
Version: 3.10   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 5335    
Bug Blocks:    
Attachments: placing an order
editing a vendor
preference values
pull down after entering values in preference
Bug 8832: Update the gist pref and hide the tax rate value if not needed
Bug 8832: Update the gist pref and hide the tax rate value if not needed
[SIGNED-OFF] Bug 8832: Update the gist pref and hide the tax rate value if not needed

Description Nicole C. Engard 2012-09-26 16:55:04 UTC
Created attachment 12536 [details]
placing an order

I can no longer enter a tax rate when adding a new vendor, editing a vendor or placing an order. It's always a pull down with 0% in it.
Comment 1 Nicole C. Engard 2012-09-26 16:55:25 UTC
Created attachment 12537 [details]
editing a vendor
Comment 2 Katrin Fischer 2012-09-27 07:31:14 UTC
Hi Nicole,

I am quite sure this is a problem introduced by bug 5335:

-    my $default_gst_rate = (C4::Context->preference('gist') * 100) || '0.0';
+    # get option values from gist syspref
+    my @gst_values = map {
+        option => $_
+    }, split( '\|', C4::Context->preference("gist") );

It seems it expects a | separated list of values now for the gist system preference. I think this maybe slipped in by accident as 5335 doesn't seem to include a database update of any sort.
Comment 3 Katrin Fischer 2012-09-27 07:32:10 UTC
Updating severity - this is a bad bug.
Comment 4 Katrin Fischer 2012-09-27 07:36:41 UTC
Ok, seems I am not fully awake yet, there is an updatedatebase in 5335, but it doesn't touch systempreferences.
Comment 5 Mark Tompsett 2012-09-27 07:39:16 UTC
Yes, but the gist system preference is actually a character field, which I thought was bizarre when I was looking around there. Have you attempted to put 0|12|13|4|string into gist, nengard?
-- By the way, that (because of string) should also trigger errors (if you select string), because we have no field validation on system preferences, as far as I know.
Comment 6 Katrin Fischer 2012-09-27 07:45:11 UTC
It still seems like an undocumented change of behaviour that maybe belongs to a different development.
Comment 7 Nicole C. Engard 2012-09-27 11:54:02 UTC
Created attachment 12548 [details]
preference values

Okay, this seems a bit silly to me, but most US libraries don't charge tax.  I followed the preference instructions and entered data like in the picture (12%|6%|7%|5%) but then it appears way wrong in the pull down. So the preference needs to be updated.  Also, what happens to those who have a tax rate already on a vendor but not in the preference and upgrade?

More importantly what happens to those who had 0 in the gist? That used to hide all of the tax fields from acq making it easier for those who don't use tax rates .... I think that this change in the preference was not thought out and I unfortunately missed it and didn't complain before now.

Nicole
Comment 8 Nicole C. Engard 2012-09-27 11:54:20 UTC
Created attachment 12549 [details]
pull down after entering values in preference
Comment 9 Mark Tompsett 2012-09-27 12:59:25 UTC
Actually, perhaps the list should be parsed for %'s and then not multiply by 100. So that way you could have 12% or 0.12 to generate the same result. ;)
Comment 10 Paul Poulain 2012-09-27 14:24:06 UTC
(In reply to comment #9)
> Actually, perhaps the list should be parsed for %'s and then not multiply by
> 100. So that way you could have 12% or 0.12 to generate the same result. ;)

you're right here: it's written on the syspref description:
"Enter in Numeric form 0.12 for 12%. First is default. If you want more than 1 value, please separate with |"

(In reply to comment #6)
> It still seems like an undocumented change of behaviour that maybe belongs
> to a different development.

Katrin, I don't know what you mean here: the main purpose of bug 5335 was to have tax rate at order line level, instead of bookseller level. In France, books have a 5.5% VAT, while CD/DVD have a 19.6%. That was a big pain for libraries.

The only problem I see here is that, if you have no tax rates defined, you just have '0'.
Maybe a follow-up could display, if the syspref is empty "No tax rate entered, go to syspref gist to define possible tax rate"

Also note that, now, the bookseller tax rate is just the *default* one. When you enter an order line, you can always choose between all taxes listed in the syspref.

> Also, what happens to those who have a tax
> rate already on a vendor but not in the preference and upgrade?
If you have already a tax that is one of the tax in the syspref, nothing specific.
If you have a tax rate that is not in the syspref, then you'll be in trouble when ordering, because you'll just get the "0"

Updating the gist syspref is something you *must* do when upgrading.
We could do a follow-up that does something like:
UPDATE syspref value= join '|', @{SELECT distinct gist FROM aqbooksellers}
(ie = put in the syspref all the distinct values that exist in the booksellers tax rates)
Comment 11 Katrin Fischer 2012-09-28 12:18:57 UTC
Hi Paul,

sorry for being confusing.

I think there are different things to think about here:

1) If the system preference is empty, the tax information and configuration fields should be hidden from display. There are libraries which don't want to use the feature and it worked like this before - so it's a regression for them.

2) We need a good update path for libraries. If we require now a list of values the update has to take care of that and prefill the list. From Nicole's description with the pull down I think that at the moment if you update your vendor the tax value will be lost if the value is not in the gist system preference. 

I didn't really test the feature yet, so this is mostly based on looking at the code and on the descriptions from others. Hope I am not too far off.
Comment 12 Paul Poulain 2012-09-28 12:28:37 UTC
(In reply to comment #11)
> 1) If the system preference is empty, the tax information and configuration
> fields should be hidden from display. There are libraries which don't want
> to use the feature and it worked like this before - so it's a regression for
> them.
OK, I hesitated between "display that the syspref is empty" or "ignore the gst". I think that libraries that want a gst will complain that there's none ( ;-) ) so OK with hiding the gst for libraries that have the syspref empty.

follow-up easy to do. Will do it quickly (Jonathan is afk for 2 more week)

> 2) We need a good update path for libraries. If we require now a list of
> values the update has to take care of that and prefill the list. From
> Nicole's description with the pull down I think that at the moment if you
> update your vendor the tax value will be lost if the value is not in the
> gist system preference. 
yes, that's why i'm proposing to fill it automatically with all distinct values that exist in aqbooksellers before the update.

> I didn't really test the feature yet, so this is mostly based on looking at
> the code and on the descriptions from others. Hope I am not too far off.
not too far, and agreeing with my proposal, so it's fine ;-)
Comment 13 Nicole C. Engard 2012-09-28 12:35:36 UTC
(In reply to comment #12)
>so OK with hiding the gst for libraries that have the syspref empty.

+1, also if the pref is 0 (as mine was) it should either hide the tax fields or it should be changed to empty.  At one point 0 was the default we were supposed to have in there to hide the tax fields.

> yes, that's why i'm proposing to fill it automatically with all distinct
> values that exist in aqbooksellers before the update.

That would be a nice option, that way people don't lose the settings they have in place already.

Nicole
Comment 14 Jonathan Druart 2012-10-15 13:48:38 UTC Comment hidden (obsolete)
Comment 15 Paul Poulain 2012-10-16 16:29:29 UTC
(In reply to comment #14)
> Created attachment 12815 [details] [review]
> Bug 8832: Update the gist pref and hide the tax rate value if not needed
> 
> The gist system preference will be updated with the existing values and
> with the supplier's gstrate values.
> The "Tax rate" part becomes hidden if the syspref is not filled.

QA comment:
* Jonathan, in your patch, you set the version to 062 instead of XXX
* I haven't tested the patch (waiting for nicole or someone else signoff), but it looks very wisely written: if the syspref has been filled, what was in is preserved, only values that are in aqbookseller.gist and not in the syspref are added. Well done !
Comment 16 Jonathan Druart 2012-10-18 13:51:53 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2012-11-02 15:04:04 UTC
Created attachment 13234 [details] [review]
[SIGNED-OFF] Bug 8832: Update the gist pref and hide the tax rate value if not needed

The gist system preference will be updated with the existing values and
with the supplier's gstrate values.
The "Tax rate" part becomes hidden if the syspref is not filled.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Paul Poulain 2012-11-08 16:46:38 UTC
QA comments:
 * small patch, despite it's size, the largest part of it is reindenting after addind a if
 * passes koha-qa.pl
 * critical problem, need to be fixed, no string change

passed QA
Comment 19 Jared Camins-Esakov 2012-11-09 04:30:01 UTC
This patch has been pushed to master.
Comment 20 Paul Poulain 2012-11-12 13:23:20 UTC
Patch pushed to branch 3.10.x
Comment 21 Chris Cormack 2013-04-22 08:41:24 UTC
Released in 3.10.0