-
Notifications
You must be signed in to change notification settings - Fork 23k
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
[ADD] l10n_vn_edi: Vietnamese E-Invoicing #160079
base: 17.0
Are you sure you want to change the base?
Conversation
6e24331
to
0825a22
Compare
2c05545
to
c419704
Compare
87be19f
to
dbe0d15
Compare
dbe0d15
to
75cf953
Compare
adfca8a
to
fcd6d58
Compare
6178d53
to
0e7e521
Compare
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.
The work you have done is very clean and I can't find much to say 😄 Here are a few comments/questions I have so far, I'll keep reviewing next week (went through some of the flows but not all of them yet).
Also, I discussed with Benj and I would like to attend the small meeting you'll have with him for the testing, just to make sure I didn't forget anything. Of course I'll do that if you agree. He told me he would prefer doing it post-freeze as this task is in 17.0.
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.
I'm far from done reviewing, but since I had answers for Hugo's review, and I can't post a message while a review is started...
Here you go!
addons/l10n_vn/demo/demo_company.xml
Outdated
@@ -8,7 +8,7 @@ | |||
<field name="country_id" ref="base.vn"/> | |||
<field name="state_id" ref="base.state_vn_VN-06"/> | |||
<field name="zip">76463</field> | |||
<field name="phone">+84 91 234 56 78</field> | |||
<field name="phone">084 91 234 56 78</field> |
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.
Why the change?
<field name="phone">084 91 234 56 78</field> | |
<field name="phone">0084 91 234 56 78</field> |
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.
Because the EDI does not support + and it would be better if our demo data work out of the box for it when possible.
Maybe a better solution would be to manage the + in the edi code, not quite sure of that.
"description": """ | ||
Vietnam - E-invoicing | ||
===================== | ||
Using SInvoice by Viettel |
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.
Shouldn't the module name contain viettel
since there are other providers?
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.
It could, I checked and saw that for other countries we didn't add this information in the name.
That's why I added it in the summary (so that searches are easier).
If you ask to add it, there, I'm ok with that.
@@ -169,3 +169,8 @@ def refund_moves(self): | |||
|
|||
def modify_moves(self): | |||
return self.reverse_moves(is_modify=True) | |||
|
|||
def _modify_move_default_values(self, origin_move): |
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.
Can't this be done through _copy_data_extend_business_fields
?
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.
I didn't know of it, but it looks like it's only for AML and not the moves themselves (which does not seem to have a similar feature)
@hupo-odoo For the testing with Benj later, of course, feel free to join if needed 😄 I'll push the changes based on the feedback from you and William when I get some time |
0e7e521
to
c8a7e45
Compare
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.
Here are some other comments 😃
files_data = self._l10n_vn_edi_get_file_data('ZIP') | ||
|
||
# Sometimes the documents are not available right away. This is quite rare, but I saw it happen a few times. | ||
# To handle that we will try up to three time to fetch the document => The impact should be negligible. | ||
threshold = 1 | ||
while not files_data['fileToBytes'] and threshold < 3: | ||
time.sleep(0.125 * threshold) | ||
files_data = self._l10n_vn_edi_get_file_data('ZIP') | ||
threshold += 1 | ||
|
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.
Are you sure that if the documents are not yet available, the file data will simply not contains the values ? I tried several times, and in some cases, I got the issue "Invoice not found", but as it was an error message, it then raises the error in the _l10n_vn_edi_get_file_data
function. I'm wondering if this is the issue that corresponds to the "documents not available" you're trying to solve, or maybe I got another error for unknown reason. Because then in my case, as the error is raised, it does not continue in the while loop, and it is directly raised.
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.
That's how it was in my case. the "Invoice not found" is most likely a separate error.
Are you able to reproduce it so that I can check?
errors.append(_('Sinvoice credentials are missing on company %s.', company.display_name)) | ||
if not company.vat: | ||
errors.append(_('VAT number is missing on company %s.', company.display_name)) | ||
if company.phone and not company.phone.replace(' ', '').isdecimal(): |
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.
checking if there are only digits might be a bit too restrictive. For example, if I try to set a US phone number (starting by 001), it is automatically formatted with +1, and will therefore raise an error when sending an invoice to this customer.
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.
Yes, it's annoying =/
The API Only supports digits. The only solution would be to automatically replace "+" by "00" in the code if it's there?
I did a simple phone number cleaning method and used that but not 100% sure it's ideal.
8ab740f
to
fbf1677
Compare
Add support for E-Invoicing in Vietnam by allowing to connect Odoo with SInvoice by Viettel. Task id # 3631616
fbf1677
to
bf755ff
Compare
Add support for E-Invoicing in Vietnam by allowing to connect Odoo with SInvoice by Viettel.
Task id # 3631616
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr