19.0 tutorial delje - #1415
19.0 tutorial delje#1415Delvaux-Jean-Baptiste wants to merge 6 commits into
Conversation
|
|
There was a problem hiding this comment.
@Delvaux-Jean-Baptiste
Nice job on the branch situation 🔥
For the commit message we write in a way that aligns with when this commit is merged it will .....
so you might change [IMP] estate: Added models to something like [IMP] estate: add estate property model or however you like to describe your changes :)
refer to Git Guidlines
but really nice job, well done 👍
also left some comments for you to check
| @@ -0,0 +1,25 @@ | |||
| from odoo import fields, models | |||
|
|
|||
There was a problem hiding this comment.
Before class ... we conventionally leave 2 lines :)
| 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
| facades= fields.Integer() | ||
| garage= fields.Boolean() | ||
| garden= fields.Boolean() | ||
| garden_orientation= fields.Selection(string='Type', |
There was a problem hiding this comment.
It's good, but it's better to commit to a convention and follow it all the way, so for example in this field you wrote the attribute string before ='Type' but you didn't do that in name field Char('Estate Name')
| garden_orientation= fields.Selection(string='Type', | ||
| selection=[('east', 'East'), | ||
| ('west', 'West'), | ||
| ('north', 'North'), | ||
| ('south', 'South') | ||
| ] | ||
| ) |
There was a problem hiding this comment.
It's correct but we style it like this (see the suggestion).
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
| garden_orientation= fields.Selection(string='Type', | |
| selection=[('east', 'East'), | |
| ('west', 'West'), | |
| ('north', 'North'), | |
| ('south', 'South') | |
| ] | |
| ) | |
| garden_orientation= fields.Selection( | |
| string='Type', | |
| selection=[ | |
| ('east', 'East'), | |
| ('west', 'West'), | |
| ('north', 'North'), | |
| ('south', 'South'), | |
| ] | |
| ) |
| _name = "estate_property" | ||
| _description = "The details of a property" | ||
|
|
||
| name= fields.Char('Estate Name', required= True) |
There was a problem hiding this comment.
We do spaces before and after the operator so it's name = fields not name= fields.
Better to check all style errors in your runbot and fix it :)
The log there provides a description for the reason of the error
[ADD] estate: add estate access for base users
|
@Delvaux-Jean-Baptiste please mention me here when you think the PR is ready for another review, typically, after each chapter :) |
added fields and default values in fields of new estate [IMP] estate: updated gitignore to exclude vs config
|
@msho-odoo I believe I am ready for the last review of the day ;-) |
3 participants
[REF] Updated branch name
[FIX] Added extra end of file line