-
Notifications
You must be signed in to change notification settings - Fork 2.6k
JUGAC Onboarding #1002
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: 19.0
Are you sure you want to change the base?
JUGAC Onboarding #1002
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.
Small comments :)
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.
Good work :)
Please look at your runbot and make it green.
Also take a look at the naming conventions: https://www.odoo.com/documentation/19.0/contributing/development/coding_guidelines.html#xml-ids-and-naming
| state = fields.Selection( | ||
| selection=[ | ||
| ('new', 'New'), | ||
| ('received', 'Received'), | ||
| ('accepted', 'Accepted') | ||
| ] | ||
| ) |
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.
| state = fields.Selection( | |
| selection=[ | |
| ('new', 'New'), | |
| ('received', 'Received'), | |
| ('accepted', 'Accepted') | |
| ] | |
| ) | |
| state = fields.Selection( | |
| selection=[ | |
| ('new', 'New'), | |
| ('received', 'Received'), | |
| ('accepted', 'Accepted') | |
| ], required=True, default='new' | |
| ) |
state should always be set, and new by default
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 again
estate/models/estate_property.py
Outdated
| buyer_id = fields.Many2one("res.partner") | ||
| salesman_id = fields.Many2one("res.users") | ||
| tag_ids = fields.Many2many("estate.property.tag") | ||
| offer_ids = fields.One2many("estate.property.offer", "partner_id") No newline at end of 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.
Missing newline
estate/models/estate_property.py
Outdated
|
|
||
| class EstateProperty(models.Model): | ||
| _name = "estate.property" | ||
| _description = "Table that stores the estate properties" |
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.
| _description = "Table that stores the estate properties" | |
| _description = "Estate Property" |
_description is the human-readable name of the model, not an actual description. It' a misnomer.
estate/views/estate_menus.xml
Outdated
|
|
||
| </menuitem> | ||
| </data> | ||
| </odoo> No newline at end of 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.
Missing newline
| </field> | ||
| </record> | ||
| </data> | ||
| </odoo> No newline at end of 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.
Missing newline
| </field> | ||
| </record> | ||
|
|
||
| </odoo> No newline at end of 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.
Missing newline
estate/__init__.py
Outdated
| @@ -0,0 +1 @@ | |||
| from . import models No newline at end of 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.
Missing newline
| <list string="Estate offers"> | ||
| <group> | ||
| <field name="price"/> | ||
| </group> | ||
| <group> | ||
| <field name="partner_id"/> | ||
| </group> | ||
| <group> | ||
| <field name="status"/> | ||
| </group> | ||
| </list> |
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.
| <list string="Estate offers"> | |
| <group> | |
| <field name="price"/> | |
| </group> | |
| <group> | |
| <field name="partner_id"/> | |
| </group> | |
| <group> | |
| <field name="status"/> | |
| </group> | |
| </list> | |
| <list string="Estate offers"> | |
| <field name="price"/> | |
| <field name="partner_id"/> | |
| <field name="status"/> | |
| </list> |
No groups needed in a list view
| <group> <field name="salesman_id"/> </group> | ||
| <group> <field name="buyer_id"/> </group> | ||
| <group> <field name="buyer_id"/> </group> |
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, only one group needed
…P] estate: attempt to resolve runbot error messages
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.
Did you test everything ? Ifeel like some parts would outright not work at all.
Good work overall though. :)
| from . import estate_type | ||
| from . import estate_tag | ||
| from . import estate_offer | ||
| from . import 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.
| from . import users | |
| from . import res_users |
Model name is res.users, so here the file name should be res_users.py
| @api.depends("date_deadline") | ||
| def _inverse_validity(self): | ||
| for record in self: | ||
| record.validity = (record.date_deadline - record.create_date).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.
What happens here if I put False in date_deadline ? date_deadline is not required
| def action_refuse(self): | ||
| for record in self: | ||
| record.status = 'refused' | ||
| return 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.
| def action_refuse(self): | |
| for record in self: | |
| record.status = 'refused' | |
| return True | |
| def action_refuse(self): | |
| record.status = 'refused' | |
| return True |
This works
| @api.model | ||
| def create(self, vals_list): | ||
|
|
||
| for offer in self: | ||
| if offer.price > vals_list.price: | ||
| raise UserError('The offer price should be greater than those already received') | ||
|
|
||
| res = super().create(vals_list) | ||
|
|
||
| if res.property_id.state != 'received': | ||
| res.property_id.state = 'received' | ||
|
|
||
| return res |
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.model | |
| def create(self, vals_list): | |
| for offer in self: | |
| if offer.price > vals_list.price: | |
| raise UserError('The offer price should be greater than those already received') | |
| res = super().create(vals_list) | |
| if res.property_id.state != 'received': | |
| res.property_id.state = 'received' | |
| return res | |
| @api.model_create_multi | |
| def create(self, vals_list): | |
| for offer in self: | |
| if offer.price > vals_list.price: | |
| raise UserError('The offer price should be greater than those already received') | |
| res = super().create(vals_list) | |
| if res.property_id.state != 'received': | |
| res.property_id.state = 'received' | |
| return res |
Did you test this ? vals_list should be a List[Dict[str, Any]], so this wouldn't work.
To make sure mark it as model_create_multi. it makes sure that vals_list is always that.
| state = fields.Selection( | ||
| selection=[ | ||
| ('new', 'New'), | ||
| ('received', 'Received'), | ||
| ('accepted', 'Accepted') | ||
| ] | ||
| ) |
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 again
| def action_set_sold(self): | ||
| for record in self: | ||
| if record.state == 'canceled': | ||
| raise UserError('A canceled property cannot be sold') | ||
| else: | ||
| record.state = 'sold' | ||
| return 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.
| def action_set_sold(self): | |
| for record in self: | |
| if record.state == 'canceled': | |
| raise UserError('A canceled property cannot be sold') | |
| else: | |
| record.state = 'sold' | |
| return True | |
| def action_set_sold(self): | |
| for record in self: | |
| if record.state == 'canceled': | |
| raise UserError('A canceled property cannot be sold') | |
| record.state = 'sold' | |
| return True |
else not needed here. The exception will break the flow anyways
| def action_set_canceled(self): | ||
| for record in self: | ||
| if record.state == 'sold': | ||
| raise UserError('A sold property cannot be canceled') | ||
| else: | ||
| record.state = 'canceled' | ||
| return 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.
| def action_set_canceled(self): | |
| for record in self: | |
| if record.state == 'sold': | |
| raise UserError('A sold property cannot be canceled') | |
| else: | |
| record.state = 'canceled' | |
| return True | |
| def action_set_canceled(self): | |
| for record in self: | |
| if record.state == 'sold': | |
| raise UserError('A sold property cannot be canceled') | |
| record.state = 'canceled' | |
| return True |
ditto
| @api.constrains('selling_price') | ||
| def _check_selling_price(self): | ||
| for record in self: | ||
| if record.state in ("new", "received"): | ||
| return | ||
|
|
||
| if float_compare(record.selling_price, (record.expected_price * 0.9), 2) < 0: | ||
| raise ValidationError("The selling price cannot be lower than 90%% of the expected price") | ||
|
|
||
| if float_is_zero(record.selling_price, 2): | ||
| raise ValidationError("The selling price should be positive") |
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.constrains('selling_price') | |
| def _check_selling_price(self): | |
| for record in self: | |
| if record.state in ("new", "received"): | |
| return | |
| if float_compare(record.selling_price, (record.expected_price * 0.9), 2) < 0: | |
| raise ValidationError("The selling price cannot be lower than 90%% of the expected price") | |
| if float_is_zero(record.selling_price, 2): | |
| raise ValidationError("The selling price should be positive") | |
| @api.constrains('selling_price') | |
| def _check_selling_price(self): | |
| for record in self.filtered(lambda p: p.state not in ("new", "received")): | |
| if float_compare(record.selling_price, (record.expected_price * 0.9), 2) < 0: | |
| raise ValidationError("The selling price cannot be lower than 90%% of the expected price") | |
| if float_is_zero(record.selling_price, 2): | |
| raise ValidationError("The selling price should be positive") |
This is more odooesque
| if created_invoice: | ||
| res = super().action_set_sold() | ||
| return res |
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 are you trying to do here ?

Create the estate module