Conversation
|
|
trcazier
force-pushed
the
19.0-tutorial-industry-trcaz
branch
from
7ee5e1f to
4b5f71c
Compare
[IMP] estate: add chapter 2 [IMP] estate: add chapter 3 [IMP] estate: add chapter 4 [IMP] estate: add chapter 5 [IMP] estate: add chapter 6 [IMP] estate: (chapter 7) add types, tags, & offers views/models and fix search group by & menu names [IMP] estate: add chapter 8 [IMP] estate: Chapter 10 [IMP] estate: add Chapter 11 [IMP] estate: apply some of Faruk's suggestions from the PR [IMP] estate: remove superfluous encoding line (runbot) [IMP] estate: add _ to UserError string parameters [IMP] estate: add chapter 12 [IMP] estate: fix the default date availability being computed only on upgrade [ADD] estate_account: add module manifest [IMP] estate: fix the default date lambda parameter error [IMP] estate_account: add chapter 13 [IMP] estate: add chapter 14 [IMP] estate, estate_account: fix author and license warning [IMP] estate: apply some PR suggestions [IMP] estate: prefix ondelete method with _unlink Co-authored-by: Cemal Faruk Güney <63407686+cgun-odoo@users.noreply.github.com> [IMP] estate: apply R&D string convention [IMP] estate: apply PR suggestions [IMP] estate_account: make action_property_sold more extensible and maintainable [IMP] estate, estate_account: implement tests and fix code to pass them [IMP] estate: implement test for garden uncheck behavior [IMP] estate: change test class names according to the conventions [IMP] estate: change references to self.properties[i] to their own variables for clarity [IMP] estate: change import style Co-authored-by: Cemal Faruk Güney <63407686+cgun-odoo@users.noreply.github.com> [IMP] estate: apply PR suggestions [IMP] estate: make test imports clean and not relative [IMP] estate_account: add test to check account.move creation on sell [IMP] estate_account: make test more robust by sorting invoice lines according to price [IMP] *: add empty tutorial commit
…e fields to property model and form view
…d in model and form
trcazier
force-pushed
the
19.0-tutorial-industry-trcaz
branch
from
f77cd91 to
bdb9f65
Compare
Co-authored-by: Abdelrahman Rashed <112213628+abdrahmanrashed@users.noreply.github.com>
abdrahmanrashed
left a comment
abdrahmanrashed
left a comment
There was a problem hiding this comment.
great job, just some small things and you are good to go
| <record id="api_public_write_rule" model="ir.rule"> | ||
| <field name="name">Public users can't write</field> | ||
| <field name="model_id" ref="real_estate_property_model"/> | ||
| <field name="domain_force">[(0, '=', 1)]</field> |
There was a problem hiding this comment.
what is the purpose of this rule?
the domain is false for all instances right?
There was a problem hiding this comment.
Yes, the tutorial said to allow read and write on the route (to allow the server action to be executed), but restrict the write user access with this domain
add an access right record to allow public users to read and write the model
prevent any write from the public user by adding a record rule for the write operation with an impossible domain (e.g. [('id', '=', False)])
| <field name="x_bedrooms"/> | ||
| <field name="x_living_area"/> | ||
| <field name="x_expected_price"/> | ||
| <field name="x_date_availability" optional="disabled"/> |
Co-authored-by: Abdelrahman Rashed <112213628+abdrahmanrashed@users.noreply.github.com>
vava-odoo
left a comment
vava-odoo
left a comment
There was a problem hiding this comment.
Well done. Just a few comments reading through the xml part. Don't hesitate to let me know if anything is unclear
| { | ||
| 'name': 'Estate (import)', | ||
| 'depends': [ | ||
| 'base', |
There was a problem hiding this comment.
Every module depends on base, so base is only mentioned if there is no other.
| 'base', |
| <field name="name">x_garden_orientation</field> | ||
| <field name="field_description">Garden Orientation</field> | ||
| <field name="ttype">selection</field> | ||
| <field name="selection">[('x_north', "North"), ('x_south', "South"), ('x_east', "East"), ('x_west', "West")]</field> |
There was a problem hiding this comment.
Wasn't it asked to use ir.model.fields.selection?
| <field name="model_id" ref="real_estate_property_model" /> | ||
| <field name="name">x_total_area</field> | ||
| <field name="field_description">Total Area</field> | ||
| <field name="ttype">float</field> |
There was a problem hiding this comment.
Makes not much sense, but since other areas are integers, this one should be an integer as well
|
|
||
| <!-- Actions --> | ||
|
|
||
| <record id="api_public_write_rule" model="ir.rule"> |
There was a problem hiding this comment.
If you have a security folder, this record should be there
| <field name="model">x_estate.property</field> | ||
| </record> | ||
|
|
||
| <record model="ir.model.access" id="access_real_estate_property_public"> |
| <field name="selection">True</field> | ||
| <field name="selection">[('x_accepted', "Accepted"), ('x_refused', "Refused")]</field> |
There was a problem hiding this comment.
it makes no sense to define twice the same field (only last one is taken into account)
| <field name="ttype">many2one</field> | ||
| <field name="relation">res.partner</field> | ||
| <field name="required">True</field> | ||
| <field name="on_delete">cascade</field> |
| <field name="name">x_estate.property.offer.view.list</field> | ||
| <field name="model">x_estate.property.offer</field> | ||
| <field name="arch" type="xml"> | ||
| <list string="Property offer list test string?"> |
| <field name="name">Property types</field> | ||
| <field name="res_model">x_estate.property.type</field> | ||
| <field name="view_mode">list</field> | ||
| <field name="context">{'search_default_available': True}</field> |
| <field name="name">Property tags</field> | ||
| <field name="res_model">x_estate.property.tag</field> | ||
| <field name="view_mode">list</field> | ||
| <field name="context">{'search_default_available': True}</field> |
vava-odoo
left a comment
vava-odoo
left a comment
There was a problem hiding this comment.
Went on and read your whole code. Overall super good, just noted a few things.
No need to update anything here, just food for thought
| @@ -0,0 +1,5 @@ | |||
| "id","name","sequence","property_ids","offer_ids","offer_count" | |||
There was a problem hiding this comment.
I bet you don't have to list all fields, only the one you define (and the required ones, obviously)
| <record id="property_1" model="estate.property"> | ||
| <field name="name">Big Villa</field> | ||
| <field name="property_type_id" ref="property_type_1"/> | ||
| <field name="state">new</field> |
There was a problem hiding this comment.
default value, not necessary
| <field name="state">new</field> |
| def _unlink_check_status(self): | ||
| for record in self: | ||
| if record.state not in ['new', 'cancelled']: | ||
| raise UserError(_("A property can only be deleted if its state is 'New' or 'Cancelled'")) |
There was a problem hiding this comment.
We try to have less technical messages for end-users, something like
| raise UserError(_("A property can only be deleted if its state is 'New' or 'Cancelled'")) | |
| raise UserError(_("A property can only be deleted if it is a new one or if it was cancelled.")) |
There was a problem hiding this comment.
Also note that _() has been mostly replaced by self.env._()
| if not record.offer_ids: | ||
| raise UserError(_("A property with no offers cannot be sold.")) |
There was a problem hiding this comment.
Isn't it too constraining the end user? You probably have specific cases where you want to bypass the "normal" flow and such conditions could be a burden for some users
There was a problem hiding this comment.
I think that was a requirement in the tutorial
| from odoo import _, api, exceptions, fields, models | ||
| from odoo.exceptions import UserError |
There was a problem hiding this comment.
| from odoo import _, api, exceptions, fields, models | |
| from odoo.exceptions import UserError | |
| from odoo import _, api, fields, models | |
| from odoo.exceptions import UserError |
| <field name="bedrooms" /> | ||
| <field name="living_area" filter_domain="[('living_area', '>=', self)]"/> | ||
| <field name="facades" /> | ||
| <filter name="available" string="Available" domain="['|', ('state', '=', 'new'), ('state', '=', 'offer_received')]"/> |
There was a problem hiding this comment.
shorter (and maybe clearer) this way
| <filter name="available" string="Available" domain="['|', ('state', '=', 'new'), ('state', '=', 'offer_received')]"/> | |
| <filter name="available" string="Available" domain="[('state', 'in', ['new', 'offer_received'])]"/> |
| } for record in self] | ||
|
|
||
| def action_property_sold(self): | ||
| invoice_vals = self.get_sold_property_invoice_values() |
There was a problem hiding this comment.
That's my coach who told me to write it like that 😇
He says it's because this way if the module is modified in another module that depends on it it only has to modify the get_ function but the action stays the same so there's no need to rewrite it entirely
|
|
||
| def action_property_sold(self): | ||
| invoice_vals = self.get_sold_property_invoice_values() | ||
| self.env['account.move'].create(invoice_vals) |
There was a problem hiding this comment.
I fear you need a sudo here, as not all you sellers will be able to create an invoice
There was a problem hiding this comment.
I don't know how that works, I haven't seen that yet
| self.invoice = self.env['account.move'].search(Domain('partner_id', '=', self.cozy_cottage.buyer_id.id) & Domain('move_type', '=', 'out_invoice')) | ||
| self.assertTrue(self.invoice) |
There was a problem hiding this comment.
If you only need to check the existence, use search_count with limit=1 (if you need the first record, then only limit=1)
| self.invoice = self.env['account.move'].search(Domain('partner_id', '=', self.cozy_cottage.buyer_id.id) & Domain('move_type', '=', 'out_invoice')) | ||
| self.assertTrue(self.invoice) | ||
|
|
||
| sorted_invoice_lines = self.invoice.invoice_line_ids.sorted(key=lambda r: r.price_unit) |
There was a problem hiding this comment.
What if the price of the house is really small? 🙃
There was a problem hiding this comment.
That's why I set the price at 250000 😇
4 participants
No description provided.