[FIX] account: log error in chatter when importing xml #163849

JulienVR · 2024-04-29T14:59:38Z

Before c02d8b1, an error occuring during the import was logged in the chatter. This commit restores this behaviour, as it is highly helpful to have the error message (it could probably avoid customers to open tickets in some cases).

Ticket link: https://www.odoo.com/web#model=project.task&id=3874857
opw-3874857

robodoo · 2024-04-29T15:00:14Z

Pull request status dashboard.

lordkrandel

The idea is noble, and this is an open pain-point (IMHO).

Yet, this bit of code is quite complicated, and re-instating the error like in your PR has several consequences:

There might be more than one decoder per attachment (see the unwrap_attachments function).
Every decoder has a check function that just looks at the filename, so let's suppose that an XML file is both checked as "OK I can handle that" by PEPPOL decoder, French EDI decoder and Italian EDI decoder. The French EDI decoder will always fail and have a traceback on all Italian invoices. This is mitigated because the "check" methods for decoders rely on having different filename patterns, but it's still possible. Furthermore, that may change in the future. We got partners asking that we look into the the structure of any XML file imported to say that it's an Italian invoice or not, so that they can import invoices with any given filename. That would require compute time and developer time.
In case the input file is wrong, we would have a (server language!) traceback in the chatter, moving the attention of the client from his own file to "there's an error in the system". Maybe he uploaded a base64-ed XML file. It happens, even by mistake.
We're not sure that an empty invoice created by get_edi_creation with errors in the chatter is the correct way to handle a failed import. Maybe we can have exception logs on the server and a UserError appearing: "Cannot import this file."
On the side - The try..except is too large, it also involves the link between the file and the Purchase Order, while it should be limited to the failing decoder. I'd like this to be addressed when touching this bit.

To rework all this, we thought with @jco-odoo and @antoine162 about opening a task and discussing about it, but no one had time to follow that.

JulienVR · 2024-05-06T06:43:37Z

Euuuh, no, it's not supposed to work like this, you have a concrete example ? In _get_edi_decoder, we use a specific criterion to know whether we import using a decoder. And it should not raise an Exception.
Suppose you receive a ticket with this error, do you prefer:
- having "Error importing attachment '%s' as invoice (decoder=%s)" without any info ? You will then have to look at the logs
- having the same error but with the actual exception that occured
We can ask ourself the question in master, but the current behaviour from 17.0 has to be fixed
Again, I'm not saying this is perfect right now, but we are already heading to the right direction with this commit, and if we decide to change this, it will be in master anyway

smetl · 2024-05-06T06:59:55Z

@lordkrandel

No. Most of the EDI are looking to some data into the decoded etree like the customizationId for Peppol. If you found a bug, maybe you should fix it instead of complaining about that.
I agree the name of a decoder and a traceback are not user friendly. We shouldn't return things like that to the end user. It has been added by your commit btw: 257007d 🤦 We should revert that imo.
Who is "we" exactly?
For whatever reason, you don't want to break the whole import in case of exception.

To rework all this, we thought with @jco-odoo and @antoine162 about opening a task and discussing about it, but no one had time to follow that.

No need to rework "all this" imo. What's the interest of doing at? 🤔

lordkrandel · 2024-05-06T09:05:46Z

No. Most of the EDI are looking to some data into the decoded etree like the customizationId for Peppol. If you found a bug, maybe you should fix it instead of complaining about that.

Don't use an aggressive tone, please, I'm a very sensitive person, and might go defensive.
I probably just had wrong/outdated assumptions on how other EDIs and the check itself work.

The Italian EDI doesn't actually get a decoded etree. Sometimes we receive CADES digitally-signed files with the .xml.p7m extension which start with a binary header (and footer) enveloping the actual XML, and we cannot parse the XML directly, so these functions were born: one, two, three

We can actually change in the Italian EDI and make it work normally:

.xml files check the first tag in the etree structure,
.xml.p7m or .p7m files get the signature removed and then the .xml decoding function is called
if it's .xml but it's CADES-signed it will fail the XML parse and we refuse the file.
The customer will open it and see it's signed, and will have to rename it to .xml.p7m

There will be people complaining anyway, since I guess this "feature" was exactly made for the reason that people were trying to import signed misnamed files.

I agree the name of a decoder and a traceback are not user friendly. We shouldn't return things like that to the end user. It has been added by your commit btw: 257007d 🤦 We should revert that imo.

The main point of the PR was to give user feedback. Before that change, the user would upload a file and get a totally blank invoice because the error was just logged. It happened a lot and tickets were opened.

We can simply do both!

the generic message in the chatter, stripped of the exception text
the logger message with the exception text.

Anyway, please take the time to conform the .xml/.p7m checks for Italy, or file a task for someone to do it. This is what I mean with rework all that: take ownership of the problem and don't just revert uncaringly, because even if it's to be fixed, it was written for a reason.

