19.0 technical training coleo - #1422
coleo-odoo wants to merge 10 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
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. |
3 participants
No description provided.