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.

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

#2658 invoice externalization in a specifc directory #3349

Closed
wants to merge 12 commits into from

Conversation

homebeaver
Copy link
Contributor

@homebeaver homebeaver commented Jan 30, 2021

Hi @marcalwestf , @e-Evolution

this PR replaces the #3257 : invoice externalization for e-invoicing UBL and CII

regards EUGen

@homebeaver
Copy link
Contributor Author

Hi Mario @marcalwestf ,

in #3257 (comment) you wrote:

Conclusion:
The changes are not intrusive and are backward-compatible.
The XMLs generated are correct.
I haven't tested if the XMLs generated are semantically correct or if they do conform to the specifications.
This PR can be accepted if the remarks on my previous comment are considered.

On fnfe-mpe.org there is a web-validator for CII-invoices.
I dropped two xmls generated with AD+this PR. Here is the result:

grafik

PS: the fnfe-mpe validator does not work for UBL. For UBL-xml there is a cmdline validator

Hope, this helps to commit the PR.
regards

@e-Evolution
Copy link
Contributor

Hi @homebeaver

This code looks much better and more isolated, less invasive than the previous one, and is built as a separate module.

I still have a question why you have to modify the core classes, with functionality that not is for general solution.

MInvoice.java
ExportFormatXML.java
PO.java

Can you give me more detail of what you are trying to do, maybe I can help you with a better approach.

I implement the electronic invoicing of my country and use the standard export formats of ADempiere reports in XML to convert to a required format using style sheets.

In my case, I added the functionality through a model validator, so as not to put specialized code in the general functionality of the system.

@e-Evolution e-Evolution self-requested a review April 7, 2021 01:18
@homebeaver
Copy link
Contributor Author

homebeaver commented Apr 7, 2021

Hi Victor, @e-Evolution
Thank you for viewing the PR.

I‘ve sketched my idea in feature request #2658 in Jun 2019.
There are some XML Standards representing Business documents. I know 3 of them pretty good:

  1. openTrans is document oriented but not wide spreaded
  2. UBL by OASIS is wide spreaded and and consists of 62 main business documents
  3. UN/CEFACT schema on the other side is process oriented. The Cross Industry Invoice (CII) spec was released Oct 2016
    CII is more mature then UBL. In the expectation that UN/CEFACT will release its own XML schemas OASIS declared not to release further versions of UBL, see link. The UN/CEFACT Supply Chain Reference Data Model released in 2019 contains Cross Industry Order.
  • Both CII and UBL are accepted syntaxes to the European Norm 16931 for electronic invoices.
  • The XML produced by Adempiere does not implemet any Standard. It just represents the AD data model of an object.

I add PO.externalize() as an abstract method to be implemented in subclass i.e. invoice or order or any other business documents.

The default extrnalization does nothing and createXML ramains unchanged to be back compatible, as you claimed in #1400 (comment) . This is the modification in ExportFormatXML.

The modification in MInvoice ensures no change to createXML and special invoice externalisation only if the client defines the SysConfig key "XML_EINVOICE_SCHEMA_NAME" as specified in issue #2658 mentioned above.

This three small changes in base guarantees minimal side effects and clients/tenants can define own externalization Schema,
i.e. Client A can use UBL , Client B CII, Client C None.
Even own implementations are possible. Client D can use openTrans, but has to implement the externalization.

I hope the answer is datailed enough. PS: My goal is not to implement e-invoice for my client/country. My goal is to extend ADs functionaly with a forward-looking feature I feel confident with.
Grüße / regards
EUGen
FI to @marcalwestf , @trifonnt

@marcalwestf
Copy link
Collaborator

@homebeaver hi Eugen, I just read the information of the links you provided.
Thanks a lot for the time and effort invested trying to explain the matter and convey your ideas.

@e-Evolution Victor's and @yamelsenih Yamel's (valid) objection is that if this PR is about a custom-made or even country-made solution, it should be implemented as a model validator.

Reading the links you provide, it seems that it is not about a custom or a country-only solution, but these are standards indeed, thus worthwhile implementing.

Perhaps can @trifonnt Trifon, @e-Evolution Victor and @yamelsenih Yamel cast their opinions.
Thanks!

@trifonnt
Copy link
Contributor

@homebeaver Eugen, @marcalwestf Mario, @e-Evolution Victor and @yamelsenih Yamel,

Reading the links you provide, it seems that it is not about a custom or a country-only solution, but these are standards indeed, thus worthwhile implementing.

I strongly support this Pull Request to be integrated into the core as this is not country specific solution.

Kind regards,
Trifon

@yamelsenih
Copy link
Member

Hi @trifonnt thanks.

I think that this cuncionality can be integrated using this way for export report: https://wiki.adempiere.net/Spin_Contribution:_FR:_Export_Format_From_Report is very easy implement it and I use this functionality for Tax declaration and other especific format.

Best regards

@homebeaver
Copy link
Contributor Author

Hi, I resoved conflicts on .classpath coming from commit 0a280d2

@homebeaver
Copy link
Contributor Author

Hi @e-Evolution Victor,
I resolved the conflict happened with commit c4c17c9
regards

@marcalwestf
Copy link
Collaborator

@e-Evolution, @yamelsenih in my opinion and after reading the documentation presented as well as @trifonnt and @homebeaver comments, this seems to me to be a solution worthwhile to be included in the trunk.

It could possibly be implemented as an export format, but I think it would be not fair towards @homebeaver to ask him to (once again) modify his implementation because after 2 years , 2 Pull Requests, several changes requested and accomplished, we should come to an end.

I suggest this pull request should be either accepted or rejected. I would accept it, but because there were different opinions and suggestions cast, I refrain from doing it on my own.

Best regards.

@marcalwestf marcalwestf added (TPA) Third Party Access Third Party Access for other apps 01 enhancement 10 Reviewed by Peer 13 Pending Acceptance For Pull Requests: Pending Acceptance labels May 28, 2021
@marcalwestf
Copy link
Collaborator

@homebeaver @trifonnt hi Eugen, Trifon,
the functionality is OK.

What Victor and Yamel are suggesting is to implement the functionality in question without touching core classes, but with a Model Validator, which would do the same.

If somebody doesn't want to use this functionality, he simply has to de-activate the Model Validator and vice-versa. This would bring some advantages like if there is a need for enhancements, improvements, or bug-fixing, they can be done without touching the core classes.

I will move this PR to Release 4.0 so the modifications can be done as suggested and this PR accepted.

@marcalwestf marcalwestf self-requested a review July 9, 2021 15:16
@marcalwestf marcalwestf added this to the 4.0 milestone Jul 9, 2021
@marcalwestf marcalwestf added the 14 Waiting for User Changes Waiting for Pull request User make changes label Jul 9, 2021
@yamelsenih
Copy link
Member

@AlbertoAbudinen AlbertoAbudinen added this to 03 - Technical review in Adempiere 394 Sep 15, 2021
@e-Evolution e-Evolution moved this from 02 - Technical review to 07 - For next version in Adempiere 394 Sep 22, 2021
@yamelsenih
Copy link
Member

Hello @homebeaver I will to close this pull request. Please if you have any change then a new pull request is welcome.

@yamelsenih yamelsenih closed this Nov 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
01 enhancement 10 Reviewed by Peer 13 Pending Acceptance For Pull Requests: Pending Acceptance 14 Waiting for User Changes Waiting for Pull request User make changes (TPA) Third Party Access Third Party Access for other apps ZZ Motionless
Projects
Adempiere 394
07 - For next version
Development

Successfully merging this pull request may close these issues.

None yet

6 participants