-
Notifications
You must be signed in to change notification settings - Fork 2.2k
18.0 real estate module shzi #820
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
…tegrate it with the database
… default home page + add basic ui for the property so I can manage them easily
…e relational database
task-#######
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've been extra picky but again 🎉
A few generic comments:
- A lot of your IDE stuff is in the diff, you need to remove that
- 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 ?
@@ -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.
Always add an EOL character at the end of your files to minimize the diffs of the next dev updating the file 👍
@@ -0,0 +1,15 @@ | |||
{ | |||
'name': 'Estate', |
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.
In python, for quoting strings we try to follow the following convention:
single quote '
around technical terms
double quotes "
around display terms
=> ('north', "North")
Let's try to follow this during this tutorial, even though you'll find many counter examples in the existing code.
estate/models/estate_property.py
Outdated
@@ -0,0 +1,110 @@ | |||
from odoo import fields, models, api |
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 imports should be
- External libraries (one per line sorted and split in python stdlib)
- Imports of odoo
- Imports from Odoo modules (rarely, and only if necessary)
Inside these 3 groups, the imported lines are alphabetically sorted. You can use RUFF to help you sort them, it's quite handy
estate/models/estate_property.py
Outdated
_name = "estate.property" | ||
_description = "Store Real Estate Properties" | ||
|
||
name = fields.Char("Estate Name", required=True, translate=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.
What do you think the translate=True here ?
estate/models/estate_property.py
Outdated
name = fields.Char("Estate Name", required=True, translate=True) | ||
description = fields.Text("Description", help="Enter the real estate item description") | ||
postcode = fields.Char("Postcode") | ||
date_availability = fields.Date("Available From", copy=False, default=lambda self: date.today() + timedelta(days=90)) |
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.
timedelta(days=90))
is not exactly 3 months ahah
date_deadline = fields.Datetime(compute="_compute_date_deadline", inverse="_inverse_date_deadline") | ||
|
||
# Computed functions | ||
@api.depends('create_date', '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.
@api.depends('create_date', 'validity') | |
@api.depends('validity') |
create_date
will never change, only place variable that change to trigger the re-compute of the method
if offer.property_id.buyer_id: | ||
raise UserError("An offer has already been accepted for this 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.
I would rather
if offer.property_id.buyer_id: | |
raise UserError("An offer has already been accepted for this property.") | |
if offer.property_id.state == 'offer_accepted': | |
raise UserError("An offer has already been accepted for this property.") |
Because the fact there is a buyer id does not mean an offer has been accepted (conceptually speaking)
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.
Again, file order is important
</list> | ||
</field> | ||
</record> | ||
</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.
Not writing it on every file, but all are missing the EOL character
<field name="partner_id" /> | ||
<field name="status" /> | ||
<button name="action_accept_offer" string="Confirm" type="object" icon="fa-check"/> | ||
<button name="action_refuse_offer" string="Cancel" type="object" icon="fa-times"/> |
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.
Your buttons are always shown, wouldn't it be better to hide them once they have been accepted or refused ?
…crosss modules + restrictions
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! Final review on the python part, it focuses on functionality, not style anymore 😄 All and all, an extremely good PR, well done 👍
On Monday, I'll teach you how to remove the .idea stuff, squash your commits and rebase your branch to have it all nice and clean.
By then, try to get your runbot green 🟢 and fix the comments from last time and this review please 👍
from . import (estate_property, estate_property_offer, estate_property_tag, | ||
estate_property_type, inherited_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.
Use several lines, it's easier to manage in the future
@api.model | ||
def create(self, vals): |
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.
Very nice and robust method override but it'd be even better if you could do it in batch to increase performances.
@api.model | |
def create(self, vals): | |
@api.model_create_multi | |
def create(self, vals_list): | |
for vals in vals_list: | |
# Do things | |
return super().create(vals_list) |
other_offers = offer.property_id.offer_ids - offer | ||
other_offers.write({"status": "refused"}) |
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.
Very nice use of recordset!!!
No description provided.