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

[11.0][IMP] sale_order_type: Find sale order type based on the products of the sale order #770

Merged
merged 3 commits into from
Feb 1, 2019

Conversation

SimoRubi
Copy link
Member

Forward port of #716

@SimoRubi
Copy link
Member Author

SimoRubi commented Jan 2, 2019

@OCA/crm-sales-marketing-maintainers please check, I don't know if this is the correct way to forward port a change, any suggestion for improvement is appreciated

@rafaelbn rafaelbn added this to the 11.0 milestone Jan 16, 2019
Copy link
Member

@rafaelbn rafaelbn left a comment

Choose a reason for hiding this comment

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

Functionally tested 👍

Well I have some doubts but It works

Rules can also be associated with sale order types.

Inside each rule, you can select any number of products and/or product categories.

Copy link
Member

Choose a reason for hiding this comment

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

Case 1

  • 1 sale order with 2 product
  • Each of that 2 products are in 2 different rules of 2 different sale_order_type

Product 1 -> Sale order type 1
Product 2 -> Sale order type 2

Sale Order TEST has product 1 + product 2

Which is the result?

Copy link
Member Author

@SimoRubi SimoRubi Jan 17, 2019

Choose a reason for hiding this comment

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

Hi, thanks for your review!
As stated few lines below, the order types are inspected following the order of their sequence field, so:
0. Suppose Type 1 has sequence 1 and Type 2 has sequence 2;

  1. Type 1 is inspected: does this match the current order?
    1. Does any rule of type 1 match any product in the order?
      *. Yes -> choose type 1 and exit
      *. No -> go on
    2. Does any rule of type 1 match any product category in the order?
      *. (same as above)
  2. Repeat step 1 for the following order types

At least this is the behavior I tried to describe in the README, if that is not clear enough I can add an example.

@SimoRubi
Copy link
Member Author

@OCA/crm-sales-marketing-maintainers can this be merged? Consider that this functionality is just forward ported from #716, thanks

@@ -36,6 +36,23 @@ def onchange_type_id(self):
if order.type_id.incoterm_id:
order.incoterm = order.type_id.incoterm_id.id

@api.multi
@api.onchange('order_line')
def _onchange_order_line(self):
Copy link
Member

Choose a reason for hiding this comment

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

This onchange shouldn't be triggered automatically.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, thanks for the review!
It is triggered automatically but it only does something if the type_id of the order is empty.
Are you suggesting it should depend on something else or should I remove it completely?

Copy link
Member

Choose a reason for hiding this comment

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

It should be removed IMO due to performance reasons.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok then I'll remove it, double-thinking about it, it might be confusing when:

  1. add first line -> type is set
  2. add second line, type should change but it doesn't

In the end, everyone would be clicking on the recompute button to be sure the type is correct..

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 65a7dc2

@pedrobaeza pedrobaeza merged commit aadd9f7 into OCA:11.0 Feb 1, 2019
@pedrobaeza
Copy link
Member

@SimoRubi please fix this as you have broken several CIs due to hard-coded dependency in test of l10n_generic_coa. Please create accounting stuff in the test itself.

@SimoRubi
Copy link
Member Author

SimoRubi commented Feb 8, 2019

Hi, I see that Travis of the merged commit is ✔️, which CIs are broken?

@pedrobaeza
Copy link
Member

Those that don't install l10n_generic_coa, which belongs to our customer ones with l10n_es. Here is "magically" working due to the auto-install of the module, but that doesn't happen always.

chienandalu pushed a commit to Tecnativa/sale-workflow that referenced this pull request Dec 21, 2020
chienandalu pushed a commit to Tecnativa/sale-workflow that referenced this pull request Dec 21, 2020
chienandalu pushed a commit to Tecnativa/sale-workflow that referenced this pull request Dec 21, 2020
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.

4 participants