mfre - Technical Training #819

mfreym · 2025-06-18T12:02:24Z

No description provided.

robodoo · 2025-06-18T12:02:27Z

aboo-odoo

Hey! Good job for this PR, I have some comments feel free to argue 🎉

A few generic comments:

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 ?

aboo-odoo · 2025-06-23T07:08:23Z

estate/__init__.py

@@ -0,0 +1,3 @@
+from . import models
+
+from odoo import api, SUPERUSER_ID


Why is this here ? 😄

aboo-odoo · 2025-06-23T07:10:07Z

estate/__manifest__.py

+    "name": "Estate Module",
+    "depends": ["base"],
+    "data": [
+        "security/ir.model.access.csv",
+        "views/estate_property_offer_views.xml",
+        "views/estate_property_type_views.xml",
+        "views/estate_property_tag_views.xml",
+        "views/estate_property_views.xml",
+        "views/estate_menus.xml",
+    ],
+    "license": "LGPL-3",


As a global comment: double quote " should be used for strings that will be displayed and single quote ' for the rest

Suggested change

"name": "Estate Module",

"depends": ["base"],

"data": [

"security/ir.model.access.csv",

"views/estate_property_offer_views.xml",

"views/estate_property_type_views.xml",

"views/estate_property_tag_views.xml",

"views/estate_property_views.xml",

"views/estate_menus.xml",

],

"license": "LGPL-3",

'name': "Estate Module",

'depends': ['base'],

'data': [

'security/ir.model.access.csv',

'views/estate_property_offer_views.xml',

'views/estate_property_type_views.xml',

'views/estate_property_tag_views.xml',

'views/estate_property_views.xml',

'views/estate_menus.xml',

],

'license': 'LGPL-3',

aboo-odoo · 2025-06-23T07:10:54Z

estate/models/estate_property.py

+from odoo import fields, models, api
+from odoo.exceptions import UserError
+from datetime import date
+from dateutil.relativedelta import relativedelta
+from odoo.tools.float_utils import float_compare, float_is_zero
+from odoo.exceptions import ValidationError


The imports should be

External libraries (one per line sorted and split in python stdlib)

Imports of odoo

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

aboo-odoo · 2025-06-23T07:12:11Z

estate/models/estate_property.py

