Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions estate/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from . import models
18 changes: 18 additions & 0 deletions estate/__manifest__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
{
'name': "Estate",
'depends': ['base'],
'application': True,
'installable': True,
"author": "daloe",
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As an odoo employee, you can't sign your work in your own name, the author of this module is "Odoo S.A." 😄

"license": "LGPL-3",
"data": [
'security/ir.model.access.csv',

'views/estate_property_views.xml',
'views/estate_property_offer_views.xml',
'views/estate_property_type_views.xml',
'views/estate_property_tag_views.xml',
'views/estate_menus.xml',
'views/res_users_views.xml'
],
}
5 changes: 5 additions & 0 deletions estate/models/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from . import estate_property
from . import estate_property_type
from . import estate_property_tag
from . import estate_property_offer
from . import res_users
Comment on lines +1 to +5
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Order your python import alphabetically 👍

Suggested change
from . import estate_property
from . import estate_property_type
from . import estate_property_tag
from . import estate_property_offer
from . import res_users
from . import estate_property
from . import estate_property_offer
from . import estate_property_tag
from . import estate_property_type
from . import res_users

196 changes: 196 additions & 0 deletions estate/models/estate_property.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,196 @@
from dateutil.relativedelta import relativedelta

from odoo import api, fields, models
from odoo.exceptions import UserError, ValidationError
from odoo.tools import float_compare


class EstateProperty(models.Model):
_name = "estate.property"
_description = "technical training estate property model"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

So the _description is used in places where there is a need to be kind of a human readable description of the model like some logs or error messages for example. It's purely preferences but we tend to have it describe more what it does and I feel like "technical training" is more of a reason for its existence and "model" is risking being a duplicate with any "Problem with model %s" error. So i would just name Estate Property. But wathever you pick, you should use uppercases 👍

Suggested change
_description = "technical training estate property model"
_description = "Estate Property"

_order = "id desc"

# RELATIONAL FIELDS
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We try to have as little comments in the code as possible so you can put it as a help for structure while you dev but they shouldn't be in your final version ready for merge. The only exceptions are very technical things that are not explicit enoug hfrom the code itself or things that might look wrong but are actually the intended behavior.

Suggested change
# RELATIONAL FIELDS

estate_property_type_id = fields.Many2one(
"estate.property.type",
ondelete="set null",
)

estate_property_tag_ids = fields.Many2many(
"estate.property.tag",
)

Comment on lines +18 to +22
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I see you like to put a lot of space in your code which is good but for fields declaration, you'll notice that they are very often a compact list of one line declarations 😅 Yo ucan keep doing that for methods and stuff but don't put blank spaces in between your fields and try to oneline those that don't have to much in the declaration like this one for example.

Suggested change
estate_property_tag_ids = fields.Many2many(
"estate.property.tag",
)
estate_property_tag_ids = fields.Many2many("estate.property.tag")

buyer_id = fields.Many2one(
"res.partner",
ondelete="set null",
)

Comment on lines +23 to +27
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would oneline this one too

seller_id = fields.Many2one(
"res.users",
ondelete="set null",
default=lambda self: self.env.user.id,
)

estate_property_offer_ids = fields.One2many(
"estate.property.offer",
"estate_property_id",
copy=False,
)

# BASIC FIELDS
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# BASIC FIELDS

name = fields.Char(
"Estate Property",
required=True,
)

active = fields.Boolean(
default=True,
)
Comment on lines +41 to +48
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

oneline these ones


status = fields.Selection(
[["new", "New"], ["offer_received", "Offer Received"], ["offer_accepted", "Offer Accepted"], ["sold", "Sold"], ["cancelled", "Cancelled"]],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Doesn't change much but we tend to use a list of tuples for selection declarations and when they get to three or four or more elements, we multiline the list for clarity and an easier modification in the future. One very important thing in R&D at odoo is to try to keep the commit history as clean as possible so anytime you multiline something, you allow someone to modify a smaller part of the code to get the same result keeping a clearer history in the end. A lot of our styling guidelines revolves around that.

Suggested change
[["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"),
],

default="new",
)

description = fields.Text(
"Description"
)

postcode = fields.Char(
"Postcode"
)
Comment on lines +55 to +61
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

oneline these one, you get the point. Just try to keep a smaller diff if it doesn't provide too much of a benefit to have a bigger one 😄


