Skip to content
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.

Sign up for GitHub

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

Add basic structs for MX fuel complement #192

Closed
wants to merge 14 commits into from
Closed

Add basic structs for MX fuel complement #192

Changes from 3 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9499268
Add basic structs for MX fuel complement
cavalle Sep 1, 2023
5fb582a
Change and document package name
cavalle Sep 1, 2023
57f722c
Make linter happy with the package name
cavalle Sep 1, 2023
e178705
Move complements to mx package
cavalle Sep 4, 2023
87305b0
Generate schema for MX complement
cavalle Sep 4, 2023
7b4ed40
Replace dashes with underscores in JSON keys
cavalle Sep 4, 2023
653e8bc
Mention CFDI standard in complement description
cavalle Sep 4, 2023
4be0288
Use custom totals struct instead of bill.Totals
cavalle Sep 4, 2023
fccc293
Add missing description
cavalle Sep 4, 2023
5a3388e
Add cal.DateTime and use it from fuel complement
cavalle Sep 4, 2023
a4a18e8
Add DateTime description comment
cavalle Sep 4, 2023
5112d09
Rename Purchaser to Seller
cavalle Sep 6, 2023
ce4472f
Merge branch 'main' into fuel-complements
cavalle Sep 12, 2023
86e2823
Flatten down FuelItem into FuelLine & minor tweaks
cavalle Sep 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions regimes/mx/complements/fuel_complement.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
// Package complements provides GOBL Complements for Mexican invoices
package complements
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's the best way of structuring complements. For the moment, I'm putting everything under a complements package within the mx path. Suggestions on how to better do this are welcome!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure... if you imagine a user who needs to set up complements for multiple countries, they'd have to deal with the namespace collisions of complements in multiple places. Could be awkward. I'd also think about the final schema URL, I was thinking of: https://gobl.org/draft-0/regimes/mx/fuel-complement

Copy link
Contributor Author

@cavalle cavalle Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Sam! Agreed, there's no need for a complements package. Moved it to the main mx package in e178705. I also generated the schema file in the URL that you advised in 87305b0.


import (
"cloud.google.com/go/civil"
"github.com/invopop/gobl/bill"
"github.com/invopop/gobl/cbc"
"github.com/invopop/gobl/num"
"github.com/invopop/gobl/org"
"github.com/invopop/gobl/tax"
)

// FuelComplement provides detailed information about fuel purchases made with
// electronic wallets. In Mexico, e-wallet suppliers are required to report this
// information as part of the invoices they issue to their customers.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to reference here the exact name of the complement and version that will be generated as a result.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Addressed in 653e8bc

