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

[FIX] base: add more information if wrong related field #31889

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@fmdl
Copy link
Contributor

fmdl commented Mar 15, 2019

Description of the issue/feature this PR addresses:
During a migration process if a field have change/removed, during the installation there is a KeyError on related field.

Before :
KeyError: "purchase_line_id"

After
KeyError: "Wrong related field, 'stock.move.purchase_order_id', relation : 'purchase_line_id.order_id', wrong field : 'purchase_line_id'"

@odony @nim-odoo

cc @sla-subteno-it

--
I confirm I have signed the CLA and read the PR guidelines at www.odoo.com/submit-pr

@Yenthe666

This comment has been minimized.

Copy link
Contributor

Yenthe666 commented Mar 16, 2019

Should make debugging easier in some cases. +1

@nim-odoo nim-odoo requested a review from Elkasitu Mar 18, 2019

@Elkasitu
Copy link
Contributor

Elkasitu left a comment

@nim-odoo can you r+?

@nim-odoo
Copy link
Contributor

nim-odoo left a comment

robodoo r+

@robodoo robodoo added the r+ 👌 label Mar 18, 2019

robodoo pushed a commit that referenced this pull request Mar 18, 2019

[FIX] add more information
closes #31889

Signed-off-by: Nicolas Martinelli (nim) <nim@odoo.com>
@odony

This comment has been minimized.

Copy link
Contributor

odony commented Mar 18, 2019

@robodoo r-
@nim-odoo @Elkasitu Could you guys take care of commit messages before merging PRs?

@xmo-odoo are you still opposed to making merge-bot more responsible for whatever it's merging? ;-)

@xmo-odoo

This comment has been minimized.

Copy link
Collaborator

xmo-odoo commented Mar 18, 2019

@xmo-odoo are you still opposed to making merge-bot more responsible for whatever it's merging? ;-)

Mostly yeah, it's not really its point or goal. I still think it would make more sense to have separate status check / checkers for constraints we want to put on commit or PR messages (and others e.g. we might eventually want to add JS linters and whatnot).

@nim-odoo

This comment has been minimized.

Copy link
Contributor

nim-odoo commented Mar 18, 2019

@fmdl Could you please make sure to write correct commit messages? As far as I can remember, I always had to rewrite your commit message for all your PRs I merged. Most of the time, it was a simple copy-paste of the PR title/description since these are fine. Thanks.

@fmdl fmdl force-pushed the fmdl:patch-138 branch from adb136e to 7107d7b Mar 18, 2019

@robodoo robodoo removed the CI 🤖 label Mar 18, 2019

@fmdl

This comment has been minimized.

Copy link
Contributor Author

fmdl commented Mar 18, 2019

@nim-odoo for the future I will try to make a better commit message.

@robodoo robodoo added the CI 🤖 label Mar 18, 2019

@fmdl fmdl force-pushed the fmdl:patch-138 branch from 7107d7b to 7abbc8e Mar 19, 2019

@robodoo robodoo removed the CI 🤖 label Mar 19, 2019

@fmdl fmdl force-pushed the fmdl:patch-138 branch from 7abbc8e to 59af791 Mar 19, 2019

@robodoo robodoo added the CI 🤖 label Mar 20, 2019

@@ -524,6 +524,9 @@ def _setup_related_full(self, model):
# determine the chain of fields, and make sure they are all set up
target = model
for name in self.related:
if name not in target._fields:

This comment has been minimized.

Copy link
@odony

odony Mar 20, 2019

Contributor

Instead of an extra lookup in target._fields, how about using target._fields.get(name) below and raising if the result is None? It's not critical but it's not more code, so why make 2 lookups when we can make only 1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.