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

[15.0][MIG] base_business_document_import #564

Closed

Conversation

@tbanevicius
Copy link
Author

@simahawk could you possibly review it? But I understood you already did but in different MR --> #492

@etobella
Copy link
Member

Can you squash the last commits? You can do

git remote update
git rebase -i origin/13.0

and merge them using fixup or squash

@simahawk
Copy link
Contributor

image

this should not be here

alexis-via and others added 25 commits April 20, 2022 09:10
…voice dict, cleaner organisation)

Code refactoring: move code in base_business_document_import, factorise code for tax matching (it was duplicated in UBL and ZUGFeRD)
Now support PDF with embedded UBL XML file
Enable unittests on account_invoice_import_ubl
More absolute xpath in account_invoice_import_ubl instead of relative xpath

WARNING: these are big changes, I may have broken a few details
Adapt module account_invoice_import_ubl to use the new base_ubl module
Small fixes
Better generation of address block in UBL (make it coherent with the datamodel of Odoo)
Add generation of several UBL blocks: language, delivery, payment terms, customer party, spplier party
Add parsing of zip in UBL party (will be used in the future for delivery partner match)
Use country code and state code to match partners
UBL: Add delivery terms and line item UBL XML block generation
UBL: add parsing of delivery block
Add unitests in base_business_document_import
Small code enhancements/simplifications
Code cleanup/minor changes
Rename key 'quantity' to 'qty' in all parsing dicts['lines']
Add common methods compare_lines() and post_create_or_update()
Make sure price_unit is always untaxed in UBL XML files
Add support for partner bank matching on invoice update (before, it was only supported on invoice creation)
[FIX] LINT

Use try/except when importing external libs
Remove self.ensure_one() that has nothing to do in an api.model method
Rename __openerp__.py to __manifest__.py and set installable to False
Add match for accounts and journals
Improve match for journals and accounts
Also port all the modules that generate the XML documents: account_invoice_ubl, account_invoice_zugferd, purchase_order_ubl and sale_order_ubl
… module

Fix spelling mistake and other remarks on README by Tarteo
OCA-git-bot and others added 14 commits April 20, 2022 09:10
Updated by "Update PO files to match POT (msgmerge)" hook in Weblate.

Translation: edi-14.0/edi-14.0-base_business_document_import
Translate-URL: https://translation.odoo-community.org/projects/edi-14-0/edi-14-0-base_business_document_import/
Currently translated at 97.4% (38 of 39 strings)

Translation: edi-14.0/edi-14.0-base_business_document_import
Translate-URL: https://translation.odoo-community.org/projects/edi-14-0/edi-14-0-base_business_document_import/fr_FR/
Currently translated at 56.4% (22 of 39 strings)

Translation: edi-14.0/edi-14.0-base_business_document_import
Translate-URL: https://translation.odoo-community.org/projects/edi-14-0/edi-14-0-base_business_document_import/nl/
Currently translated at 94.8% (37 of 39 strings)

Translation: edi-14.0/edi-14.0-base_business_document_import
Translate-URL: https://translation.odoo-community.org/projects/edi-14-0/edi-14-0-base_business_document_import/nl/
Currently translated at 94.8% (37 of 39 strings)

Translation: edi-14.0/edi-14.0-base_business_document_import
Translate-URL: https://translation.odoo-community.org/projects/edi-14-0/edi-14-0-base_business_document_import/fr/
Reduce _match_partner complexity
@tbanevicius tbanevicius force-pushed the 15.0-base_business_document_import branch from d7bbb73 to 4b36100 Compare April 20, 2022 06:12
@tbanevicius
Copy link
Author

@etobella @simahawk fixed it.

@etobella
Copy link
Member

Just one last thing, can you rename the last two commits with the right name, [15.0-mig-base] shouldn't be there

@tbanevicius tbanevicius force-pushed the 15.0-base_business_document_import branch from 4b36100 to a34078a Compare April 20, 2022 06:23
@tbanevicius
Copy link
Author

@etobella should be okey now.

@tbanevicius
Copy link
Author

Hi @pedrobaeza . In old MR you've mentioned that this module should reflect on Odoo 15 core EDI. But are you sure? This module only contains helper methods for base document import.

@pedrobaeza
Copy link
Member

My thoughts are that Odoo is able to do the same stuff without this, so I suppose they have their tooling for that, so let's reuse it.

@tbanevicius
Copy link
Author

My thoughts are that Odoo is able to do the same stuff without this, so I suppose they have their tooling for that, so let's reuse it.

Thank you for your insight. Unfortunately I don't have time to do re-design anytime soon.

@simahawk
Copy link
Contributor

I agree. We should start getting rid of such gigantic I-do-all-you-want models.
For starting, I found out that the PDF parsing is also duplicated in base_ubl.
I'm moving it out here #596 (please review if you have time).

While adding some more test cov I've found also that some modules are partially broken on v14 because of missing inheritance on this big model which from my POV it shouldn't be at all inherited. See #595.

Very likely many other methods can be simplified or re-organized (eg: smaller models by topic) or moved out.
For v15 we can simply deprecate calls.
Of course this requires a lot of time and knowledge of the whole impact on depending modules (which I definitely miss ATM).
But we should start from something at least... :)

A good starting point would be to add more test coverage so that we'll know (more likely) if something gets broken while refactoring.

Today I see:

base_business_document_import/tests/test_business_document_import.py .........                                                                                     [100%]

---------- coverage: platform linux, python 3.8.12-final-0 -----------
Name                                                               Stmts   Miss  Cover   Missing
------------------------------------------------------------------------------------------------
base_business_document_import/__init__.py                              1      0   100%
base_business_document_import/models/__init__.py                       1      0   100%
base_business_document_import/models/business_document_import.py     575    246    57%   22-23, 46, 53, 61, 78, 91-102, 126-132, 134-145, 161-166, 173, 242, 244, 264, 268-271, 276, 291, 300, 350, 377, 379, 392, 396, 412, 450-501, 535, 537, 615, 617, 624, 639, 658, 677-689, 700, 723, 725, 734, 750-761, 787-788, 830, 832, 841, 916-1001, 1006-1022, 1044, 1050, 1052, 1077, 1089-1097, 1108-1122, 1134-1141, 1152-1167, 1183-1208, 1212-1239, 1245-1278, 1282-1299
------------------------------------------------------------------------------------------------
TOTAL                                                                577    246    57%

Which means we have plenty of work to do on this side.

@tbanevicius could you have a look at non tested lines and see if you can improve the situation?

@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 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 PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 16, 2022
@github-actions github-actions bot closed this Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet