OCA / edi

GNU Affero General Public License v3.0
119 stars 302 forks source link

RFC 14.0 migration, call for ideas #246

Closed bealdav closed 2 years ago

bealdav commented 3 years ago

Here is some a collection of ideas to refactor this repo to next version

Feel free to add you own ideas

pedrobaeza commented 3 years ago

Proposal A: Embrace #243 as base for everything

bealdav commented 3 years ago

base_ubl mixin

https://github.com/OCA/edi/blob/13.0/base_ubl/models/ubl.py

This mixin contains a lot of methods which could be separated in several mixins.

example here https://github.com/OCA/edi/blob/13.0/base_ubl/models/ubl.py#L294 This method is related to line models (sale.order.line and purchase.order.line) but is attached to sale or purchase. Then it's difficult to override lineItem generation depending of your order.line. Possible but it could be made better.

Proposal B : split in different mixins matching real models

@pedrobaeza you can rename your proposal to A

simahawk commented 3 years ago

base_ubl mixin This mixin contains a lot of methods which could be separated in several mixins.

If you want to keep python code it would be probably better to use components (convention to define).

In general, for all modules generating outputs we could rework them - partially or completely - on top of https://github.com/OCA/edi/pull/259. I will soon add support for GS1 which outputs XML. That will give you a real world example.

etobella commented 3 years ago

We have also integrated spanish e-invoice with edi. I think that it would be great to integrate all modules with this base. WDYT? https://github.com/OCA/l10n-spain/pull/1519 @OCA/edi-maintainers

nilshamerlinck commented 3 years ago

Hi @OCA/edi-maintainers

This components-based edi-pocalypse effort looks great ;-)

However, I'm starting to think that it could make sense to introduce a new repo to handle the transition more smoothly: OCA/edi-legacy, that would host the "old" modules, eventually migrated to 14.0 without refactoring, for users that would prefer to stick to the old approach.

What do you think?

simahawk commented 3 years ago

Hi @OCA/edi-maintainers

This components-based edi-pocalypse effort looks great ;-)

However, I'm starting to think that it could make sense to introduce a new repo to handle the transition more smoothly: OCA/edi-legacy, that would host the "old" modules, eventually migrated to 14.0 without refactoring, for users that would prefer to stick to the old approach.

What do you think?

I'm not sure is a good idea to split the repo. If we want to keep both versions in parallel for a while, it's probably better to “play" with names. All modules refactored on top of edi framework should be prefixed w/ edi_ (eg: edi_ubl). I'm not against (personally I don't care where modules come from) but likely it will be confusing. My $0.02 :)

alexis-via commented 3 years ago

Recently, a PR was submitted that adds a module account_invoice_ubl_peppol https://github.com/OCA/edi/pull/323 Maybe it would be a good idea to have peepol compliance in base_ubl/account_invoice_ubl in v14, instead a having it in a separate module. I don't have a strong opinion on this yet. But PEPPOL is very important in the UBL world, so we could have base_ubl / account_invoice_ubl that produce XML that is PEPPOL compliant, and if someone needs additional XML tags that are not allowed in PEPPOL, then they would extend XML generation in another module.

alexis-via commented 3 years ago

About the new edi modules, the main question for me is the articulation with account_invoice_import. In the PR that ports account_invoice_import to v13 https://github.com/OCA/edi/pull/213 they modified base_business_document_import to add a dependancy on edi_account. edi_account depends on edi and component_event. edi depends on base_edi, component_event and base_sparse_field. component_event depends on component. In the past, I got comments from users that told me they didn't use account_invoice_import because there was too many dependencies across too many projects... I fear they may have a heart-attack when they look at v13 !

edi is great and I hope someday all companies will have access to edi systems to connect to their suppliers and customers. But, when I look at present, I see that only few SMBs use edi systems to connect to their suppliers and customers. So it is reasonable to oblige everybody to install the edi modules that they won't use ? I think it's possible to have an architecture where the edi modules depend on account_invoice_import instead of the opposite. For example, in v10, account_invoice_download depends on account_invoice_import and not the opposite. What do you think @simahawk and @etobella ?

simahawk commented 3 years ago

Disclaimer: I didn't know that @yvaucher started refactoring account_invoice_import on top of edi :sweat_smile:

oblige everybody to install the edi modules that they won't use

Well, IMO "number of dependencies" is not a good value for judging. The main reason why I've started w/ edi is that I've be doing for years always the same things (eg: produce/process/send/receive files, talk w/ an external system, track changes on related models, etc etc etc). And I'm not the only one: many OCA modules here and many many customer-only modules are plenty of such "copy-pastes" (eg: talk to an ftp or webservice).

So, regarding account_invoice_import: let's see what is really required there. It might be that some pieces of edi should be split out or that the whole thing is not needed there but we should refactor the module in a way that is more compatible w/ edi and integrate them through a glue module. I don't know that module but I can have a look and give my feedback regarding the integration.

My $0.02 :)

etobella commented 3 years ago

