[WIP] Implementation of estate module - #1372
Conversation
|
|
YassinWalid
left a comment
YassinWalid
left a comment
There was a problem hiding this comment.
Thanks for the incredible work. I have added an initial review, with some comments on the PR, and other general comments below.
For the naming convention of models, the record id in xml, or the id & name of records in the security access csv file, you can have a look at the coding guidelines.
Don't forget the empty line at the end of each file, to have better code traceability.
There was a problem hiding this comment.
you should put each file on its own line, since this is better for the history (git blame) of the file
There was a problem hiding this comment.
check the coding guidelines for the name here
|
|
||
|
|
||
| class Estate(models.Model): | ||
| _name = "estate_property" |
There was a problem hiding this comment.
check the coding guidelines for the names of models (as well as the name of classes, and the naming of files).
(this also applies for other models)
| raise UserError("Cancelled properties cannot be sold") | ||
| property.state = "sold" | ||
|
|
||
| return True |
There was a problem hiding this comment.
| return True |
You don't need to explicitly return True in actions
| def action_sell_property(self): | ||
| for property in self: | ||
| if property.state == "cancelled": | ||
| raise UserError("Cancelled properties cannot be sold") |
There was a problem hiding this comment.
| raise UserError("Cancelled properties cannot be sold") | |
| raise UserError(self.env._("Cancelled properties cannot be sold")) |
Don't forget to translate your errors
| self.property_id.selling_price = 0 | ||
| self.property_id.buyer_id = None |
There was a problem hiding this comment.
but what if another offer was already accepted? wouldn't this cause wrong data?
| <odoo> | ||
| <menuitem id="menu_estate_root" name="Real Estate"> | ||
| <menuitem id="menu_estate_property" name="Advertisements" action="estate_property_action"/> | ||
| <menuitem id="menu_estate_property_types" name="Settings"> |
There was a problem hiding this comment.
I think the id of this menuitem might not be as representative of what it holds? (also, check the guidelines for the ids of the records)
| <filter name="New/Offer Received" domain="['|', ('state', '=', 'new'), ('state', '=', 'offer_received')]"></filter> | ||
| <separator/> | ||
| <filter name="New/Offer Received2" domain="['|', ('state', '=', 'new'), ('state', '=', 'offer_received')]"></filter> | ||
| <filter name="New/Offer Received3" domain="['|', ('state', '=', 'new'), ('state', '=', 'offer_received')]"></filter> |
There was a problem hiding this comment.
why do we need three identical filters here?
| "name": "Real Estate", | ||
| "version": "1.0.0", | ||
| "depends": ["base"], | ||
| "author": "Amr (amgom)", |
There was a problem hiding this comment.
this is ok since this is only training, but normally it should be Odoo S.A.
There was a problem hiding this comment.
This should not be included here, maybe make git ignore it?
3 participants
To be filled when finishing...
Tasks