Skip to content

[ADD] estate: Chapters 1-11 #873

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

Choose a reason for hiding this comment

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

For multiple reasons, the main one being that we always want to keep the changes history as clean as possible, we always end files with a blank line so that if someone edits your file and add a line at the end, it doesn't modify your last line to add the line break character.

Suggested change
from . import models
from . import models

17 changes: 17 additions & 0 deletions estate/__manifest__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
'name': "estate",
'depends':['base'],
'application': True,
'data':['security/ir.model.access.csv',
'views/estate_property_views.xml',
'views/estate_property_tag_views.xml',
'views/estate_property_offer_views.xml',
'views/estate_property_type_views.xml',
'views/estate_property_menus.xml',
'views/inherited_model_view.xml'


]


}

Choose a reason for hiding this comment

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

Indentation is super important for readability so we always indent files in a pythonish way even in file types that don't require it.

Suggested change
{
'name': "estate",
'depends':['base'],
'application': True,
'data':['security/ir.model.access.csv',
'views/estate_property_views.xml',
'views/estate_property_tag_views.xml',
'views/estate_property_offer_views.xml',
'views/estate_property_type_views.xml',
'views/estate_property_menus.xml',
'views/inherited_model_view.xml'
]
}
{
'name': "estate",
'depends':['base'],
'application': True,
'data':[
'security/ir.model.access.csv',
'views/estate_property_views.xml',
'views/estate_property_tag_views.xml',
'views/estate_property_offer_views.xml',
'views/estate_property_type_views.xml',
'views/estate_property_menus.xml',
'views/inherited_model_view.xml'
]
}

1 change: 1 addition & 0 deletions estate/models/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
from . import estate_property, estate_property_type, estate_property_tag, estate_property_offer, inherited_model

Choose a reason for hiding this comment

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

Missing EOL (End Of Line character (empty last line at the end of your file))

119 changes: 119 additions & 0 deletions estate/models/estate_property.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
from odoo import models, fields, api
from odoo.exceptions import UserError, ValidationError
from odoo.tools import float_compare, float_is_zero

Choose a reason for hiding this comment

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

The guidelines for the imports notably asks to sort all import (sources and imported elements alphabtically) so:

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


Choose a reason for hiding this comment

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

The runbot ci/style probably says that but we always have two blank lines between the imports and the models

Suggested change

class TestModel(models.Model):

Choose a reason for hiding this comment

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

The class name of a model is always its _name attribute but without the dots and all first letters of words in uppercase (almost camelCase but first word is capitalized too)

Suggested change
class TestModel(models.Model):
class EstateProperty(models.Model):

_name = "estate_property"

Choose a reason for hiding this comment

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

The _name attribute of the model is always formated as words separated by dots so:

Suggested change
_name = "estate_property"
_name = "estate.property"

_description = "Basic fields in estate property"
_order = "id desc"



Choose a reason for hiding this comment

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

I haven't checked if ci/style flags this but only ever one blank line everywhere except between the imports and the models.

Other occurences of this lower in the diff 👍

name = fields.Char(required = True)
description = fields.Text()
postcode = fields.Char()
date_availability = fields.Date(copy = False, default = fields.Date.add(fields.Date.today(), months = 3))

Choose a reason for hiding this comment

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

Probably flagged by ci/style but i'll still say it, no spaces around the operators in a field definition 👍

Suggested change
date_availability = fields.Date(copy = False, default = fields.Date.add(fields.Date.today(), months = 3))
date_availability = fields.Date(copy=False, default=fields.Date.add(fields.Date.today(), months=3))

Other occurences of this lower in the diff 👍

expected_price = fields.Float(required = True)#
selling_price = fields.Float(readonly=True, copy = False) #

Choose a reason for hiding this comment

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

Suggested change
selling_price = fields.Float(readonly=True, copy = False) #
selling_price = fields.Float(readonly=True, copy = False)

bedrooms = fields.Integer(default = 2)
living_area = fields.Integer()
facades = fields.Integer()#

Choose a reason for hiding this comment

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

Suggested change
facades = fields.Integer()#
facades = fields.Integer()

garage = fields.Boolean()
garden = fields.Boolean()
garden_area = fields.Integer()
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.

the selection attribute of Selection fields is a list of paired values in tuples. The first one is the technical value, the one written in database. The second one is the user value, used for display. There is no need for both values in a pair to be the same text BUT it is required that the technical value is lowercase only and if multiple words are used, that they are separated by underscores so:

Suggested change
garden_orientation = fields.Selection(
string='Garden Orientation',
selection=[('North', 'North'),('South', 'South'), ('East', 'East'), ('West', 'West')])
garden_orientation = fields.Selection(
string='Garden Orientation',
selection=[('nort', 'North'), ('south', 'South'), ('east', 'East'), ('west', 'West')]),

There is also another small detail here is that when you define a field on multiple lines, even the last lines takes a coma so that if someone was to add a line, he wouldn't apply a diff to yours to add his.

active = fields.Boolean('Active', default = True)
# New, Offer Received, Offer Accepted, Sold and Cancelled
state = fields.Selection(
string='State',
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.

Related to my last comment, here there are multiple words technical values so they should look like that:

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')],

required = True,
default = "New")

Choose a reason for hiding this comment

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

Last line takes a coma and the closing parenthesis of the definition should be aligned with the field's name:

Suggested change
default = "New")
default = "New",
)

property_type_id = fields.Many2one("estate_property_type", string = "Property Type")
salesperson = fields.Many2one('res.users', string='Salesman', default=lambda self: self.env.user)
buyer = fields.Many2one('res.partner', string='Buyer', copy = False)
tag_ids = fields.Many2many("estate_property_tag", string="Property Tags")
offer_ids = fields.One2many(comodel_name="estate_property_offer", inverse_name = "property_id")
total_area = fields.Integer(compute = "_compute_total_area")
best_price = fields.Float(compute = "_compute_best_offer")


Choose a reason for hiding this comment

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

Too many blank lines

Other occurences of this lower in the diff 👍

@api.depends('living_area', 'garden_area')
def _compute_total_area(self):
for property in self:
property.total_area = property.living_area + property.garden_area



@api.depends('offer_ids')
def _compute_best_offer(self):
for property in self:
if len(property.offer_ids) > 0:
property.best_price = max(property.offer_ids.mapped('price'))
else:
property.best_price = 0.0


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

Choose a reason for hiding this comment

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

Since you will have changed the technical value of the garden_orientation field, you will to check for == 'north'

Suggested change
self.garden_orientation = "North"
self.garden_orientation = 'north'

Other occurences of this lower in the diff 👍

self.garden_area = 10
else:
self.garden_area = 0
self.garden_orientation = ''

Choose a reason for hiding this comment

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

It'd be better to set it as False for readability (doesn't change anything nor to the execution to the result)

Suggested change
self.garden_orientation = ''
self.garden_orientation = False




def sell(self):
for record in self:
if record.state == 'Cancelled':
raise UserError("Cancelled properties cannot be sold!")

else:
record.state = "Sold"
return True



def cancel(self):
for record in self:
if record.state == 'Sold':
raise UserError("Sold properties cannot be cancelled!")

else:
record.state = "Cancelled"
return True

_sql_constraints = [
('strictly_positive_expected_price',
'CHECK(expected_price > 0)',
'The expected price should be strictly positive'),

('positive_seling_price',
'CHECK(selling_price >= 0)',
'The selling price should be positive'),

]


@api.constrains('expected_price', 'selling_price')
def filter_bad_offers(self):
for property in self:
if not float_is_zero(property.selling_price, precision_digits = 6) and float_compare(property.selling_price, 0.9*property.expected_price, precision_digits=6) < 0:
raise ValidationError('The selling price is below the 90% threshold of the expected price')

Choose a reason for hiding this comment

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

Depends on the team you will join but we tend to always wrap user messages in between douoble quotes. At least in R&D Accounting. But whatever the team you will join, you will be asked to be consistent with your use of single and double quotes so as you used doubles quotes for all teh other errors before this one, I'd suggest you do the same here 👍

Suggested change
raise ValidationError('The selling price is below the 90% threshold of the expected price')
raise ValidationError("The selling price is below the 90% threshold of the expected price")

return True



@api.ondelete(at_uninstall=False)
def _unlink_if_offer_not_new_not_cancelled(self):
if any(property.state not in ["New", "Cancelled"] for property in self):
raise UserError("Can't delete an offer which is not cancelled or new!")





112 changes: 112 additions & 0 deletions estate/models/estate_property_offer.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
from odoo import models, fields, api
from datetime import datetime
from odoo.exceptions import UserError
from odoo.tools import float_compare
Comment on lines +1 to +4

Choose a reason for hiding this comment

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

Here is an instance of two blocks of imports. You have external ones and odoo ones so your imports should look like this:

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


Choose a reason for hiding this comment

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

missing one blank space

class EstatePropertyOffer(models.Model):
_name = "estate_property_offer"

Choose a reason for hiding this comment

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

Suggested change
_name = "estate_property_offer"
_name = "estate.property.offer"

_order = "price desc"

Choose a reason for hiding this comment

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

too many blank spaces, more of this lower in the diff 👍



name = fields.Char(required = True, default = "Offer")

Choose a reason for hiding this comment

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

no space around operators for fields attributes

price = fields.Float()
state = fields.Selection(
string='State',
selection=[('Accepted', 'Accepted'),('Refused','Refused')],

Choose a reason for hiding this comment

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

technical values in Selection fields' selection in lowercase

copy = False)

Choose a reason for hiding this comment

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

Suggested change
copy = False)
copy = False,
)

partner_id = fields.Many2one('res.partner', string='Partner',required = True)
property_id = fields.Many2one('estate_property',required = True, ondelete='cascade')
date_deadline = fields.Date(compute = '_compute_deadline', inverse = '_compute_validity')
validity = fields.Integer(default = 7)

Choose a reason for hiding this comment

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

You can but we don't usually leave blank space inside the field definitions part of the model 👍

property_state = fields.Selection(
related='property_id.state',
readonly=True,
)

property_type_id = fields.Many2one(
related='property_id.property_type_id',
stored = True

Choose a reason for hiding this comment

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

coma on last line of a multi-line field definition 👍

)



@api.depends('validity')
def _compute_deadline(self):
for property in self:
if not property.create_date:
property.date_deadline = fields.Date.add(fields.Date.today(), days = property.validity)

Choose a reason for hiding this comment

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

days is an argument of the fields.Date.add() method so no spaces around the operators 👍

Suggested change
property.date_deadline = fields.Date.add(fields.Date.today(), days = property.validity)
property.date_deadline = fields.Date.add(fields.Date.today(), days=property.validity)

same just below 😉


else:
property.date_deadline = fields.Date.add(property.create_date, days = property.validity)




def _compute_validity(self):
for property in self:
if not property.create_date:

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


else:
property.validity = (datetime.combine(property.date_deadline, datetime.min.time()) - property.create_date).days




def action_confirm(self):
for record in self:
if record.property_id.state not in ["Cancelled", "Sold", "Offer Accepted"]:
record.state = 'Accepted'
record.property_id.buyer = record.partner_id
record.property_id.selling_price = record.price
record.property_id.state = "Offer Accepted"
elif record.property_id.state == "Offer Accepted":
if record.state != "Accepted":
raise UserError("Only one offer can be accepted!")

else:

Choose a reason for hiding this comment

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

The else works great but in a project like odoo with hundreds of devs, the quicker it is to understand what you did, the best so we tend to make everything as explicit as possible. In this case, you could replace your else by the last value(s) possible of the preperty' state field to make it a tiny bit more readable 👍

Suggested change
else:
elif record.property_id.state == "New":

raise UserError("Property not available!")

return True