@alexis-via Well, edi can be used with account_invoice_import, so I don't think that it should depend on that. We already have some examples that started using edi for sending information (Spanish eInvoice for B2G). At the end, what we created on 11.0 and 12.0 was similiar to edi and now it makes more sense, also, all this kinds of integrations can be managed this way (VAT integration with Gobernment, eInvoice B2G, einvoice with a specific platform...)

About the dependencies, we understand that it is hard to apply sometimes this kinds of extra addons for all the implications. If this is a problem we can keep it unrelated but make the integration possible as @simahawk proposed

bealdav commented 3 years ago

Probably a dedicated issue for the complex topic above could be done. Thanks

bealdav commented 3 years ago

As mentionned by @rvalyi https://github.com/OCA/edi/pull/323#issuecomment-781603193 an option for future versions could be to use https://github.com/tefra/xsdata#readme When you deal with edi/ubl it's painful to override nodes for specific customer needs in real EDI ecosystem with many partners. Use a pythonic library (object built from xsd it seems after quick look) could be nice in a such case. Not sure that must be done in v14 or later EDIT: probably overkill because produce a lot of code.

rvalyi commented 3 years ago

As mentionned by @rvalyi #323 (comment) an option for future versions could be to use https://github.com/tefra/xsdata#readme When you deal with edi/ubl it's painful to override nodes for specific customer needs in real EDI ecosystem with many partners. Use a pythonic library (object built from xsd it seems after quick look) could be nice in a such case. Not sure that must be done in v14 or later EDIT: probably overkill because produce a lot of code.

xsdata does not produce a lot of code. It only generates python DRY dataclasses that match exactly the xsd spec. Then xsdata is able to populate these Python objects from an XML file and also export XML once you initialized the Python dataclasses.

In OCA/l10n-brazil, we still use generateDS instead which was built before the Python data classes availability and is verbose indeed. But xsdata, its replacement, is the DRYest code you could ever write down given an xsd file. in OCA/l10n-brazil, 50% or more of our fiscal fields do NOT have an existing Odoo or OCA existing equivalent. Not all of these hundreds of fields are used at the same time, but from time to time some are used and if you don't have them here you would just spend your time on support implementing them on the fly invoice after invoice. This is why here we have extra layers that come atop of generateDS (and soon xsdata) to create Odoo mixins with the proper xsd fields so we have the XML marshalling/unmarshalling all the way down from XML to Odoo back and forth. I could take more time to detail it if you like.

You may not need these extra layers for UBL.

Well in fact you can have 2 different approaches:

**1. You may support only the UBL/PEPPOL subset that Odoo+OCA already has business models and fields for.

  1. You may support the whole UBL/PEPPOL spec instead.**

In this last case 2, and only in this last case, the approach I did in OCA/l10n/brazil would be the cleanest and easiest to maintain I believe.

In both cases, I think adopting an xsdata binding already say in version 14, would allow to evolve smoothly from case 1 to case 2.

The manual code approach can solve case 1. But it is important one take a deep dive into the UBL xsd spec (I did it as a way to battle test my stack) and acknowledge you will never achieve case 2 (full spec support) with manual code in an open source prospective.

yvaucher commented 3 years ago

Disclaimer: I didn't know that @yvaucher started refactoring account_invoice_import on top of edi :sweat_smile:

Disclaimer's disclaimer: We had 3 PR on porting account_invoice_import and I merged them together. I didn't do the refactoring. The hard work was done by @etobella here: https://github.com/NL66278/edi/pull/1

I have no strong opinion on basing it on EDI or not, I agree we could want to extract some parts though. Especially as in Switzerland we will have our very specifics own ways of exchanging documents with financial institutions. AFAIK we don't have use for PEPPOL/UBL in our case. We have alternatives like the eBill platform for invoices.

Adding dependencies is not the best on one hand, on the other hand it makes complete sense for the ones that want to use both the invoice import and EDI.

Components :heart:

alexis-via commented 3 years ago

I had a talk last week with @simahawk and we came to the conclusion that edi and account_invoice_import should be independent modules and that we should have a glue module that depend on both to make them work together. So I'll migrate account_invoice_import without a dependency on edi_account (and base_business_document_import will NOT depend on edi_account either). Of course, we'll do what is necessary in the code of account_invoice_import and base_business_document_import to have a smooth and easy integration with edi via the glue module. So the architecture will be different from the current v13 PR https://github.com/OCA/edi/pull/213/files (in that PR, base_business_document_import depends on edi_account).

alexis-via commented 3 years ago

My port of account_invoice_import will be based on Yannick's work on the v13 port in https://github.com/OCA/edi/pull/213/files up to the commit where it added dependency on account_edi in base_business_document_import.

yvaucher commented 3 years ago

Ok, then we should revert too this in v13 to be coherent and ease the migration process from one version to an other.

alexis-via commented 3 years ago

