Skip to content

[10.0]base_exception Remove empty self feature. - #1620

Closed
hparfr wants to merge 1 commit into
OCA:10.0from
akretion:10.0-base_exception-remove_empty_self
Closed

hparfr wants to merge 1 commit into
OCA:10.0from
akretion:10.0-base_exception-remove_empty_self

Conversation

@hparfr

@hparfr hparfr commented Jul 10, 2019

Copy link
Copy Markdown
Contributor

This recently added feature is counter intuitive, error prone and is
already causing bugs in sale_workflow.

Like this:
https://github.com/OCA/sale-workflow/blob/10.0/sale_exception/models/sale_order_line.py#L28

(sale_order_line)
    @api.multi
    def _detect_exceptions(self, rule):
        records = super(SaleOrderLine, self)._detect_exceptions(rule)
    return records.mapped('order_id')

I think it's the correct and obvious way to write this code. But the actual implementation, will fail if the sale order has no lines. It will test exceptions on all the sale order line (empty self = search([])) and return exceptions tied to other sale orders.

Please merge it with a major version number bump.

This recently added feature is counter intuitive, error prone and is
already causing bugs in sale_workflow.
@hparfr

hparfr commented Jul 10, 2019

Copy link
Copy Markdown
Contributor Author

@florian-dacosta florian-dacosta left a comment

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.

LGTM

@yvaucher yvaucher 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.

I agree you want to check the exception on a selected set of object. When you start to have thousands of objects this feature is meaningless and resource greedy.

Just got some MemoryError due to this lastly being run while preparing data with onchanges with addition of a api.constraints which was making the check on all the 3.1 M sale order lines in the system. That was not easy to spot. Thanks for the fix!

@gurneyalex

Copy link
Copy Markdown
Member

/ocabot merge major

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Rebased to 10.0-ocabot-merge-pr-1620-by-gurneyalex-bump-major, awaiting test results.

@OCA-git-bot

Copy link
Copy Markdown
Contributor

Congratulations, your PR was merged at 02d1a55. Thanks a lot for contributing to OCA. ❤️

PS: Don't worry if GitHub says there are unmerged commits: it is due to a rebase before merge. All commits of this PR have been merged into 10.0.

@OCA-git-bot OCA-git-bot closed this Aug 7, 2019
OCA-git-bot added a commit that referenced this pull request Aug 7, 2019
Signed-off-by gurneyalex
SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024
Syncing from upstream OCA/server-tools (16.0)
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.

6 participants