Bug 18267

Summary: Update price and tax fields in EDI to reflect DB changes
Product: Koha Reporter: Colin Campbell <colin.campbell>
Component: AcquisitionsAssignee: Colin Campbell <colin.campbell>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: major    
Priority: P1 - high CC: andrewfh, bc, david.bourgault, J.P.Knight, jonathan.druart, katrin.fischer, kyle, m.de.rooy, martin.renvoize, nick, victor
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.07, 19.11.13
Attachments: Proposed patch
Proposed patch
Proposed patch
proposed patch
proposed patch
Fix unexported interface to Acq routine
patch to populate price fields using new methods
patch to populate price fields using new methods (16.11 version)
Remove dead code - price method replaced by more specific methods
Record tax rate from the invoice
Unit Tests for tax-rate
Unit tests for price methods
Update doc for changed methods
Bug 18267 Update doc for Koha::EDI
Bug 18267 populate_order_with_prices should be exported
Bug 18267 Populate new price fields in EDI Processing
Bug 18267 Remove obsolete Edifact::Line price method
Bug 18267 - Record tax rate on receipt from EDI Invoice
Bug 18267 Add test for new tax_rate method
Bug 18267 Add tests for all price methods
Bug 18267 Update doc for Koha::EDI
Bug 18267 [QA Followup] - Supress warnings
Patch to handle Price and related information from Quotes and Invoices
Bug 18267 Refactored Edifact Price Calculations
Bug 18267: (QA follow-up) fix typo: overriden => overridden

Description Colin Campbell 2017-03-14 16:35:56 UTC
The aqorders price and tax fields were renamed/ added to in bug 13321 but the changes were not propagated to EDI.pm which is still creating orders with the prices in the old fields. Update the fields used and check the correct Edifact elements are being assigned
Comment 1 Colin Campbell 2017-03-24 10:04:03 UTC
Created attachment 61600 [details] [review]
Proposed patch
Comment 2 Colin Campbell 2017-03-24 10:04:56 UTC
Created attachment 61601 [details] [review]
Proposed patch
Comment 3 Colin Campbell 2017-03-24 10:05:38 UTC
Created attachment 61602 [details] [review]
Proposed patch
Comment 4 Colin Campbell 2017-03-24 10:06:21 UTC
Patches should apply to both master and 16.11.x
Comment 5 Colin Campbell 2017-03-24 10:51:16 UTC
Created attachment 61605 [details] [review]
proposed patch
Comment 6 Colin Campbell 2017-03-24 11:22:01 UTC
Created attachment 61606 [details] [review]
proposed patch

Add test for new method added to line
Comment 7 Barry Cannon 2017-05-02 10:49:58 UTC
Applied to customer instance and confirmed working.
Comment 8 Nick Clemens 2017-05-05 20:37:31 UTC
Marking as bug, asking Kyle to look as he is working with EDI
Comment 9 Marcel de Rooy 2017-05-19 07:33:20 UTC
Code looks good at first glance.

But we are missing unit tests here. Sub process_invoice is changed in this patch set and sub update_price_from_invoice is added. Although it seems to be a private sub and could have been marked as such with an underscore.

Apart from missing tests for the complete Koha/EDI.pm module, the patches do not have any test plans.

I understand that it takes time to write unit tests, but if we keep passing by exception, they will not be written.

Minor detail: The four titles "Proposed patch" do not really convey much information. We miss signoff lines.
Comment 10 Colin Campbell 2017-07-25 15:38:38 UTC
Created attachment 65244 [details] [review]
Fix unexported interface to Acq routine

routine is meant to be a standard interfacre but is not exported
Comment 11 Colin Campbell 2017-07-25 15:39:53 UTC
Created attachment 65245 [details] [review]
patch to populate price fields using new methods
Comment 12 Colin Campbell 2017-07-25 15:43:11 UTC
Created attachment 65246 [details] [review]
patch to populate price fields using new methods (16.11 version)

main patch uses newer interface to borrowers which is not available before 17.05 this uses the direct dbix interface as previously
Comment 13 Colin Campbell 2017-07-25 15:44:47 UTC
Created attachment 65250 [details] [review]
Remove dead code - price method replaced by more specific methods
Comment 14 Colin Campbell 2017-07-25 15:46:58 UTC
Created attachment 65251 [details] [review]
Record tax rate from the invoice
Comment 15 Colin Campbell 2017-07-25 15:47:55 UTC
Created attachment 65252 [details] [review]
Unit Tests for tax-rate
Comment 16 Colin Campbell 2017-07-25 15:49:07 UTC
Created attachment 65253 [details] [review]
Unit tests for price methods
Comment 17 Colin Campbell 2017-07-25 15:50:35 UTC
Created attachment 65254 [details] [review]
Update doc for changed methods