FYI, I'm currently working on adding support for Order-X: I'll develop a module purchase_order_orderx (and later sale_order_orderx for order acknowledgement). For invoices, we have a module account_einvoice_generate that adds a configuration parameter where you can specify which XML files will be attached to the PDF invoice (FacturX or UBL). I think I'll do the same for purchase order: add a module purchase_eorder_generate (feel free to propose a better name). With this module, you would be able to configure which XML file you attach to the purchase order (UBL or Order-X). And I think it should also propose an option "all" ; if you select it, then it would attach them all.

pedrobaeza commented 3 years ago

I sincerely think that all of that should go through edi suite, as it's intended for avoiding all of that bridge modules for having a consistent framework for any possible model, being order, invoice, picking or whatever.

alexis-via commented 3 years ago

@pedrobaeza We discussed the dependency on the "edi" module earlier in this thread (starting Feb 18). The conclusion was that the edi module is great, but we can't have all the modules depend on it, because it adds a lot of complexity and dependencies. The "bridge" module is needed for PDF generation with attachment.

pedrobaeza commented 3 years ago

I know that discussion, but my summary is that just don't want to learn something new. For me it's more complex all this ugly bridge modules.

alexis-via commented 3 years ago

@pedrobaeza Thanks for your nice comment for me. But you're not answering on the points I raised against having all the modules depend on the "edi" module. Why didn't you participate in the discussion ? You didn't give your opinion, and now you say that you disagree...

pedrobaeza commented 3 years ago

Several reasons for not participating:

But now that you raises the question about OrderX, I see the duplicity of things and proliferation of satellite modules that when I'll come to the party, it will be the same argument as with bank-payment: now that this is the structure, it's too late to change it, and we will have two implementations for the same formats, as I want to have homogenized all the formats (FacturaE, SII, FacturX, Voxel...) through the same gateway, and to not use more and more bridges between "your way" and the "EDI way".

And your current claims about the need of such modules is where edi modules takes all their sense, as it's a common ground for allowing:

So at the end, you will need the same number of modules, but doing "customized" things instead of using the facilities of the OCA edi framework.

alexis-via commented 3 years ago

What discussion of OCA/bank-payment are you referring to ?? Where do we have 2 implementations of the same formats in bank-payment ?

pedrobaeza commented 3 years ago

bank-payment was another thing, but the same end: I wanted to change module names and location of them instead of bank-payment, and you argumented about too late, but it was just an example of a possible problem if all these modules arises and then we try to converge.

simahawk commented 3 years ago

@pedrobaeza while I agree on the principle with you I understand that refactoring all those modules on top of edi is a lot of work.

That being said, I do not agree w/ @alexis-via that there are too many dependencies, but we can talk about this :wink:

FTR In the case of account_invoice_import as of v13 it made sense to not touch anything because there was too much work for little gain in v13.

IMO we could proceed like this:

  1. plan an overall refactoring/cleanup/merge/whatever of modules for v15
  2. meanwhile any new module or new feature added to the existing modules should keep this in mind and tend to simplify and split by concern the code/logic (keep separated the model or component that produces a certain result eg: generate or parse an xml or validate an xml, whatever).

My $0.02

pedrobaeza commented 3 years ago

Yeah, indeed, but that's what a good design means: re-evaluate each time you take the occasion. That's what we do when migrating modules, and this is also the time for this new additions. Going to the "minimum effort" is less time at the beginning, but a future trap. In Spain, it's called "Pan para hoy, y hambre para mañana". I have been too rude with Alexis for giving such facepalm, but of course I appreciate his personal and the rest of his company contributions. It's just that I would like that he participates in this with a little more effort for having a good global solution (not like Odoo's EDI, which is very partial). I will as always put efforts on helping you, even if not yet working with UBL/Factur-OrderX.

etobella commented 3 years ago

By the way, It would be interesting to change the module names on 13.0 from edi framework to oca_edi following the comments from https://github.com/odoo/odoo/pull/69215

I would prefer to keep the edi but it could be a problem on 15 an so on, mainly on migrations and would make necessary to review how modules are loaded :disappointed:

pedrobaeza commented 3 years ago

Better edi_oca than oca_edi. And for the rest, edi_account_oca, and so on. Not ideal indeed, but it's the only solution for not having headaches.

etobella commented 3 years ago

Well, the good thing is that making the change on 14 would allow to add the change of the names on the OpenUpgrade 14 scripts

pedrobaeza commented 3 years ago

Yes, indeed I prefer to do it ASAP.

simahawk commented 3 years ago

I hate that apps store :smile: I agree: the sooner the better. Directly in v13 if anyone is willing to do it. I don't have time for now. Maybe in few weeks. +1 for _oca as suffix.

etobella commented 3 years ago

The problem is that on v13 there are already other modules and maybe someone installed on their repos :cry: I propose to do the change on 14, as the module has not been migrated yet

simahawk commented 3 years ago

Fine, go for v14 then. The only problem - not for me as I don't care about apps.odoo.com - is that none of those modules will be visible.

github-actions[bot] commented 2 years ago

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.