Test commit & push MEPL #166
Conversation
Mathilde411
force-pushed
the
18.0-tutorials-mepl
branch
2 times, most recently
from
9119b6c to
11ab1a4
Compare
|
Sorry @aboo-odoo I force-pushed over you because I did not see you commited :x |
|
Hey @Mathilde411, no worries, I'll push it again on your branch (but be careful when working with someone in the future, as you might delete his/her work). Go on with your tutorial for now and when you'll want to push, try to fetch this first (and make it work). 👍 😄 |
Yeah, I wasn't really thinking that someone else could have pushed on my branch right now ^^ |
People should warn you beforehand, I was just testing you a bit 😁 |
Mathilde411
force-pushed
the
18.0-tutorials-mepl
branch
from
db7d901 to
82547cd
Compare
Mathilde411
force-pushed
the
18.0-tutorials-mepl
branch
from
7dc945e to
cb259d2
Compare
Added Real estate app built up to the end of chapter 11
Chapter 12 inheritance
Diverged a bit from chapter. Digged into sales order code to adapt it while really understanding instead of following tutorial
Mathilde411
force-pushed
the
18.0-tutorials-mepl
branch
from
cb259d2 to
c444774
Compare
| ('check_bedrooms', 'CHECK(bedrooms >= 0)', 'The number of bedrooms must be positive.'), | ||
| ('check_living_area', 'CHECK(living_area > 0)', 'The living area must be positive.'), | ||
| ('check_facades', 'CHECK(facades > 0)', 'The number of facades must be positive.'), | ||
| ('check_name_unique', 'UNIQUE(name)', 'The Property name must be unique.') |
There was a problem hiding this comment.
This constraint was not asked by the tutorial. You can add it if you feel it is better, as it is a tutorial, but you might want to adapt the corresponding field... As of now, duplicating the property raises an error.
| if not float_is_zero(record.selling_price, precision_digits=2) and float_compare(record.selling_price, | ||
| 0.9 * record.expected_price, | ||
| precision_digits=2) < 0: |
There was a problem hiding this comment.
| if not float_is_zero(record.selling_price, precision_digits=2) and float_compare(record.selling_price, | |
| 0.9 * record.expected_price, | |
| precision_digits=2) < 0: | |
| if ( | |
| not float_is_zero(record.selling_price, precision_digits=2) | |
| and float_compare(record.selling_price, 0.9 * record.expected_price, precision_digits=2) < 0 | |
| ): |
This is overall preferable and more clear (nitpicky, I admit) :D
| partner_id = fields.Many2one('res.partner', string='Partner', required=True) | ||
| price = fields.Float() | ||
| property_id = fields.Many2one('estate.property', string='Property', required=True) | ||
| create_date = fields.Date(string='Create Date', default=lambda self: fields.Datetime.now()) |
There was a problem hiding this comment.
create_date is an automatic field and should not be added here (see https://www.odoo.com/documentation/18.0/developer/tutorials/server_framework_101/03_basicmodel.html#:~:text=of%20the%20model.-,create_date,-().
I see you're already using fields.Datetime.now(): you can just use it instead of create_date if create_date is not set, in the relevant methods :)
| for record in self: | ||
| record.deadline_date = record.create_date + relativedelta(days=record.validity) | ||
|
|
||
| @api.depends('validity', 'deadline_date', 'create_date') |
There was a problem hiding this comment.
This is not needed for an inverse method (I guess it was just copy-pasted from above)
| def action_accept(self, propagate=True): | ||
| for record in self: | ||
| if propagate: | ||
| record.property_id.set_accepted_offer(record) | ||
| record.state = 'accepted' | ||
| return True | ||
|
|
||
| def action_refuse(self, propagate=True): | ||
| for record in self: | ||
| if record.state == 'accepted' and propagate: | ||
| record.property_id.set_accepted_offer(None) | ||
| record.state = 'refused' | ||
| return True | ||
|
|
||
| def action_reset(self, propagate=True): | ||
| for record in self: | ||
| if record.state == 'accepted' and propagate: | ||
| record.property_id.set_accepted_offer(None) | ||
| record.state = 'received' | ||
| return True |
There was a problem hiding this comment.
It's better to bring all code about set_accepted_offer in this model and remove it from estate.property as the action comes from here
There was a problem hiding this comment.
I was doing this because of ownership. Isn't it better to let each class manage its own variables ?
There was a problem hiding this comment.
While it can be true in "classic" frameworks, in Odoo, a lot of modules depend on each other, and might want to adapt the behavior of the inherited model, or add features. If we add everything in the inherited class, it would become monstrously huge and hard to work in.
In that sense, we can apply the same thinking with files within a same module: as the "offer acceptance feature" comes from this file, it can handle the property variables by itself. We don't usually use setters and getters unless there are strong conditions to respect.
At the end of the day, it is just a (heavy) preference and your way of doing might be accepted, I was particularly nitpicky because your implem was very well done 😄
| _inherit = 'res.users' | ||
|
|
||
| property_ids = fields.One2many('estate.property', 'salesperson_id', string='Properties', | ||
| domain="[('date_availability', '<=', context_today().strftime('%Y-%m-%d'))]") |
There was a problem hiding this comment.
| domain="[('date_availability', '<=', context_today().strftime('%Y-%m-%d'))]") | |
| domain="[('date_availability', '<=', fields.Date.today())]) |
It doesn't work the way you did: you can try yourself by going to the Setting App -> Users & Companies and looking into the user you set as salesperson (with unavailable properties set up for him): you'll see that even unavailable properties are still shown by default
| </record> | ||
|
|
||
| <record id="estate_property_offer_action" model="ir.actions.act_window"> | ||
| <field name="name">Property Types</field> |
| <field name="arch" type="xml"> | ||
| <form string="Property Types"> | ||
| <sheet> | ||
| <div class="oe_button_box" name="button_box"> |
There was a problem hiding this comment.
Isn't it like that to add smart buttons ? I saw it in the sales module.
There was a problem hiding this comment.
Have you tried without ? I think this should work with just the inside node (<button class="oe_stat_button"...), if not, you can ignore my comment
| <div class="oe_button_box" name="button_box"> | ||
|
|
||
| </div> |
There was a problem hiding this comment.
It is here to let modules add box buttons without stepping on each other.
Ex: If 2 modules add buttons they already have the parent div, and do not need to step on each other to add it
There was a problem hiding this comment.
That is good thinking if you know it will be used for inheritance ! But usually you never know if that will be the case, and we avoid adding extra nodes like this unless we know this part will particularly be inherited multiple times. Or if it can't be done otherwise, but here you can use expr="//div[@class='oe_title']" in your inheriting view to get it.
In this case I insists because I don't think an extra <div class="oe_button_box" name="button_box"> is needed around the oe-stat button, but if it is, you can eventually leave it like this
| 'version': '1.0', | ||
| 'category': 'Tutorials', | ||
| 'depends': [ | ||
| 'base', |
There was a problem hiding this comment.
This is not needed as the inheritance path of account already include base
There was a problem hiding this comment.
A bit more nitpicks :D
More hints: usually we try to keep the file names and class names close to the model name: model 'estate.property' -> file 'estate_property.py' and class 'EstatePropery'. This allow the devs to more easily search the file we need and it appears more clear when looking into files with multiple models inside.
It's a good thing to go look into what's existing in Odoo - you'll learn a lot. But it is usefull to look at different implementations of what you're looking for: some modules are heavy in processing while others might be way more simple and quick... It depends on what's needed 🤷♂️ .
Very good work so far, you're quick to learn !
| # invoice_status = fields.Selection([ | ||
| # ('upselling', 'Upselling Opportunity'), | ||
| # ('invoiced', 'Fully Invoiced'), | ||
| # ('to invoice', 'To Invoice'), | ||
| # ('no', 'Nothing to Invoice') | ||
| # ], string='Invoice Status', compute='_get_invoice_status', store=True, readonly=True) |
There was a problem hiding this comment.
Don't forget to remove your old comments
|
|
||
| res = { | ||
| 'display_type': 'product', | ||
| 'sequence': sequence, |
There was a problem hiding this comment.
The sequence is already automatically handled, no use adding it here
|
|
||
| # trying to understand sales invoice creation line by line | ||
| def _create_invoices(self, grouped=False, date=None): | ||
| if not self.env['account.move'].has_access('create'): |
There was a problem hiding this comment.
The ORM handles access errors by itself, you can keep it simpler :D
| invoice_vals_list = [] | ||
| for invoice_item_sequence, record in enumerate(self): | ||
| if record.state != 'sold': | ||
| raise UserError('You can only invoice sold properties.') | ||
|
|
||
| if record.buyer_id.lang: | ||
| record = record.with_context(lang=record.buyer_id.lang) | ||
|
|
||
| invoice_vals = record._prepare_invoice() | ||
| invoice_vals['invoice_line_ids'].append( | ||
| fields.Command.create(record._prepare_invoice_line(sequence=invoice_item_sequence)) | ||
| ) | ||
| invoice_vals_list.append(invoice_vals) | ||
|
|
||
| if not grouped: | ||
| new_invoice_vals_list = [] | ||
| invoice_grouping_keys = self._get_invoice_grouping_keys() | ||
| invoice_vals_list = sorted( | ||
| invoice_vals_list, | ||
| key=lambda x: [ | ||
| x.get(grouping_key) for grouping_key in invoice_grouping_keys | ||
| ] | ||
| ) | ||
|
|
||
| for _grouping_keys, invoices in groupby(invoice_vals_list, | ||
| key=lambda x: (x.get(grouping_key) for grouping_key in | ||
| invoice_grouping_keys)): | ||
| origins = set() | ||
| refs = set() | ||
| ref_invoice_vals = None | ||
| for invoice_vals in invoices: | ||
| if not ref_invoice_vals: | ||
| ref_invoice_vals = invoice_vals | ||
| else: | ||
| ref_invoice_vals['invoice_line_ids'] += invoice_vals['invoice_line_ids'] | ||
| origins.add(invoice_vals['invoice_origin']) | ||
| refs.add(invoice_vals['ref']) | ||
|
|
||
| ref_invoice_vals.update({ | ||
| 'ref': ', '.join(refs)[:2000], | ||
| 'invoice_origin': ', '.join(origins) | ||
| }) | ||
| new_invoice_vals_list.append(ref_invoice_vals) | ||
| invoice_vals_list = new_invoice_vals_list | ||
|
|
||
| return self.env['account.move'].sudo().with_context(default_move_type='out_invoice').create( | ||
| invoice_vals_list) |
There was a problem hiding this comment.
This whole method is way too complicated for our case. You can practically use the same example as shown in the tutorial to create a move with the correct invoice_line_ids ;)
There was a problem hiding this comment.
I was thinkig it'd be cool to keep the possibility of creating 1 invoice for multiple properties :D
There was a problem hiding this comment.
That would be nice indeed. You can then use a small method that can add a invoice line to an existing move, or put that simple code directly into the method where you create the move (assuming you change a bit the flow to allow creating a move after selecting multiple properties for example), but this code is still doing way too complicated (and useless in our case) things :P
| action = self.env['ir.actions.actions']._for_xml_id('account.action_move_out_invoice_type') | ||
| form_view = [(self.env.ref('account.view_move_form').id, 'form')] | ||
| if 'views' in action: | ||
| action['views'] = form_view + [(state, view) for state, view in action['views'] if view != 'form'] | ||
| else: | ||
| action['views'] = form_view | ||
| action['res_id'] = self.invoice_id.id | ||
|
|
||
| return action |
There was a problem hiding this comment.
This is also too complicated with what is needed. In python, you can return a dict to communicate an action (unless the method is called from a simple other python method). Your case should not need more than this (you'll probably want to adapt it to make it work):
| action = self.env['ir.actions.actions']._for_xml_id('account.action_move_out_invoice_type') | |
| form_view = [(self.env.ref('account.view_move_form').id, 'form')] | |
| if 'views' in action: | |
| action['views'] = form_view + [(state, view) for state, view in action['views'] if view != 'form'] | |
| else: | |
| action['views'] = form_view | |
| action['res_id'] = self.invoice_id.id | |
| return action | |
| return { | |
| 'name': 'Invoice', | |
| 'view_mode': 'form', | |
| 'res_model': 'account.move', | |
| 'type': 'ir.actions.act_window', | |
| 'res_id': self.invoice_id.id, | |
| } |
No description provided.