ollet - Technical Training by ollet-odoo · Pull Request #1414 · odoo/tutorials

ollet-odoo · 2026-09-15T09:54:24Z

task-6573598

robodoo · 2026-09-15T09:54:37Z

task-6573598

msho-odoo

Good job 🔥

Just for some commit messages, we write it in a way that aligns with when this commit is merged it will ..... so for example estate: Basic security can be add access rules to estate_property or however you would like to describe your change :)
refer to Git Guidlines

I also left you some comments :)
feel free to ping me when you need another review, also always make sure your runbot is green ahead please :)

msho-odoo · 2026-09-15T14:11:20Z

+            <menuitem id="estate_property_menu_action" action="estate_property_action"/>
+        </menuitem>
+    </menuitem>
+</odoo>


Always add a new line at end of files :)

msho-odoo · 2026-09-15T14:11:59Z

+<?xml version="1.0"?>
+<odoo>
+    <menuitem id="estate_menu_root" name="Real Estate">
+        <menuitem id="estate_first_level_menu" name="Advertisements">


it's better to give ids a descriptive name rather than first_level_menu :)

msho-odoo · 2026-09-15T14:13:14Z

+        <field name="res_model">estate.property</field>
+        <field name="view_mode">list,form</field>
+    </record>
+        <record id="estate_property_view_tree" model="ir.ui.view">


Suggested change

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

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

it's not gonna break the xml but it's better aligned to be debugged easier in the future

msho-odoo · 2026-09-15T14:13:38Z

@@ -0,0 +1 @@
+from .property import Property


EOF new line in all files :)

msho-odoo · 2026-09-15T14:15:54Z

+    _name = "estate.property"
+    _description = "Real estate property"
+
+    name = fields.Char(required=True, string='Title')


it's just a convention to have the string as the first attribute

msho-odoo · 2026-09-15T14:17:53Z

+    living_area = fields.Integer(string="Living Area (sqm)")
+    faces = fields.Integer()
+    garage = fields.Boolean()
+    garden = fields.Boolean()


Good practice to have boolean fields on the form is_something or has_something so you might change this to has_garden and has_garage

msho-odoo · 2026-09-15T14:21:03Z

+    garden_orientation = fields.Selection(
+        selection=(('north', 'North'), ('east', 'East'), ('south', 'South'), ('west', 'West')))


Suggested change

garden_orientation = fields.Selection(

selection=(('north', 'North'), ('east', 'East'), ('south', 'South'), ('west', 'West')))

garden_orientation = fields.Selection(

selection=[

('north', 'North'),

('east', 'East'),

('south', 'South'),

('west', 'West'),

]

)

We usually use the [] for the selection attribute and style it like this with multiple values.

if you want to apply the nitpick mentioned in the previous commit which is used by some teams, you will have ('north', "North") instead of ('north', 'North')

msho-odoo · 2026-09-15T14:21:21Z

+    state = fields.Selection(default='new', required=True, copy=False,
+                             selection=(('new', 'New'), ('offer_received', 'Offer Received'),
+                                        ('offer_accepted', 'Offer Accepted'), ('sold', 'Sold'),
+                                        ('cancelled', 'Cancelled')))


Apply same styling here :)

msho-odoo · 2026-09-15T14:22:50Z

@@ -0,0 +1,2 @@
+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,0


no need to add estate. here in the id, the file is already defined in that module, but you may want to add _user at the end of the id so that it's access_estate_property_user, it's just gives you the chance to make other rules for the same module with different users like _manager for example

msho-odoo · 2026-09-15T14:23:15Z

+        'data/estate_property_views.xml',
+        'data/estate_menus.xml'
+    ],
+    'author': "Olivier Le Thanh Duong",


author is usually Odoo S.A.

task-6573598

ollet-odoo · 2026-09-15T14:56:34Z

@msho-odoo Thanks for the review, i have corrected according to your remarks. I don't know if I should mark the conversation as resolved when I fix them or if you will do it when you do the review again

msho-odoo · 2026-09-15T15:13:13Z

@msho-odoo Thanks for the review, i have corrected according to your remarks. I don't know if I should mark the conversation as resolved when I fix them or if you will do it when you do the review again

Usually resolving the comment would depend on the team or the reviewer and what they prefer :)
but here if you're sure you've done the change, feel free to resolve it 👍

msho-odoo

Good work 👍
Just try to use git commit --amend instead of just git commit when fixing few stuff like spaces, names, quotes, etc. because the same commit still applies, no need to add the fixes to a new commit.
Also, left you a few changes.
Keep it up 🔥

msho-odoo · 2026-09-15T15:15:06Z

+<?xml version="1.0"?>
+<odoo>
+    <menuitem id="estate_menu_root" name="Real Estate">
+        <menuitem id="estate_Advertisements_menu" name="Advertisements">


Suggested change

<menuitem id="estate_Advertisements_menu" name="Advertisements">

<menuitem id="estate_advertisements_menu" name="Advertisements">

I would stick to the snake case here :), let's make it all small letters

msho-odoo · 2026-09-15T15:18:42Z

+                        </h1>
+                    </div>
+                    <group>
+


nitpick: when xml becomes bigger, it makes it easier to read if we get rid of these inside empty lines 😅
You can leave an empty line between records maybe but not preferably here :)

msho-odoo · 2026-09-15T15:20:57Z

+    state = fields.Selection(default='new', required=True, copy=False,
+                             selection=[
+                                 ('new', "New"),
+                                 ('offer_received', "Offer Received"),
+                                 ('offer_accepted', "Offer Accepted"),
+                                 ('sold', "Sold"),
+                                 ('cancelled', "Cancelled")
+                             ]
+                            )


Suggested change

state = fields.Selection(default='new', required=True, copy=False,

selection=[

('new', "New"),

('offer_received', "Offer Received"),

('offer_accepted', "Offer Accepted"),

('sold', "Sold"),

('cancelled', "Cancelled")

]

)

state = fields.Selection(

default='new',

required=True,

copy=False,

selection=[

('new', "New"),

('offer_received', "Offer Received"),

('offer_accepted', "Offer Accepted"),

('sold', "Sold"),

('cancelled', "Cancelled")

]

)

msho-odoo · 2026-09-15T15:21:09Z

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


EOF line :)

msho-odoo · 2026-09-15T15:23:37Z

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


No need to add the estate. before the id, it's implicit since it's in the same module.
I believe you should give unlink access to the user, here you're setting the perm_unlink to 0

msho-odoo · 2026-09-15T15:24:41Z

+    'depends': ['base'],
+    'data': [
+        'security/ir.model.access.csv',
+        'data/estate_property_views.xml',


The views file and the menus file should be in views folder not data folder

[ADD] estate: Add real estate module scafolding

b592011

task-6573598

ollet-odoo force-pushed the 19.0-server-tuto-ollet branch from 189c49d to b592011 Compare September 15, 2026 11:32

ollet-odoo changed the title ~~[add] estate: Add real estate module scafolding~~ ollet - Technical Training Sep 15, 2026

[IMP] estate: Add estate.property model

9282f17

task-6573598

ollet-odoo force-pushed the 19.0-server-tuto-ollet branch from 072738c to acc1335 Compare September 15, 2026 13:26

msho-odoo requested changes Sep 15, 2026

View reviewed changes

ollet-odoo added 9 commits September 15, 2026 16:52

[IMP] estate: add first access rule to estate_property

339767a

task-6573598

[IMP] estate: add Menu, form and list view

433fc25

task-6573598

[IMP] estate: Improve Property model

7a55a96

[CLN] estate: Menu, form and list view

703a614

task-6573598

[IMP] estate: Add list view

5ec1f59

[FIX] estate Property set active by default

b365a35

[IMP] estate: Property do not duplicate state

2dd8a20

[IMP] estate: Property form

8badc9b

[CLN] estate: Correct review comments

18d1370

ollet-odoo force-pushed the 19.0-server-tuto-ollet branch from 37f6405 to 18d1370 Compare September 15, 2026 14:53

ollet-odoo added 2 commits September 15, 2026 17:06

[IMPL] estate: display name and state in views

9d75d04

[IMP] estate: add search, filter and group by on property

c768c21

msho-odoo requested changes Sep 15, 2026

View reviewed changes

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

		@@ -0,0 +1 @@
		from .property import Property No newline at end of file

		garden_orientation = fields.Selection(
		selection=(('north', 'North'), ('east', 'East'), ('south', 'South'), ('west', 'West')))

		@@ -0,0 +1,2 @@
		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,0 No newline at end of file

	<menuitem id="estate_Advertisements_menu" name="Advertisements">
	<menuitem id="estate_advertisements_menu" name="Advertisements">

		@@ -0,0 +1,2 @@
		id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink
		estate.access_estate_property_user,access_estate_property,model_estate_property,base.group_user,1,1,1,0 No newline at end of file

Uh oh!

Conversation

ollet-odoo commented Sep 15, 2026

Uh oh!

robodoo commented Sep 15, 2026

Uh oh!

msho-odoo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

ollet-odoo commented Sep 15, 2026

Uh oh!

msho-odoo commented Sep 15, 2026

Uh oh!

msho-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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

msho-odoo left a comment •

edited

Loading