Conversation
AlessandroLupo
left a comment
There was a problem hiding this comment.
Nice work! :D I added a couple of comments. Please make sure that your commit messages follow the guidelines. 👍
estate/models/__init__.py
Outdated
| # -*- coding: utf-8 -*- | ||
| # Part of Odoo. See LICENSE file for full copyright and licensing details. | ||
|
|
||
| from . import estate_property No newline at end of file |
There was a problem hiding this comment.
Remember to add an empty line at the end of each file ;)
91e995b to
34e5c4f
Compare
AlessandroLupo
left a comment
There was a problem hiding this comment.
Thanks for the changes, it looks fine now! 👍
AlessandroLupo
left a comment
There was a problem hiding this comment.
Good job, it's nice that you got a green runbot. 👍 I added a couple of minor comments.
As a git exercise, you could try to do an interactive rebase and leave only one commit per chapter (you could also add the chapter number to the commit messages). To achieve this, you could just squash the commit fixing the runbot with the previous commit. Let me know if you need support with git!
estate/views/estate_list_views.xml
Outdated
| <field name="arch" type="xml"> | ||
| <list string="Properties"> | ||
| <field name="name"/> | ||
| <field name="tag_ids"/> |
There was a problem hiding this comment.
You could use widget="many2many_tags" to have the actual tag names displayed on each line ;)
estate/security/ir.model.access.csv
Outdated
| estate.access_estate_property,access_estate_property,estate.model_estate_property,base.group_user,1,1,1,1 | ||
| estate.access_property_type,access_property_type,estate.model_estate_property_type,base.group_user,1,1,1,1 | ||
| estate.access_property_tag,access_property_tag,estate.model_estate_property_tag,base.group_user,1,1,1,1 | ||
| estate.access_property_offer,access_property_offer,estate.model_estate_property_offer,base.group_user,1,1,1,1 No newline at end of file |
There was a problem hiding this comment.
No empty line at the end of file 😄
|
|
||
| class EstatePropertyType(models.Model): | ||
| _name = "estate.property.type" | ||
| _description = "A table for estate properties types" |
There was a problem hiding this comment.
Nitpick: briefly explore the Odoo codebase searching for _descrption. You will see that we tend to use a more concise style. I would use something like "Estate Property Type".
e9bada5 to
f8501db
Compare
a2148ad to
c501952
Compare
AlessandroLupo
left a comment
There was a problem hiding this comment.
Good job with the git exercise! 👍
estate/views/estate_list_views.xml
Outdated
| <field name="model">estate.property</field> | ||
| <field name="arch" type="xml"> | ||
| <list string="Properties"> | ||
| <list string="Properties" decoration-success="state == 'offer_received' or state == 'offer_accepted'" decoration-bf="state == 'offer_accepted'" decoration-muted="state == 'sold'"> |
There was a problem hiding this comment.
You could also use decoration-success="state in ('offer_received', 'offer_accepted')"
There was a problem hiding this comment.
Try to search the Odoo codebase for the line _inherit = 'res.users'. You will see what is the common style for inherited classes. The class should be named "ResUsers" and the file should be named "res_users". ;)
1bfcf89 to
0338f3c
Compare
| from odoo import models | ||
|
|
||
|
|
||
| class EstatePropertyInherited(models.Model): |
There was a problem hiding this comment.
Nitpick: the conventional name here would be EstateProperty (no need to add "Inherited") ;)
72594a6 to
f7ee509
Compare
awesome_owl/static/src/card/card.xml
Outdated
| <h5 class="card-title"><t t-esc="props.title"/></h5> | ||
| <p class="card-text"> | ||
| <t t-out="props.content"/> |
There was a problem hiding this comment.
You could also do:
<h5 class="card-title" t-out="props.title"/>
<p class="card-text" t-out="props.content"/>
| if (this.props.onChange) { | ||
| this.props.onChange(); | ||
| } |
There was a problem hiding this comment.
Here you could use optional chaining: this.props.onChange?.().
| } | ||
|
|
||
| addTodo(ev) { | ||
| if (ev.keyCode === 13 && ev.target.value != "") { |
There was a problem hiding this comment.
You could also use ev.key === "Enter" (see here). keyCode is deprecated
| static template = "awesome_owl.todo_list"; | ||
|
|
||
| setup() { | ||
| this.todos = useState([]); |
There was a problem hiding this comment.
Here I would do this.state = useState({todos: []}), and later access the todos array using this.state.todos, so it is clear that todos is in the component state.
AlessandroLupo
left a comment
There was a problem hiding this comment.
You're doing well! Just two comments
| static props = { | ||
| title: String, | ||
| content: String, | ||
| title: { type: String }, |
There was a problem hiding this comment.
Just to be clear, title: String was fine too (it is assumed implicitly that you are defining the type), but feel free to use the style you prefer 👍
| removeTodo = (todoId) => { | ||
| const index = this.todos.findIndex((t) => t.id === todoId); | ||
| if (index > 0) { | ||
| this.todos.splice(index, 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Will this work if we try to remove the first element in the todo list (index 0)?
|
Thanks for your comments. I'm on it |
86c1fa8 to
4c76c44
Compare
4c76c44 to
4efd9e6
Compare
AlessandroLupo
left a comment
There was a problem hiding this comment.
You're doing good, I left couple of comments. 👍 Don't hesitate to ask if something is not clear!
|
|
||
| increment() { | ||
| this.state.value++; | ||
| this.props.onChange?.(this.props.onChange()); |
There was a problem hiding this comment.
Probably what you want to do here is this.props.onChange?.(), which means "if onChange exists, call it".
The way you coded it, this.props.onChange() gets evaluated once, and then passed as an argument to a second call to this.props.onChange() (that's why the counter increases by two times instead of one). Moreover the purpose of ?.() is undermined, because if this.props.onChange does not exist, you would still get an error. ;)
| } | ||
|
|
||
| focusInput() { | ||
| this.myRef.el.focus(); |
There was a problem hiding this comment.
Something seems incomplete here, right? myRef is not defined and focusInput is never called?
See how it is done here, for example ;)
b9b6b92 to
bc8b5a3
Compare
AlessandroLupo
left a comment
There was a problem hiding this comment.
The dashboard is looking nice! 👍 Two minor comments
| const loadStatistics = memoize(async () => { | ||
| return rpc("/awesome_dashboard/statistics", {}); | ||
| }); |
There was a problem hiding this comment.
Just a small remark. The function return rpc("/awesome_dashboard/statistics", {}); does nothing asyncronous. It actually just returns the promise from rpc without awaiting for it. So tecnically the keyword async is not needed. Anyway since it does no harm and makes the code clearer, someone may prefer to add it. So it is just a matter of style, up to you 👍
Also, return can be made implicit:
const loadStatistics = memoize(async () => rpc("/awesome_dashboard/statistics", {}));
There was a problem hiding this comment.
You could have .o_dashboard_row, .o_header_dashboard_item, and .o_average_quantity nested in .o_dashboard
fa2964f to
f55c683
Compare
f55c683 to
13432e1
Compare

No description provided.