XAFR - Technical Training #740
Conversation
|
|
estate/__manifest__.py
Outdated
| An application module that aims to serve as an onboarding sandbox. | ||
| """, | ||
| 'application': "True", | ||
| } |
There was a problem hiding this comment.
Don't forget to always add an empty line at the end of your files :)
There was a problem hiding this comment.
Ok I updated and I'll do that in the future. Why do I need to add that empty line?
There was a problem hiding this comment.
It is one of the PEP8 recommendations
estate/models/estate_property.py
Outdated
| name = fields.Char('Title', required=True, translate=True) | ||
| description = fields.Text('Description') | ||
| postcode = fields.Integer('Postcode', help="Your post code in 4 or 5 digits.") | ||
| date_availability = fields.Date('Date Availability') | ||
| expected_price = fields.Float('Expected Price', required=True) | ||
| selling_price = fields.Float('Selling Price') | ||
| bedrooms = fields.Integer(string='Bedrooms') | ||
| living_area = fields.Integer('Living Area (sqm)') | ||
| facades = fields.Integer('Facades') | ||
| garage = fields.Boolean('Garage') | ||
| garden = fields.Boolean('Garden') | ||
| garden_area = fields.Integer('Garden Area (sqm)') | ||
| garden_orientation = fields.Selection( | ||
| string='Garden Orientation', | ||
| selection=[('north', 'North'), ('west', 'West'), ('south', 'South'), ('east', 'East')], | ||
| help="The selection of the garden orientation.") |
There was a problem hiding this comment.
All good but most of the field display names are not required.
If not set, Odoo will basically deduct the display name of the field based on the field name. It'll separate it on the underscore and capitalize every word. (In case of a Many2One, Many2Many or One2Many behaviour will be slightly different but we'll see that when you'll have some). For example:
description -> Description
expected_price -> Expected Price
Basically, except name, living_area and garden_area none of them are required to be explicitly mentioned. Not a big issue but helps code look cleaner.
There was a problem hiding this comment.
Ok thank you that's good to know :)
…ing message on db launch, also removed code from estate_property.py because it was clutter that will be added in chapter 10)
46f80ea to
92195f6
Compare
estate/models/__init__.py
Outdated
| @@ -1 +1,4 @@ | |||
| from . import estate_property | |||
| from . import estate_property_type | |||
| from . import estate_property_tags | |||
| from . import estate_property_offer | |||
There was a problem hiding this comment.
Missing newline at the end of file (CI indicated it as well)
There was a problem hiding this comment.
Ok yeah I missed that one thank you for pointing it out
| @@ -27,3 +27,8 @@ class EstateModel(models.Model): | |||
| copy=False, | |||
| default='new', | |||
| selection=[('new', 'New'), ('offer_received', 'Offer Received'), ('offer_accepted', 'Offer Accepted'), ('sold', 'Sold'), ('cancelled', 'Cancelled')]) | |||
| property_type_id = fields.Many2one("estate.property.type", string="Property Type") | |||
There was a problem hiding this comment.
As you now have a Many2One as example, I can explain how it works.
If you follow the naming correctly, Odoo can generate the correct name here as well:
property_type_id will give Property Type
For One2many, same should happen:
offer_ids will become Offers
Added property models offer, type and tags
3a7900e to
a5ea174
Compare
| selection=[('accepted', 'Accepted'), ('refused', 'Refused')], | ||
| copy=False,) | ||
| partner_id = fields.Many2one("res.partner", string="Partner", required=True) | ||
| property_id = fields.Many2one("estate.property", string="Offer", required=True) |
There was a problem hiding this comment.
That's a nasty copy paste (string still indicates it's an Offer 😄). Same for these Many2ones
There was a problem hiding this comment.
I actually named it offer_id at first but then the instructions said to name it property_id and it ended up not being displayed anyway.. Should I just omit string="..." as much as possible and try to rely on the automated system from odoo you referenced here: #740 (comment) ?
| from odoo import fields, models | ||
|
|
||
|
|
||
| class EstateTagsModel(models.Model): |
There was a problem hiding this comment.
We try to follow the _name for naming a class. This should be EstatePropertyTag (keep it singular as well)
There was a problem hiding this comment.
Yeah I realized afterward that I had pluralized everything. I'll fix it in the next push and also I indeed forgot a word
| 67E6 | from odoo import fields, models | |
|
|
||
|
|
||
| class EstateTypeModel(models.Model): |
Added property computed fields and onchange
0ca24e8 to
93b7190
Compare
Added action buttons logic
|
|
||
|
|
||
| class EstateOfferModel(models.Model): | ||
| class EstatePropertyOfferModel(models.Model): |
There was a problem hiding this comment.
You should leave out the Modelin the class name :)
There was a problem hiding this comment.
Ok will update in the next push :)
| for record in self: | ||
| if record.create_date: |
There was a problem hiding this comment.
Can be merged as one line
for record in self.filtered("create_date"'):
| for record in self: | ||
| if record.create_date: |
| def _inverse_deadline(self): | ||
| for record in self: | ||
| if record.create_date: | ||
| record.validity = (record.date_deadline - fields.Date.to_date(record.create_date)).days |
There was a problem hiding this comment.
Are you sure you need to convert the record.create_date? 🤔
If it is about it being a datetime rather than a date, record.create_date.date() is cleaner IMO
There was a problem hiding this comment.
I originally went about it that way because on the Odoo documentation about Date and Datetime fields, the suggested method (and only one that I found to convert from date to datetime) was to use to_date...
But yeah this is much cleaner thank you! :)
estate/models/estate_property.py
Outdated
| if record.state != 'cancelled': | ||
| record.state = "sold" | ||
| else: | ||
| raise UserError("This property is already cancelled and thus cannot be sold anymore.") |
There was a problem hiding this comment.
Nothing bad with this code but, if you use _() method it'll be able to handle translations.
raise UserError(_("This property is already cancelled and thus cannot be sold anymore."))
There was a problem hiding this comment.
Should I get used to adding _() for any field that is user facing?
estate/models/estate_property.py
Outdated
| if record.state != 'sold': | ||
| record.state = "cancelled" | ||
| else: | ||
| raise UserError("This property is already sold and thus cannot be cancelled anymore.") |
| def action_btn_accept(self): | ||
| for record in self: | ||
| record.status = "accepted" | ||
| record.property_id.buyer_id = self.partner_id.id | ||
| record.property_id.selling_price = self.price | ||
| return True |
There was a problem hiding this comment.
When designing such actions you need to think about the usecase. Will you run this method on more than one record at a time? In this case as it is displayed in a list view that is inside a form, it won't.
You can then use a self.ensure_one() at the begining to check that it is being run on a single record (not 0 nor more than 1).
If you still want to keep the loop, you should be careful as you're looping over self and assigning a value to record but yet using self.partner_id.id and self.price. If you have more than one partner_id or different price it'll raise an error.
Also, don't forget to update the property state record.property_id.state = 'offer_accepted'
There was a problem hiding this comment.
Oh I see! Thank you for the correction. I'll try and think further ahead
Constraints
e01239a to
6ee2a07
Compare
Add the sprinkles Fixes according to review comments
e6ca176 to
b4b888b
Compare
Inheritance
b4b888b to
1742244
Compare
awesome_gallery/__manifest__.py
Outdated
| @@ -4,7 +4,7 @@ | |||
| 'summary': """ | |||
| Starting module for "Master the Odoo web framework, chapter 3: Create a Gallery View" | |||
| """, | |||
|
|
|||
| 'author': "unknown", | |||
There was a problem hiding this comment.
But why? At least put Odoo if you have to fill something
There was a problem hiding this comment.
I just wanted to get rid of the warning messages but yeah should have put odoo. I'm changing it now.
|
|
||
| name = fields.Char('Title', required=True, translate=True) | ||
| line_ids = fields.One2many("estate.property", "property_type_id") | ||
| manual_ordering = fields.Integer('Sequence', default=1, help="Used to order stages. Lower is better.") |
There was a problem hiding this comment.
Normally we'd call the field sequence as it is something we have quite a bit in the standard code. But it is not a big issue
| line_ids = fields.One2many("estate.property", "property_type_id") | ||
| manual_ordering = fields.Integer('Sequence', default=1, help="Used to order stages. Lower is better.") | ||
| offer_ids = fields.One2many("estate.property.offer", "property_type_id") | ||
| offer_count = fields.Integer(compute='_compute_offers') |
There was a problem hiding this comment.
By convention the compute should include the name of the computed field.
_compute_offer_count should be the one here 😄
.run/rd-demo.run.xml
Outdated
| <component name="ProjectRunConfigurationManager"> | ||
| <configuration default="false" name="rd-demo" type="Odoo" factoryName="Odoo"> | ||
| <module name="tutorials" /> | ||
| <option name="ENV_FILES" value="" /> | ||
| <option name="INTERPRETER_OPTIONS" value="" /> | ||
| <option name="PARENT_ENVS" value="true" /> | ||
| <envs> | ||
| <env name="PYTHONUNBUFFERED" value="1" /> | ||
| </envs> | ||
| <option name="SDK_HOME" value="$PROJECT_DIR$/../../../virtualenvs/18.0/bin/python/" /> | ||
| <option name="WORKING_DIRECTORY" value="" /> | ||
| <option name="IS_MODULE_SDK" value="true" /> | ||
| <option name="ADD_CONTENT_ROOTS" value="true" /> | ||
| <option name="ADD_SOURCE_ROOTS" value="true" /> | ||
| <EXTENSION ID="PythonCoverageRunConfigurationExtension" runner="coverage.py" /> | ||
| <option name="SCRIPT_NAME" value="$PROJECT_DIR$/../../../worktrees/saas-18.3/odoo/odoo-bin" /> | ||
| <option name="PARAMETERS" value="-c $PROJECT_DIR$/.odev/training_odoo.conf -u awesome_owl,awesome_clicker,awesome_gallery,awesome_dashboard,awesome_kanban,estate" /> | ||
| <option name="SHOW_COMMAND_LINE" value="false" /> | ||
| <option name="EMULATE_TERMINAL" value="true" /> | ||
| <method v="2" /> | ||
| </configuration> | ||
| </component> |
There was a problem hiding this comment.
Yeah I understand. I tried to add it to the .gitignore but it did not work and I did not want to unselect it everytime I made a new push.. 😬
I will still add it to the .gitignore
estate/models/estate_property.py
Outdated
|
|
||
| @api.ondelete(at_uninstall=False) | ||
| def _unlink_if_new_or_cancelled_property(self): | ||
| if any(estate_property.state in ['offer_received', 'offer_accepted', 'sold'] for estate_property in self): |
There was a problem hiding this comment.
This is fine but know that you can do this like this as well:
if self.filtered(lambda ep: ep.state in ['offer_received', 'offer_accepted', 'sold']):
or
if {'offer_received', 'offer_accepted', 'sold'}.intersect(set(self.mapped('state'))):
There was a problem hiding this comment.
Is there a preferred one? If so what is the priority order / where can I find it? 🤔
estate/models/res_users.py
Outdated
| class ResUsers(models.Model): | ||
| _inherit = "res.users" | ||
|
|
||
| property_ids = fields.One2many("estate.property", "salesman_id", domain="") |
There was a problem hiding this comment.
Don't add domain if you don't plan to use it
There was a problem hiding this comment.
Good catch I was supposed to add a domain there then forgot about it. I will fix that and upload in the next push.
Inter-module interactions
Inter-module interactions Optimized code to correct a DRY violation that led to unnecessary calls and wasted compute power. Co-authored-by: Sébastien Dieu <118258604+sedi-odoo@users.noreply.github.com>
3215a94 to
6a42330
Compare
.gitignore
Outdated
| @@ -127,3 +127,6 @@ dmypy.json | |||
|
|
|||
| # Pyre type checker | |||
| .pyre/ | |||
|
|
|||
| # PyCharm Demo Config | |||
| .run/rd-demo.run.xml | |||
There was a problem hiding this comment.
EOL 😅
Good thing to add it to the .gitignore. But as the file is already tracked, git won't care about this.
You should run git update-index --skip-worktree .run/rd-demo-run.xml (can be done on any file you wan't to refresh the tracking on).
(Best way would be to remove the file and then put it back after having removed it with a commit but let's just keep this as is now)
There was a problem hiding this comment.
Ah I see ok I'll fix it using the last method that's usually how I do it too :)
| # @SEDI J'ai pas réussi à faire fonctionner avec la plus simple que tu as suggérée. Elle return res.partner(3,) au lieu de res.partner() comme l'autre mais du coup ça marche pas.. | ||
| # partner_id = self.salesman_id.partner_id | ||
| partner_id = self.env['estate.property'].browse(self.env.context.get('active_id')).salesman_id.partner_id | ||
| invoicing_price = self.selling_price * 0.06 | ||
|
|
||
| values = { | ||
| 'partner_id': partner_id, |
There was a problem hiding this comment.
What error did you get? I think you'd just need to replace 'partner_id': partner_id by 'partner_id': partner_id.id
QWeb Templates
Coding Guidelines Review
| <field name="view_mode">list,form</field> | ||
| <field name="context">{'search_default_availability': True}</field> | ||
| <field name="view_mode">list,form,kanban</field> | ||
| <!-- <field name="context">{'search_default_availability': True}</field>--> |
| Best Offer: | ||
| <field name="best_offer"/>€ | ||
| </div> | ||
| <div t-if="record.state.raw_value == 'offer_accepted'"> |
estate/models/estate_property.py
Outdated
| if tools.float_utils.float_compare((record.expected_price * 0.9), record.selling_price, precision_rounding=0.01) >= 0 and not tools.float_utils.float_is_zero(record.selling_price, precision_rounding=0.001): | ||
| raise ValidationError(_("Offer price cannot be lower than 90% of the expected price.")) |
There was a problem hiding this comment.
I'd rather have an import that is
from odoo.tools.float_utils impot float_compare, float_is_zero
That will make the code more readable IMO
estate/models/estate_property.py
Outdated
| @api.ondelete(at_uninstall=False) | ||
| def _unlink_if_new_or_cancelled_property(self): | ||
| if any(estate_property.state in ['offer_received', 'offer_accepted', 'sold'] for estate_property in self): | ||
| raise UserError("Can't delete a property that is not new of cancelled!") |
| self.property_id.buyer_id = self.partner_id.id | ||
| self.property_id.selling_price = self.price | ||
| self.property_id.state = 'offer_accepted' |
There was a problem hiding this comment.
This can be done with a write and a dict
self.property_id.write({
'buyer_id': self.partner_id.id,
'selling_price': self.price,
'state': 'offer_accepted'
})There was a problem hiding this comment.
Updated! I will try to go about it that way in the future
| for record in self: | ||
| record.status = "refused" |
There was a problem hiding this comment.
Can just be done with records.status = "refused" no need to loop on them
| @@ -4,7 +4,7 @@ | |||
| <field name="name">Properties</field> | |||
| <field name="res_model">estate.property</field> | |||
| <field name="view_mode">list,form,kanban</field> | |||
| <!-- <field name="context">{'search_default_availability': True}</field>--> | |||
| <field name="context">{'search_default_availability': True}</field> | |||
There was a problem hiding this comment.
Guess that eludes my review on that part 😄
Owl Components
2955b71 to
27cf297
Compare
| [options] | ||
| addons_path = /home/odoo/odev/worktrees/18.0/odoo/addons,/home/odoo/odev/worktrees/18.0/enterprise,/home/odoo/odev/worktrees/18.0/design-themes,/home/odoo/odev/repositories/odoo-ps/tutorials | ||
| db_name = web_tutorial | ||
| limit_time_cpu = 999999 | ||
| limit_time_real = 999999 | ||
| limit_memory_hard = 0 | ||
| log_handler = odoo.addons.base.models.ir_attachment:WARNING | ||
| http_port = 8069 | ||
| gevent_port = 8072 |
| import { Component, useState, markup } from "@odoo/owl"; | ||
| import { Counter } from "../counter/counter"; | ||
| import {TodoList} from "../todo/todo_list"; | ||
| import {TodoItem} from "../todo/todo_item"; |
There was a problem hiding this comment.
Be consistent on the format of your imports
awesome_owl/static/src/card/card.js
Outdated
|
|
||
| static components = { Counter }; | ||
|
|
||
| static template = "awesome_owl.card" |
There was a problem hiding this comment.
You should start component names by a capital letter
static template = "awesome_owl.Card"
| <t t-slot="default"> | ||
| <Counter/> | ||
| </t> |
There was a problem hiding this comment.
I think the purpose of this part of the tutorial is to create a slot and populate it from outside the component
Basically you should not have the Counter inside of this component but rather in the Playground
| <t t-out="html_bloc_1"/> | ||
| <t t-out="html_bloc_2"/> |
There was a problem hiding this comment.
Same about this, tutorial said to use the markup in the playground
|
|
||
| <t t-name="awesome_owl.counter"> | ||
| <div> | ||
| <button class="btn btn-primary" t-on-click="increment">Referenced XML Button</button> |
There was a problem hiding this comment.
Why not Increment instead of Referenced XML Button?
|
|
||
| export class TodoItem extends Component { | ||
|
|
||
| static template = "awesome_owl.todo_item" |
There was a problem hiding this comment.
static template = "awesome_owl.TodoItem"
| <?xml version="1.0" encoding="UTF-8" ?> | ||
| <templates xml:space="preserve"> | ||
|
|
||
| <t t-name="awesome_owl.todo_item"> |
|
|
||
| <t t-name="awesome_owl.todo_item"> | ||
| <div t-att-class="{'text-muted': props.todo.isCompleted, 'text-decoration-line-through': props.todo.isCompleted}"> | ||
| <input type="checkbox" t-att-checked="props.todo.isCompleted" t-on-change="toggleCheckBox" style="margin-right: 5px"/> |
There was a problem hiding this comment.
That's one way of doing it, know that you could also have done this for the t-on-change
t-on-change="() => props.todo.isCompleted = !props.todo.isCompleted"
or better, you can call a method on the todo (if you move it to a JS class)
| this.input = useRef("keyboard_input"); | ||
| onMounted(() => { | ||
| this.input.el.focus(); | ||
| }); |
There was a problem hiding this comment.
Weren't you supposed to use useAutofocus instead?
Build a dashboard
Build a dashboard
A PR to log and facilitate the review of the completed chapters.