Skip to content
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

first commit #167

Open
wants to merge 3 commits into
base: 18.0
Choose a base branch
from
Open

first commit #167

wants to merge 3 commits into from

Conversation

dabo-odoo
Copy link

No description provided.

Copy link

@Megaaaaaa Megaaaaaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello 👋 here's a first review for you 😄

Some teams won't be as nitpicking as i was but in all cases thoses guidelines should get most people to approve your code 👍

@@ -0,0 +1,6 @@
# Part of Odoo. See LICENSE file for full copyright and licensing details.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not required in any file anymore, you can just remove it and start on the first line of the file 👍

from . import estate_property
from . import estate_property_type
from . import estate_property_tags
from . import estate_property_offer
Copy link

@Megaaaaaa Megaaaaaa Oct 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ⛔ (click on the file hyperlink) signs means your forgot to add a newline at the end of file. All files should end with an empty new line 👍

See: https://runbot.odoo.com/runbot/batch/1661174/build/69657389#:~:text=tutorials/estate/models/__init__.py%3A6

tags_ids = fields.Many2many("estate.property.tags", string='Tags')
offer_ids = fields.One2many("estate.property.offer", "property_id", string="Offers")
total_area = fields.Integer(compute=("_compute_total"))
@api.depends("living_area", "garden_area")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All methods should be preceded by a blank line and precede a blank line.

See: https://runbot.odoo.com/runbot/batch/1661174/build/69657389#:~:text=tutorials/estate/models/estate_property.py%3A40

if record.create_date == 0:
record.validity = 7
else:
record.validity = (record.date_deadline - record.create_date).day

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing empty line after file's last line 😄

Comment on lines 10 to 25
state = fields.Selection(
string='State',
selection=[('new', 'New'), ('received', 'Offer Received'), ('accepted', 'Offer Accepted'), ('sold', 'Sold'), ('cancelled', 'Cancelled')],
required=True,
copy=False,
default='new'
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When fields definition get too big (and it happens quite often with Selection fields, we try to put as much as possible on different lines, for selection fields, it means you can (and will probably be asked too anyway) do that:

Suggested change
state = fields.Selection(
string='State',
selection=[('new', 'New'), ('received', 'Offer Received'), ('accepted', 'Offer Accepted'), ('sold', 'Sold'), ('cancelled', 'Cancelled')],
required=True,
copy=False,
default='new'
)
state = fields.Selection(
[
('new', 'New'),
('received', 'Offer Received'),
('accepted', 'Offer Accepted'),
('sold', 'Sold'),
('cancelled', 'Cancelled')
],
string='State',
required=True,
copy=False,
default='new'
)

Also the selection list is expected first so it's nice and cleaner to put it first without the selection=

Comment on lines 49 to 50
validity = fields.Integer(compute="_compute_deadline", inverse="_compute_validity", string="Validity", default=7)
date_deadline = fields.Date(compute="_compute_validity", inverse="_compute_deadline",string="Deadline", default=fields.Date.today())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fields should be before all methods, put that back to the top of the class just before the methods 😄

def _compute_total(self):
for record in self:
record.total_area = record.living_area + record.garden_area
best_offer = fields.Float(compute="_compute_maximum")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All fields should be before all methods, put that back to the top of the class just before the methods 😄

Comment on lines 9 to 13
status = fields.Selection(
string='Status',
selection=[('accepted', 'Accepted'), ('refused', 'Refused')],
help="Offer status",
copy=False)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another selection field that can be slightly improved regarding the selection= and the string=, up to you but in any way, the closing parenthsis has to go to next line 😄

estate.access_estate_property,access_estate_property,estate.model_estate_property,base.group_user,1,1,1,1
estate.access_estate_property_type,access_estate_property_type,estate.model_estate_property_type,base.group_user,1,1,1,1
estate.access_estate_property_tags,access_estate_property_tags,estate.model_estate_property_tags,base.group_user,1,1,1,1
estate.access_estate_property_offer,access_estate_property_offer,estate.model_estate_property_offer,base.group_user,1,1,1,1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing EOL on last line 😉

<menuitem id="estate_property_tags_menu_action" action="estate_property_tags_action"/>
</menuitem>
</menuitem>
</odoo>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing EOL on last line 😉 There are a few more, i won't tag them all 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants