odoo
odoo copied to clipboard
odoo
[FIX] web: JS traceback when trying to load unexisting field
To reproduce (V16):
- Force modify a view with an unexisting field For example
res.users.treewith:<field name="unexisting_field"/>( note that odoo won't allow to do so, you will have to do it in raw PSQL * ) - Open Settings > User & Companies > Users => JS traceback error:
UncaughtPromiseError > TypeError
Uncaught Promise > Cannot read properties of undefined (reading 'string')
TypeError: Cannot read properties of undefined (reading 'string')
at http://localhost:8069/web/assets/debug/web.assets_backend.js:67523:84 (/web/static/src/legacy/legacy_load_views.js:67)
...
- : In practice, it happens to Odoo customers after incorrect installations/removal of apps
After this commit: The JS traceback is much clearer regarding the error:
UncaughtPromiseError
Uncaught Promise > Missing field string information for the field 'unexisting_field' from the 'res.users' model
OPW-3071843
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr
@aab-odoo (or anyone who review this PR ) Hey :) Would you mind reviewing this PR please?
I'm not a big fan of the changes that I proposed as I think the previous version was more readable. But as I was calling variable that would have been used after, I prefered to assigned them beforehand to use them instead. Let me know if you see a better way to solve this issue.
@lse-odoo @aab-odoo
@tranlam20220101 I don't really understand the point that you are trying to make with your screenshot, could you give me more details on it please ?
Hello @lse-odoo If there's something to be done, I'd try to do it server-side (for instance in _postprocess_tag_field). It's closer to the source. What do you think?
@aab-odoo
I did discuss on the matter with @rco-odoo, who thinks that it won't be that simple.
In particular in the case in which some fields are "created dynamically" (like the groups user_groups_view on the users).
I did look at previous Odoo version to see how they act in the same scenario, there is actually a Python error:
ValueError: Invalid field 'unexisting_field' on model 'res.users'.
Which is trigger when the JS do a search_read on the model. So I do assume that the same would happen if we just ignore the current error that we have.
Do you think it would be a good idea to use a placeholder value in case that the .string can't be gathered and just move on ?
( it goes a bit too far for the technical support, I'll forward this issue to the bug-fix as it looks like it needs more time and thought)
@lse-odoo I don't think this error has a lot of added value. If someone needs to investigate such an issue, he gets the exact same information by simply stopping on the exception (i.e. the field I'm looking for is not there, but you don't know why, and it could be for a bunch of different reasons, like the one of this issue, a server-side error in an override, a js error in a specific flow...). So for that reason, + because you need to bypass the orm to produce the situation, I would not do anything. Also, here you only changed the legacy codebase. A similar fix would have been needed in the wowl codebase. As I said, this is something you would like to detect server-side.
@aab-odoo I do agree with you, but at the same time "by simply stopping on the exception in the code" is not that simple for the functional support as they would have to learn the debug assets mode, how to breakpoint and how to read the values from their. It's simple for us in the technical team for sure, but not for them and they end up losing a lot of time on this kind of issues. For me, the error message have to be clearer, I do agree with you in the sense that the ORM should in theory be responsible for it. But it's not the case and fixing it will be much more complicated than the JS in this case (I think). Note that on the matter I did delegate the issue to the bug fix as it would take me too much time, the ticket is: https://www.odoo.com/web#id=3081124&menu_id=4720&cids=1&action=3531&model=project.task&view_type=form
Why would you want to prevent a crash in that situation? The db is fucked, there're nonexistent fields in views. There's no point trying to handling this situation client side. It's better to have a direct crash, s.t. you know there's a problem. Not to mention you add 100+loc and complexity for that. Moreover, there are constraints on the model, so this should never happen, unless you directly write in the arch in psql... And if this happens when uninstalling module, maybe we should try to make these operations more robust.
As I already said, the current situation isn't that bad. Simply break on the error and you know what's happening. But if you really want, go back to the original proposition of @lse-odoo which throws a more meaningful message.
@aab-odoo I suggested this approach to avoid the view to crash. Maybe that was not a good idea, but I don't want to be the customer being blocked because some installation fucked up my database. If that happens to me, I might be very aggressive on the support team if such a view is part of my main business.
My initial reaction was that handling that case server-side in _postprocess_tag_field is not an option. That's not the purpose of that method, and it will be costly performance-wise in most cases where it is not needed. We try to stick to the philosophy that correct data should be optimal performance-wise, and incorrect data may have a performance hit for reporting/fixing. The current design choice is to validate the views only when editing them; building views for the client should not validate the existence of fields (which is costly because of some model having "virtual fields".)
If the support team is okay with a slightly better error message, that's fine for me.
@aab-odoo @rco-odoo Speaking in the name of support. I think both approach are acceptable and you are both rights at the same time. Having a better error message was the "simpler" solution I was willing to propose with the amount of this JS error that we have to handle weekly in our pipe (as most of the functional support can't handle this kinds of tickets). Could you please decide together for a solution ? RCO, would you maybe see a better place in the ORM to check if the field is existent which would be stable ? Otherwise, I think we will have to raise a better JS error as I originally proposed (or the ghost field as proposed higher).
And if this happens when uninstalling module, maybe we should try to make these operations more robust.
Yes, it is in progress. But it's not an easy task as it also includes its share of limitations and complexity
@lse-odoo @hubvd
RCO, would you maybe see a better place in the ORM to check if the field is existent which would be stable ?
Nope.
Otherwise, I think we will have to raise a better JS error as I originally proposed (or the ghost field as proposed higher).
I suggest to go back to your improved JS error, if that is good enough. The "ghost field" approach may be a bit overkill.
@lse-odoo I reverted back to your original commit. If that's ok with you, it should be fine
@aab-odoo could you re-review it please (I did change a bit as some mobile test was failing due to my fix)