-
Notifications
You must be signed in to change notification settings - Fork 2.6k
MATHO Onboarding #999
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: 19.0
Are you sure you want to change the base?
MATHO Onboarding #999
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.
Hi @matho-odoo 👋,
Here is a first review of your code. There are some little things to correct. It is almost about odoo coding guidelines. The naming is very important because it helps use to understand the main topic of the code. For example, having a file named estate_property_tag_views will help us to understand that the file only contains views related to the estate.property.tag model.
And you should add a title and a title to your PR. The title must have the same structure than your commits ->[IMP/ADD/... ] app: what the PR is doing. And for the description and should be mainly why you do the PR not how you do it, it is the same for the commits.
Move at your own pace, don't care about the one of the others. It seems that you want to understand what going on inside the framework code and that's very good!
Thanks for you job 😃 !
estate/models/__init__.py
Outdated
@@ -0,0 +1 @@ | |||
from . import estate_property No newline at end of file |
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.
The blank line at the end of the file is missing. All your files must have one 😃.
estate/views/estate_menus.xml
Outdated
</menuitem> | ||
</menuitem> | ||
</data> | ||
</odoo> No newline at end of file |
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.
</record> | ||
|
||
</data> | ||
</odoo> No newline at end of file |
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/__init__.py
Outdated
@@ -0,0 +1 @@ | |||
from . import models No newline at end of file |
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/__manifest__.py
Outdated
'views/estate_property_views.xml', | ||
'views/estate_menus.xml', | ||
] | ||
} No newline at end of file |
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.
<field name="search_view_id" ref="perso_recherche"/> | ||
</record> | ||
|
||
<record id="perso_colonnes" model="ir.ui.view"> |
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.
The id should be estate_property_view_form. The naming for the view is <model_name>view<view_type>.
<field name="selling_price"/> | ||
<field name="date_availability"/> | ||
</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.
Be careful with the indentation 😃
</sheet> | ||
</form> | ||
</field> | ||
</record> |
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.
Something fail with the indentation. It comes from the starting form tag. The code underneath is also impacted.
</field> | ||
</record> | ||
|
||
<record id="perso_recherche" model="ir.ui.view"> |
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.
The id should be estate_property_search.
estate/__manifest__.py
Outdated
# -*- coding: utf-8 -*- | ||
# Part of Odoo. See LICENSE file for full copyright and licensing details. | ||
|
||
|
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.
Those lines are not required anymore. You can still see it in a lot of manifests but it is because they have been created long time ago.
No description provided.