[ADD] estate: Chapters 1-11 #873
Conversation
|
|
There was a problem hiding this comment.
Hello 👋
First of all, good job until now, here's a first review for you 😄
There's quite a lot of stuff but don't worry about it, it's a lot of the same details that repeat themselves.
I have commented on a few things that have been flagged by the runbot but not all of them so first things first once you decide to apply the changes I asked you should be to check the runbot ci/style and look at the errors it raises.
Then you can go through all my comments and apply what I suggested or bring your own alternative. Keep in mind that a review is not the utlimate solution, it's only someone seeing things he would have done differently and offering his alternative. You're always free to answer with an other alternative or even say you disagree and bring your arguments.
What I would suggest you do and continue to do while you're not the most comfortable with odoo's structure is to not apply all the changes at once. For example, changing a model's _name has a lot of impact and if you change everything at once, you will probably have a lot of errors when running you database. Splitting the review in multiple steps and trying to run your db in between those can save you a lot of time in the long run.
Also a quick tip for the methodology when applying reviews changes: What I like to do it put a reaction like a thumbsup on a message once I have made the change locally. Then when everything is marked with something, I push, go through my diff and mark as resolved the things that are now outdated so nothing is left forgotten.
If you have any question, don't hesitate to ask them!
estate/__init__.py
Outdated
| @@ -0,0 +1 @@ | |||
| from . import models | |||
There was a problem hiding this comment.
For multiple reasons, the main one being that we always want to keep the changes history as clean as possible, we always end files with a blank line so that if someone edits your file and add a line at the end, it doesn't modify your last line to add the line break character.
| from . import models | |
| from . import models | |
estate/__manifest__.py
Outdated
| { | ||
| 'name': "estate", | ||
| 'depends':['base'], | ||
| 'application': True, | ||
| 'data':['security/ir.model.access.csv', | ||
| 'views/estate_property_views.xml', | ||
| 'views/estate_property_tag_views.xml', | ||
| 'views/estate_property_offer_views.xml', | ||
| 'views/estate_property_type_views.xml', | ||
| 'views/estate_property_menus.xml', | ||
| 'views/inherited_model_view.xml' | ||
|
|
||
|
|
||
| ] | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
Indentation is super important for readability so we always indent files in a pythonish way even in file types that don't require it.
| { | |
| 'name': "estate", | |
| 'depends':['base'], | |
| 'application': True, | |
| 'data':['security/ir.model.access.csv', | |
| 'views/estate_property_views.xml', | |
| 'views/estate_property_tag_views.xml', | |
| 'views/estate_property_offer_views.xml', | |
| 'views/estate_property_type_views.xml', | |
| 'views/estate_property_menus.xml', | |
| 'views/inherited_model_view.xml' | |
| ] | |
| } | |
| { | |
| 'name': "estate", | |
| 'depends':['base'], | |
| 'application': True, | |
| 'data':[ | |
| 'security/ir.model.access.csv', | |
| 'views/estate_property_views.xml', | |
| 'views/estate_property_tag_views.xml', | |
| 'views/estate_property_offer_views.xml', | |
| 'views/estate_property_type_views.xml', | |
| 'views/estate_property_menus.xml', | |
| 'views/inherited_model_view.xml' | |
| ] | |
| } |
estate/models/__init__.py
Outdated
| @@ -0,0 +1 @@ | |||
| from . import estate_property, estate_property_type, estate_property_tag, estate_property_offer, inherited_model | |||
There was a problem hiding this comment.
Missing EOL (End Of Line character (empty last line at the end of your file))
estate/models/estate_property.py
Outdated
| from odoo import models, fields, api | ||
| from odoo.exceptions import UserError, ValidationError | ||
| from odoo.tools import float_compare, float_is_zero |
There was a problem hiding this comment.
The guidelines for the imports notably asks to sort all import (sources and imported elements alphabtically) so:
| from odoo import models, fields, api | |
| from odoo.exceptions import UserError, ValidationError | |
| from odoo.tools import float_compare, float_is_zero | |
| from odoo import api, fields, models | |
| from odoo.exceptions import UserError, ValidationError | |
| from odoo.tools import float_compare, float_is_zero |
| from odoo import models, fields, api | ||
| from odoo.exceptions import UserError, ValidationError | ||
| from odoo.tools import float_compare, float_is_zero | ||
|
|
There was a problem hiding this comment.
The runbot ci/style probably says that but we always have two blank lines between the imports and the models
| @@ -0,0 +1,72 @@ | |||
| <?xml version="1.0"?> | |||
There was a problem hiding this comment.
Lots of blank lines that shouldn't be here in this file. 😄
|
|
||
|
|
||
| <record id="estate_property_tag_list_view" model="ir.ui.view"> | ||
| <field name="name">List View</field> |
There was a problem hiding this comment.
I changed it to Property Tags List View
| <field name="context"> | ||
| {'search_default_Available': True} | ||
| </field> |
There was a problem hiding this comment.
You don't need to put this field on multiple lines but if you do, one more indent when opening a group.
| <field name="context"> | |
| {'search_default_Available': True} | |
| </field> | |
| <field name="context"> | |
| {'search_default_Available': True} | |
| </field> |
or keep it simple
| <field name="context"> | |
| {'search_default_Available': True} | |
| </field> | |
| <field name="context">{'search_default_Available': True}</field> |
| </record> | ||
|
|
||
|
|
||
| </odoo> |
| </xpath> | ||
| </field> | ||
| </record> | ||
| </odoo> |
No description provided.