Choose a reason for hiding this comment

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

You don't need a return value for this method. Doesn't hurt, but doesn't help either 😄

Suggested change
return True




def action_reject(self):
for record in self:
if record.property_id.state not in ["Cancelled", "Sold"]:
if record.state == 'Accepted':
record.property_id.state = "Offer Received"
record.property_id.buyer = ''
record.property_id.selling_price = 0.0
record.state = 'Refused'

else:

Choose a reason for hiding this comment

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

Same here. To clarify a bit, I don't think anyone will ever ask you to do it so if you don't it's fine. There are places where it's really better and some not so much. Just keep in mind you spend 99% of your time in code that is not yours so it's always better when it's easy to understand 👍

raise UserError("Property not available!")



return True


_sql_constraints = [
('strictly_positive_offer_price',
'CHECK(price > 0)',
'The offer price should be strictly positive')

]


@api.model
def create(self, vals):

property_object = self.env["estate_property"].browse(vals['property_id'])
if float_compare(property_object.best_price, vals['price'], precision_digits=6) > 0:
raise UserError(f"Offer must be higher than {property_object.best_price}!")

offer = super().create(vals)
offer.property_id.state = 'Offer Received'
return offer
13 changes: 13 additions & 0 deletions estate/models/estate_property_tag.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from odoo import models, fields

Choose a reason for hiding this comment

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

As this file is shorter, i'll just leave the list of the things i saw here and let you look after what it means in terms changes to be made 😄

  • Import order guidelines
  • Module technical name naming convention
  • Blank spaces
  • Missing EOL
  • Field attribute operator spacing


class EstatePropertyTag(models.Model):
_name = "estate_property_tag"
_order = "name"
name = fields.Char(required = True)
color = fields.Integer()



_sql_constraints = [
('is_tag_unique', 'UNIQUE(name)', 'Tag name must be unique!')
]
24 changes: 24 additions & 0 deletions estate/models/estate_property_type.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
from odoo import models, fields, api

Choose a reason for hiding this comment

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

This one i'll let you do by yourself with the comments i wrote before. There's a bunch of repeating stuff 😄


class EstatePropertyType(models.Model):
_name = "estate_property_type"
_order = "sequence,name"

Choose a reason for hiding this comment

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

Suggested change
_order = "sequence,name"
_order = "sequence, name"


name = fields.Char(required = True)
property_ids = fields.One2many(comodel_name="estate_property", inverse_name = "property_type_id")
sequence = fields.Integer('Sequence', default=1)
offer_ids = fields.One2many(comodel_name="estate_property_offer", inverse_name = "property_type_id")
offer_count = fields.Integer(compute = "_compute_offer_count")



_sql_constraints = [
('is_property_type_unique', 'UNIQUE(name)', 'Property type name must be unique!')
]


@api.depends('offer_ids')
def _compute_offer_count(self):
for property_type in self:
property_type.offer_count = len(property_type.offer_ids)

7 changes: 7 additions & 0 deletions estate/models/inherited_model.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from odoo import fields, models

Choose a reason for hiding this comment

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

And this one too is pretty straight forward with what i wrote before 😄


class InheritedModel(models.Model):
_inherit = "res.users"

property_ids = fields.One2many(comodel_name="estate_property", inverse_name = "salesperson", domain = [('state', 'in', ['New', 'Offer Received'])])

5 changes: 5 additions & 0 deletions estate/security/ir.model.access.csv
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
id,name,model_id/id,group_id/id,perm_read,perm_write,perm_create,perm_unlink
estate.access_estate_property,access_estate_property,estate.model_estate_property,base.group_user,1,1,1,1
estate.access_estate_property_type,access_estate_property_type,estate.model_estate_property_type,base.group_user,1,1,1,1
estate.access_estate_property_tag,access_estate_property_tag,estate.model_estate_property_tag,base.group_user,1,1,1,1
estate.access_estate_property_offer,access_estate_property_offer,estate.model_estate_property_offer,base.group_user,1,1,1,1
13 changes: 13 additions & 0 deletions estate/views/estate_property_menus.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?xml version="1.0"?>
<odoo>
<menuitem id="test_menu_root" name="Real Estate">
<menuitem id="test_first_level_menu" name="Advertisements">
<menuitem id="test_model_menu_action" action="test_model_action"/>
</menuitem>
<menuitem id="estate_property_type_settings_menu" name="Settings">
<menuitem id="property_type_action" action="estate_property_type_properties"/>
<menuitem id="estate_property_tag_menu_tags" action="estate_property_tag_action"/>
</menuitem>

