-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[ADD] estate: Server Framework 101 implementation #789
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
base: 18.0
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! And you're fast !
Here are some notes I suggest you to apply here and there in your code to fit Odoo's standards.
My comments are basically about readability and Git.
Don't forget to keep an eye on the runbot and apply needed changes to keep it green. The runbot is quite explicit and you can find your build here
🚀
estate/__manifest__.py
Outdated
"name": "Real Estate", | ||
"version": "1.0", | ||
"description": "Real Estate Management System", | ||
"summary": "Real Estate Management System", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"name": "Real Estate", | |
"version": "1.0", | |
"description": "Real Estate Management System", | |
"summary": "Real Estate Management System", | |
'name': 'Real Estate', | |
'version': '1.0', | |
'description': "Real Estate Management System", | |
'summary': 'Real Estate Management System', |
Prefer using double quotes ("") for long strings, multiline or user inputs/displays.
estate/__manifest__.py
Outdated
"version": "1.0", | ||
"description": "Real Estate Management System", | ||
"summary": "Real Estate Management System", | ||
"author": "Lud0do1202", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"author": "Lud0do1202", | |
'author': 'Odoo', |
Usually, we don't add the author on modules.
If you really want to keep it clean, please use "Odoo" (or "Odoo SA", ...) as your work for Odoo.
estate/__manifest__.py
Outdated
"license": "LGPL-3", | ||
"depends": ["base"], | ||
"data": [ | ||
# Security |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Security |
Avoid redundant information. The path already shows it's about security.
estate/__manifest__.py
Outdated
"license": "LGPL-3", | ||
"depends": ["base"], | ||
"data": [ | ||
# Security | ||
"security/ir.model.access.csv", | ||
# Views | ||
"views/estate_property_tag.xml", | ||
"views/estate_property_type.xml", | ||
"views/estate_property_offer.xml", | ||
"views/estate_property.xml", | ||
"views/estate_menus.xml", | ||
], | ||
"auto_install": False, | ||
"application": True, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"license": "LGPL-3", | |
"depends": ["base"], | |
"data": [ | |
# Security | |
"security/ir.model.access.csv", | |
# Views | |
"views/estate_property_tag.xml", | |
"views/estate_property_type.xml", | |
"views/estate_property_offer.xml", | |
"views/estate_property.xml", | |
"views/estate_menus.xml", | |
], | |
"auto_install": False, | |
"application": True, | |
} | |
'license': 'LGPL-3', | |
'depends': ['base'], | |
'data': [ | |
# Security | |
'security/ir.model.access.csv', | |
# Views | |
'views/estate_property_tag.xml', | |
'views/estate_property_type.xml', | |
'views/estate_property_offer.xml', | |
'views/estate_property.xml', | |
'views/estate_menus.xml', | |
], | |
'auto_install': False, | |
'application': True, | |
} |
estate/models/estate_property.py
Outdated
|
||
class EstateProperty(models.Model): | ||
_name = "estate.property" | ||
_description = "estate.property" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A description should be a bit more readable.
@@ -0,0 +1,13 @@ | |||
<odoo> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<odoo> | |
<?xml version="1.0" encoding="utf-8"?> | |
<odoo> |
Please, keep the XML declaration as it may cause issues with some parsers
estate/views/estate_menus.xml
Outdated
@@ -0,0 +1,13 @@ | |||
<odoo> | |||
<!-- Root --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- Root --> |
The menuitem
's id
is explicit.
estate/views/estate_menus.xml
Outdated
<menuitem id="settings_menu" name="Settings" parent="real_estate_root_menu" sequence="20"/> | ||
<menuitem id="settings_property_types_menu" name="Property Types" action="estate_property_types_action" parent="settings_menu" sequence="10"/> | ||
<menuitem id="settings_property_tags_menu" name="Property Tags" action="estate_property_tags_action" parent="settings_menu" sequence="10"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<menuitem id="settings_menu" name="Settings" parent="real_estate_root_menu" sequence="20"/> | |
<menuitem id="settings_property_types_menu" name="Property Types" action="estate_property_types_action" parent="settings_menu" sequence="10"/> | |
<menuitem id="settings_property_tags_menu" name="Property Tags" action="estate_property_tags_action" parent="settings_menu" sequence="10"/> | |
<menuitem id="settings_menu" name="Settings" | |
<menuitem id="settings_property_types_menu" name="Property Types" action="estate_property_types_action"/> | |
<menuitem id="settings_property_tags_menu" name="Property Tags" action="estate_property_tags_action"/> | |
</menuitem> |
Nesting menus make it more readable.
estate/views/estate_property.xml
Outdated
@@ -0,0 +1,123 @@ | |||
<odoo> | |||
<!--========= LIST =========--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!--========= LIST =========--> |
The record
's `ids explicit.
<field name="price"/> | ||
<field name="partner_id"/> | ||
<field name="status"/> | ||
<!-- ???? Cannot update a badge value ???? --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- ???? Cannot update a badge value ???? --> |
Avoid sending questions in your code. Write a message to the person you want to ask the question, go discuss with that person or exchange on it in the Conversation
section of your PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check all your files, the newline is missing in some of them and required to get runbt green ✅
estate/security/ir.model.access.csv
Outdated
estate.access_estate_property,access_estate_property,estate.model_estate_property,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_type,access_estate_property_type,estate.model_estate_property_type,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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
estate.access_estate_property_offer,access_estate_property_offer,estate.model_estate_property_offer,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 | |
Always newline character at the end of your files for UNIX compatibility.
@Lud0do1202 |
c98ef99
to
7ce73f4
Compare
@Lud0do1202 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lud0do1202
Great changes integration!
Some comments though you'll find below and can you please rename your files to match standards? You can find the conventions here
.gitignore
Outdated
# Style | ||
pyproject.toml | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't push your local configuration
estate/views/estate_menus.xml
Outdated
<odoo> | ||
<menuitem id="real_estate_root_menu" name="Real Estate" sequence="10"/> | ||
|
||
<!-- Advertissements --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<!-- Advertissements --> |
The name of the menu seems to be explicit
estate/views/estate_property.xml
Outdated
<!-- Title --> | ||
<h1> | ||
<field name="name" class="mb16"/> | ||
</h1> | ||
<group class="mb-2"> | ||
<field name="tag_ids" widget="many2many_tags" options="{'color_field': 'color'}" nolabel="1"/> | ||
</group> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is the title, try to use oe_title
class so it will follow future Odoo UI changes.
estate/models/estate_property.py
Outdated
copy=False, | ||
default=lambda self: fields.Date.today() + relativedelta(months=3), | ||
) | ||
expected_price = fields.Float(string='Expected Price', required=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expected_price = fields.Float(string='Expected Price', required=True) | |
expected_price = fields.Float('Expected Price', required=True) |
FYI, you can bypass the explicit string
parameter naming if set first as per Field
's definition
Up to you.
estate/views/estate_property.xml
Outdated
<field name="expected_price" string="Minimal Price" filter_domain="[('expected_price', '>=', self)]"/> | ||
<field name="expected_price" string="Maximal Price" filter_domain="[('expected_price', '<=', self)]"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try harmonizing your code by following the same conventions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does "harmonizing" here mean using >= and <= for XML safety ?
Because that’s what I did — let me know if you meant something else!
self.env['account.move'].create( | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.env['account.move'].create( | |
{ | |
self.env['account.move'].create({ |
Depending on the team.
Up to you.
@Lud0do1202 |
val_price = val.get('price') | ||
val_property_id = val.get('property_id') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avec you checked the behavior is correct when val_price
and val_property_id
are null ?
Maybe worth writing a test to ensure it never happens 🧐
estate/models/estate_property.py
Outdated
###### CRUD ###### | ||
def unlink(self): | ||
if any(record.state not in ['new', 'cancelled'] for record in self): | ||
raise UserError('You cannot delete a property that is not new or cancelled.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise UserError('You cannot delete a property that is not new or cancelled.') | |
raise UserError(self.env._('You cannot delete a property that is not new or cancelled.')) |
As Séna would say:
In Odoo, translatable strings in "data"-type content are automatically exported for translation. However, other user-facing strings, Odoo cannot automatically know whether they are translatable. To ensure these strings are exported for translation, you need to explicitly mark them. This is achieved by wrapping the literal string with the self.env._() function. The _() function signals to Odoo's translation system that the string should be included in the translation catalog.
More info here: https://www.odoo.com/documentation/18.0/developer/howtos/translations.html#explicit-exports
You can check this line here:
https://github.com/odoo/odoo/blob/05cff3b7d866f6bc95c4b32f343ae14a4da946f2/addons/account/models/account_move.py#L2075
- Create models for properties, types, tags, and offers - Add views: form, list, kanban, search with filters and grouped displays - Implement business logic: compute fields, onchanges, actions (sold, cancel, accept, refuse), constraints (SQL and Python) - Use field attributes: readonly, invisible, optional - Apply UI enhancements: decorations, badges, smart buttons - Add seller-based property listings - Prevent deletion of properties unless in allowed states - Restrict offer creation to higher prices than existing ones - Add default ordering on models - Create estate_account module to generate an invoice upon property sale
760e5cd
to
00c56fc
Compare
Server Framework 101
This PR follows the official Odoo backend tutorial:
🔗 https://www.odoo.com/documentation/18.0/developer/tutorials/server_framework_101.html
✅ Summary of features implemented:
Create models:
EstateProperty
,EstatePropertyType
,EstatePropertyTag
,EstatePropertyOffer
Add views: form, list, kanban, and search (with filters and groupings)
Implement business logic:
UI improvements:
success
,danger
, etc.)readonly
,invisible
,optional
)Seller experience:
Set default ordering on models
Add a new module:
estate_account