-
Notifications
You must be signed in to change notification settings - Fork 2.3k
[ADD] Training: server framework 101 #871
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 👋
Quite a nice PR here, good job!
It seems like there's a lot of stuff but don't worry about it, it's a lot of the same details that repeat themselves.
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 Selection field's selection
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!
.vscode/settings.json
Outdated
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.
That's not something you want to include in your commit 😄
estate/__init__.py
Outdated
@@ -0,0 +1,2 @@ | |||
|
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.
Useless blank line 👍
estate/__manifest__.py
Outdated
'name': 'Estate', | ||
'depends': ['base'], | ||
'application': True, | ||
'installable': True, | ||
'data': [ | ||
'security/ir.model.access.csv', | ||
'views/estate_property_views.xml', | ||
'views/estate_property_type_views.xml', | ||
'views/estate_property_tag_views.xml', | ||
'views/estate_property_offer_views.xml', | ||
'views/res_user_views.xml', | ||
'views/estate_menus.xml', | ||
] |
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.
Indentation always by groups of 4 spaces (
)
'name': 'Estate', | |
'depends': ['base'], | |
'application': True, | |
'installable': True, | |
'data': [ | |
'security/ir.model.access.csv', | |
'views/estate_property_views.xml', | |
'views/estate_property_type_views.xml', | |
'views/estate_property_tag_views.xml', | |
'views/estate_property_offer_views.xml', | |
'views/res_user_views.xml', | |
'views/estate_menus.xml', | |
] | |
'name': 'Estate', | |
'depends': ['base'], | |
'application': True, | |
'installable': True, | |
'data': [ | |
'security/ir.model.access.csv', | |
'views/estate_property_views.xml', | |
'views/estate_property_type_views.xml', | |
'views/estate_property_tag_views.xml', | |
'views/estate_property_offer_views.xml', | |
'views/res_user_views.xml', | |
'views/estate_menus.xml', | |
] |
estate/models/__init__.py
Outdated
|
||
from . import estate_property | ||
from . import estate_property_type | ||
from . import estate_property_tag | ||
from . import estate_property_offer | ||
from . import res_users |
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.
Useless blank line and then for __init__.py
, we always import in alphabetic order by convention
from . import estate_property | |
from . import estate_property_type | |
from . import estate_property_tag | |
from . import estate_property_offer | |
from . import res_users | |
from . import estate_property | |
from . import estate_property_offer | |
from . import estate_property_tag | |
from . import estate_property_type | |
from . import res_users |
estate/models/estate_property.py
Outdated
from odoo import api, fields, models, exceptions | ||
from odoo.exceptions import ValidationError |
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 only import exceptions to use exceptions.UserError
and you import ValidationError
from odoo.exceptions
too. Put them together 👍
from odoo import api, fields, models, exceptions | |
from odoo.exceptions import ValidationError | |
from odoo import api, fields, models | |
from odoo.exceptions import UserError, ValidationError |
<field name="date_deadline" string="Deadline Date"/> | ||
<button name="action_offer_accept" string="Accept" type="object" icon="fa-check" invisible="status"/> | ||
<button name="action_offer_refuse" string="Refuse" type="object" icon="fa-times" invisible="status"/> | ||
<!-- <field name="status"/> --> |
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.
👀
<!-- <field name="offer_ids"/> | ||
<field name="offer_count"/> --> |
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.
👀
estate_account/__init__.py
Outdated
@@ -0,0 +1,2 @@ | |||
|
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.
Useless blank line 😄
some other occurences later in the diff
estate_account/__manifest__.py
Outdated
@@ -0,0 +1,4 @@ | |||
{ | |||
'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.
name
is what's displayed in the Apps
app as title for your module. It's text for the user, write it like you would write an error message.
'name': 'Estate_Account', | |
'name': 'Estate Account', |
_inherit = "estate.property" | ||
|
||
def action_sold(self): | ||
# print(self.buyer_id) |
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.
👀
finished toturial [FIX] estate fixed format [FIX] estate: formating [FIX] estate: format errors [FIX] estate: format errors [FIX] estate: parse error [FIX] estate: parse error [ADD] estate: Server Framework 101 [ADD] estate: All features added [FIX] estate fixed format [FIX] estate: formating [FIX] estate: format errors [FIX] estate: format errors [FIX] estate: parse error [FIX] estate: parse error [FIX] estate: format errors Followed the changes in the pr [FIX] estate: syntax error in manifest
a49bb2e
to
ded334c
Compare
No description provided.