date_availability = fields.Date(
"Available From",
copy=False,
default=fields.Date.today() + relativedelta(months=+3),
)

expected_price = fields.Float(
"Expected Price",
required=True,
)

selling_price = fields.Float(
"Selling Price",
copy=False,
readonly=True,
)

bedrooms = fields.Integer(
"Bedrooms",
default=2,
)

living_area = fields.Integer(
"Living Area (sqm)"
)

facades = fields.Integer(
"Facades"
)

garage = fields.Boolean(
"Garage",
default=False,
)

garden = fields.Boolean(
"Garden",
default=False,
)

garden_area = fields.Integer(
"Garden Area"
)

garden_orientation = fields.Selection(
[["north", "North"], ["east", "East"], ["south", "South"], ["west", "West"]],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

List of tuples 👍 Even though it's a selection field, I would keep this one in a single line because I don't expect a new orientation to be added to the selection 😄

)

# COMPUTED FIELDS
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# COMPUTED FIELDS

total_area = fields.Integer(compute="_compute_total_area")

best_price = fields.Float("Best Offer", compute="_compute_best_price")

# SQL CONSTRAINTS & INDEXES
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# SQL CONSTRAINTS & INDEXES

_check_expected_price_positive = models.Constraint(
"CHECK(expected_price > 0)",
"expected_price <= 0"
)

_check_selling_price_positive = models.Constraint(
"CHECK(selling_price is null or selling_price > 0)",
"selling_price <= 0"
)

# COMPUTE & INVERSE & SEARCH
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# COMPUTE & INVERSE & SEARCH

What I like though is that you nicely ordered fields -> constrains -> methods without having a bit of each everywhere. This is how we tend to structure of models declaration and it's nice that you've already doing it intuitively 👍

@api.depends("living_area")
def _compute_total_area(self):
for estate in self:
null_safe_garden_area = 0
if estate.garden_area:
null_safe_garden_area = estate.garden_area

estate.total_area = estate.living_area + null_safe_garden_area
Comment on lines +129 to +135
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a bit strange looking to me. Probably because of the name of the variable that can be actually wrong if there is a garden_area and therefore the value is not null. If I'm not mistaken, since garden_area is a fields.Integer, you don't need to check for truthy. Falsy on fields.Integer acutally gives 0.

Suggested change
def _compute_total_area(self):
for estate in self:
null_safe_garden_area = 0
if estate.garden_area:
null_safe_garden_area = estate.garden_area
estate.total_area = estate.living_area + null_safe_garden_area
def _compute_total_area(self):
for estate in self:
estate.total_area = estate.living_area + estate.garden_area


@api.depends("estate_property_offer_ids")
def _compute_best_price(self):
for estate in self:
if len(estate.estate_property_offer_ids) > 0:
estate.best_price = max(self.estate_property_offer_ids.mapped("price"))
else:
estate.best_price = 0
Comment on lines +140 to +143
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Mapped doesn't raise on an empty recordset so you don't even need an if else here. The worst thing that can happen is return an empty list or recordset 👍 Becareful that max() needs an iterable though :)

Suggested change
if len(estate.estate_property_offer_ids) > 0:
estate.best_price = max(self.estate_property_offer_ids.mapped("price"))
else:
estate.best_price = 0
estate.best_price = max(estate.mapped('estate_property_offer_ids.price') or [0])


# PYTHON CONSTRAINS & ONCHANGE
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# PYTHON CONSTRAINS & ONCHANGE

@api.constrains('selling_price')
def _check_ninety_percent_of_expected(self):
for estate in self:
if estate.selling_price > 0: # restrict checks to only when offer acceptance happens
if float_compare(estate.selling_price, 0.9 * estate.expected_price, 2) < 0:
raise ValidationError("Offer is <90% of expected")

@api.onchange("garden")
def _onchange_garden_orientation(self):
if self.garden:
self.garden_area = 10
self.garden_orientation = "north"
else:
self.garden_area = None
self.garden_orientation = None

# ORM OVERRIDES
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# ORM OVERRIDES

@api.ondelete(at_uninstall=False)
def _unlink_if_new_cancelled(self):
for user in self:
if user.status not in ("new", "cancelled"):
error = self.env._("Cannot delete active listing")
raise UserError(error)
Comment on lines +167 to +168
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Don't spend the extra step putting it in a variable 👍

