-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Extensions refactor #184
Extensions refactor #184
Conversation
4c78ab7
to
3372466
Compare
regimes/it/examples/freelance.json
Outdated
@@ -80,7 +80,7 @@ | |||
{ | |||
"cat": "IRPEF", | |||
"percent": "20.0%", | |||
"rate": "self-employed-habitual" | |||
"code": "A" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it some more, I'm not suer-happy with this approach. The fundamental problem here is that there is no reference to what the (in this case) "A"
comes from or implies. Perhaps a better approach would be to add the concept of a "codes hash", so that the usage could be more like:
{
"cat": "IRPEF",
"percent": "20.0%",
"codes": {
"it-sdi-nature": "A"
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed the second example is better that the original change. And "codes" could become "ext" as proposed here.
Looks that the |
7321c5b
to
fc8214d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work, Sam! 🙌 Overall, I think the changes make things clearer, more direct and succinct.
I have a couple of concerns that I commented in the code plus one more: rolling out the changes and backwards compatibility issues. I think the ideal scenario would be to release a backwards compatible version first so that integrators can adapt to the changes, release them and test them at their own time. Otherwise, we will basically need to synchronise the release of our changes with the release of the changes of all our affected customers, which seems very complex and will delay the changes quite a lot. On the other side, making the change backwards compatible will require extra work. So, I'm not sure what are your thoughts here!
"ext": { | ||
"mx-cfdi-fiscal-regime": "601" | ||
"mx-cfdi-use": "G01" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super-sweet! Much more succint
regimes/pt/tax_categories.go
Outdated
@@ -12,39 +12,10 @@ import ( | |||
|
|||
// Tax rate exemption tags | |||
const ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't we leave it for backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering about that for PT... given that only Amenitiz are using this at the moment, I can check with them directly.
tax/totals.go
Outdated
return rt.percentagesMatch(c) | ||
} | ||
if c.Rate.IsEmpty() || rt.Key == c.Rate { | ||
if rt.Ext.Equals(c.Ext) { | ||
return rt.percentagesMatch(c) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grouping the totals by the ext data feels a bit odd and unnatural. This may or may not be the desired behaviour we want depending on the meaning of the specific extensions used in each case in each regime. My concern is that this could be a limitation in future regimes that may need to use extensions but not grouping the totals by them (just by rate).
I guess we need this for the specific Italian case that requires totals to be grouped by the exemption reason. In that case, maybe having different exemption rates may be justified. Or we could leave the fatturapa conversor do the aggregations themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Valid point, but my expectation here is that in practical terms invoices will not have more than one usage of the ext
, its a "just in case" feature. My feeling is that we should wait until we have more concrete examples where grouping like this is a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be right that this won't become a real problem. Thing is, if it weren't because of the Italian case, it would have never crossed our minds to implement this. My feeling is that implementing this kind of unintuitive behaviours to solve individual cases adds accidental complexity that makes things increasingly harder to understand and change in the future than it could be. And if we wait to discover real cases where a behaviour like this is problematic, it may be too late (i.e. too expensive) to change it.
// Extensions defines a list of keys for codes to use as an alternative to choosing a | ||
// rate for the tax category. Every key must be defined in the Regime's extensions | ||
// table. | ||
Extensions []cbc.Key `json:"extensions,omitempty" jsonschema:"title=Extensions"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that we define (and validate) the list of valid extensions for rates and categories, but not for other entities such as items or parties. What's the reason for this? Why not do the same for items, parties and other "extendable" struts? Couldn't we just let each regime's validators check that each extension is used where and how it should be, if that's relevant to them, to keep things simpler in this initial version at least?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get your point. At the moment we don't have a way to define the "Extensions" for org.Item and org.Party, but it would be more consistent if we did. Also, the more structured validation rules we have, the easier it is to implement UIs that can make suggestions automatically.
The blocker for me is how to structure these rules inside the regime definition...
ext
CodeMap
s defined in different locations throughout a GOBL document.identities
and the invoice linetax.rate
are now migrated automatically in Portugal an Mexico. This change should now be just a drop-in replacement, although users will need to upgrade ASAP to avoid incompatibility issues.With this change, tax combos can look like:
Code "A" in this situation reflects the special freelance rate for income tax in Italy.
Extension codes in México for example would be used like this: