-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
18.0 hello worl sdev #632
base: 18.0
Are you sure you want to change the base?
18.0 hello worl sdev #632
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.
Hello ! Here is a review, some picky remarks mainly regarding coding guidelines.
Try to keep your diff as clean as possible, even during development / WIP stages, it will help you knowing better what you did and will avoid noise :)
estate/models/tate_property
Outdated
Table "public.estate_property" | ||
Column | Type | Collation | Nullable | Default | ||
------------------+-----------------------------+-----------+----------+--------------------------------------------- | ||
id | integer | | not null | nextval('estate_property_id_seq'::regclass) | ||
number_of_months | integer | | not null | | ||
sequence | integer | | | | ||
create_uid | integer | | | | ||
write_uid | integer | | | | ||
name | jsonb | | not null | | ||
active | boolean | | | | ||
create_date | timestamp without time zone | | | | ||
write_date | timestamp without time zone | | | | ||
Indexes: | ||
"estate_property_pkey" PRIMARY KEY, btree (id) | ||
Foreign-key constraints: | ||
"estate_property_create_uid_fkey" FOREIGN KEY (create_uid) REFERENCES res_users(id) ON DELETE SET NULL | ||
"estate_property_write_uid_fkey" FOREIGN KEY (write_uid) REFERENCES res_users(id) ON DELETE SET NULL | ||
|
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.
I think you added an unnecessary file :)
estate/models/__init__.py
Outdated
@@ -0,0 +1 @@ | |||
from . import 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.
Global remark: we expect one empty line at the end of each file
estate/models/estate_property.py
Outdated
name = fields.Char(required = True) | ||
postcode = fields.Char() | ||
date_availability = fields.Date() | ||
expected_price = fields.Float(required = True) | ||
selling_price = fields.Float() | ||
bedrooms = fields.Integer() | ||
living_area = fields.Integer() | ||
facades = fields.Integer() | ||
garage = fields.Boolean() | ||
garden = fields.Boolean() | ||
garden_area = fields.Integer() |
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.
Always set the label of the field. even if it's obvious and even if the orm will generate one for you using the field name.
postcode = fields.Char("Postcode") or postcode = fields.Char(string="Postcode")
-> better the first in this case, unless string is not the first param. (typically for relational fields)
@@ -0,0 +1,7 @@ | |||
{ |
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 the version and a description of the module.
estate/models/estate_property.py
Outdated
from odoo import fields, models | ||
|
||
class EstateProperty(models.Model): | ||
_name = "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.
use '.' instead of '_' in model naming
<field name="bedrooms"/> | ||
<field name="living_area"/> | ||
<field name="facades"/> | ||
<filter string="Archived" name="available" domain="['|', ('state', '=', 'new'), ('state', '=', 'received')]"/> |
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 also use this syntax :
domain="[('state', 'in', ['new', 'received'])]"
<list string = "Channel"> | ||
<field name = "name"/> | ||
<field name = "postcode"/> | ||
<field name = "bedrooms"/> | ||
<field name = "living_area"/> | ||
<field name = "expected_price"/> | ||
<field name = "selling_price"/> | ||
<field name = "date_availability"/> |
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.
remove all unecessary white spaces
<record id="test_model_view_tree" model="ir.ui.view"> | ||
<field name="name">test.model.list</field> |
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.
guidelines : id and name should be identical (except with '.' and '_')
estate/models/estate_property.py
Outdated
state = fields.Selection([('new', 'New'), ('received', 'Offer Received'), ('accepted', 'Offer Accepted'), ('sold', 'Sold'), ('cancelled', 'Cancelled')], | ||
required = True, copy = False, default = '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.
state = fields.Selection([('new', 'New'), ('received', 'Offer Received'), ('accepted', 'Offer Accepted'), ('sold', 'Sold'), ('cancelled', 'Cancelled')], | |
required = True, copy = False, default = 'new') | |
state = fields.Selection([ | |
('new', 'New'), | |
('received', 'Offer Received'), | |
('accepted', 'Offer Accepted'), | |
('sold', 'Sold'), | |
('cancelled', 'Cancelled'), | |
], copy=False, default='new', required=True, string="Status") |
just a matter of coding style :)
estate/models/estate_property.py
Outdated
living_area = fields.Integer() | ||
facades = fields.Integer() | ||
garage = fields.Boolean() | ||
garden = fields.Boolean() | ||
garden_area = fields.Integer() |
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.
Always set the label of the field. even if it's obvious and even if the orm will generate one for you using the field name.
postcode = fields.Char("Postcode") or postcode = fields.Char(string="Postcode")
-> better the first in this case, unless string is not the first param. (typically for relational fields)
Completed till Chapter 6.