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

[16.0][ADD] crm_exception #527

Open
wants to merge 1 commit into
base: 16.0
Choose a base branch
from

Conversation

AungKoKoLin1997
Copy link

@AungKoKoLin1997 AungKoKoLin1997 commented Nov 13, 2023

This module allows you to attach several customizable exceptions to your opportunities.
@qrtl

Copy link

@rinaldifirdaus rinaldifirdaus left a comment

Choose a reason for hiding this comment

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

Functional Test: Works as expected.
image

Copy link
Member

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.

Comment on lines 33 to 36
@api.constrains("ignore_exception", "stage_id")
def _check_quantity_positive(self):
for record in self:
record._check_exception()
Copy link
Member

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"
Copy link
Member

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?

Copy link
Author

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

Copy link
Member

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.

@yostashiro
Copy link
Member

@AungKoKoLin1997 Please add tests as well.

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-crm_exception branch 5 times, most recently from 92da9fb to 359c128 Compare November 29, 2023 05:18
Comment on lines 26 to 39
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
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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)

Copy link
Author

@AungKoKoLin1997 AungKoKoLin1997 Nov 30, 2023

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">
Copy link
Member

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?

Comment on lines 32 to 37
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)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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
Copy link
Member

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.

@yostashiro
Copy link
Member

@AungKoKoLin1997 Please add an explanation on the limitation on extending the create method.

@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-crm_exception branch 2 times, most recently from 3c52cd4 to a9da33c Compare November 30, 2023 09:49
Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

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

Code review.

@OCA-git-bot
Copy link
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). 🤖

@AungKoKoLin1997
Copy link
Author

@oca-crm-maintainers
Is it possible to take a look this PR?

Copy link

github-actions bot commented May 5, 2024

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.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label May 5, 2024
@github-actions github-actions bot removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label May 12, 2024
@AungKoKoLin1997 AungKoKoLin1997 force-pushed the 16.0-add-crm_exception branch 3 times, most recently from 99b72e4 to 684b856 Compare June 27, 2024 04:12
@AungKoKoLin1997
Copy link
Author

Tests fails are not related with this PR.

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