Skip to content

Technical Training - alnav - #1413

Open
Kermut572 wants to merge 6 commits into
odoo:19.0from
odoo-dev:19.0-tutorials-alnav
Open

Kermut572 wants to merge 6 commits into
odoo:19.0from
odoo-dev:19.0-tutorials-alnav

Conversation

@Kermut572

Copy link
Copy Markdown

Creation of the required manifest and init files for the real estate module.

@robodoo

robodoo commented Sep 15, 2026

Copy link
Copy Markdown

Pull request status dashboard

@Kermut572 Kermut572 changed the title [ADD] Real estate module base files Technical Training - alnav Sep 15, 2026

@msho-odoo msho-odoo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great work!

Left you a couple of comments.
And for the commit message, it usually should align with if this commit is merged it will .... so you can write something like [ADD] estate: add estate property module or whatever message you think is better :)
refer to the Git Guidelines

Comment thread estate/__manifest__.py Outdated
'data': [],
# data files containing optionally loaded demonstration data
'demo': [],
} No newline at end of file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Please add a new line at the end of files :)
Also always refer to your runbot to address the styling errors ans the warnings

Comment thread estate/__manifest__.py Outdated
Comment on lines +11 to +14
# data files always loaded at installation
'data': [],
# data files containing optionally loaded demonstration data
'demo': [],

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 guess no need to add them if they are not used (unless it gave you warnings on the log?)

Comment thread estate/__manifest__.py Outdated
'name': "Real Estate",
'version': '1.0',
'depends': ['base'],
'author': "alnav",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

usually author is Odoo S.A. :)

@msho-odoo
msho-odoo marked this pull request as ready for review September 15, 2026 12:03

@msho-odoo msho-odoo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice work, left a couple of tiny comments

When you try to fix some code after reviewing, there is no need to create a new commit just for the fix, it makes the history unnecessarily crowded, try using git commit --amend instead of git commit, this command lets you edit the last commit and also pushes your current changes to it without making a new one :)

Comment thread estate/models/estate_property.py Outdated
bedrooms = fields.Integer("Number of Bedrooms")
living_area = fields.Integer("Living Area")
facades = fields.Integer("Number of Facades")
garage = fields.Boolean("Has a Garage")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

good practice for boolean fields to be is_something or has_something so when someone reads the name in the middle of the code know it's a boolean, so you might make it has_garden and has_garage

Comment thread estate/models/estate_property.py
Comment thread estate/models/estate_property.py Outdated
garage = fields.Boolean("Has a Garage")
garden = fields.Boolean("Has a Garden")
garden_area = fields.Integer("Garden Area")
garden_orientation = fields.Selection(string="Garden Orientation", selection=[('north', "North"), ("south", "South"), ("east", "East"), ("west", "West")])

@msho-odoo msho-odoo Sep 15, 2026

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
garden_orientation = fields.Selection(string="Garden Orientation", selection=[('north', "North"), ("south", "South"), ("east", "East"), ("west", "West")])
garden_orientation = fields.Selection(
string="Garden Orientation",
selection=[
("north", "North"),
("south", "South"),
("east", "East"),
("west", "West"),
]
)

@msho-odoo msho-odoo Sep 15, 2026

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 better and conventional styling at odoo for bracketed objects if you would say :)
nitpick regarding the quotes: some teams might stick to single quotes being only on technical strings (the ones that the user don't see) and double quotes are for the strings that the user can see)
so for example the selection can be ('east', "East") instead of ('east', 'East') but some teams don't do this, they just stick to their own convention which is all single quotes (if possible) or all double quotes so you can pick a convention and stick to it for now :)

@Kermut572
Kermut572 force-pushed the 19.0-tutorials-alnav branch 2 times, most recently from 4982cd4 to c5c3d74 Compare September 15, 2026 12:52
@msho-odoo

Copy link
Copy Markdown

@Kermut572 just forgot to tell you, you can mention me here when you think the PR is ready for another review, typically, after each chapter :)

@Kermut572

Copy link
Copy Markdown
Author

@Kermut572 just forgot to tell you, you can mention me here when you think the PR is ready for another review, typically, after each chapter :)

Alright I'll keep that in mind, thanks!

@Kermut572

Copy link
Copy Markdown
Author

@msho-odoo just finished chapter 5, PR is ready for review :)

@msho-odoo msho-odoo left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice work 💯
left you some comments

nitpick about the commit message, I would make it add instead of added just to align with guidelines because you would say This commit will add property model actions... not This commit will added property model actions but it's not a big thing, well done.
Also have a look at the runbot, see why ci/tutorials is giving you a warning ant try to solve it.
Keep up the good work 🔥

Comment thread estate/models/estate_property.py
Comment thread estate/models/estate_property.py
Comment thread estate/models/estate_property.py Outdated
descritpion = fields.Text("Property Description")
postcode = fields.Char("Postcode")

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Using default=fields.Date.add(fields.Date.today(), months=3) makes this only get valued first time it's imported and it's stuck there, you need to use lambda function instead, you will find multiple examples in the codebase, it's good practice to search for previous code there as well :)

Comment thread estate/views/estate_menus.xml Outdated
<?xml version="1.0"?>
<odoo>
<menuitem id="estate_menu_root" name="Real Estate">
<menuitem id="estate_menu_first_level" name="Advertisements">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's better to give your ids a more descriptive name so that it's are understood when it's read somewhere in the code without needing to go back to its definition

Comment thread estate/__manifest__.py Outdated
"application": True,
"data": [
"security/ir.model.access.csv",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

why the extra line :)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In the account_accountant enterprise module they have something like this
`'data/account_accountant_tour.xml',
'data/ir_config_parameters.xml',

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

'views/account_account_views.xml',
'views/account_fiscal_year_view.xml',
...`

I thought this was the standard way to format it, but looking at the other modules it seems to be more of an exception than a rule so I'll change it :D

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

okaay, maybe some teams stick to that if you saw it somewhere :)
but I think mostly teams don't :)

@Kermut572

Copy link
Copy Markdown
Author

@msho-odoo chapter 6 ready for review :)

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.

3 participants