Suggested change
error = self.env._("Cannot delete active listing")
raise UserError(error)
raise UserError(self.env._("Cannot delete active listing"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same on the two next methods


# ACTIONS
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# ACTIONS

def action_set_sold(self):
self.ensure_one() # this button should only exist on a form
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's the reason for half the ensure_one() in the codebase, you don't need to state it explicitely 😉

Suggested change
self.ensure_one() # this button should only exist on a form
self.ensure_one()

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

same on the next method

if not self.active:
error = self.env._("Cannot set inactive/cancelled listing to sold")
raise UserError(error)

self.status = "sold"
return True

def action_set_cancelled(self):
self.ensure_one() # this button should only exist on a form
if self.status == "sold":
error = self.env._("Cannot set sold listing to cancelled")
raise UserError(error)
self.status = "cancelled"
return True

def action_uncancel(self):
self.ensure_one()
self.status = "new"
return True

# BUSINESS LOGIC
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# BUSINESS LOGIC

def set_offer_received(self):
for estate in self:
estate.status = "offer_received"
137 changes: 137 additions & 0 deletions estate/models/estate_property_offer.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
from datetime import timedelta

from odoo.exceptions import UserError
from odoo import models, fields, api
Comment on lines +1 to +4
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Keep this piece of documentation close, it's very (very very very) useful 😄

Suggested change
from datetime import timedelta
from odoo.exceptions import UserError
from odoo import models, fields, api
from datetime import timedelta
from odoo import api, fields, models
from odoo.exceptions import UserError



class EstatePropertyOffer(models.Model):
_name = "estate.property.offer"
_description = "tech training estate property tag"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

At least the uppercases then any name you want 👍

Suggested change
_description = "tech training estate property tag"
_description = "Estate Property Tag"

_order = "price desc"

# RELATIONAL FIELDS
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# RELATIONAL FIELDS

estate_property_id = fields.Many2one(
"estate.property",
ondelete="cascade",
required=True,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Again a lot of blank spaces that we will just not keep in a final version before merge 😄

property_type_id = fields.Many2one(
related="estate_property_id.estate_property_type_id",
store=True,
)

buyer_id = fields.Many2one(
"res.partner",
string="Partner",
ondelete="cascade",
default=lambda self: self.env.user.partner_id.id,
required=True,
)

# BASIC FIELDS
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# BASIC FIELDS

price = fields.Float(
string="Price",
required=True,
aggregator="max",
)

status = fields.Selection(
[["new", "New"], ["refused", "Refused"], ["accepted", "Accepted"]],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

List of tuples and this one could be multilines I feel, up to you really 👍

default="new",
copy=False,
)

# COMPUTED FIELDS
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# COMPUTED FIELDS

validity = fields.Integer(default=7)

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

# SQL CONSTRAINTS & INDEXES
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# SQL CONSTRAINTS & INDEXES

_check_positive_offer = models.Constraint(
"CHECK(price > 0)",
"Offer price must be greater than 0"
)

# COMPUTE & INVERSE & SEARCH
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# COMPUTE & INVERSE & SEARCH

@api.depends("validity")
def _compute_date_deadline(self):
for estate in self:
if estate.create_date:
estate.date_deadline = estate.create_date + timedelta(days=estate.validity)
else:
estate.date_deadline = fields.Date.today() + timedelta(days=estate.validity)
Comment on lines +60 to +63
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Personnal preference but since you don't expect an elif here, I would just :

Suggested change
if estate.create_date:
estate.date_deadline = estate.create_date + timedelta(days=estate.validity)
else:
estate.date_deadline = fields.Date.today() + timedelta(days=estate.validity)
estate.date_deadline = (estate.create_date or fields.Date.today()) + timedelta(days=estate.validity)


def _inverse_date_deadline(self):
for estate in self:
new_validity = (estate.date_deadline - fields.Date.today()).days
if new_validity < 0:
raise UserError("validity < 0")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Any user error and even more generally any string that will be displayed to the user needs to be a translatable sentence 👍

Suggested change
raise UserError("validity < 0")
raise UserError(_("Validity cannot be negative."))

estate.validity = (estate.date_deadline - fields.Date.today()).days

# ORM OVERRIDES
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# ORM OVERRIDES

@api.model
def create(self, vals):
for new_offer in vals:
current_property_offers_price_asc = self.env["estate.property.offer"].search(
[('estate_property_id', '=', new_offer.get("estate_property_id"))]
).sorted("price asc")
Comment on lines +76 to +78
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Have you tried to only get the maximum current offer with order='price asc' and limit=1 ?

            current_lowest_offer = self.env["estate.property.offer"].search(
                [
                    ('estate_property_id', '=', new_offer.get("estate_property_id"))
                ], order="price asc", limit=1,
            )

Could be helpful here 👀


estate_property = self.env["estate.property"].browse(new_offer.get("estate_property_id"))
if not current_property_offers_price_asc or len(current_property_offers_price_asc) == 0:
if estate_property.status == "new":
estate_property.set_offer_received()
return super().create(vals)

if new_offer.get("price") < current_property_offers_price_asc[0].price:
raise UserError("submitted offer < lowest current offer")

if estate_property.status == "new":
estate_property.set_offer_received()
return super().create(vals)

@api.model
def write(self, vals):
updated_price = vals.get("price")
if not updated_price:
return super().write(vals) # just skip, since logic only applies if there's updated price
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
return super().write(vals) # just skip, since logic only applies if there's updated price
return super().write(vals)


current_property_offers_price_asc = self.estate_property_id.estate_property_offer_ids.sorted("price asc")

if self == current_property_offers_price_asc[0]:
return super().write(vals)

if updated_price < current_property_offers_price_asc[0].price:
raise UserError("submitted offer < lowest current offer")

return super().write(vals)

# ACTIONS
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
# ACTIONS

def action_accept_offer(self):
self.ensure_one()
for offer in self:
if fields.Date.today() > offer.date_deadline:
raise UserError("offer expired")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

UserError -> Translatable


estate = offer.estate_property_id
for recur_offer in estate.estate_property_offer_ids:
if recur_offer.status == "accepted":
# more foolproof than just checking estate_property.status
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not sure this one would be kept or not but I would at least reformulate it 🤔

Suggested change
# more foolproof than just checking estate_property.status
# Safer than just checking estate_property.status.

or even if you it more professional

Suggested change
# more foolproof than just checking estate_property.status
# Guard against multiple accepted offers, independent of property status.

raise UserError("another offer already accepted")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Translatable


estate.buyer_id = offer.buyer_id
estate.selling_price = offer.price
estate.status = "offer_accepted"

offer.status = "accepted"
return True

def action_refuse_offer(self):
self.ensure_one()
self.status = "refused"

estate = self.estate_property_id
estate.buyer_id = None
estate.selling_price = None
estate.status = "offer_received"
return True
29 changes: 29 additions & 0 deletions estate/models/estate_property_tag.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
from odoo import models, fields
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
from odoo import models, fields
from odoo import fields, models



class EstatePropertyTag(models.Model):
_name = 'estate.property.tag'
_description = 'tech training estate property tag'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
_description = 'tech training estate property tag'
_description = 'Estate Property Tag'

_order = 'name asc'

# RELATIONAL FIELDS
estate_property_ids = fields.Many2many(
"estate.property",
)

# BASIC FIELDS
name = fields.Char(
"Tag",
required=True,
)

color = fields.Integer(
string="Color",
default=0,
)

# SQL CONSTRAINTS & INDEXES
_check_unique_name = models.UniqueIndex(
"(UPPER(name))",
"Tag should be unique"
)
Comment on lines +9 to +29
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file is a nice place for a full example 😄

Suggested change
# RELATIONAL FIELDS
estate_property_ids = fields.Many2many(
"estate.property",
)
# BASIC FIELDS
name = fields.Char(
"Tag",
required=True,
)
color = fields.Integer(
string="Color",
default=0,
)
# SQL CONSTRAINTS & INDEXES
_check_unique_name = models.UniqueIndex(
"(UPPER(name))",
"Tag should be unique"
)
estate_property_ids = fields.Many2many(estate.property")
name = fields.Char("Tag", required=True)
color = fields.Integer(string="Color", default=0)
_check_unique_name = models.UniqueIndex(
"(UPPER(name))",
"Tag should be unique"
)

It'll look more like this in 99.9% of the files 👍

Loading