-
-
Notifications
You must be signed in to change notification settings - Fork 406
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
[16.0][ADD] crm_exception #527
base: 16.0
Are you sure you want to change the base?
Conversation
5576ca2
to
76344ab
Compare
cdc6f1a
to
9f86c0e
Compare
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.
9f86c0e
to
5e766ce
Compare
crm_exception/readme/USAGE.rst
Outdated
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 CONFIGURE.md instead.
crm_exception/models/crm_lead.py
Outdated
@api.constrains("ignore_exception", "stage_id") | ||
def _check_quantity_positive(self): | ||
for record in self: | ||
record._check_exception() |
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 should actually extend the write() method to ensure that the check is not restricted to certain fields. For example, with the current logic, even if a field is made required for a specific stage by an exception rule, the field value can be removed once the opportunity is saved with that stage.
name="stage_ids" | ||
widget="many2many_tags" | ||
attrs="{'invisible': [('model', '!=', 'crm.lead')]}" | ||
groups="base.group_system" |
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.
Why assign this group here?
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.
I want to use model
field in attrs
and this field can be accessed by base.group_system
.
https://github.com/OCA/server-tools/blob/97be5cf17cc32f4ef3e21fa493868426a5054b2d/base_exception/views/base_exception_view.xml#L44-L45
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.
@AungKoKoLin1997 We shouldn't need this, then, since the parent node already has the group.
@AungKoKoLin1997 Please add tests as well. |
92da9fb
to
359c128
Compare
crm_exception/models/crm_lead.py
Outdated
base_rule_domain = super()._rule_domain() | ||
if self.stage_id: | ||
rule_domain = expression.AND( | ||
[ | ||
base_rule_domain, | ||
[ | ||
"|", | ||
("stage_ids", "in", tuple(self.stage_id.ids)), | ||
("stage_ids", "=", False), | ||
], | ||
] | ||
) | ||
return rule_domain | ||
return base_rule_domain |
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.
base_rule_domain = super()._rule_domain() | |
if self.stage_id: | |
rule_domain = expression.AND( | |
[ | |
base_rule_domain, | |
[ | |
"|", | |
("stage_ids", "in", tuple(self.stage_id.ids)), | |
("stage_ids", "=", False), | |
], | |
] | |
) | |
return rule_domain | |
return base_rule_domain | |
rule_domain = super()._rule_domain() | |
if self.stage_id: | |
rule_domain = expression.AND( | |
[ | |
rule_domain, | |
[ | |
"|", | |
("stage_ids", "in", self.stage_id.ids), | |
("stage_ids", "=", False), | |
], | |
] | |
) | |
return rule_domain |
Any reason to use tuple here? tuple(self.stage_id.ids)
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.
If we don't convert to tuple in here, cache lookeup error(TypeError: unhashable type: 'list'
) will occur from this.
https://github.com/OCA/server-tools/blob/590cf43c30698fc8dd2fca306fe5bb6444ecb903/base_exception/models/exception_rule.py#L81
@@ -0,0 +1,12 @@ | |||
<?xml version="1.0" encoding="utf-8" ?> | |||
<odoo noupdate="1"> | |||
<record id="crm_excep_no_partner" model="exception.rule"> |
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.
Shouldn't this be created as demo data?
self.opportunity.stage_id = self.stage_qualified.id | ||
self.assertEqual(self.opportunity.stage_id, self.stage_qualified) | ||
self.opportunity.stage_id = self.stage_proposition.id | ||
self.assertEqual(self.opportunity.stage_id, self.stage_proposition) | ||
self.opportunity.stage_id = self.stage_won.id | ||
self.assertEqual(self.opportunity.stage_id, self.stage_won) |
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.
self.opportunity.stage_id = self.stage_qualified.id | |
self.assertEqual(self.opportunity.stage_id, self.stage_qualified) | |
self.opportunity.stage_id = self.stage_proposition.id | |
self.assertEqual(self.opportunity.stage_id, self.stage_proposition) | |
self.opportunity.stage_id = self.stage_won.id | |
self.assertEqual(self.opportunity.stage_id, self.stage_won) | |
self.opportunity.stage_id = self.stage_qualified.id | |
self.opportunity.stage_id = self.stage_proposition.id | |
self.opportunity.stage_id = self.stage_won.id |
self.opportunity.stage_id = self.stage_won.id | ||
self.assertEqual(self.opportunity.stage_id, self.stage_won) | ||
|
||
# Check exception only for qualified and won stages |
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.
Split the following into a separate test.
@AungKoKoLin1997 Please add an explanation on the limitation on extending the create method. |
3c52cd4
to
a9da33c
Compare
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.
Code review.
This PR has the |
@oca-crm-maintainers |
There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. |
a9da33c
to
53e3486
Compare
53e3486
to
7ce4276
Compare
99b72e4
to
684b856
Compare
Tests fails are not related with this PR. |
684b856
to
7870f71
Compare
7870f71
to
b9ff0a4
Compare
b9ff0a4
to
c947070
Compare
This module allows you to attach several customizable exceptions to your opportunities.
@qrtl