Skip to content

Conversation

@haall-odoo
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Nov 18, 2025

Pull request status dashboard

Copy link
Contributor

@clbr-odoo clbr-odoo left a comment

Choose a reason for hiding this comment

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

Very good job, continue like this !

The module name in the commit title should be lower case, it's the technical name of the module, which is equivalent to the folder name.

Copy link
Contributor

@clbr-odoo clbr-odoo left a comment

Choose a reason for hiding this comment

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

Good job !

_name = "estate.property"
_description = "Estate Property"
_order = "id desc"
_check_expected_price = models.Constraint(
Copy link
Contributor

Choose a reason for hiding this comment

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

Constraints should be just below field definition. Check the coding guidelines for the order of the methods and fields because it's not the only one misplaced :)

@api.depends("offer_ids")
def _compute_best_offer(self):
for record in self:
record.best_price = max(record.offer_ids.mapped("price")) if len(record.offer_ids) > 0 else 0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
record.best_price = max(record.offer_ids.mapped("price")) if len(record.offer_ids) > 0 else 0.0
record.best_price = max(record.offer_ids.mapped("price"), default=0.0)
  1. You never need to check the length if you want to check if a recordset is filled, just do if record.offer_ids:
  2. You can simplify a bit like this ☝️

record.garden_orientation = 'north'

def estate_property_action_sold(self):
self.__estate_property_action_sold_cancel('sold', "A cancelled property cannot be sold!", "This property is already sold!")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.__estate_property_action_sold_cancel('sold', "A cancelled property cannot be sold!", "This property is already sold!")
self._estate_property_action_sold_cancel('sold', "A cancelled property cannot be sold!", "This property is already sold!")

@api.onchange("offer_ids")
def _onchange_offer_ids(self):
for record in self:
if len(record.offer_ids) > 0:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(record.offer_ids) > 0:
if record.offer_ids:

Comment on lines +40 to +41
if record.status == 'accepted':
return False
Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird to have a method that returns False in the middle of the loop.
This will lead to inconsistencies.

Comment on lines +57 to +58
for record in self:
record.property_id.selling_price = 0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

You can actually "batch write" it self.property_id.selling_price = 0.0

<field name="arch" type="xml">
<form string="My new house">
<header>
<button name="estate_property_action_sold" type="object" string="Sold" invisible="state == 'sold' or state == 'cancelled'"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<button name="estate_property_action_sold" type="object" string="Sold" invisible="state == 'sold' or state == 'cancelled'"/>
<button name="estate_property_action_sold" type="object" string="Sold" invisible="state in ('sold', 'cancelled')"/>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants