Onboarding ALMAG #1073
Onboarding ALMAG #1073
Conversation
|
|
|
Hello, can you please ensure that your runbot is green 😄 |
I fixed the stylistic issues and it passes the build tests. The only thing that makes it red at this point is the message convention for my first commit (I didn't add [ADD] at the start). For now I want to continue working on the tutorial, but I can fix that later if it's critical? |
|
My recommendation would be to fix your first commit now to maintain a clean base. To achieve this, you can use a simple interactive rebase. 😄 |
lost-odoo
left a comment
lost-odoo
left a comment
There was a problem hiding this comment.
I already made a small review. I just have some nitpicking stuff. Nothing too bad. 😄
estate/__manifest__.py
Outdated
| 'version': '0.1', | ||
| 'depends': ['base', 'web'], | ||
| 'application': True, | ||
| 'installable': True, |
There was a problem hiding this comment.
Here installable is not mandatory as application is set to true. When application is set, installable is automatically set.
There was a problem hiding this comment.
No but I think someone is doing some ci tests to unify the manifests file so it is better to try to keep it at the minimum
magai2002
force-pushed
the
master-onboarding-almag
branch
from
ebdec66 to
0f1ce9e
Compare
|
Hey don't forget to close all the comments I did on the review when you did them 😄 |
magai2002
force-pushed
the
master-onboarding-almag
branch
from
aa1f0b8 to
546648b
Compare
LoganStaelens
left a comment
LoganStaelens
left a comment
There was a problem hiding this comment.
This will be the final review. There are some small stuff nothing bad 😄 Good work !
Be careful about loops and the use of the orm. If it can be done outside the loop do it instead. It is always better to batch it whenever possible.
estate/models/estate_property.py
Outdated
| from odoo import api, fields, models | ||
| from dateutil.relativedelta import relativedelta | ||
| from datetime import date | ||
| from odoo.exceptions import UserError, ValidationError | ||
| from odoo.tools import float_compare, float_is_zero | ||
|
|
There was a problem hiding this comment.
For the imports try to order them by
any external dependency at first then odoo ones
estate/models/estate_property.py
Outdated
| @api.depends('offer_ids.price') | ||
| def _compute_best_price(self): | ||
| for record in self: | ||
| offers = record.offer_ids.mapped('price') |
There was a problem hiding this comment.
It is not offers but prices here so renaming this should be better IMO
estate/models/estate_property.py
Outdated
| raise ValidationError("The selling price cannot be lower than 90% of the expected price!") | ||
|
|
||
| @api.ondelete(at_uninstall=False) | ||
| def _unlink_if_new_or_cancelled(self): |
There was a problem hiding this comment.
Maybe you could rename to something like this: _ondelete_property. It is just a suggestion.
estate/models/estate_property.py
Outdated
| def _unlink_if_new_or_cancelled(self): | ||
| for record in self: | ||
| if record.state not in ['new', 'cancel']: | ||
| raise UserError("Only new or cancelled properties can be deleted!") |
There was a problem hiding this comment.
Don't forget to translate the error strings using self.env._("The text")
| @api.depends('create_date', 'validity') | ||
| def _compute_date_deadline(self): | ||
| for record in self: | ||
| start_date = record.create_date.date() if record.create_date else fields.Date.today() |
There was a problem hiding this comment.
Isn't record.create_date better here ? instead of record.create_date.date().
| start_date = record.create_date.date() if record.create_date else fields.Date.today() | |
| start_date = record.create_date or fields.Date.today() |
|
|
||
| name = fields.Char(required=True) | ||
| sequence = fields.Integer('Sequence', default=1, help="Used to order stages. Lower is better.") | ||
| property_ids = fields.One2many('estate.property', 'property_type_id') |
There was a problem hiding this comment.
Try to name your parameters whenever it is possible so it is easier to read 😄
estate_account/__manifest__.py
Outdated
| 'author': "Odoo ALMAG", | ||
| 'website': "https://www.odoo.com", | ||
| 'category': 'Tutorials', | ||
| 'version': '0.1', |
There was a problem hiding this comment.
| 'version': '0.1', |
| { | ||
| "name": "Estate Accounting", | ||
| 'summary': """ | ||
| App module created specifically for the Server Framework 101 tutorial. |
There was a problem hiding this comment.
Maybe add better summary and description.
estate_account/__manifest__.py
Outdated
| "estate", | ||
| "account", | ||
| ], | ||
| "data": [], |
| { | ||
| "partner_id": record.buyer_id.id, | ||
| "move_type": "out_invoice", | ||
| "journal_id": self.env["account.journal"].search([("type", "=", "sale")], limit=1).id, | ||
| "invoice_line_ids": [ | ||
| Command.create({ | ||
| "name": f"Commission for {record.name}", | ||
| "quantity": 1, | ||
| "price_unit": record.selling_price * 0.06 | ||
| }), | ||
| Command.create({ | ||
| "name": "Administrative Fees", | ||
| "quantity": 1, | ||
| "price_unit": 100.00 | ||
| }), | ||
| ], | ||
| } |
There was a problem hiding this comment.
Here create takes a list, a single dict is kinda deprecated.
| { | |
| "partner_id": record.buyer_id.id, | |
| "move_type": "out_invoice", | |
| "journal_id": self.env["account.journal"].search([("type", "=", "sale")], limit=1).id, | |
| "invoice_line_ids": [ | |
| Command.create({ | |
| "name": f"Commission for {record.name}", | |
| "quantity": 1, | |
| "price_unit": record.selling_price * 0.06 | |
| }), | |
| Command.create({ | |
| "name": "Administrative Fees", | |
| "quantity": 1, | |
| "price_unit": 100.00 | |
| }), | |
| ], | |
| } | |
| [{ | |
| "partner_id": record.buyer_id.id, | |
| "move_type": "out_invoice", | |
| "journal_id": self.env["account.journal"].search([("type", "=", "sale")], limit=1).id, | |
| "invoice_line_ids": [ | |
| Command.create({ | |
| "name": f"Commission for {record.name}", | |
| "quantity": 1, | |
| "price_unit": record.selling_price * 0.06 | |
| }), | |
| Command.create({ | |
| "name": "Administrative Fees", | |
| "quantity": 1, | |
| "price_unit": 100.00 | |
| }), | |
| ], | |
| }] |
|
Also as a final exercise, can I ask you to squash all your commits into one, fix the runbot and give a good title and description to this PR. |
4 participants
No description provided.