Description
Colin Campbell
2017-03-14 16:35:56 UTC
Created attachment 61600 [details] [review] Proposed patch Created attachment 61601 [details] [review] Proposed patch Created attachment 61602 [details] [review] Proposed patch Patches should apply to both master and 16.11.x Created attachment 61605 [details] [review] proposed patch Created attachment 61606 [details] [review] proposed patch Add test for new method added to line Applied to customer instance and confirmed working. Marking as bug, asking Kyle to look as he is working with EDI 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. Created attachment 65244 [details] [review] Fix unexported interface to Acq routine routine is meant to be a standard interfacre but is not exported Created attachment 65245 [details] [review] patch to populate price fields using new methods 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 Created attachment 65250 [details] [review] Remove dead code - price method replaced by more specific methods Created attachment 65251 [details] [review] Record tax rate from the invoice Created attachment 65252 [details] [review] Unit Tests for tax-rate Created attachment 65253 [details] [review] Unit tests for price methods Created attachment 65254 [details] [review] Update doc for changed methods perldoc was for old methods update to reflect the new ones 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. No longer appears to apply cleanly to master or 16.11.x heads with a git bz apply. Hi Jon, in this case please set 'Patch doesn't apply' as status. I have no problem appling these patches to a clean master or 16.11 branch - can Jon supply more details 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. 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 (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)? (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 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 68714 [details] [review] Bug 18267 [QA Followup] - Supress warnings Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Lack of tests and POD coverage. 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 Handles tax and discount information from the vendor and deals with different encodings of quantity. Ensures all populatable fields are handled. *** Bug 17279 has been marked as a duplicate of this bug. *** 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> Pushed to master for 20.11, thanks to everybody involved! Created attachment 113897 [details] [review] Bug 18267: (QA follow-up) fix typo: overriden => overridden Hi, a small typo was found while trying the QA script with various patches to troubleshoot issues. (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. Backported to 20.05 for 20.05.07 Backported to 19.11.x branch for 19.11.13 |