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.
(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 (?).
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
Created attachment 25888 [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: No regression should be found on displaying total basketgroup for a vendor with 'list prices' don't include tax and 'invoice prices' include tax. No regression should be found on generating pdf. If no gstrate is defined for the vendor, the gstrate used should be the first one defined in the gist syspref. On parcel.pl, this patch only removes dead code. The $gst variable is not used.
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.
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.
(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.
Patch tested with a sandbox, by sonia.bouis@univ-lyon3.fr <2>
Created attachment 34198 [details] [review] Bug 12567: Make catalog statistics wizard publication year work for MARC21 Publication year was useless for MARC21, as it was using the biblioitems.publicationyear (UNIMARC) instead of biblio.copyrightdate (MARC21). This patch tries to fix the search options to work for both MARC flavours. To test: In Reports > Statistics wizards > Catalog: - Test the publication year search options on a UNIMARC and a MARC21 installation. - Make sure search results make sense for both. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> The changes are fine with me, but this script is certainly not bug free. Also note that all its SQL should be moved out according to our coding guidelines. When doing so, a rewrite would be appropriate. Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com> Signed-off-by: sonia.bouis@univ-lyon3.fr <2>
Will be taken care of with the tax rewrite.