Skip to content

18.0 real estate module shzi #820

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

Open
wants to merge 13 commits into
base: 18.0
Choose a base branch
from

Conversation

shzi-odoo
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Jun 18, 2025

Pull request status dashboard

Copy link

@aboo-odoo aboo-odoo left a comment

Choose a reason for hiding this comment

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

Hey! Good job for this PR, I've been extra picky but again 🎉

A few generic comments:

  • A lot of your IDE stuff is in the diff, you need to remove that
  • Could you add a description to the PR and change the PR title according to the guideline [XYZ] module: short desc. like for a commit message link
  • Usually, you'll want one commit / task to keep the chain of commits as clean and small as possible. As here, the task is to create the module Estate, it should be the one commit on your branch. Could you squash your commits into 1 ?

@@ -0,0 +1 @@
from . import models

Choose a reason for hiding this comment

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

Always add an EOL character at the end of your files to minimize the diffs of the next dev updating the file 👍

@@ -0,0 +1,15 @@
{
'name': 'Estate',

Choose a reason for hiding this comment

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

In python, for quoting strings we try to follow the following convention:

single quote ' around technical terms
double quotes " around display terms

=> ('north', "North")

Let's try to follow this during this tutorial, even though you'll find many counter examples in the existing code.

@@ -0,0 +1,110 @@
from odoo import fields, models, api

Choose a reason for hiding this comment

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

The imports should be

  1. External libraries (one per line sorted and split in python stdlib)
  2. Imports of odoo
  3. Imports from Odoo modules (rarely, and only if necessary)

Inside these 3 groups, the imported lines are alphabetically sorted. You can use RUFF to help you sort them, it's quite handy

_name = "estate.property"
_description = "Store Real Estate Properties"

name = fields.Char("Estate 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.

What do you think the translate=True here ?

name = fields.Char("Estate Name", required=True, translate=True)
description = fields.Text("Description", help="Enter the real estate item description")
postcode = fields.Char("Postcode")
date_availability = fields.Date("Available From", copy=False, default=lambda self: date.today() + timedelta(days=90))

Choose a reason for hiding this comment

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

timedelta(days=90)) is not exactly 3 months ahah

date_deadline = fields.Datetime(compute="_compute_date_deadline", inverse="_inverse_date_deadline")

# Computed functions
@api.depends('create_date', 'validity')

Choose a reason for hiding this comment

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

Suggested change
@api.depends('create_date', 'validity')
@api.depends('validity')

create_date will never change, only place variable that change to trigger the re-compute of the method

Comment on lines 45 to 46
if offer.property_id.buyer_id:
raise UserError("An offer has already been accepted for this property.")

Choose a reason for hiding this comment

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

I would rather

Suggested change
if offer.property_id.buyer_id:
raise UserError("An offer has already been accepted for this property.")
if offer.property_id.state == 'offer_accepted':
raise UserError("An offer has already been accepted for this property.")

Because the fact there is a buyer id does not mean an offer has been accepted (conceptually speaking)

Choose a reason for hiding this comment

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

Again, file order is important

</list>
</field>
</record>
</odoo>

Choose a reason for hiding this comment

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

Not writing it on every file, but all are missing the EOL character

<field name="partner_id" />
<field name="status" />
<button name="action_accept_offer" string="Confirm" type="object" icon="fa-check"/>
<button name="action_refuse_offer" string="Cancel" type="object" icon="fa-times"/>

Choose a reason for hiding this comment

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

Your buttons are always shown, wouldn't it be better to hide them once they have been accepted or refused ?

Copy link

@aboo-odoo aboo-odoo left a comment

Choose a reason for hiding this comment

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

Hey! Final review on the python part, it focuses on functionality, not style anymore 😄 All and all, an extremely good PR, well done 👍

On Monday, I'll teach you how to remove the .idea stuff, squash your commits and rebase your branch to have it all nice and clean.

By then, try to get your runbot green 🟢 and fix the comments from last time and this review please 👍

Comment on lines +1 to +2
from . import (estate_property, estate_property_offer, estate_property_tag,
estate_property_type, inherited_res_users)

Choose a reason for hiding this comment

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

Use several lines, it's easier to manage in the future

Comment on lines +43 to +44
@api.model
def create(self, vals):

Choose a reason for hiding this comment

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

Very nice and robust method override but it'd be even better if you could do it in batch to increase performances.

Suggested change
@api.model
def create(self, vals):
@api.model_create_multi
def create(self, vals_list):
for vals in vals_list:
# Do things
return super().create(vals_list)

Comment on lines +100 to +101
other_offers = offer.property_id.offer_ids - offer
other_offers.write({"status": "refused"})

Choose a reason for hiding this comment

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

Very nice use of recordset!!!

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