Choose a reason for hiding this comment

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

Useless blank line 👍

</menuitem>
</odoo>
72 changes: 72 additions & 0 deletions estate/views/estate_property_offer_views.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<?xml version="1.0"?>

Choose a reason for hiding this comment

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

Lots of blank lines that shouldn't be here in this file. 😄

<odoo>

<record id="estate_property_offer_action" model="ir.actions.act_window">
<field name="name">Offers</field>
<field name="res_model">estate_property_offer</field>
<field name="view_mode">list,form</field>
<field name="domain">[('property_type_id', '=', active_id)]</field>
</record>
Comment on lines +5 to +10

Choose a reason for hiding this comment

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

Suggested change
<record id="estate_property_offer_action" model="ir.actions.act_window">
<field name="name">Offers</field>
<field name="res_model">estate_property_offer</field>
<field name="view_mode">list,form</field>
<field name="domain">[('property_type_id', '=', active_id)]</field>
</record>
<record id="estate_property_offer_action" model="ir.actions.act_window">
<field name="name">Offers</field>
<field name="res_model">estate_property_offer</field>
<field name="view_mode">list,form</field>
<field name="domain">[('property_type_id', '=', active_id)]</field>
</record>










<record id="estate_property_offer_list_view" model="ir.ui.view">

<field name="name">List View</field>

Choose a reason for hiding this comment

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

Try to make it a bit more specific 👍

Suggested change
<field name="name">List View</field>
<field name="name">Offers List View</field>
or
<field name="name">Property Offers</field>
or even just
<field name="name">Offers</field>

<field name="model">estate_property_offer</field>
<field name="arch" type="xml">
<list string="Offer List" editable="bottom" decoration-success = "state == 'Accepted'" decoration-danger = "(state == 'Refused') or (state != 'Accepted' and property_state in ('Offer Accepted', 'Cancelled', 'Sold'))">

<field name="price" string = "Price"/>

Choose a reason for hiding this comment

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

operators spacing 😄

<field name="partner_id" string = "Partner"/>
<field name="validity" string = "Validity (days)"/>
<field name="date_deadline" string = "Deadline"/>

<button name="action_confirm" string="Accept" type="object" icon="fa-check" invisible="(state in ('Accepted','Refused')) or property_state in ('Cancelled', 'Offer Accepted', 'Sold')"/>
<button name="action_reject" string="Reject" type="object" icon="fa-times" invisible="(state in ('Accepted','Refused')) or property_state in ('Cancelled', 'Offer Accepted', 'Sold')"/>


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






<record id="estate_property_offer_form_view" model="ir.ui.view">
<field name="name">Form View</field>
<field name="model">estate_property_offer</field>
<field name="arch" type="xml">
<form string="Property Form">
<sheet>

<div class="estate_property_offer_oe_title">
<h1> <field name="name" nolabel="1"/> </h1>
</div>

<group>
<field name="price" string = "Price"/>
<field name="partner_id" string = "Partner"/>
<field name="validity" string = "Validity (days)"/>
<field name="date_deadline" string = "Deadline"/>
<field name="state" string = "State"/>
Comment on lines +46 to +50

Choose a reason for hiding this comment

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

always indent by gorups of 4 spaces ( ). A same level of indent should be aligned 😄


</group>

</sheet>
</form>
</field>
</record>


</odoo>


Choose a reason for hiding this comment

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

missing EOL

Loading