[ADD] estate,estate_account: technical onboarding commit #1285
[ADD] estate,estate_account: technical onboarding commit #1285jupir-odoo wants to merge 1 commit into
Conversation
|
|
jupir-odoo
force-pushed
the
19.0-tuto-server-jupir
branch
from
07fb131 to
2b8c4ec
Compare
jupir-odoo
changed the title
|
@aboo-odoo kind reminder :D |
jupir-odoo
force-pushed
the
19.0-tuto-server-jupir
branch
from
2b8c4ec to
5c36dbc
Compare
jupir-odoo
changed the title
aboo-odoo
left a comment
aboo-odoo
left a comment
There was a problem hiding this comment.
Fenomenal work 🥳 I'm being extra-picky so that I can transmit as much information as possible. If there is anything you disagree with, please share it without fear.
Some comments apply at several places but I've only written it once to save some time and to not cumbersome the PR, when you check and update according to one of my comment, check that this comment does not apply elsewhere before moving to the next one 😄
A generic comment:
- your PR/commit body could be a little more exhaustive a good template is to follow is
STEPS to reproduce the bug (only for bugfixes)
PROBLEM - what is the problem
GOAL - what do we want to achieve
SOLUTION - how did we achieve it
task-XXXXXX
There was a problem hiding this comment.
In a Model attribute order should be:
- Private attributes (_name, _description, _inherit, …)
- Default method and default_get
- Field declarations
- SQL constraints and indexes
- 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.
| copy=False, default=datetime.today() + relativedelta(months=3) | ||
| ) | ||
| expected_price = fields.Float(required=True) | ||
| _expected_price_constraint = models.Constraint( |
There was a problem hiding this comment.
The two constraints should be just after the _order attribute 😄
|
|
||
|
|
||
| class EstatePropery(models.Model): | ||
| _name = "estate.property" |
There was a problem hiding this comment.
There is a tacit convention that strings viewed by the users are surrounded by ", otherwise it is '
| _name = "estate.property" | |
| _name = 'estate.property' |
| garden_area = fields.Integer() | ||
| garden_orientation = fields.Selection( | ||
| string="Orientation", | ||
| selection=[("N", "North"), ("E", "East"), ("S", "South"), ("W", "West")], |
There was a problem hiding this comment.
Consider selection and a kind of dictionary, for dictionaries, you would write the keys in lower letters
| selection=[("N", "North"), ("E", "East"), ("S", "South"), ("W", "West")], | |
| selection=[('north', "North"), ('east', "East"), ('south', "South"), ('west', "West")], |
Also, don't hesitate to be exhaustive in your name; estate.garden_orientation = 'north' is a bit clearer than estate.garden_orientation = 'n'
| default="new", | ||
| ) | ||
|
|
||
| def action_cancel_property(self): |
There was a problem hiding this comment.
self is a recordset, i.e. a collection of records. Your method would failed if called on several records when trying to perform self.state.
Currently, through the UI, you cannot trigger the error, but from the command line or a server action, users could get the traceback.
side note: you can run odoo in terminal mode by adding shell after odoo-bin: python odoo-bin shell --addons-path .... From there, you could perform
estates = self.env['estate.property'].create([{'name': 'TEST1', ADD the other relevant key-value pairs}, {'name': 'TEST2', ADD the other relevant key-value pairs}])
estates.action_cancel_property() # This should trigger the traceback
There are two ways to solve the issue:
- begin your method with
self.ensure_one(), quite self explanatory - or use a for loop like in _compute methods
| ) | ||
|
|
||
| def action_accept(self): | ||
| if self.property_id.state == "sold": |
There was a problem hiding this comment.
What happens if the property is cancelled ?
| @api.model | ||
| def create(self, vals): |
There was a problem hiding this comment.
It would be preferable that you create the records in a batch rather than one by one, better performances 😄
| @api.model | |
| def create(self, vals): | |
| @api.model_create_multi | |
| def create(self, vals_list): |
| if float_compare(property.best_price, record["price"], 2) == 1: | ||
| raise UserError("You already have a higher offer") | ||
|
|
||
| if hasattr(property, "state"): |
There was a problem hiding this comment.
The property model always has the state attribute (you defined it as field), in what scenario could you not have one ?
| ], | ||
| "installable": True, | ||
| "application": True, | ||
| "author": "Julien (jupir)", |
There was a problem hiding this comment.
Everything you create for Odoo belongs to "Odoo S.A." hehe
| "author": "Julien (jupir)", | |
| "author": "Odoo S.A.", |
| delta = timedelta(days=offer.validity) | ||
| offer.date_deadline = ( | ||
| offer.create_date.date() + delta | ||
| if offer.create_date | ||
| else today() + delta | ||
| ) |
There was a problem hiding this comment.
Alternatively (one-liner)
| delta = timedelta(days=offer.validity) | |
| offer.date_deadline = ( | |
| offer.create_date.date() + delta | |
| if offer.create_date | |
| else today() + delta | |
| ) | |
| offer.date_deadline = (offer.create_date or today()).date() + timedelta(days=offer.validity) |
jupir's technical onboarding task-6229399
jupir-odoo
force-pushed
the
19.0-tuto-server-jupir
branch
from
5c36dbc to
4fd2d3b
Compare
3 participants
jupir's technical onboarding
task-6229399