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] models.py: Raise user friendly error message on sql constraints #36458

Closed
wants to merge 12 commits into from

Conversation

@tivisse
Copy link
Contributor

commented Sep 5, 2019

Purpose

On a record creation, a valid and user friendly error message is raised
when a sql constraint is violated. (Eg: 'The start date must be anterior
to the end date').

On a record modification, this is not always the case anymore, since the
latest ORM modifications.

Eg: Write on a leave with a date_from > date_to, you got the postgreSQL
error, which is not understable by a classic human.

This is due to the fact that the error is correctly catched and logged,
but the result is set into the cache, triggering the execution of computation
methods, and calling _validate_fields. It is highly probable that a flush
is done in the constraint method (search, read, ...). In that case the
postgreSQL error is converted into a ValidationError, with a stringified
version of the IntegrityError, and thus is not catched by the check wrapper.

Description of the issue/feature this PR addresses:

Current behavior before PR:

Desired behavior after PR is merged:

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

@robodoo robodoo added the seen 🙂 label Sep 5, 2019

@C3POdoo C3POdoo added the RD label Sep 5, 2019

@tivisse tivisse force-pushed the odoo-dev:saas-12.5-fixes-yti branch 3 times, most recently from 96c65f5 to 39d47d4 Sep 5, 2019

@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Sep 5, 2019

@tivisse tivisse force-pushed the odoo-dev:saas-12.5-fixes-yti branch from 52b50a0 to 158a7b0 Sep 5, 2019

@tivisse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@tivisse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@robodoo rebase-ff

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

Merge method set to rebase and fast-forward

@robodoo robodoo added the r+ 👌 label Sep 5, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

'ci/runbot' failed on this reviewed PR.

2 similar comments
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

'ci/runbot' failed on this reviewed PR.

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

'ci/runbot' failed on this reviewed PR.

@tivisse tivisse force-pushed the odoo-dev:saas-12.5-fixes-yti branch from 158a7b0 to 80b46d7 Sep 10, 2019

@robodoo robodoo removed the r+ 👌 label Sep 10, 2019

@tivisse tivisse force-pushed the odoo-dev:saas-12.5-fixes-yti branch from 80b46d7 to 9a0f650 Sep 10, 2019

@tivisse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

robodoo pushed a commit that referenced this pull request Sep 10, 2019
[FIX] models.py: Don't commit after fetching a translation
Purpose
=======

When trying the translate a sql_contraint message after an integrity error
we use the current cursor in a 'with', just to make a SELECT query.

That way, the cursor is commited at the end of the with statement, trying
to commit the inconsitent state of the cursor that was the reason of the
integrity error.

Just close the cursor, as we only fetch translations from the database.

closes #36458

Signed-off-by: Yannick Tivisse (yti) <yti@odoo.com>
@@ -357,6 +357,7 @@ def create(self, vals_list):
templates._create_variant_ids()

# This is needed to set given values to first variant after creation
self.flush()

This comment has been minimized.

Copy link
@beledouxdenis

beledouxdenis Sep 10, 2019

Contributor

We wonder what's the use of this. Could you specify? Maybe with a comment?

This comment has been minimized.

Copy link
@tivisse

tivisse Sep 11, 2019

Author Contributor

To avoid a cache miss. When trying to update the variant values according to the template values, the newly created records should be flushed.

Don't you think that the commit message is not sufficient ? :)

In fact, maybe we could add to the guidelines the fact that all the calls to flush should be documented in that case.

What do you think ?

cc @jem-odoo @tde-banana-odoo

This comment has been minimized.

Copy link
@beledouxdenis

beledouxdenis Sep 11, 2019

Contributor

Well, even with this comment I still don't get it.

the newly created records should be flushed.

creates are not delayed. So if you need a a template id for your variants, it will be there.

Unless you require a specific stored field for the template that has just been created, this call to flush shouldnt be needed. And if this is the case, then should precise the field in the call to flush so this is explicit. For instance

self.flush(['amount_total'])

What is the use case leading to the issue/traceback?

This comment has been minimized.

Copy link
@tivisse

tivisse Sep 11, 2019

Author Contributor

Hi.

After a small investigation, it seems you're right ! ;)

This commit bb505de fix both issues.

Thanks.

robodoo pushed a commit that referenced this pull request Sep 10, 2019
[FIX] models.py: Don't commit after fetching a translation
Purpose
=======

When trying the translate a sql_contraint message after an integrity error
we use the current cursor in a 'with', just to make a SELECT query.

That way, the cursor is commited at the end of the with statement, trying
to commit the inconsitent state of the cursor that was the reason of the
integrity error.

Just close the cursor, as we only fetch translations from the database.

closes #36458

Signed-off-by: Yannick Tivisse (yti) <yti@odoo.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Staging failed: ci/runbot on 56be306196ee4ba3dfd1a1765f193a954c4e5ce0 (view more at http://runbot.odoo.com/runbot/build/614541)

@robodoo robodoo added error 🙅 and removed r+ 👌 labels Sep 11, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Staging failed: ci/runbot on bbadab5dce0f014e229c18048a5c39bd5c6fb458 (view more at http://runbot.odoo.com/runbot/build/615179)

RomainLibert and others added 11 commits Sep 9, 2019
[FIX] web: Display event name on calendar popup instead of undefined
Purpose
=======

When you edit a Time Off Request from the dashboard. The title in the header
is undefined

https://drive.google.com/open?id=1vBg9AlVCN7rc5D6DUel8hx2uY-oOfwRs
[FIX] models.py: Raise user friendly error message on sql constraints
Purpose
=======

On a record creation, a valid and user friendly error message is raised
when a sql constraint is violated. (Eg: 'The start date must be anterior
to the end date').

On a record modification, this is not always the case anymore, since the
latest ORM modifications.

Eg: Write on a leave with a date_from > date_to, you got the postgreSQL
error, which is not understable by a classic human.

This is due to the fact that the error is correctly catched and logged,
but the result is set into the cache, triggering the execution of computation
methods, and calling _validate_fields. It is highly probable that a flush
is done in the constraint method (search, read, ...). In that case the
postgreSQL error is converted into a ValidationError, with a stringified
version of the IntegrityError, and thus is not catched by the check wrapper.
[FIX] hr_holidays: Uniformize allocation validation for responsibles
Purpose
=======

Have the same mechanism for time off responsibles between leaves and allocations.
[FIX] hr_holidays: write on date_from change request_date_from
When writing on date_from or date_to, request_date_from and
request_date_to should also change
[FIX] hr_holidays: allow copy of leaves and allocations
In the case of allocations, it should simply be possible to copy them

In the case of leave requests, we want to have a "beautiful" message for
the user in case he cannot duplicate the leave request (overlapping
leave requests)
But it makes no sense to not allow at copying it in case the date_from
and date_to are actually provided

@tivisse tivisse force-pushed the odoo-dev:saas-12.5-fixes-yti branch from 9a0f650 to bb505de Sep 11, 2019

@robodoo robodoo removed the error 🙅 label Sep 11, 2019

[FIX] models.py: Don't commit after fetching a translation
Purpose
=======

When trying the translate a sql_contraint message after an integrity error
we use the current cursor in a 'with', just to make a SELECT query.

That way, the cursor is commited at the end of the with statement, trying
to commit the inconsitent state of the cursor that was the reason of the
integrity error.

Just close the cursor, as we only fetch translations from the database.

@tivisse tivisse force-pushed the odoo-dev:saas-12.5-fixes-yti branch from bb505de to a904fe1 Sep 11, 2019

@tivisse

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

@robodoo r+ rebase-ff

@robodoo robodoo added the r+ 👌 label Sep 11, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Merge method set to rebase and fast-forward

@robodoo robodoo added the CI 🤖 label Sep 11, 2019

@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Linked pull request(s) odoo/enterprise#5521 not ready. Linked PRs are not staged until all of them are ready.

robodoo pushed a commit that referenced this pull request Sep 11, 2019
[FIX] models.py: Don't commit after fetching a translation
Purpose
=======

When trying the translate a sql_contraint message after an integrity error
we use the current cursor in a 'with', just to make a SELECT query.

That way, the cursor is commited at the end of the with statement, trying
to commit the inconsitent state of the cursor that was the reason of the
integrity error.

Just close the cursor, as we only fetch translations from the database.

closes #36458

Signed-off-by: Yannick Tivisse (yti) <yti@odoo.com>
@robodoo

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2019

Merged at 1a0a437, thanks!

@robodoo robodoo closed this Sep 11, 2019

@tivisse tivisse deleted the odoo-dev:saas-12.5-fixes-yti branch Sep 11, 2019

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