Skip to content

Conversation

@tskv
Copy link

@tskv tskv commented Nov 18, 2025

No description provided.

@robodoo
Copy link

robodoo commented Nov 18, 2025

Pull request status dashboard

Copy link

@yoba-odoo yoba-odoo left a comment

Choose a reason for hiding this comment

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

Impressive work 👏
Just some small nitpicks. Would also be great if we can squash the FIX commits to their parent IMP commits so we can have a clean commit history.

Keep up the good work 👌

""",
'data': [
'security/ir.model.access.csv',

Choose a reason for hiding this comment

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

No need for this empty line

active = fields.Boolean('Active', default=True)
description = fields.Text('Description')
postcode = fields.Char('Postcode')
date_availability = fields.Date('Available From', copy=False, default=fields.Datetime.now() + timedelta(days=90))

Choose a reason for hiding this comment

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

Suggested change
date_availability = fields.Date('Available From', copy=False, default=fields.Datetime.now() + timedelta(days=90))
date_availability = fields.Date('Available From', copy=False, default=fields.Date.add(fields.Date.today(), months=3)

What you are doing is correct as well but we have a helper in odoo to do the same so it is better to use it

id="properties_menu"
name="Properties"
parent="estate_menu_root"
action="estate.estate_property_action"

Choose a reason for hiding this comment

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

Suggested change
action="estate.estate_property_action"
action="estate_property_action"

Since we are in the same folder as the action we don't have to use estate. to call the action it should be read automatically

id="properties_type_menu"
name="Property Types"
parent="settings_menu"
action="estate.estate_property_type_action"

Choose a reason for hiding this comment

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

Suggested change
action="estate.estate_property_type_action"
action="estate_property_type_action"

Same

id="properties_tag_menu"
name="Property Tags"
parent="settings_menu"
action="estate.estate_property_tag_action"

Choose a reason for hiding this comment

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

Suggested change
action="estate.estate_property_tag_action"
action="estate_property_tag_action"

Same

@tskv tskv force-pushed the 19.0-tutorials-taskv branch 2 times, most recently from 1490adf to a786d01 Compare November 20, 2025 08:28
@tskv tskv force-pushed the 19.0-tutorials-taskv branch from a786d01 to 347453f Compare November 20, 2025 08:31
@tskv tskv force-pushed the 19.0-tutorials-taskv branch from bab10bf to d490741 Compare November 20, 2025 13:03
@tskv tskv force-pushed the 19.0-tutorials-taskv branch from 3aa1cb8 to 220923d Compare November 20, 2025 14:36
@tskv tskv force-pushed the 19.0-tutorials-taskv branch from e737a99 to b722b3e Compare November 20, 2025 16:11
Copy link

@antonrom1 antonrom1 left a comment

Choose a reason for hiding this comment

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

Good job! Mostly small comments. Keep it going like this for JS next week :P

Comment on lines +1 to +2
# Part of Odoo. See LICENSE file for full copyright and licensing details.

Choose a reason for hiding this comment

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

The odoo license in every file is not needed anymore. You can remove it!

Comment on lines +19 to +20
'demo': [
],

Choose a reason for hiding this comment

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

you don't need to specify demo if it's empty

Comment on lines +3 to +7
from . import estate_property
from . import estate_property_type
from . import estate_property_tag
from . import estate_property_offer
from . import res_users

Choose a reason for hiding this comment

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

You just need to sort all imports by alphabetical order like here
https://www.odoo.com/documentation/19.0/contributing/development/coding_guidelines.html#imports
Screenshot 2025-11-21 at 10 39 33

Comment on lines +1 to +2
# Part of Odoo. See LICENSE file for full copyright and licensing details.

Choose a reason for hiding this comment

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

Also you can remove that license, it's not needed anymore for any new odoo files

Comment on lines +25 to +30
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.

This indentation is a little bit strange, I would do something like this

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

if 'property_id' in vals and vals.get('property_id'):
current_property = self.env['estate.property'].browse(vals['property_id'])
if 'price' in vals and current_property.best_offer > vals.get('price', 0):
raise exceptions.ValidationError(f'The offer must be higher than {current_property.best_offer}')

Choose a reason for hiding this comment

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

Not a problem for the tutorial, but with an actual module, we usually don't use fstrings for errors, as you cannot translate these

Here is an example of how we usually format translated strings with parameters
https://github.com/odoo/odoo/blob/fc658b2e3d9c5afbfa5d00b579bf8928a90dab87/addons/barcodes/models/barcode_rule.py#L41

@@ -0,0 +1,17 @@
# Part of Odoo. See LICENSE file for full copyright and licensing details.

Choose a reason for hiding this comment

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

No need license

<field name="res_model">estate.property</field>
<field name="view_mode">list,form,kanban</field>
<field name="search_view_id" ref="estate_property_view_search"/>
<field name="context">{'search_default_filter_available':1}</field>

Choose a reason for hiding this comment

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

Keep the spacing consistent in XMLs too

Suggested change
<field name="context">{'search_default_filter_available':1}</field>
<field name="context">{'search_default_filter_available': 1}</field>

Comment on lines +11 to +14
'data': [
],
'demo': [
],

Choose a reason for hiding this comment

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

Don't need to define these if no data or demo


def action_property_sold(self):
invoice_vals_list = []
for record in self:

Choose a reason for hiding this comment

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

In super().action_property_sold you call self.ensure_one(), and here you suppose that it's an iterable. You should pick one.

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.

4 participants