Skip to content

[19.0][FIX] endpoint_route_handler: precompute=True on route field - #136

Open
AungKoKoLin1997 wants to merge 1 commit into
OCA:19.0from
qrtl:19.0-fix-endpoint_route_handler
Open

[19.0][FIX] endpoint_route_handler: precompute=True on route field#136
AungKoKoLin1997 wants to merge 1 commit into
OCA:19.0from
qrtl:19.0-fix-endpoint_route_handler

Conversation

@AungKoKoLin1997

Copy link
Copy Markdown

Related: #135

Without precompute, the required computed field is evaluated after INSERT (during flush), so creating an endpoint whose route is derived from other fields by a downstream module fails with "Missing required value for the field 'Route'" — the INSERT runs before the compute can populate the column.

@qrtl QT6769

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hi @simahawk,
some modules you are maintaining are being modified, check this out!

@OCA-git-bot OCA-git-bot added series:19.0 mod:endpoint_route_handler Module endpoint_route_handler labels May 26, 2026
@AungKoKoLin1997
AungKoKoLin1997 force-pushed the 19.0-fix-endpoint_route_handler branch from b81ed4d to 1d55f71 Compare May 26, 2026 04:53

@yostashiro yostashiro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review. LGTM.

@smorita7749 smorita7749 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional review: LGTM
The error is fixed.

@OCA-git-bot

Copy link
Copy Markdown
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

new_route.route += "/new"
self.assertNotEqual(new_route.endpoint_hash, first_hash)

def test_route_field_precomputed(self):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need this test? It's kind of testing an odoo core feature 🤔

@AungKoKoLin1997 AungKoKoLin1997 Jul 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My purpose is to be sure the fix is working well.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simahawk
To clarify further — the test is not meant to verify Odoo's precompute mechanism itself. It's a regression guard to ensure precompute=True stays on the route field. If someone removes it during a future migration or refactor, this test will catch the breakage. Without it, the only way to notice would be a failure in a downstream module at runtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, you could rewrite the docstring of the test by removing repeated information (sounds quite verbose ATM for little value) and add a synthetic version of the purpose that you've just explained.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@simahawk I updated the docstring.

Without precompute, the required computed field is evaluated after
INSERT (during flush), so creating an endpoint whose route is derived
from other fields by a downstream module fails with "Missing required
value for the field 'Route'" — the INSERT runs before the compute can
populate the column.
@AungKoKoLin1997
AungKoKoLin1997 force-pushed the 19.0-fix-endpoint_route_handler branch from 1d55f71 to fb83e40 Compare July 22, 2026 10:04
@AungKoKoLin1997
AungKoKoLin1997 requested a review from simahawk July 22, 2026 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants