-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[17.0][ADD] sale_product_approval #3108
base: 17.0
Are you sure you want to change the base?
Conversation
0e8419f
to
218cfe1
Compare
Depends on PR OCA#3108
Depends on PR OCA#3108
/ocabot rebase |
Sorry @patrickrwilson you are not allowed to rebase. To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons. If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the |
218cfe1
to
530e79e
Compare
exceptions_sale_approval_confirm = fields.Boolean( | ||
compute="_compute_exceptions", string="Exception", default=False | ||
) | ||
override_exception = fields.Boolean(default=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, this name is a little bit too broad. Should reflect the 'sale product approval' flow to avoid names collisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need of default=False
as default
_inherit = "sale.order" | ||
|
||
exceptions_sale_approval_confirm = fields.Boolean( | ||
compute="_compute_exceptions", string="Exception", default=False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compute="_compute_exceptions", string="Exception", default=False | |
compute="_compute_exceptions_sale_approval_confirm", string="Exception", default=False |
user_id=order.user_id.id or SUPERUSER_ID, | ||
) | ||
|
||
def _render_product_state_excep(self, order, product_id): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer explicit naming than abbreviations
class SaleOrderLine(models.Model): | ||
_inherit = "sale.order.line" | ||
|
||
approved_sale_confirm = fields.Boolean( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really needed as stored value ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, see. For the search.
note = self._render_product_state_excep(order, product_id) | ||
order.activity_schedule( | ||
"mail.mail_activity_data_warning", | ||
date.today(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this timezone safe ?
candidate_sale_confirm = fields.Boolean( | ||
string="Candidate to be Sold", | ||
) | ||
can_edit_candidate = fields.Boolean(compute="_compute_can_edit_candidate") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name should maybe reflect 'product approval' flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We went over the naming of these fields internally quite a bit and ended up settling on 'candidate' because the approval part is in the states and having the 'approved to be' on the products confused the users because they struggled to make the connection between the setting on the product depends on the state. So we settled on 'Candidate' to say basically its configured to be 'Sold', 'Purchased', etc as long as the state it's in allows it.
|
||
def _compute_can_edit_candidate(self): | ||
for product in self: | ||
product.can_edit_candidate = self.env.user.has_group( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do without loop (as record value depends on self value):
product.can_edit_candidate = self.env.user.has_group( | |
self.update({"can_edit_candidate": self.env.user.(...)}) |
) | ||
|
||
@api.onchange("candidate_sale_confirm") | ||
def _onchange_candidate_sale_confirm(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the field candidate_sale
as compute/readonly=False/stored field to avoid onchanges.
|
||
|
||
class TestSaleOrderLineDates(TransactionCase): | ||
def setUp(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use classmethod instead
db28a4b
to
6526638
Compare
This PR adds the sale_product_approval module to v17 - Note that this module was introduced in earlier versions but PR's were not merged.
6526638
to
6b054cf
Compare
@rousseldenis I think all your comments have been addressed and ready for a re-review, thanks for your effort. |
Depends on PR OCA#3108
This PR adds the sale_product_approval module to v17 - Note that this module was introduced in earlier versions but PR's were not merged.