-
Notifications
You must be signed in to change notification settings - Fork 2.2k
[ADD] estate: adding new estate module #727
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: 18.0
Are you sure you want to change the base?
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.
Great first PR! Some remarks though 👍
estate/__init__.py
Outdated
@@ -0,0 +1 @@ | |||
from . import models |
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.
Missing blank line at the end of your file.
estate/__manifest__.py
Outdated
'depends': [ | ||
'base', | ||
] | ||
} |
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.
Same here.
estate/models/estate_property.py
Outdated
garden_area = fields.Integer() | ||
garden_orientation = fields.Selection( | ||
string='Orientation', | ||
selection=[('north','North'), ('south','South'), ('east', 'East'), ('west', 'West')] |
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.
selection=[('north','North'), ('south','South'), ('east', 'East'), ('west', 'West')] | |
selection=[('north', 'North'), ('south', 'South'), ('east', 'East'), ('west', 'West')] |
Add manifest and dependencies...
fd89ef7
to
0b23021
Compare
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're probably need a linter in your IDE to avoid these kind of errors. See below.
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.
Ok for me. Just a little typo :)
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.
Good! 👍
@naja628 For you :) |
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 :) -- Good work !
You have one commit left whose title doesn't conform with the guidelines.
I left some more comments on your code
estate/models/estate_property.py
Outdated
name = fields.Char(required=True) | ||
description = fields.Text() | ||
postcode = fields.Char() | ||
date_availability = fields.Date(copy=False, default=du.add(date.today(), months=3)) |
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.
Here date.today()
will be evaled once when the whole class is evaled (i.e. everytime you launch odoo i think).
When it needs to be re-evaled everytime you create new records you can pass a lambda or function to the default
kwarg instead
d06d33d
to
313ec7d
Compare
Added a functional module to create new listings following all the chapter 5 guidelines.
Chapter 6 views: search, list, form done
Repo now is up to date with chapter 7
Added an onchange field and some compute fields, following Chapter 8 rules. Also refactored different views to separate them
Added actions as mentioned in chapter 9 did some refactoring to follow latest comments
313ec7d
to
3a9a5a2
Compare
Added constraints to follow chapter 10 Refactored to fix a bug in chapter 9
Made some adjustments to follow chapter 11 guidelines
3108bd3
to
6316118
Compare
Repo now represents edits made to follow chapter 12 guidelines, inherited view and class created and CRUD modifications were made
1688c2c
to
6f9cc29
Compare
estate_account linking module created, now follows chapter 13 guidelines
99c4242
to
3c56016
Compare
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 -- good work :)
I left some comments
def create(self, vals): | ||
for val in vals: | ||
single_property = self.env["estate.property"].browse(val["property_id"]) | ||
if val["price"] < single_property.best_price: |
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.
here price may not always be in the vals. You could use .get
Guidelines follow chapter 14
0a1d15b
to
203b4b9
Compare
45f1d4e
to
bbd1d95
Compare
Add manifest and dependencies...