-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
[9.0][MIG] account_invoice_import_invoice2data and dependencies: Migration to 9.0 #11
Conversation
Can you please check Travis? |
this will stay red until OCA/partner-contact#364 is merged |
and then OCA/community-data-files#20 |
Ouch, I have launched rebuilds... |
I have merged all the dependencies, but Travis is still red. Please check. |
The field ean13 has been renamed ; code needs to be updated accordingly |
de3b2e2
to
d03bf73
Compare
now it's only the lacking chart of accounts. Do we have a general solution for this or should every test create accounts as necessary? |
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.
Hi @hbrunn,
I've some small requests. Other than that LGTM.
In account_invoice_import:
To use this module, go to the menu Accounting > Suppliers > Import Invoices and upload a PDF or XML invoice of your supplier.
This menu item does not exists. I think it should be "Invoicing -> Purchases -> Import Invoice"
Can you move the models to the models directory (e.g. partner.py to models/partner.py), please?
In account_invoice_import_invoice2data:
French users should also install the module l10n_fr_invoice_import available in the French localization, cf this PR.
The hyperlink for "This PR" can be removed; it refers to a PR for version 8.0.
In base_business_document_import:
additionnal -> additional
I'll look into that next time I work on this. Will take a while I guess. Next time, please use github's review feature, this makes it a lot simpler to follow up on a review because then the comments are attached to the line of code the comment talks about |
@hbrunn I agree, but I don't know how to review lines that are not changed. :( |
@tarteo I took into account your remarks on my PR for v10 |
@tarteo Press the little button that looks like and the file will expand. |
@thomaspaulb I know that button. But when I expand the file contents I still cannot review the lines that aren't changed (the + button does not appear at the line). |
@tarteo done |
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.
@hbrunn Thanks 👍
811f32a
to
70ac9e2
Compare
49f3c01
to
0621e65
Compare
0621e65
to
39ba10d
Compare
1f19223
to
ebc3d9f
Compare
@@ -24,6 +24,7 @@ addons: | |||
# Search your packages here: | |||
# https://github.com/travis-ci/apt-package-whitelist/blob/master/ubuntu-precise | |||
- wkhtmltopdf # only add if needed and check the before_install section below | |||
- poppler-utils # for invoice2data |
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.
@alexis-via I was doubting my sanity because for the life of me, I couldn't get the tests work. Then I got the suspicion that pdftotext parses the test invoice incorrectly, and that;s the case, see https://travis-ci.org/OCA/edi/jobs/230782037 - check the install phase, there I first print the output of standard pdftotext on travis, then the version you install. As you see, the total line is botched in the second version. I guess you needed the private version because back then travis had an ancient version only? Any objections to do it the way I propose? And I guess that's also the reason why this still runs on the sudo infrastructure, any objections to change this to the current OCA standard?
1 similar comment
@alexis-via @pedrobaeza it's green finally :-) |
@alexis-via can we merge this? |
@alexis-via can we merge in order to avoid duplicated efforts as in #23 ? |
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. |
No description provided.