-
Notifications
You must be signed in to change notification settings - Fork 2.8k
19.0 Estate tutorial (VIKRI) #1078
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: 19.0
Are you sure you want to change the base?
Conversation
barracudapps
left a comment
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.
Some comments here and there. Can you also please review your PR title and description following our guidelines (https://www.odoo.com/documentation/19.0/contributing/development/git_guidelines.html)?
A clear commit message is really important to describe what you did and why you did it.
real_estate/__init__.py
Outdated
| @@ -0,0 +1 @@ | |||
| from . import models No newline at end of file | |||
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 forget Unix conventions on text files' structure based on a line definition (cf. https://peps.python.org/pep-0008/ and https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206)
| from . import models | |
| from . import models | |
Please review all your files
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.
thanks for the documentation clear. I have added ruff now so it'll do the formatting from now on.
real_estate/__manifest__.py
Outdated
| 'version': '0.1', | ||
|
|
||
| 'application': True, | ||
| 'installable': 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.
installable is true by default (at least with application) so not required
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.
removed installable and used application only as suggested.
real_estate/__manifest__.py
Outdated
| # Categories can be used to filter modules in modules listing | ||
| # Check https://github.com/odoo/odoo/blob/15.0/odoo/addons/base/data/ir_module_category_data.xml | ||
| # for the full 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.
Not needed here as obvious, just remove these lines
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.
removed the unused comment lines
real_estate/__manifest__.py
Outdated
| 'summary': """ | ||
| This is a custom real estate application for understanding 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.
Try to keep as much single-lines as possible. Same further.
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 mentioned, since it's just one line made one liner string.
real_estate/models/estate.py
Outdated
|
|
||
| class Estate(models.Model): | ||
| _name = 'estate' | ||
| _description = 'Real_Estate' |
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.
Use double quotes for non-technical strings
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.
got it and made the non-technical strings in single quotes
real_estate/views/estate_menu.xml
Outdated
|
|
||
| <menuitem id="estate_first_level_menu" name="Advertisements" parent="estate_menu_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.
| <menuitem id="estate_first_level_menu" name="Advertisements" parent="estate_menu_root"/> | |
| <menuitem id="estate_first_level_menu" name="Advertisements" parent="estate_menu_root"/> |
Avoid useless spaces. Here, the file is still readable without spaces
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.
thanks will removed the spaces and extra lines.
website_airproof/data/presets.xml
Outdated
| <field name="active" eval="False"/> | ||
| </record> | ||
| <!-- Enable product categories in the left // View template in: odoo/addons/website_sale/views/templates.xml --> | ||
| <!-- Enable product categories in the left // View template in: odoo/addons/website_sale/views/estate_templates.xml --> |
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.
This can be moved to the commit message if you want to explicitly explain your choice
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.
it was a mistake from my end and reverted the changes in this line
Completed till chapter 7 many to one
Added new tags model to understand many2many
| postcode = fields.Char(string="Postcode") | ||
| date_availability = fields.Datetime(string="Date Availability", default=_set_default_start_date(), copy=False) | ||
| date_availability = fields.Datetime( | ||
| string="Date Availability", default=_set_default_start_date(), copy=False |
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.
Add a trailing coma here so if someone has to add a new parameter on a new line, that person won't have to change your line (and change the git history)
real_estate/data/res_estate_data.xml
Outdated
| <field name="description">Description 3</field> | ||
| <field name="postcode">1040</field> | ||
| <field name="date_availability" eval="DateTime.now()" /> | ||
| <field name="date_availability" eval="DateTime.now()"/> |
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.
date_availability is already pre-defined in your model with your default value
| state = fields.Selection( | ||
| string="State", | ||
| selection=[ | ||
| ("new", "New"), |
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.
Use double quotes (") only for non-technical strings.
Please check all your files
| access_estate_property,access.estate.property,model_estate_property,base.group_user,1,1,1,1 | ||
| access_estate_property_type,access.estate.property.type,model_estate_property_type,base.group_user,1,1,1,1 | ||
| access_estate_property_type,access.estate.property.type,model_estate_property_type,base.group_user,1,1,1,1 | ||
| access_estate_property_tags,access.estate.property.tags,model_estate_property_tags,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.
Don't forget Unix conventions on text files' structure based on a line definition (cf. https://peps.python.org/pep-0008/ and https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206)
Check other files too
real_estate/views/estate_menu.xml
Outdated
| <menuitem | ||
| id="estate_menu_action" | ||
| action="test_model_action" | ||
| parent="estate_menu_root" | ||
| sequence="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.
You can keep this on a single line. It's not always easy to know when to write multiple lines but the basic rule is that you try to optimize readability AND number of lines
| <?xml version="1.0"?> | ||
| <odoo> | ||
| <!-- Root menuitem action which will help us to see the default action window--> | ||
| <!-- Root menuitem action which will help us to see the default action window--> |
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.
This comment is useless as the majority of the developers will easily know what your code is doing
Check the style guide and resolve the issues

Here I have created new add-on for on-boarding tutorial project