Skip to content

Comments

Tutorials#1174

Open
coder-odoo wants to merge 16 commits intoodoo:19.0from
odoo-dev:19.0-Tutorials-coder
Open

Tutorials#1174
coder-odoo wants to merge 16 commits intoodoo:19.0from
odoo-dev:19.0-Tutorials-coder

Conversation

@coder-odoo
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Feb 16, 2026

Pull request status dashboard

@AlessandroLupo AlessandroLupo self-requested a review February 16, 2026 13:56
Copy link

@AlessandroLupo AlessandroLupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! :D I added a couple of comments. Please make sure that your commit messages follow the guidelines. 👍

# -*- 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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to add an empty line at the end of each file ;)

Copy link

@AlessandroLupo AlessandroLupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, it looks fine now! 👍

Copy link

@AlessandroLupo AlessandroLupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

<field name="arch" type="xml">
<list string="Properties">
<field name="name"/>
<field name="tag_ids"/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use widget="many2many_tags" to have the actual tag names displayed on each line ;)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No empty line at the end of file 😄


class EstatePropertyType(models.Model):
_name = "estate.property.type"
_description = "A table for estate properties types"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

@coder-odoo coder-odoo force-pushed the 19.0-Tutorials-coder branch 4 times, most recently from a2148ad to c501952 Compare February 19, 2026 12:08
Copy link

@AlessandroLupo AlessandroLupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job with the git exercise! 👍

<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'">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use decoration-success="state in ('offer_received', 'offer_accepted')"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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". ;)

Copy link

@AlessandroLupo AlessandroLupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! 👍

from odoo import models


class EstatePropertyInherited(models.Model):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: the conventional name here would be EstateProperty (no need to add "Inherited") ;)

@coder-odoo coder-odoo force-pushed the 19.0-Tutorials-coder branch 2 times, most recently from 72594a6 to f7ee509 Compare February 20, 2026 10:56
Copy link

@AlessandroLupo AlessandroLupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work 👍

Comment on lines 7 to 9
<h5 class="card-title"><t t-esc="props.title"/></h5>
<p class="card-text">
<t t-out="props.content"/>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also do:

<h5 class="card-title" t-out="props.title"/>
                <p class="card-text" t-out="props.content"/>

Comment on lines +15 to +17
if (this.props.onChange) {
this.props.onChange();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you could use optional chaining: this.props.onChange?.().

}

addTodo(ev) {
if (ev.keyCode === 13 && ev.target.value != "") {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use ev.key === "Enter" (see here). keyCode is deprecated

static template = "awesome_owl.todo_list";

setup() {
this.todos = useState([]);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@AlessandroLupo AlessandroLupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're doing well! Just two comments

static props = {
title: String,
content: String,
title: { type: String },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Comment on lines 38 to 43
removeTodo = (todoId) => {
const index = this.todos.findIndex((t) => t.id === todoId);
if (index > 0) {
this.todos.splice(index, 1);
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work if we try to remove the first element in the todo list (index 0)?

@coder-odoo
Copy link
Author

Thanks for your comments. I'm on it

@coder-odoo coder-odoo force-pushed the 19.0-Tutorials-coder branch 3 times, most recently from 86c1fa8 to 4c76c44 Compare February 23, 2026 09:45
Copy link

@AlessandroLupo AlessandroLupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something seems incomplete here, right? myRef is not defined and focusInput is never called?
See how it is done here, for example ;)

Copy link

@AlessandroLupo AlessandroLupo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dashboard is looking nice! 👍 Two minor comments

Comment on lines 5 to 7
const loadStatistics = memoize(async () => {
return rpc("/awesome_dashboard/statistics", {});
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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", {}));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have .o_dashboard_row, .o_header_dashboard_item, and .o_average_quantity nested in .o_dashboard

@coder-odoo coder-odoo force-pushed the 19.0-Tutorials-coder branch 4 times, most recently from fa2964f to f55c683 Compare February 24, 2026 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants