daloe - Technical Training#1276
Conversation
|
|
This adds search, list, and form view to the estate property model following server 101 chapter 6.
Estates can now have multiple offers, tags, and one type. Offers can be made directly on an estate form, and all offers can be viewed via a property offers list menu.
Estate form now adds default data when garden is checked, and clears data when unchecked. Estate form now also shows current best offer. Offer form will compute validity days on deadline selection, and also inversely adjust validity days to match final saved deadline on save.
…ccept and refuse actions on offers
…ice, and type/tag uniqueness
…ctionality with redirection. Some default views set
Creating offers will set property to offer_received, and offers lower than current minimum can no longer be created. available properties can be accessed through their seller's user profile
…as sold, create an invoice when accepting an offer
daloe-odoo
force-pushed
the
19.0-technical-training-daloe
branch
from
cb2724a to
7968990
Compare
|
@Megaaaaaa ready for review |
Megaaaaaa
left a comment
Megaaaaaa
left a comment
There was a problem hiding this comment.
Hello 👋
Here's your review 🫡 That's a really nice PR 👍
There's quite a lot of stuff but don't worry about it, it's a lot of the same small things that repeat themselves.
Keep in mind that a review is not the utlimate solution, it's only someone seeing things he would have done differently and offering an alternative. You're always free to answer with an other alternative or even say you disagree and bring your arguments. For stuff that's not just styling or conventions, don't tak the wrong habit of just blindly applying comments, you spent time developping the thing, everything you've tried doesn't show up in the diff, only the final one. You probably have very interesting things to say to the reviewer about the decisions you made.
What I would suggest you do and continue to do while you're not the most comfortable with odoo's structure is to not apply all the changes at once. For example, changing a model's _name has a lot of impact and if you change everything at once, you might end up with a lot of errors when running the database. Splitting the review in multiple steps and trying to run your db in between those can save you a lot of time in the long run.
Also a quick tip for the methodology when applying reviews changes: What I like to do it put a reaction like a thumbsup on a message once I have made the change locally. Then when everything is marked with something, I push, go through my diff and mark as resolved the things that are now outdated so nothing is left forgotten.
If you have any question, don't hesitate to ask!
| from . import estate_property | ||
| from . import estate_property_type | ||
| from . import estate_property_tag | ||
| from . import estate_property_offer | ||
| from . import res_users |
There was a problem hiding this comment.
Order your python import alphabetically 👍
| from . import estate_property | |
| from . import estate_property_type | |
| from . import estate_property_tag | |
| from . import estate_property_offer | |
| from . import res_users | |
| from . import estate_property | |
| from . import estate_property_offer | |
| from . import estate_property_tag | |
| from . import estate_property_type | |
| from . import res_users |
|
|
||
| class EstateProperty(models.Model): | ||
| _name = "estate.property" | ||
| _description = "technical training estate property model" |
There was a problem hiding this comment.
So the _description is used in places where there is a need to be kind of a human readable description of the model like some logs or error messages for example. It's purely preferences but we tend to have it describe more what it does and I feel like "technical training" is more of a reason for its existence and "model" is risking being a duplicate with any "Problem with model %s" error. So i would just name Estate Property. But wathever you pick, you should use uppercases 👍
| _description = "technical training estate property model" | |
| _description = "Estate Property" |
| _description = "technical training estate property model" | ||
| _order = "id desc" | ||
|
|
||
| # RELATIONAL FIELDS |
There was a problem hiding this comment.
We try to have as little comments in the code as possible so you can put it as a help for structure while you dev but they shouldn't be in your final version ready for merge. The only exceptions are very technical things that are not explicit enoug hfrom the code itself or things that might look wrong but are actually the intended behavior.
| # RELATIONAL FIELDS |
| copy=False, | ||
| ) | ||
|
|
||
| # BASIC FIELDS |
| ) | ||
|
|
||
| status = fields.Selection( | ||
| [["new", "New"], ["offer_received", "Offer Received"], ["offer_accepted", "Offer Accepted"], ["sold", "Sold"], ["cancelled", "Cancelled"]], |
There was a problem hiding this comment.
Doesn't change much but we tend to use a list of tuples for selection declarations and when they get to three or four or more elements, we multiline the list for clarity and an easier modification in the future. One very important thing in R&D at odoo is to try to keep the commit history as clean as possible so anytime you multiline something, you allow someone to modify a smaller part of the code to get the same result keeping a clearer history in the end. A lot of our styling guidelines revolves around that.
| [["new", "New"], ["offer_received", "Offer Received"], ["offer_accepted", "Offer Accepted"], ["sold", "Sold"], ["cancelled", "Cancelled"]], | |
| selection=[ | |
| ("new", "New"), | |
| ("offer_received", "Offer Received"), | |
| ("offer_accepted", "Offer Accepted"), | |
| ("sold", "Sold"), | |
| ("cancelled", "Cancelled"), | |
| ], |
| 'depends': ['base'], | ||
| 'application': True, | ||
| 'installable': True, | ||
| "author": "daloe", |
There was a problem hiding this comment.
As an odoo employee, you can't sign your work in your own name, the author of this module is "Odoo S.A." 😄
| @@ -0,0 +1,3 @@ | |||
| id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink | |||
| access_inherited_estate_property,access_inherited_estate_property,model_estate_property,base.group_user,1,1,1,1 | |||
| access_inherited_estate_property_offer,access_inherited_estate_property_offer,model_estate_property_offer,base.group_user,1,1,1,1 No newline at end of file | |||
| 'name': 'Estate Accounting', | ||
| 'depends': ['estate', 'account'], | ||
| 'installable': True, | ||
| "author": "daloe", |
| try: | ||
| estate_property.check_access('write') | ||
| except AccessError: | ||
| return self.env['account.move'] |
There was a problem hiding this comment.
Returning an emty recordset as an error is misleading imo as the user will not have feedback on the error. I would raise the AccessError honestly. Why'd you do this way ?
| def action_set_sold(self): | ||
| for estate_property in self: | ||
| if not self.env['account.move'].has_access('create'): | ||
| try: | ||
| estate_property.check_access('write') | ||
| except AccessError: | ||
| return self.env['account.move'] | ||
|
|
||
| self.env['account.move'].create({ | ||
| "partner_id": estate_property.buyer_id.id, | ||
| "move_type": "out_invoice", | ||
| "invoice_line_ids": [ | ||
| Command.create({ | ||
| "name": "6% selling price", | ||
| "quantity": 1, | ||
| "price_unit": estate_property.selling_price * 0.06 | ||
| }), | ||
| Command.create({ | ||
| "name": "admin fees", | ||
| "quantity": 1, | ||
| "price_unit": 100000 | ||
| }) | ||
| ] | ||
| }) | ||
|
|
||
| return super().action_set_sold() |
There was a problem hiding this comment.
Actually I would change this entirely to just:
| def action_set_sold(self): | |
| for estate_property in self: | |
| if not self.env['account.move'].has_access('create'): | |
| try: | |
| estate_property.check_access('write') | |
| except AccessError: | |
| return self.env['account.move'] | |
| self.env['account.move'].create({ | |
| "partner_id": estate_property.buyer_id.id, | |
| "move_type": "out_invoice", | |
| "invoice_line_ids": [ | |
| Command.create({ | |
| "name": "6% selling price", | |
| "quantity": 1, | |
| "price_unit": estate_property.selling_price * 0.06 | |
| }), | |
| Command.create({ | |
| "name": "admin fees", | |
| "quantity": 1, | |
| "price_unit": 100000 | |
| }) | |
| ] | |
| }) | |
| return super().action_set_sold() | |
| def action_set_sold(self): | |
| result = super().action_set_sold() | |
| for estate_property in self: | |
| self.env['account.move'].create({ | |
| "partner_id": estate_property.buyer_id.id, | |
| "move_type": "out_invoice", | |
| "invoice_line_ids": [ | |
| Command.create({ | |
| "name": "6% selling price", | |
| "quantity": 1, | |
| "price_unit": estate_property.selling_price * 0.06 | |
| }), | |
| Command.create({ | |
| "name": "admin fees", | |
| "quantity": 1, | |
| "price_unit": 100000 | |
| }) | |
| ] | |
| }) | |
| return result |
And remove the estate_porperty_offer.py file as the set sold of the property will already handle that and you risk behavior (particularily invoice) duplication and it's easier to maintain a single spot anyway.
3 participants
No description provided.