Skip to content

[12.0]base_exception Remove empty self feature. #1620 - #1625

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

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

Conversation

@hparfr

@hparfr hparfr commented Jul 25, 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.

same as #1620

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

hparfr commented Jul 25, 2019

Copy link
Copy Markdown
Contributor Author

@florian-dacosta

@hparfr hparfr changed the title Remove the feature when self is empty. [12.0]base_exception Remove empty self feature. #1620 Jul 25, 2019

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

thanks

@hparfr

hparfr commented Aug 7, 2019

Copy link
Copy Markdown
Contributor Author

@yvaucher @guewen you can approve this one too

@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). 🤖

@gurneyalex

Copy link
Copy Markdown
Member

/ocabot merge major

@OCA-git-bot

Copy link
Copy Markdown
Contributor

This PR looks fantastic, let's merge it!
Rebased to 12.0-ocabot-merge-pr-1625-by-gurneyalex-bump-major, awaiting test results.

@yvaucher

Copy link
Copy Markdown
Member

damn the merge failed here a while ago.

Will handle the rebase manually.

@yvaucher

Copy link
Copy Markdown
Member

Seems it has already been merged there: 60d3d62

@yvaucher yvaucher closed this Dec 16, 2019
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.

5 participants