-
Notifications
You must be signed in to change notification settings - Fork 2.2k
First commit #823
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?
First commit #823
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.
Hey! Good job for this PR, I have left some comments for you to fix 🎉
I'd say the biggest thing you need to pay attention to is your consistency and overall code cleanliness. Sometimes you place spaces, sometimes you don't, you put upper case when you shouldn't, etc.
A few generic comments:
- Could you add a description to the PR and change the PR title according to the guideline [XYZ] module: short desc. like for a commit message link
- Usually, you'll want one commit / task to keep the chain of commits as clean and small as possible. As here, the task is to create the module Estate, it should be the one commit on your branch. Could you squash your commits into 1 ?
'author': "PRIN", | ||
'category': 'Category', | ||
'description': """ | ||
Just a dummy estate app |
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.
Not nice to call it dummy ... It has feeling too :/
@@ -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.
Try to always add a EOL in each file so that when someone adds lines in the future, the last line of the previous devs is not in the diff
@@ -0,0 +1,74 @@ | |||
from odoo import api,fields,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.
from odoo import api,fields,models | |
from odoo import api, fields, models |
_name = "estate.property" | ||
_description = "Estate property file" | ||
|
||
name = fields.Char('Name',required=True, translate=True, default='Unknown') |
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.
What do you think the translate does here ?
|
||
class estate_property_offer(models.Model): | ||
_name = "estate.property.offer" | ||
_description = "Estate property offer 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.
file ? Your description what this model is, not the file
@api.depends("validity") | ||
def _compute_date_deadline(self): | ||
for record in self: | ||
record.date_dateline=fields.Date.context_today(self)+relativedelta(days=record.validity) |
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.
Should the validity be computed from the creation date ? I mean, you create an offer today with a validity of 7 days. It should expire next Monday. Currently, it always expires 7 days after today so, if I look tomorrow, it will expire next Tuesday
|
||
def _inverser_date_deadline(self): | ||
for record in self: | ||
record.validity = (record.date_dateline - fields.Date.context_today(self)).days |
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
</list> | ||
</field> | ||
</record> | ||
<record id="estate_property_offer_view" model="ir.actions.act_window"> |
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 creating an action not a view here. The convention is <model_name>_action
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.
Mind that the convention for model ordering is:
- Private attributes (_name, _description, _inherit, _sql_constraints, …)
- Default method and default_get
- Field declarations
- Compute, inverse and search methods in the same order as field declaration
- Selection method (methods used to return computed values for selection fields)
- Constrains methods (@api.constrains) and onchange methods (@api.onchange)
- CRUD methods (ORM overrides)
- Action methods
- And finally, other business methods.
Blabla task-#######
0140323
to
f14ce88
Compare
Utils:
|
Now that your onboarding is done, could you close the PR ? 😄 |
No description provided.