+    _sql_constraints = [
+        (
+            "expected_price_strictly_positive",
+            "CHECK(expected_price > 0)",


Same comment about the " and '

aboo-odoo · 2025-06-23T07:12:59Z

estate/models/estate_property.py

+
+    @api.constrains("expected_price", "selling_price")
+    def _check_selling_price(self):
+        for record in self:


record is a bit too generic, it's fine for small method like this. But when you'll work in huuuge methods, you'll be happy to have a name a bit more meaningful trust me

Suggested change

for record in self:

for estate in self:

aboo-odoo · 2025-06-23T07:43:04Z

estate/models/estate_property.py

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


This default value will be computed once the server starts, if I start and let it run 3 months, then the default availability date will be today(the today 3 month ago) + 3 months = today aha

Like for the salesperson_id it should use a lambda function.

aboo-odoo · 2025-06-23T07:48:06Z

estate/models/estate_property_offer.py

Again, the order in the file is important. Here, I was like, where is the offer status to only find it at the bottom. Keep it consistent

aboo-odoo · 2025-06-23T07:50:11Z

estate/models/estate_property_offer.py

+                record.date_deadline = record.create_date.date() + timedelta(
+                    days=record.validity
+                )
+            else:
+                record.date_deadline = fields.Datetime.now().date() + timedelta(
+                    days=record.validity
+                )


Suggested change

record.date_deadline = record.create_date.date() + timedelta(

days=record.validity

)

else:

record.date_deadline = fields.Datetime.now().date() + timedelta(

days=record.validity

)

record.date_deadline = (record.create_date.date() or fields.Date.now()) + timedelta(

days=record.validity

)

aboo-odoo · 2025-06-23T07:53:15Z

estate/views/estate_menus.xml

+            <menuitem id="estate_property_tag_menu" action="estate_property_tag_action" />
+        </menuitem>
+    </menuitem>
+</odoo>


Try to always add a EOL in each file so that when someone adds lines in the future, the last line of the previous devs is not in the diff

aboo-odoo · 2025-06-23T07:54:40Z

estate/views/estate_property_offer_views.xml

@@ -0,0 +1,33 @@
+<odoo>
+    <record id="view_estate_property_offer_tree" model="ir.ui.view">


Convention for naming views are <model_name>view<view_type>

closes odoo#764 Signed-off-by: Antoine Vandevenne (anv) <[email protected]> [ADD] chapter n°1 [ADD] chapter n°2+n°3 [ADD] chapter n°4 [ADD] chapter n°5+n°6 [ADD] chapter n°7 [ADD] estate: Add a special mention section task-####### [FIX] fixed styling issues and license in the manifest [ADD] chapter n°8 [ADD] chapter n°9+n°10 [ADD] chapter n°11 to n°15

aboo-odoo

Hey! Final review on the python part, it focuses on functionality, not style anymore 😄

By the end of the, try to get your runbot green 🟢 and fix the comments in this review please 👍

Also, there is a lot of noise in your PR (all the website_airproof stuff); it'd be nice to remove them 👍
I can help you do it if you need help :P

aboo-odoo · 2025-06-30T07:23:55Z

estate_account/models/estate_account.py

+    def action_sold(self):
+        for property_record in self:
+            if property_record.state == 'cancelled':
+                raise UserError("A cancelled property cannot be set as sold")


Most strings are automatically made translatable: field names, module and model names, etc. But this is not the case for error messages. To make them translatable, use the _() method like so

Suggested change

raise UserError("A cancelled property cannot be set as sold")

raise UserError(_"A cancelled property cannot be set as sold"))

This applies to all your error messages

aboo-odoo · 2025-06-30T07:25:27Z

estate_property/models/estate_property.py

+            if property.offer_ids:
+                property.best_offer = max(map(lambda r: r.price, property.offer_ids))
+            else:
+                property.best_offer = None


Suggested change

if property.offer_ids:

property.best_offer = max(map(lambda r: r.price, property.offer_ids))

else:

property.best_offer = None

property.best_offer = max(property.offer_ids.mapped('price'), default=0.0)

aboo-odoo · 2025-06-30T07:28:29Z

estate_property/models/estate_property_offer.py

+    @api.model
+    def create(self, vals):
+        property_id = vals.get('property_id')
+
+        property = self.env['estate.property'].browse(property_id)
+
+        property.state = 'offer_received'
+
+        existing_offers = self.env['estate.property.offer'].search([('property_id', '=', property_id), ('price', '>=', vals['price'])])
+
+        if existing_offers:
+            raise ValidationError("The offer value must be higher than existing offers.")
+
+        return super().create(vals)


We'd prefer for your create to be done in batch for better performances. Use

Suggested change

@api.model

def create(self, vals):

property_id = vals.get('property_id')

property = self.env['estate.property'].browse(property_id)

property.state = 'offer_received'

existing_offers = self.env['estate.property.offer'].search([('property_id', '=', property_id), ('price', '>=', vals['price'])])

if existing_offers:

raise ValidationError("The offer value must be higher than existing offers.")

return super().create(vals)

@api.model_create_multi

def create(self, vals_list):

for vals in vals_list:

...

return super().create(vals_list)

aboo-odoo · 2025-06-30T07:29:07Z

estate_property/models/estate_property_offer.py

+
+        property.state = 'offer_received'
+
+        existing_offers = self.env['estate.property.offer'].search([('property_id', '=', property_id), ('price', '>=', vals['price'])])


A search is unnecessary since you have the property already

Suggested change

existing_offers = self.env['estate.property.offer'].search([('property_id', '=', property_id), ('price', '>=', vals['price'])])

existing_offers = property.offer_ids.filter(lambda offer: offer.price > vals['price']

aboo-odoo · 2025-07-01T12:01:37Z

Now that your onboarding is done, could you close the PR ? 😄

mfreym force-pushed the 18.0-tutorials-mfre branch from 7d64637 to 31cdaf8 Compare June 19, 2025 12:31

aboo-odoo reviewed Jun 23, 2025

View reviewed changes

mfreym force-pushed the 18.0-tutorials-mfre branch from d4a6118 to aa107c0 Compare June 25, 2025 10:12

aboo-odoo reviewed Jun 30, 2025

View reviewed changes

mfreym closed this Jul 1, 2025

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

		from odoo import api, SUPERUSER_ID

		@@ -0,0 +1,33 @@
		<odoo>
		<record id="view_estate_property_offer_tree" model="ir.ui.view">

	raise UserError("A cancelled property cannot be set as sold")
	raise UserError(_"A cancelled property cannot be set as sold"))


		property.state = 'offer_received'

		existing_offers = self.env['estate.property.offer'].search([('property_id', '=', property_id), ('price', '>=', vals['price'])])

	existing_offers = self.env['estate.property.offer'].search([('property_id', '=', property_id), ('price', '>=', vals['price'])])
	existing_offers = property.offer_ids.filter(lambda offer: offer.price > vals['price']

mfre - Technical Training #819

mfre - Technical Training #819

Uh oh!

Conversation

mfreym commented Jun 18, 2025

Uh oh!

robodoo commented Jun 18, 2025

Uh oh!

aboo-odoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboo-odoo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aboo-odoo commented Jul 1, 2025

Uh oh!

Uh oh!