valvi - technical training - #1388
Conversation
|
|
qucol-odoo
left a comment
qucol-odoo
left a comment
There was a problem hiding this comment.
Helloo, thank you for your great work 🙏 I left a few comments, try to apply them whenever you find the time :) It's also a good practice to try to have a green runbot, you can see the current issues on the runbot build page :)
d6af469 to
056ee98
Compare
52ec011 to
807efd6
Compare
modified: estate/views/estate_property_views.xml new file: git-challenge.txt modified: estate/views/estate_property_views.xml
d875cee to
c1c10ba
Compare
qucol-odoo
left a comment
qucol-odoo
left a comment
There was a problem hiding this comment.
Very good job overall 👍 👏 I just left a few comments
In a real case scenario, it wouldn't make much sense to have commits fixing 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]/[CLN]/... 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) :)
| def action_status_accepted(self): | ||
| self.ensure_one() | ||
|
|
||
| if self.status == "accepted" or self.status == "refused": |
There was a problem hiding this comment.
| if self.status == "accepted" or self.status == "refused": | |
| if self.status in ('accepted', 'refused'): |
| <search> | ||
| <field name="name" string="Estate" /> | ||
| <field name="selling_price" /> | ||
| <field name="living_area" string="Living Area (sqm)" filter_domain="[('living_area', '>=', self)]" /> |
There was a problem hiding this comment.
[Optional]
Although it technically doesn't break, the XML parser can sometimes struggle with some characters, such as >.
| <field name="living_area" string="Living Area (sqm)" filter_domain="[('living_area', '>=', self)]" /> | |
| <field name="living_area" string="Living Area (sqm)" filter_domain="[('living_area', '>=', self)]" /> |
| @tagged("post_install", "-at_install") | ||
| class TestEstatePropertyHttp(HttpCase): | ||
| def test_garden_reset(self): | ||
| f = Form(self.env["estate.property"]) |
There was a problem hiding this comment.
This is ok because the test/method is short, but you should try to use self-explanatory variable name. An easy and short alternative here was form or even property_form.
3 participants
No description provided.