perldoc was for old methods update to reflect the new ones
Comment 18 David Bourgault 2017-09-22 20:30:47 UTC
Both test files pass without an issue, but without a test plan I have no idea how to test the feature.

Also I don't think this needed to be 8 patches, it just makes it more confusing IMHO.
Comment 19 Jon Knight 2017-10-16 17:40:00 UTC
No longer appears to apply cleanly to master or 16.11.x heads with a git bz apply.
Comment 20 Katrin Fischer 2017-10-16 21:59:26 UTC
Hi Jon, in this case please set 'Patch doesn't apply' as status.
Comment 21 Colin Campbell 2017-10-19 09:06:32 UTC
I have no problem appling these patches to a clean master or 16.11 branch - can Jon supply more details
Comment 22 Katrin Fischer 2017-10-22 10:17:32 UTC
Hi Colin, there is a tiny conflict in EDI.pm:

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 18267 populate_order_with_prices should be exported
Applying: Bug 18267 Populate new price fields in EDI Processing
Applying: Bug 18267 Populate new price fields in EDI Processing
Using index info to reconstruct a base tree...
M	Koha/EDI.pm
M	Koha/Edifact/Line.pm
Falling back to patching base and 3-way merge...
Auto-merging Koha/EDI.pm
CONFLICT (content): Merge conflict in Koha/EDI.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 18267 Populate new price fields in EDI Processing
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/patch-to-populate-price-fields-using-new-methods-1-4SHHyf.patch

