Skip to content

[ADD] estate: Added a new module named estate #733

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 20 commits into
base: 18.0
Choose a base branch
from

Conversation

msho-odoo
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Apr 22, 2025

Pull request status dashboard

@msho-odoo msho-odoo force-pushed the 18.0-add-module-estate-msho branch from a35abec to 329a381 Compare April 22, 2025 20:22
'installable': True,
'application': True,
'auto_install': False
}

Choose a reason for hiding this comment

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

Don't forget the license key to avoid warnings in the log.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@SergeBayet
Copy link

SergeBayet commented Apr 23, 2025

@msho-odoo For the title of the commit, be aware that the commit message header should make a valid sentence once concatenated with if applied, this commit will header. For example [IMP] base: prevent to archive users linked to active partners is correct as it makes a valid sentence if applied, this commit will prevent users to archive....
Don't use uppercase for the title.

[ADD] estate: add a new estate property module for example.

src: https://www.odoo.com/documentation/18.0/contributing/development/git_guidelines.html#commit-message-header

Comment on lines 5 to 9
'data': [
'security/ir.model.access.csv',
'views/estate_property_views.xml',
'views/estate_menu.xml',
],

Choose a reason for hiding this comment

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

Suggested change
'data': [
'security/ir.model.access.csv',
'views/estate_property_views.xml',
'views/estate_menu.xml',
],
'data': [
'security/ir.model.access.csv',
'views/estate_property_views.xml',
'views/estate_menu.xml',
],

@@ -0,0 +1,2 @@
id,name,model_id/id,group_id/id,perm_read,perm_write,perm_create,perm_unlink
access_estate_property, access_estate_property,model_estate_property,base.group_user,1,1,1,1

Choose a reason for hiding this comment

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

EOF 👍

<menuitem id="estate_property_action_menu" action="estate_property_action"></menuitem>
</menuitem>
</menuitem>
</odoo>

Choose a reason for hiding this comment

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

Same here

<field name="res_model">estate.property</field>
<field name="view_mode">list,form</field>
</record>
</odoo>

Choose a reason for hiding this comment

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

Same here

Copy link
Author

@msho-odoo msho-odoo Apr 23, 2025

Choose a reason for hiding this comment

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

@SergeBayet Noted.
I fixed linting and EOF problems

@SergeBayet
Copy link

image

@msho-odoo msho-odoo force-pushed the 18.0-add-module-estate-msho branch from 6f5effe to 450100a Compare April 23, 2025 11:13
@@ -10,7 +10,7 @@ class EstateProperty(models.Model):
description = fields.Text('description')
postcode = fields.Char('postcode')
date_availability = fields.Date(
'availabilty date', copy=False, default=fields.Date.today()+relativedelta(months=3))
'availabilty date', copy=False, default=fields.Date.today() + relativedelta(months=3))

Choose a reason for hiding this comment

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

For this diff you could use git commit --amend to correct the previous commit :)

Copy link
Author

@msho-odoo msho-odoo Apr 23, 2025

Choose a reason for hiding this comment

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

@SergeBayet
yes, I wasn't used to using it in that sense but sure it makes sense.
Thank you for pointing these things out, it helps :)

@@ -37,3 +37,5 @@ class EstateProperty(models.Model):
"res.users", string="Salesperson", default=lambda self: self.env.user)
buyer_id = fields.Many2one("res.partner", string="Buyer", copy=False)
tag_ids = fields.Many2many("estate.property.tag", string="Tags")
offer_ids = fields.One2many(
"estate.property.offer", "property_id", string="Offers")

Choose a reason for hiding this comment

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

Silly little remark, but choose if you want to use double quotes or single quotes and stick to it :) In Python, this kind of convention depends of the team you'll be in.

Copy link

@SergeBayet SergeBayet left a comment

Choose a reason for hiding this comment

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

Some remarks :)


@api.depends("offer_ids")
def _compute_best_offer(self):
try:

Choose a reason for hiding this comment

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

Why this try/except block? What could happen?

Copy link
Author

Choose a reason for hiding this comment

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

It was giving me some error but I fixed it later, removing it

if len(record.offer_ids) > 0:
record.best_offer = max(record.offer_ids.mapped("price"))
else:
record.best_offer = 0.0

Choose a reason for hiding this comment

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

A more pyhtonic way:

for prop in self:
      prop.best_price = max(prop.offer_ids.mapped("price")) if prop.offer_ids else 0.0

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

@api.depends("validity")

Choose a reason for hiding this comment

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

It depends of create_date too, isn't it?

for record in self:
start_date = fields.Date.today()
if record.create_date:
start_date = record.create_date

Choose a reason for hiding this comment

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

Be aware that record.create_date is a DateTime field. You can extract the date with the method date()
So maybe this could be better:

        for offer in self:
            date = offer.create_date.date() if offer.create_date else fields.Date.today()
            offer.date_deadline = date + relativedelta(days=offer.validity)

for record in self:
start_date = fields.Date.today()
if record.create_date:
start_date = record.create_date

Choose a reason for hiding this comment

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

Same than above.


def _inverse_date_deadline(self):
for record in self:
start_date = fields.Date.today()

Choose a reason for hiding this comment

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

start_date is never used...

@msho-odoo msho-odoo force-pushed the 18.0-add-module-estate-msho branch 2 times, most recently from eec66c2 to 658e49e Compare April 24, 2025 11:33
@SergeBayet
Copy link

@vava-odoo You can take over this one. :) Thanks

@vava-odoo
Copy link

@SergeBayet I don't review red runbot PRs 😇
cc @msho-odoo

@msho-odoo msho-odoo force-pushed the 18.0-add-module-estate-msho branch from 658e49e to fc150fb Compare April 24, 2025 13:06
@msho-odoo
Copy link
Author

@vava-odoo
the runbot is green now :)

Copy link

@vava-odoo vava-odoo left a comment

Choose a reason for hiding this comment

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

A few comments passing by. Good luck with next chapters 💪

],
'installable': True,
'application': True,
'auto_install': False,

Choose a reason for hiding this comment

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

Default value are not needed (in community, this would raise a warning in runbot)

Suggested change
'auto_install': False,

@@ -0,0 +1,81 @@
from odoo import api, fields, models
from odoo.exceptions import UserError
from dateutil.relativedelta import relativedelta

Choose a reason for hiding this comment

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

Guideline: external import first


class EstateProperty(models.Model):
_name = "estate.property"
_description = "Estate Property discription"

Choose a reason for hiding this comment

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

discription 👀

Comment on lines 25 to 26
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.

usually we indent a bit differently

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

Comment on lines 36 to 37
property_type_id = fields.Many2one(
"estate.property.type", string="property type")

Choose a reason for hiding this comment

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

We avoid long lines, but don't be extreme. Here a one-liner is fine

Suggested change
property_type_id = fields.Many2one(
"estate.property.type", string="property type")
property_type_id = fields.Many2one("estate.property.type", string="property type")

Comment on lines 55 to 56
record.best_offer = max(record.offer_ids.mapped(
"price")) if record.offer_ids else 0.0

Choose a reason for hiding this comment

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

Could use default kwarg

Suggested change
record.best_offer = max(record.offer_ids.mapped(
"price")) if record.offer_ids else 0.0
record.best_offer = max(record.offer_ids.mapped("price"), default=0.0)


@api.onchange("garden")
def _onchange_garden(self):
if self.garden:

Choose a reason for hiding this comment

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

I'd say has_garden is a clearer name, but that is debatable

Copy link
Author

Choose a reason for hiding this comment

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

agree, just for the sake of the tutorial

Comment on lines 68 to 71
for record in self:
if record.state == "cancelled":
raise UserError("You can't sell a cancelled property")
record.state = "sold"

Choose a reason for hiding this comment

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

Better to raise first and assign in batch. For example:

Suggested change
for record in self:
if record.state == "cancelled":
raise UserError("You can't sell a cancelled property")
record.state = "sold"
if "cancelled" in record.mapped('state'):
raise UserError("You can't sell a cancelled property")
self.state = "sold"

<field name="name">properties.view.type.form</field>
<field name="model">estate.property.type</field>
<field name="arch" type="xml">
<form string="properties type from">

Choose a reason for hiding this comment

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

typo

Suggested change
<form string="properties type from">
<form string="properties type form">

Do you need to define this view? I think a default one is automatically created, and not quite different

Copy link
Author

Choose a reason for hiding this comment

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

yes you're right.
I was following the tutorial in case I would add something later to the view, so I did it

<field name="expected_price"/>
<field name="selling_price"/>
<field name="availability_date"/>
<filter string="Available" name="state" 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.

Simpler:

Suggested change
<filter string="Available" name="state" domain="['|',('state','=','new'),('state','=','offer_received')]"/>
<filter string="Available" name="state" domain="[('state', 'in', ['new', 'offer_received'])]"/>

@msho-odoo msho-odoo force-pushed the 18.0-add-module-estate-msho branch from c011c65 to 8ebd0af Compare April 24, 2025 17:03
@msho-odoo msho-odoo force-pushed the 18.0-add-module-estate-msho branch from 8ebd0af to a3d2623 Compare April 28, 2025 08:55
@msho-odoo msho-odoo force-pushed the 18.0-add-module-estate-msho branch from a3d2623 to 57d42f0 Compare April 28, 2025 09:10
Copy link

@vava-odoo vava-odoo left a comment

Choose a reason for hiding this comment

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

Few comments passing by your PR 🙂

@api.model
def create(self, vals):
property_record = self.env["estate.property"].browse(vals["property_id"])
if vals["price"] < property_record.best_offer:

Choose a reason for hiding this comment

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

Pay attention that price is not required, you need to handle cases where it is not defined

Copy link
Author

Choose a reason for hiding this comment

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

@vava-odoo
I made a sql constraint on the price so it can't be left empty or 0

Comment on lines 57 to 58
@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.

We tend to prefer recordset over single records in odoo, could you try to adapt this to support multiple-records creation?

_name = "estate.property.tag"
_description = "Estate property tag"
_sql_constraints = [
("check_unique_name", "UNIQUE(name)", "Property tag must be unqie")

Choose a reason for hiding this comment

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

unqie 👀

<header>
<button name="action_sell" type="object" string="Mark as Sold" invisible="state in ['sold', 'cancelled']"/>
<button name="action_cancel" type="object" string="Cancel" invisible="state in ['sold', 'cancelled']"/>
<field name="state" widget="statusbar" statusbar_visible="new,offer_received,offer_accepted,sold"/>

Choose a reason for hiding this comment

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

is statusbar_visible necessary when you use all values of the selection field?

Copy link
Author

Choose a reason for hiding this comment

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

@vava-odoo
There is also "cancelled", but the request was to show only those states

'name': 'Estate Account',
'depends':
['estate', 'account'],
'data': [],

Choose a reason for hiding this comment

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

no need 🤷

Suggested change
'data': [],

Comment on lines 6 to 7
'installable': True,
'application': True,

Choose a reason for hiding this comment

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

Ok, but would be nice to have it installed as soon as dependency modules are

Copy link
Author

Choose a reason for hiding this comment

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

@vava-odoo
makes sense.
Great comments 👍

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.

4 participants