-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[ADD] estate: tutorial chapter 1-11 #877
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.
Hello 👋
Really nice PR here, very good job so far! 👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing EOL (End Of Line character). We always add a blank line at then end of every file so that someone adding a new line is not overwriting the last editor of the previous (your) last line. This is just to keep diffs minimals and keep a clean history.
estate/models/estate_property.py
Outdated
from odoo import models, fields, api, exceptions | ||
from odoo.tools import float_utils |
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.
We never import exeptions
. We tend to import the exceptions we need instead.
Also the import are always stored alphabetically. This is the kind of things found in the coding guidelines which is a page that will help you a lot during your time at odoo 👍
from odoo import models, fields, api, exceptions | |
from odoo.tools import float_utils | |
from odoo import api, fields, models | |
from odoo.exceptions import AccessError, UserError, ValidationError | |
from odoo.tools import float_utils |
copy=False, | ||
required=True, | ||
) | ||
|
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.
This is really a nitpick but we don't usually put spaces inside the fields list. It's not that big of a deal but depending on the team you end up going to, it might be something you'll have systematic comments on. 👍
for record in self: | ||
maxi = 0 | ||
for line in record.offer_ids: | ||
maxi = line.price if line.price > maxi else maxi | ||
record.best_price = maxi |
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 use max()
instead altough this is totally correct so feel free to keep it if you prefer it.
for record in self: | |
maxi = 0 | |
for line in record.offer_ids: | |
maxi = line.price if line.price > maxi else maxi | |
record.best_price = maxi | |
for record in self: | |
record.best_price = max(offer.price for offer in record.offer_ids) |
estate/models/estate_property.py
Outdated
if ( | ||
len(record.offer_ids) != 0 | ||
and float_utils.float_compare( | ||
record.selling_price, | ||
0.9 * record.expected_price, | ||
precision_digits=5, | ||
) | ||
== -1 | ||
): | ||
raise exceptions.ValidationError( | ||
r"The selling price cannot be lower than 90% of the expected 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.
We don't really care about line length in the python files so you can go and put that on two lines.
if ( | |
len(record.offer_ids) != 0 | |
and float_utils.float_compare( | |
record.selling_price, | |
0.9 * record.expected_price, | |
precision_digits=5, | |
) | |
== -1 | |
): | |
raise exceptions.ValidationError( | |
r"The selling price cannot be lower than 90% of the expected price" | |
) | |
if len(record.offer_ids) != 0 and float_utils.float_compare(record.selling_price, 0.9 * record.expected_price, precision_digits=5) == -1: | |
raise exceptions.ValidationError("The selling price cannot be lower than 90% of the expected price.") |
estate/views/res_users_views.xml
Outdated
|
||
</record> | ||
</data> | ||
</odoo> |
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 EOL
estate_account/__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 EOL
estate_account/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.
Missing EOL
estate_account/__manifest__.py
Outdated
@@ -0,0 +1,7 @@ | |||
{ | |||
"name": "Estate_account", |
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 name
attribute in the manifest is the title of the module in the Apps app so it it text like you put in errors and all.
"name": "Estate_account", | |
"name": "Estate Account", |
from odoo import models, Command | ||
|
||
|
||
class InheritedProperty(models.Model): |
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.
Like previously, inherit model is the same name as the parent one.
class InheritedProperty(models.Model): | |
class EstateProperty(models.Model): |
No description provided.