Who is "we" exactly?

Me, @antoine162, @jco-odoo, the people involved in the discussion when the "no-user-feedback in the EDI when an exception occurs" problem arose.

Suppose you receive a ticket with this error, do you prefer having "Error importing attachment '%s' as invoice (decoder=%s)" without any info ? You will then have to look at the logs having the same error but with the actual exception that occured.

By no means I want the traceback in the chatter.
A modal UserError with the traceback would be better than that.

If we both post and log, the customer support will then know that in the server logs of the instance there is the exception text, and the user won't get too scared and maybe look inside the file to see that he's uploaded something appropriate before opening a ticket.

smetl · 2024-05-06T09:52:44Z

@lordkrandel Understand me well: I just discover you merged things with @jco-odoo like
c02d8b1 with

no explanation on the commit message
no test at all anywhere
without asking me a review

I don't want to be aggressive but I don't like the way you are doing things. You are talking much about opening a task and discuss nicely about it but it's already merged and I guess I'm just supposed to deal with it.
As you said, it's complex code, difficult to maintain/debug/understand and the handling of errors is not easy. That's clearly something that need to be changed very carefully

lordkrandel · 2024-05-06T10:10:21Z

no explanation on the commit message

no test at all anywhere

You're correct on the first two, the idea was clearly to have more work on it... but didn't happen, so I'm sorry for that.
We've been working in "emergency mode" non-stop on l10n_it_edi for 2 years after we opened a non-ready totally untested module to the public (that still misses law-required features) with no help from you or anyone.

I'll make sure that from now on every part that it's not directly l10n_it_edi specific is clearly stated, tested and gets notified.

without asking me a review

I thought the reviewer was Josse, it was very clear to him what I was doing. Please talk to @qdp-odoo to state it clearly if the process is different for EDI matters and it has some kind of hierarchy or process I didn't know of.

I don't want to be aggressive but I don't like the way you are doing things.

Neither did I like the entire account_edi framework I had to get rid of or silencing any errors coming from import (which is known to be a source of errors and tracebacks) to the logs with no user feedback. Still I don't come and blame people for their code I don't like, as I know that things happen for reasons.

I'm just supposed to deal with it.

What? I didn't make the code that had no user feedback at all, and I had to put something there in the first place. If the patch is not good or not enough, we are just discussing and making it better. I can do the (other) task myself soon, if you don't have time.

lordkrandel · 2024-05-06T10:50:03Z

addons/account/models/account_move.py

+                except Exception as e:
+                    message = _(
+                        "Error importing attachment '%s' as invoice (decoder=%s): %s",
+                        file_data['filename'],
+                        decoder.__name__,
+                        str(e),
+                    )
                    invoice.sudo().message_post(body=message)
                    _logger.exception(message)


Suggested change

except Exception as e:

message = _(

"Error importing attachment '%s' as invoice (decoder=%s): %s",

file_data['filename'],

decoder.__name__,

str(e),

)

invoice.sudo().message_post(body=message)

_logger.exception(message)

except Exception as e:

chatter_message = _(

"Error importing attachment '%s' as invoice (decoder=%s)."

" Check the file itself, or read the application logs"

" for more information.",

file_data['filename'],

decoder.__name__

)

invoice.sudo().message_post(body=chatter_message)

_logger.exception(

"Error importing attachment '%s' as invoice (decoder=%s): %s",

file_data['filename'],

decoder.__name__,

str(e)

)

Would this do?

Sorry, I made a few edits after post ^^.
~~Still re-reading, but~~ I hope you get what I mean with the suggestion.

@lordkrandel I'm still not convinced by the invoice chatter message. The user doesn't know what a decoder is. Do we really need to log something in that case? :(
(the _logger is better yes)

Look, let's say I'm a user.

I upload an invoice XML.

It appears a blank invoices with no lines, no quantities, no errors at all. Blank draft and the invoice attached. Almost like I pressed the "new" button.

I think I did something wrong, so I retry.

Another new invoice completely blank.

I don't know what's wrong, but there's a header on top (totally unrelated ofc but he doesn't know) saying "you have no credits to do OCR"

I open a ticket about l10n_it_edi and OCR

Support knows nothing about this. They don't even know that in the logs there might be an import exception. And they ask... PGI about the ticket.

This happened many times.

What is hard to understand from "We need user feedback somewhere"?
It's a concept that in other contexts I think we well cover with UserErrors, ValidationErrors, warnings... (tracebacks even!) We have user feedback when we send, we have all kinds of warnings and "Rejected" states.
But if the user imports a file, and it goes wrong, it gets a blank invoice.

I don't care if he knows what a decoder is, but somehow he needs to know there was an error somewhere.

@lordkrandel OK then if you insist

If find this really painful for a user having errors, if he doesn't have access to the logs, he will have to wait for 3 weeks to get an answer from the support... but I see you're not convinced 🤷‍♂️

