aryep - Technical training

[aryep-odoo]

No description provided.

[robodoo]

[qucol-odoo]

Thank you for your great work 🙏 I left a few comments, try to apply the suggestions if you agree with them :)

Could you try to rename your commits to match our guideline?

[aryep-odoo]

@qucol-odoo thank you so much for your first batch of comments! There were definitely several things and details I had missed initially but most of them should have been resolved by now

I left some follow-ups in some of your recommendations with some questions. Let me know what you think, and once again, thanks for taking the time to check the PR

[qucol-odoo]

Very nice work, good job 👌 I left a few comments :)

Regarding your commit history, I have two comments to share:

1. You should only have two [ADD] commits, because you're only adding two modules: estate and estate_account. All the other commits that are not adding new modules should (probably) be [IMP]. As a reminder, you can see all the tags here.
2. In a real case scenario, it wouldn't make much sense to have commits fixing logic/syntax issues from other commits belonging to the same PR, because it means that you are introducing an issue and fixing it simultaneously: it would be better to simply not introduce it at all. It would be a great exercise for you to try to get rid of all the [FIX] commits in your PR (hint: you should probably use git rebase -i). Ultimately you could also squash commits to have one per chapter, or even squash everything as one big [ADD] commit, it's kinda up to you, as long as you experiment a bit with the interactive rebasing (mostly the edit and squash options) :)

[qucol-odoo]

Good job finding the translation function! It is a better practice to use it as self.env._(...) than simply _, as explained in its docstring, for the reasons explained in this commit.

[qucol-odoo]

Few things to say here:

1. An action should (usually) only operate on one record, so you don't need to loop over self, and instead you should call self.ensure_one() at the very start of the action.
2. When updating multiple fields of a same record, you can batch the writes using the \write method (as you did further down the method actually)
3. You should avoid using search in a loop. In this case it ends up being fine because we're removing the loop, but otherwise it would have been a bad practice. An alternative is to use _read_group and then to loop over the result (example)

Suggested change

	def action_accept(self):
	for record in self:
	record.status = "accepted"
	record.property_id.state = "offer_accepted"
	record.property_id.partner_id = record.partner_id
	record.property_id.selling_price = record.price
	# Refuse other offers for the same property
	other_offers = self.search([
	("property_id", "=", record.property_id.id),
	("id", "!=", record.id),
	])
	other_offers.write({"status": "refused"})
	def action_accept(self):
	self.ensure_one()
	self.status = "accepted"
	self.property_id.write({
	'state': 'offer_accepted',
	'partner_id': self.partner_id.id,
	'selling_price': self.price,
	})
	# Refuse other offers for the same property
	self.search([
	("property_id", "=", self.property_id.id),
	("id", "!=", record.id),
	]).write({"status": "refused"})

[qucol-odoo]

Suggested change

	for record in self:
	self.ensure_one()

[qucol-odoo]

If you don't use them, do not create them 👌

[qucol-odoo]

Creating in batch is a great practice, but do you really need two properties? Their only difference is the price, which I don't think really changes anything in your tests. I would either:

• Create only one property
• Create two properties that have significant differences (for example, one with a garden and one without)

As a side note, keeping the two properties together doesn't sound like the best idea here, since it forces you to call `search' at the start of each test, which is unnecessarily costly. Instead, you could do the following:

Suggested change

	cls.estate_properties = cls.env["estate.property"].create([
	cls.property_1, cls.property_2 = cls.env["estate.property"].create([

with more appropriate names if your properties have significant differences (e.g., property_with_garden, ...).

[qucol-odoo]

You could just create a property with those values from the start, instead of searching and editing an existing one

[qucol-odoo]

The idea of the _sell_main_property is good, but here you search for the main_property twice because of it. You can fix that by applying my proposition above (separate the properties at creation), or by updating the _sell_main_property to something like _sell_property(property).

[qucol-odoo]

Couldn't you just use the other property, that doesn't have any offers? Unless you want to test the deletion of offers, but then you should precise it in the docstring.

[qucol-odoo]

Some characters can be misinterpreted by the XML parser, it's always better to be cautious

Suggested change

	<field name="living_area" filter_domain="[('living_area', '>=', self)]"/>
	<field name="living_area" filter_domain="[('living_area', '&gt;=', self)]"/>