19.0 technical training coleo - #1422
coleo-odoo wants to merge 17 commits into
Conversation
This added the folder estate with __init_.py and __manifest__.py with minimal configuration to add an estate application
msho-odoo
left a comment
There was a problem hiding this comment.
Nice work 🔥
Well done on the commit messages format, just a nitpick you can ignore for now, that CLN tag is usually for heavy cleaning of the code base but usual small cleaning can have the IMP tag.
But it's worth to note that usually when you fix something after a review, no need to add a new commit, you can use git commit --amend which allows you to edit your commit message and also include your current staged changes to the same commit without making a new one.
Also left you some comments, and always remember to have your runbot checked when you push and make sure it's green :)
| @@ -0,0 +1,3 @@ | |||
| { | |||
There was a problem hiding this comment.
ops, I should't see that file :)
see how to avoid adding such environment files in git pushes
| bedrooms = fields.Integer() | ||
| living_area = fields.Integer() | ||
| facades = fields.Integer() | ||
| garage = fields.Boolean() |
There was a problem hiding this comment.
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
| garden_orientation = fields.Selection( | ||
| string='Type', | ||
| selection=[('north', 'North'), ('south', 'South'), ('east', 'East'), ('west', 'West')] | ||
| ) |
There was a problem hiding this comment.
The styling is fine by me but when we have multiple values it's better to have them on separate files.
| garden_orientation = fields.Selection( | |
| string='Type', | |
| 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'), | |
| ] | |
| ) |
nitpick: 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.
I see you are stick to single quotes, fine be me (though it's not always possible :) )
| @@ -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,1 No newline at end of file | |||
There was a problem hiding this comment.
Always add new line at the end of files.
no need to add the estate., the file is already in that module.
no need to add the double quotes as well, you can remove them.
Also as an improvement you can make the id access_estate_property_user (I added _user at the end) in case you want to add another rule on the same module but for the _manager not the base user
| @@ -0,0 +1,12 @@ | |||
| { | |||
| 'name': 'Real Estate', | |||
| 'author': 'coleo', | |||
|
@coleo-odoo just forgot to tell you, you can mention me here when you think the PR is ready for another review, typically, after each chapter :) |
Chapter 5 of tutorials
|
@msho-odoo I think the PR is ready for review. |
msho-odoo
left a comment
There was a problem hiding this comment.
Thanks for the nice work 👍
Left you a couple of comments.
nitpick: I would stick the commit message to be estate: create... instead of estate: created... since it should make a whole sentence when it's concatenated with this commit will .... according to git guidlines
| date_availability = fields.Date() | ||
| date_availability = fields.Date( | ||
| copy=False, | ||
| default=fields.Date.add(fields.Date.today(), months=3) |
There was a problem hiding this comment.
In this default the date will get valued when this is first imported and will not change later with each creation, you should use lambda function to fix this. Search the codebase for lambda functions used in the default values of fields and apply it here :)
| <menuitem id="estate_menu_action" action="estate_action" name="Properties"/> | ||
| </menuitem> | ||
| </menuitem> | ||
| </odoo> No newline at end of file |
There was a problem hiding this comment.
Always remember the EOF new line and Always remember to fix red runbot to be green before reviews :)
| @@ -0,0 +1,7 @@ | |||
| <odoo> | |||
| <menuitem id="estate_menu_root" name="Real Estate"> | |||
| <menuitem id="estate_first_level_menu" name="Advertisements"> | |||
There was a problem hiding this comment.
Better to make ids more descriptive so that they indicate what they are without having to read the definition
| <field name="view_mode">tree,form</field> | ||
| <field name="view_mode">list,form</field> | ||
| </record> | ||
| </odoo> No newline at end of file |
There was a problem hiding this comment.
EOF line in ALL files please: and remember the red runbot is already guiding you to fix this stuff so please check it before the review :)
| ('sold', 'Sold'), | ||
| ('cancelled', 'Cancelled') | ||
| ], | ||
| default="new", |
There was a problem hiding this comment.
Stick to a convention about quotes :)
| 'name': 'Real Estate', | ||
| 'author': 'Odoo S.A.', | ||
| 'license': 'LGPL-3', | ||
| 'depends': [ |
There was a problem hiding this comment.
nitpick: usually the depends list is written in one line since it's usually just small module names but no biggie
offer : compute date_deadline according to validity property : compute total_area according to garden_area and living_area compute best_price according to the best offer add on_change to has_garden filling or clearing fields accordingly
a2b4c84 to
98ccc2f
Compare
…ea and Garden Orientation
…rs and less than 90% of the expected price for the selling price
|
@msho-odoo The PR should be ready for review |

No description provided.