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

[17.0][MIG] crm_partner_assign: Migration to 17.0 #606

Open
wants to merge 6 commits into
base: 17.0
Choose a base branch
from

Conversation

MohamedOsman7
Copy link

Same changes from #538 with cleaned commit history.

@MohamedOsman7 MohamedOsman7 mentioned this pull request Nov 12, 2024
20 tasks
@rousseldenis
Copy link

/ocabot migration crm_partner_assign

Copy link

@CRogos CRogos left a comment

Choose a reason for hiding this comment

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

LGTM

@MohamedOsman7
Copy link
Author

MohamedOsman7 commented Nov 19, 2024

@peluko00 @ppyczko could you please make a review

help="Partner this case has been assigned to.",
related="partner_contact_assigned_id.commercial_partner_id",
store=True,
readonly=True,

Choose a reason for hiding this comment

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

Please put the readonly in view.

date_partner_assign = fields.Date(
compute="_compute_date_partner_assign",
string="Partner Assignment Date",
readonly=False,

Choose a reason for hiding this comment

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

Remove here

@MohamedOsman7 MohamedOsman7 force-pushed the 17.0-mig-crm_partner_assign branch from 264c642 to 439aef5 Compare November 19, 2024 13:47
@MohamedOsman7
Copy link
Author

@peluko00 thanks for your review, i made the requested changes

Copy link

@peluko00 peluko00 left a comment

Choose a reason for hiding this comment

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

LGTM, code review and tested in runboat

Copy link

@ppyczko ppyczko left a comment

Choose a reason for hiding this comment

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

LGTM!

@MohamedOsman7
Copy link
Author

@rousseldenis could you please merge

@CRogos
Copy link

CRogos commented Dec 19, 2024

@pedrobaeza could you merge?

Comment on lines 6 to 8
"summary": """
Assign a Partner to an Opportunity/Lead/Partner to indicate Partnership,
""",
Copy link
Member

Choose a reason for hiding this comment

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

Incorrect change. If it doesn't fit, you should split the string, but don't introduce linebreaks with """.

Suggested change
"summary": """
Assign a Partner to an Opportunity/Lead/Partner to indicate Partnership,
""",
"summary": "Assign a Partner to an Opportunity/Lead/Partner"
" to indicate Partnership",
""",

Copy link

Choose a reason for hiding this comment

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

done

@CRogos CRogos force-pushed the 17.0-mig-crm_partner_assign branch from 439aef5 to 202c573 Compare December 19, 2024 21:15
class ResPartner(models.Model):
_inherit = "res.partner"

assigned_partner_id = fields.Many2one(

Choose a reason for hiding this comment

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

Reading the fields names here does not reflect for which purpose they have been set.

Maybe making crm appear or... could be great.

Copy link

Choose a reason for hiding this comment

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

Because I was not sure what you meant with your comments I looked up this variable and found another module which has a very similar to this implementation in the standard Odoo.
https://github.com/odoo/odoo/tree/17.0/addons/website_crm_partner_assign

Is this intended that a subset of this module is this module?

@api.depends("implemented_partner_ids", "implemented_partner_ids.active")
def _compute_implemented_partner_count(self):
for partner in self:
partner.implemented_count = len(partner.implemented_partner_ids)

Choose a reason for hiding this comment

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

Shouldn't this use read_group instead for performances ?

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.

9 participants