type FuelComplement struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When creating the structs of this complement, I considered two general approaches:

  1. Mirror the target CFDI structure: The first option was to replicate the CFDI fields and structure directly (not unlike what we've done with ext fields – straightforward and explicit).

  2. Align with GOBL's idioms: I chose this. It should make the structs more intuitive for GOBL developers who may not be familiar with CFDIs, but will definitely be used to GOBL. It also ensures that the resulting GOBL document looks harmonious and not like a Frankenstein.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I think we should at least try to smooth out the edges.

// Customer's e-wallet account number
WalletAccountNumber string `json:"wallet-account-number" jsonschema:"title=Wallet Account Number"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOBL convention is to use _ (underscore) to separate words. The initial aim was to be database compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My oversight. Fixed on 7b4ed40. Good catch!

// List of fuel purchases made with the customer's e-wallets
Lines []*FuelLine `json:"lines" jsonschema:"title=Lines"`
// Summary of all the purchases totals, including taxes (calculated)
Totals bill.Totals `json:"totals" jsonschema:"title=Totals" jsonschema_extras:"calculated=true"`
Copy link
Contributor Author

@cavalle cavalle Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reusing bill.Totals here but I'm not 100% sure about this as we only need the Total and TotalWithTaxes fields from that struct. It may be confusing for developers to have all the other fields available.

A penny for your thoughts!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm inclined to agree... this strikes me as a new structure alongside the complements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree! Addressed in 4be0288

}

// FuelLine represents a single fuel purchase made with an e-wallet issued by
// the invoice's supplier to the invoice's customer.
type FuelLine struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the "line" word because the whole structure resembles very much to the bill.Line struct. We could even extend the bill.Line struct here with the specific fields we need in this context, which enable to potentially reuse part of the validation/calculation logic. However, I thought that binding the evolution of the bill.Line struct and behaviour to this struct may not be a good idea if those diverge in the future. Also, like I mentioned in other comments, bill.Line has fields that won't be used here, and that could confuse developers.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're okay recreating the structures for this use-case. FuelLine makes sense.

// Identifier of the e-wallet used to make the purchase
WalletID cbc.Code `json:"wallet-id" jsonschema:"title=Wallet Identifier"`
// Date and time of the purchase
PurchaseDateTime civil.DateTime `json:"purchase-date" jsonschema:"title=Purchase Date"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have cal.DateTime at the moment. Should we add it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 interesting. It might indeed be a good idea to have a cal.DateTime which enforces ISO formatting. We have a similar "at" package in github.com/invopop/lib which does something similar with database storage. I'll think a bit more about this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! Leaving civil.DateTime for now

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you also considered two fields? I'm not convinced, and we'd still need a new type, but "Date" and "Time" types could make sense.

Copy link
Contributor Author

@cavalle cavalle Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was right now drafting a cal.DateTime type that would marshal and unmarshall in the ISO format. I personally don't see any major problem on having a DateTime type, it's quite universally and very well standardised. What concerns do you have? Separating Date and Time is definitely an option, but then we will need a new cal.Time type too, wouldn't we?

Copy link
Contributor Author

@cavalle cavalle Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed 5a3388e with a barebones cal.DateTime implementation (I'll complete it once we confirm that's the like we'd like to follow). The only concern I have about the type is whether or not it should support time zones. The CFDI expects a date and time without a timezone, but in the future we may need a Date and Time with Timezones for other purposes. Not sure we'll want to add just another type then.

// Tax ID (RFC) of the purchaser
PurchaserTaxID cbc.Code `json:"purchaser-tax-id" jsonschema:"title=Purchaser's Tax ID"`
// Code of the service station where the purchase was made
ServiceStationCode cbc.Code `json:"service-station-code" jsonschema:"title=Service Station Code"`
// Amount of fuel units purchased
Quantity num.Amount `json:"quantity" jsonschema:"title=Quantity"`
// Details about the fuel purchased
Item FuelItem `json:"item" jsonschema:"title=Item"`
// Identifier of the purchase (folio)
PurchaseID cbc.Code `json:"purchase-id" jsonschema:"title=Purchase Identifier"`
// Result of quantity multiplied by the item's price (calculated)
Sum num.Amount `json:"sum" jsonschema:"title=Sum" jsonschema_extras:"calculated=true"`
// Map of taxes applied to the purchase
Taxes tax.Set `json:"taxes" jsonschema:"title=Taxes"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reusing GOBL's tax.Set. This is the case where reusing makes more sense to me, we need all the fields and can take advantage of the rate calculation using regime's data.

}

// FuelItem provides the details of the fuel purchased.
type FuelItem struct {
Copy link
Contributor Author

@cavalle cavalle Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in other parts of this PR, the similarity of this struct with org.Item makes it tempting to use it directly. In this particular case, I decided not to, but the considerations discussed in other comments could be applied here to make a different decision. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be tempted to store the FuelItem fields inside the FuelLine... the criteria is dependent on how re-usable the Item needs to be. Will the FuelItem ever need to be persisted independently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a separate FuelItem struct just to mirror the existing pattern in bill.Line and org.Item. It neatly separates item details from purchase info, and it's an idiom that GOBL developers will be familiar with. I also expect this data to be persisted separately in the source systems.

On the other hand, in the CFDI the line and item data are at the same level. So if you're not convinced by this approach, let me know and I'll flatten down here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I accidentally deleted your last comment here. Sorry for the mess, I'm bringing it back:

@samlown said:

My thoughts are around the path of least resistance for users trying to figure out what to do. I think it might be a good idea in this case to stick as closely as possible to the original source structure, with translated fields...

Finding the path of least resistance for users is the same design principle I'm trying to follow. Question is which path is that! From my perspective, one of the main benefits that GOBL brings to developers is providing them with a single unified language, with a consistent vocabulary and set of idioms and patterns that they can use across regimes. That's why I thought the least resistance path was modelling lines and items resembling the ones in GOBL.

In any case, I'll propose an alternative version sticking as closely as possible to the original source structure with translated fields.

// Reference unit of measure used in the price and the quantity
Unit org.Unit `json:"unit" jsonschema:"title=Unit"`
// Type of fuel (c_ClaveTipoCombustible codes)
Type cbc.Code `json:"type,omitempty" jsonschema:"title=Type"`
Copy link
Contributor Author

@cavalle cavalle Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are only a bunch of simple values that can be used here:

c_ClaveTipoCombustible Tipo de combustible
1 Gasolina menor a 92 octanos
2 Gasolina mayor o igual a 92 octanos
3 Diesel
4 Diesel Marino
5 Otros

We can simply use a cbc.Code with explicit codes (and validate them), or go fancy and create a new type (FuelType) that is a cbc.Keyand with valid values are more meaningful (diesel, gasoline…) like we do with the org.Units and in many other places in GOBL.

I opted for the simple solution, but I can see the merits of the meaningful key option. Any thoughts about this one?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial impression: I think unless there is a fuel type elsewhere, we can just use the code specific for this document. Given the local nature of this, I'm not as worried... I envisage most use-cases of this will be hard coded in code and very unlikely to use a front-end... my impression anyway.

// Name of the fuel
Name string `json:"name" jsonschema:"title=Name"`
// Base price of a single unit of the fuel
Price num.Amount `json:"price" jsonschema:"title=Price"`
}