Bug 11733 - "gist" system preference inconsistently treated as single-value or multi-value
Summary: "gist" system preference inconsistently treated as single-value or multi-value
Status: RESOLVED MOVED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-02-10 15:54 UTC by Galen Charlton
Modified: 2015-03-06 16:03 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 11733: gist can be multi-valued (3.15 KB, patch)
2014-03-06 10:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11733: gist can be multi-valued (2.83 KB, patch)
2014-07-11 10:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12567: Make catalog statistics wizard publication year work for MARC21 (4.00 KB, patch)
2014-12-09 09:25 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2014-02-10 15:54:33 UTC
The "gist" system preference, which specifies tax rates available for use in various places in acquisitions, lets the user specify multiple values separated by pipe symbols.  The first value is taken to be the default tax rate to apply.

However, there are some places in the code where it effectively assumes that gist contains only one value:

acqui/basketgroup.pl:        if ($bookseller->{invoiceincgst} && ! $bookseller->{listincgst} && ( $bookseller->{gstrate} // C4::Co
acqui/basketgroup.pl:            my $gst = $bookseller->{gstrate} // C4::Context->preference("gist");
acqui/basketgroup.pl:    my $pdf = printpdf($basketgroup, $bookseller, $baskets, \%orders, $bookseller->{gstrate} // C4::Context->
acqui/invoice.pl:my $gist = $bookseller->{gstrate} // C4::Context->preference("gist") // 0;
acqui/parcel.pl:my $gst = $bookseller->{gstrate} // C4::Context->preference("gist") // 0;

and other places where it assumes (correctly) that it can have multiple values:

acqui/neworderempty.pl:}, split( '\|', C4::Context->preference("gist") );
acqui/supplier.pl:    }, split( '\|', C4::Context->preference("gist") );

Routines should be written for fetching either the list of tax rates or the default tax rate as needed.

See also bug 11680.
Comment 1 Jacek Ablewicz 2014-02-13 10:36:18 UTC
(In reply to Galen Charlton from comment #0)

> acqui/basketgroup.pl:    my $pdf = printpdf($basketgroup, $bookseller,
> $baskets, \%orders, $bookseller->{gstrate} // C4::Context->

Looks like all existing pdfprint() variants are not using this gstrate argument any longer (that is probably good - they should use gst rates from invoice records instead).

> acqui/invoice.pl:my $gist = $bookseller->{gstrate} //
> C4::Context->preference("gist") // 0;

This one is fixed by "Bug 10613 - Gst is not calculated correctly on the invoice page" - which we will really like to be pushed ;)

> acqui/parcel.pl:my $gst = $bookseller->{gstrate} //
> C4::Context->preference("gist") // 0;

Dead code ($gst is set, but never used)?

Yet another improper usage of gist is in the parcels.pl - this is an odd one

   GST => C4::Context->preference('gist'),

I guess it's a part of (apparently never fully implemented) feature for choosing gst rate of the shipping costs (?).
Comment 2 Mathieu Saby 2014-02-17 19:00:47 UTC
Hi
By the way, the description of "gist" syspref is confusing, as gist does not contain "a default value" (see bz 11458). And the name "gist" is not very good (see last comment on bz 10327).

I'm going to write a patch to amend the description of the syspref.

Mathieu
Comment 3 Jonathan Druart 2014-03-06 10:00:29 UTC Comment hidden (obsolete)
Comment 4 Galen Charlton 2014-05-06 16:32:49 UTC
Comment on attachment 25888 [details] [review]
Bug 11733: gist can be multi-valued

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

::: acqui/basketgroup.pl
@@ +82,5 @@
> +            unless ( defined $gst ) {
> +                ($gst) = split /\|/, C4::Context->preference("gist");
> +            }
> +            $gst //= 0;
> +            $total = $total * ( $gst / 100 + 1 );

I think there is another bug lurking here.  The 'gist' contains a list of decimal values, so dividing the default one by 100 leads to the wrong result if the default tax rate isn't 0.
Comment 5 Jonathan Druart 2014-07-11 10:40:33 UTC
Created attachment 29623 [details] [review]
Bug 11733: gist can be multi-valued

The gist syspref should always be treated as multi-value.

In basketgroup.pl, parcel.pl and invoice.pl it is treated as
single-value.
The invoice.pl script will be fixed by bug 10613.

If the pref contains several values, the first one is the default one
(according to the syspref description).

Test plan:
basketgroup.pl:
 create a vendor with List prices don't include tax and invoice prices include tax.
 add 1 basket with 1+ order(s)
 receive them and check that the total amount on the basket page is the
 same as on the basketgroup page
Verify this is right for other vendor configurations.

On parcel.pl, this patch only removes dead code. The $gst variable is
not used.
Comment 6 Jonathan Druart 2014-07-11 10:41:18 UTC
(In reply to Galen Charlton from comment #4)
> I think there is another bug lurking here.  The 'gist' contains a list of
> decimal values, so dividing the default one by 100 leads to the wrong result
> if the default tax rate isn't 0.

Actually this code does not work and should simply be removed.
Comment 7 Biblibre Sandboxes 2014-12-09 09:25:08 UTC
Patch tested with a sandbox, by sonia.bouis@univ-lyon3.fr <2>
Comment 8 Biblibre Sandboxes 2014-12-09 09:25:30 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2015-03-06 16:03:33 UTC
Will be taken care of with the tax rewrite.