Created attachment 12537 [details]
editing a vendor

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.

Updating severity - this is a bad bug.

Ok, seems I am not fully awake yet, there is an updatedatebase in 5335, but it doesn't touch systempreferences.

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.

It still seems like an undocumented change of behaviour that maybe belongs to a different development.

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

Created attachment 12549 [details]
pull down after entering values in preference

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. ;)

(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)

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.

(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 ;-)

(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

(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 !

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>

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

This patch has been pushed to master.

Patch pushed to branch 3.10.x

Released in 3.10.0