chklop · 2024-05-14T07:15:18Z

Chiming in:

We're handling unforeseen errors. It will happen, we need a clean way to handle that for the users. A traceback is bad because it would also block the creation of the record, and thus the user. The good solution is to try/catch the error to allow the creation of an empty record, allowing the user to do it manually.
We agree that many of those cases should also be fixed once they're detected, but that does not remove the need above.
Having some kind of error message logged somewhere accessible would increase the user (if he's technical enough), or the support team, to investigate the matter.

I'm not a super fan of just putting that just in the logs, as almost nobody will be able to access that. The chatter is good, but I also agree that random technical error messages in the chatter is bad.

Do we have a possibility to put the error message in the chatter, but in debug mode (which would live up to its name for once) ?
If not, then maybe logs are the best place to put that ?

lordkrandel · 2024-05-14T07:21:06Z

I'm not a super fan of just putting that just in the logs, as almost nobody will be able to access that. The chatter is good, but I also agree that random technical error messages in the chatter is bad.

If there's one (or more) deity/ies, thanks for Product Owners.

Modify the error message to state that the exception can be found in the logs. NB: Before c02d8b1, an error occuring during the import was logged in the chatter, but we don't want to show it to the user. For instance: opw-3874857

Italian EDI used to look at the filename to determine whether the uploaded attachment is related to it or not. With this change we look at the structure of the XML file, making it more standard - it's what all other EDIs do. CADES signed files (.xml.p7m) have their signature removed and then they are treated like normal XML files. In order to be able to do this, we have to move the code that specially handles the account_predictive_bills for Italy to the `account` level. Related PR: odoo#163849 Related issue: odoo#150382

JulienVR requested review from a team and alialfie and removed request for a team April 29, 2024 14:59

C3POdoo added the OE the report is linked to a support ticket (opw-...) label Apr 29, 2024

lordkrandel self-requested a review May 3, 2024 15:00

lordkrandel requested changes May 3, 2024

View reviewed changes

lordkrandel requested review from jco-odoo and antoine162 May 3, 2024 15:18

lordkrandel requested changes May 6, 2024

View reviewed changes

JulienVR force-pushed the 17.0-error-edi-attachment-juvr branch from 59d160b to c65a13b Compare May 14, 2024 07:13

JulienVR force-pushed the 17.0-error-edi-attachment-juvr branch from c65a13b to fe17810 Compare May 14, 2024 07:45

lordkrandel mentioned this pull request May 15, 2024

[IMP] account, l10n_it_edi: check XML structure instead of filename #165673

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIX] account: log error in chatter when importing xml #163849

[FIX] account: log error in chatter when importing xml #163849

JulienVR commented Apr 29, 2024 •

edited by lordkrandel

robodoo commented Apr 29, 2024

lordkrandel left a comment •

edited

JulienVR commented May 6, 2024

smetl commented May 6, 2024

lordkrandel commented May 6, 2024 •

edited

smetl commented May 6, 2024 •

edited

lordkrandel commented May 6, 2024 •

edited

lordkrandel May 6, 2024 •

edited

lordkrandel May 6, 2024 •

edited

smetl May 13, 2024 •

edited

lordkrandel May 13, 2024 •

edited

smetl May 13, 2024

JulienVR May 14, 2024

chklop commented May 14, 2024

lordkrandel commented May 14, 2024

[FIX] account: log error in chatter when importing xml #163849

Are you sure you want to change the base?

[FIX] account: log error in chatter when importing xml #163849

Conversation

JulienVR commented Apr 29, 2024 • edited by lordkrandel

robodoo commented Apr 29, 2024

lordkrandel left a comment • edited

Choose a reason for hiding this comment

JulienVR commented May 6, 2024

smetl commented May 6, 2024

lordkrandel commented May 6, 2024 • edited

smetl commented May 6, 2024 • edited

lordkrandel commented May 6, 2024 • edited

lordkrandel May 6, 2024 • edited

Choose a reason for hiding this comment

lordkrandel May 6, 2024 • edited

Choose a reason for hiding this comment

smetl May 13, 2024 • edited

Choose a reason for hiding this comment

lordkrandel May 13, 2024 • edited

Choose a reason for hiding this comment

smetl May 13, 2024

Choose a reason for hiding this comment

JulienVR May 14, 2024

Choose a reason for hiding this comment

chklop commented May 14, 2024

lordkrandel commented May 14, 2024

JulienVR commented Apr 29, 2024 •

edited by lordkrandel

lordkrandel left a comment •

edited

lordkrandel commented May 6, 2024 •

edited

smetl commented May 6, 2024 •

edited

lordkrandel commented May 6, 2024 •

edited

lordkrandel May 6, 2024 •

edited

lordkrandel May 6, 2024 •

edited

smetl May 13, 2024 •

edited

lordkrandel May 13, 2024 •

edited