Closed (fixed)
Project:
Commerce Xero
Version:
8.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
3 Jan 2019 at 18:18 UTC
Updated:
23 Jan 2025 at 18:39 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
mradcliffeComment #3
mradcliffeI started work on this, but it's not complete.
I think I can do one invoice plugin, then mutate it into a payment depending on if another plugin is enabled. That will require some API changes to the plugins included in this patch.
It also requires a change to xero which is in the 8.x-1.x branch, but no tag yet.
Comment #4
mradcliffeAlso found that with commerce_tax enabled it'll be required to add on either the tax plugin or maybe even do tax calculations in the invoice CX data type plugin. Xero will default to using whatever the organization's setup with unless TaxType/TaxAmount is set correctly and/or a special "tax" line item is provided. That latter thing wasn't available when I was working with Drupal 7.
Comment #5
mradcliffeAdds test coverage for the new plugins.
Comment #6
mradcliffeFixes some coding standards caused by copy/paste in PHPStorm.
Comment #7
mradcliffeWorked some more on this today.
Found some bugs with invoice lookup and recording invoice payments since Payment type didn't have PaymentID. Updated xero to 1.2-alpha7.
This patch doesn't have any additional test coverage.
Comment #8
jeff veit commentedA few notes on this.
Commerce has just introduced a number pattern. The issue was that the order ids are not sequential with no gaps, especially with Commerce Recurring. This is a legal requirement for many invoice numbers.
https://www.drupal.org/node/2730131
https://www.drupal.org/node/3080247
I think this will be used on Commerce receipts and the final purchase pane.
-----
It looks to me like in your code you are only looking at order level adjustments but Commerce allows adjustments at both line level and order level.
I think, if tax is not included in the entered product price, then the tax is included at line level.
Promotions are also handled as adjustments, so they can also be at line level, or on the whole order. Or both.
I think it's possible for tax to be at both, depending on the store locality.
-----
When I started looking at Commerce Xero, I didn't understand enough about how it was working, so I've upgraded an old 7.x Commerce Invoice module. The guts of it are going to be very similar to the invoice code here, so when I have it working, I'll paste that part as a comment. I'd like to be able to switch to Commerce Xero because it's more general.
There are a few big differences:
* No strategies; it's just to send orders as invoices. Some of it's likely to be specific to the client it's for.
* I started using Queue, but switched to Advanced Queue because Commerce Recurring uses Advanced. I don't know their reasoning but I didn't want the end users to have to worry about Queues and Advanced Queues which have separate user interfaces.
* When I'm finished, it's likely to bundle up a few invoices and send them together, to reduce the Xero call count.
-----
Comment #9
mradcliffeThank you for the review, I appreciate it.
The issue with taxes - if the tax calculation doesn't come in as Xero expects/calculates it, then Xero will reject the entire thing. Commerce and Xero never calculate the same way and there have been slight differences :(
The idea here architecturally (there should be a markdown file with more detail) is that this handles the data type plugin and posting to xero, but #2918799: Implement tax processor plugin (or other contrib/custom) could come in with a process plugin and change the data to be what's expected. Having something similar to add other adjustments such as promotions makes sense too.
Comment #10
jeff veit commentedAha about the tax processor! One thing I noticed was the fixed account code of 460 for all order level adjustments, and I was thinking that it needed something to be able to vary the account code, depending on the adjustment (promotion, tax, etc) and the user or buyer; 460 doesn't exist in the Xero Demo chart of accounts. But it makes much more sense if there's a processor for it.
Comment #11
jonathanshawThe new processor plugins introduced in this patch specify for their 'types' annotation "commerce_xero_invoice" which is the id of the invoice type plugin. I believe they should specify 'xero_invoice' which is the actual name of the type, specified on the commerce_xero_invoice plugin.
Comment #12
jonathanshawThis bit me. Let's not assume that orders always have profiles.
Better
if ($profile && $profile->hasField('address')) {Comment #13
jonathanshawFixes #11 and #13
Comment #14
jonathanshawEncoding issues with patch should be fixed here.
Comment #15
jonathanshawStraight reroll.
Comment #17
jonathanshawBetter reroll, I missed a use statement.
Comment #18
jonathanshawComment #19
jonathanshawFix test fails. Also we should use the Xero Query factory. And also need the user module as a kernel test dependency because of using the user data service for tokens in XeroTokenManager.
Comment #22
mradcliffeThis was merged in a while ago.