Hate to admit, but not sure which is the right line, can you take a look? Leaving status, another tester might have a better understanding of it than I do.
Comment 23 Colin Campbell 2017-10-23 10:17:26 UTC
The error is because there are teo versions of patch populate price fields using new methods one for 16.11.x and one for all subsequent versions applying both versions will fail
Comment 24 Jon Knight 2017-10-23 16:42:32 UTC
(In reply to Colin Campbell from comment #23)
> The error is because there are teo versions of patch populate price fields
> using new methods one for 16.11.x and one for all subsequent versions
> applying both versions will fail

Ah ha, yes that's was my issue.  Now applies cleanly if I run interactive 'git bz apply 18267' and delete the line for patch 65246.  Is there anything to test before signing off other than making sure that both t/EdiInvoice.t and t/Edifact.t run successfully (they do on master)?
Comment 25 Colin Campbell 2017-10-25 15:04:02 UTC
(In reply to Jon Knight from comment #24)
> (In reply to Colin Campbell from comment #23)
> > The error is because there are teo versions of patch populate price fields
> > using new methods one for 16.11.x and one for all subsequent versions
> > applying both versions will fail
> 
> Ah ha, yes that's was my issue.  Now applies cleanly if I run interactive
> 'git bz apply 18267' and delete the line for patch 65246.  Is there anything
> to test before signing off other than making sure that both t/EdiInvoice.t
> and t/Edifact.t run successfully (they do on master)?

No the tests cover the new methods. Thanks for testing
Comment 26 Jon Knight 2017-10-25 15:11:17 UTC
Created attachment 68536 [details] [review]
Bug 18267 Update doc for Koha::EDI

Document change to _discounted_price's parameters
and wrapper around new Acquisitions routine

Signed-off-by: Jon Knight <J.P.Knight@lboro.ac.uk>
Comment 27 Kyle M Hall 2017-10-27 13:52:51 UTC
Created attachment 68707 [details] [review]
Bug 18267 populate_order_with_prices should be exported

Although routine is used in acqui scripts, access to it
is by backdoor methods. Allow it to be exported in a standard way
to allow more maintainable code.

Effectively this patch merely documents that this routine
is part of C4/Acquisition's interface and allows users to import
it in the standard way.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 28 Kyle M Hall 2017-10-27 13:52:58 UTC
Created attachment 68708 [details] [review]
Bug 18267 Populate new price fields in EDI Processing

New price fields have been added to aqorders with the result
that sites using EDI found prices appearing as zero in
displays as the prices were not duplicated to the
nrew fields as in manual acquisitions.

Invoicing now records values using the standard
populate_order_with_prices routine from C4::Acquisitions

However when creating an order from an edifact quote the
routine makes invalid assumptions about what data is available.
In a quote prices are always tax inclusive, and no information
is supplied on the tax rate and amount, these appear in the invoice
message on receipting. Koha::EDI therefore sets the relevant
fields directly in order creation.

To clarify the field used, we now call the price_info method rather than
the older price method.

When calculating discount, we now check if a discounted price
has been supplied. (In practice we dont see this but we are
prepared if it occurs )

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 29 Kyle M Hall 2017-10-27 13:53:01 UTC
Created attachment 68709 [details] [review]
Bug 18267 Remove obsolete Edifact::Line price method

The old price method duplicated the function of price_info as this
was the only type of price field in quotes.
The more specific field is now used and the old method is
no longer called. Remove the obsolete code

Add comment documenting the allowed values in the Edifact
PRI segment

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall 2017-10-27 13:53:04 UTC
Created attachment 68710 [details] [review]
Bug 18267 - Record tax rate on receipt from EDI Invoice

As part of the newly introduced price fields there
is also a tax_rate_on_receiving that needs setting.

This records the main tax rate on receipting. Edifact has the
potential to be more complex than Koha allows i.e. the
line amount may be made of different amounts (incorporating
servicing, etc) at different tax rates. This concentrates
on the main tax rate.

The tax rate will be receiced as a percentage value e.g. 20.00
need to convert to a decimal fraction 0.20 as assumed in Koha
calculations

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2017-10-27 13:53:07 UTC
Created attachment 68711 [details] [review]
Bug 18267 Add test for new tax_rate method

Test return for new method added to Edifact::Line

Note tax rate is not specified in edifact quotes so tax_rate_on_ordering
can not be set.
Tax rate is only specified in invoice messages

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall 2017-10-27 13:53:10 UTC
Created attachment 68712 [details] [review]
Bug 18267 Add tests for all price methods

Add test for price_gross in addition to price_net in Invoices
Test price_info and price_info_inclusive in quotes
NB test is that price_info_inclusive returns undef which we
expect to be the normal condition

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Kyle M Hall 2017-10-27 13:53:13 UTC
Created attachment 68713 [details] [review]
Bug 18267 Update doc for Koha::EDI

Document change to _discounted_price's parameters
and wrapper around new Acquisitions routine

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Kyle M Hall 2017-10-27 13:53:16 UTC
Created attachment 68714 [details] [review]
Bug 18267 [QA Followup] - Supress warnings

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Jonathan Druart 2017-10-27 20:23:12 UTC
Lack of tests and POD coverage.
Comment 36 Colin Campbell 2020-11-12 16:27:07 UTC
Created attachment 113569 [details] [review]
Patch to handle Price and related information from Quotes and Invoices

These modified routines have been running in production and have been tested against all known Edifact vendors messages, both in parsing and populating the database and ensuring the total amounts match the summary totals in the incoming files
Comment 37 Colin Campbell 2020-11-12 16:30:07 UTC
Handles tax and discount information from the vendor and deals with different encodings of quantity. Ensures all populatable fields are handled.
Comment 38 Colin Campbell 2020-11-12 16:34:47 UTC
*** Bug 17279 has been marked as a duplicate of this bug. ***
Comment 39 Kyle M Hall 2020-11-13 12:53:26 UTC
Created attachment 113597 [details] [review]
Bug 18267 Refactored Edifact Price Calculations

Refactored the price calculations for Quotes and Invoices
This takes in to account various combinations used by different
vendors. Makes the extraction of basic price, quantity and tax
information more reliable. Tests are in Edifact.t and EdiInvoice.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 40 Jonathan Druart 2020-11-16 12:49:49 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 41 Victor Grousset/tuxayo 2020-11-20 15:50:55 UTC
Created attachment 113897 [details] [review]
Bug 18267: (QA follow-up) fix typo: overriden => overridden
Comment 42 Victor Grousset/tuxayo 2020-11-20 15:52:18 UTC
Hi, a small typo was found while trying the QA script with various patches to troubleshoot issues.
Comment 43 Jonathan Druart 2020-11-25 08:59:25 UTC
(In reply to Victor Grousset/tuxayo from comment #41)
> Created attachment 113897 [details] [review] [review]
> Bug 18267: (QA follow-up) fix typo: overriden => overridden

Patch pushed to master.
Comment 44 Andrew Fuerste-Henry 2020-12-09 19:57:46 UTC
Backported to 20.05 for 20.05.07
Comment 45 Victor Grousset/tuxayo 2020-12-09 20:33:34 UTC
Backported to 19.11.x branch for 19.11.13