Skip to content

Conversation

@djhan-odoo
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Nov 18, 2025

Pull request status dashboard

Copy link

@yoba-odoo yoba-odoo left a comment

Choose a reason for hiding this comment

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

Some nitpicks 😇
Keep up the good work 💪

Comment on lines 22 to 23
'assets': {
},

Choose a reason for hiding this comment

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

We don't have to declare the assets key if it is empty so you can remove it until you actually need it

class EstateProperty(models.Model):
_name = "estate.property"
_description = "Estate properties"
name = fields.Char('Property Name', required=True, translate=True)

Choose a reason for hiding this comment

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

Always have an empty line between the fields declaration and the class properties

<menuitem id="estate_property_menu_action" action="estate_property_action" />
</menuitem>
</menuitem>
</odoo> No newline at end of file

Choose a reason for hiding this comment

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

You should have an empty line by the end of every file

</p>
</field>
</record>
</odoo> No newline at end of file

Choose a reason for hiding this comment

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

Same, empty line by the end of the file

Copy link

@yoba-odoo yoba-odoo left a comment

Choose a reason for hiding this comment

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

Nice work!!
Some comments to be taken into consideration!

garden_area = fields.Float('Garden Area (sqm)')
garden_orientation = fields.Selection(
string='Garden Orientation',
selection=[('north', 'North'), ('south', 'South'), ('east', 'East'), ('west', 'West')])

Choose a reason for hiding this comment

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

Suggested change
selection=[('north', 'North'), ('south', 'South'), ('east', 'East'), ('west', 'West')])
selection=[
('north', 'North'),
('south', 'South'),
('east', 'East'),
('west',` 'West')
])

Would be better for readability to have them in multiple lines

required=True,
default='new',
copy=False,
selection=[('new', 'New'), ('offer_received', 'Offer Received'), ('offer_accepted', 'Offer Accepted'), ('sold', 'Sold'), ('cancelled', 'Cancelled')])

Choose a reason for hiding this comment

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

Suggested change
selection=[('new', 'New'), ('offer_received', 'Offer Received'), ('offer_accepted', 'Offer Accepted'), ('sold', 'Sold'), ('cancelled', 'Cancelled')])
selection=[
('new', 'New'),
('offer_received', 'Offer Received'),
('offer_accepted', 'Offer Accepted'),
('sold', 'Sold'),
('cancelled', 'Cancelled')
])

@api.depends('offer_ids.price')
def _compute_best_price(self):
for record in self:
record.best_price = max(record.offer_ids, key=lambda p: p.price).price

Choose a reason for hiding this comment

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

if the offer_ids recordset is empty it will trigger a ValueError so it is better to either check that offer_ids has something before we call max or we fallback on a default value

Suggested change
record.best_price = max(record.offer_ids, key=lambda p: p.price).price
record.best_price = max(record.offer_ids.mapped('price'), default=0.0)

raise exceptions.UserError('Cancelled properties cannot be sold')
else:
record.state = 'sold'
return True No newline at end of file

Choose a reason for hiding this comment

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

missing EOL


class EstatePropertyOffer(models.Model):
_name = "estate.property.offer"
_description = "Estate properties Offers"

Choose a reason for hiding this comment

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

Better to have the description here as title case

Suggested change
_description = "Estate properties Offers"
_description = "Estate Properties Offers"

<field name="name">estate.property.offer.list</field>
<field name="model">estate.property.offer</field>
<field name="arch" type="xml">
<list string="Channel">

Choose a reason for hiding this comment

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

Suggested change
<list string="Channel">
<list string="Offers">

Would be reflecting better what is shown 😄

<field name="facades" />
<separator />
<filter string="Available properties" name="available_properties"
domain="['|', ('state', '=', 'new'), ('state', '=', 'offer_received')]" />

Choose a reason for hiding this comment

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

Suggested change
domain="['|', ('state', '=', 'new'), ('state', '=', 'offer_received')]" />
domain="[('state', 'in', ('new', 'offer_received'))]"/>

More precise

<field name="name">estate.property.list</field>
<field name="model">estate.property</field>
<field name="arch" type="xml">
<list string="Channel">

Choose a reason for hiding this comment

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

Suggested change
<list string="Channel">
<list string="Properties">

</p>
</field>
</record>
</odoo> No newline at end of file

Choose a reason for hiding this comment

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

Missing EOL

</list>
</field>
</record>
</odoo> No newline at end of file

Choose a reason for hiding this comment

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

Missing EOL

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