Skip to content

[ADD] estate: New module to manage estates #814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 42 commits into
base: 18.0
Choose a base branch
from

Conversation

baje-odoo
Copy link

No description provided.

@robodoo
Copy link

robodoo commented Jun 17, 2025

Pull request status dashboard

Copy link

@bso-odoo bso-odoo left a comment

Choose a reason for hiding this comment

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

Here are a few cosmetic comments.

@baje-odoo baje-odoo force-pushed the 18.0-fix-invoices-baje branch 2 times, most recently from 5f9528b to 8bf2ec2 Compare June 18, 2025 09:01
@bso-odoo
Copy link

Actually, the [ADD] prefix in commit titles is only when adding a new module, we use the prefix [IMP] for improvement when adding stuff inside a module, and [FIX] when making a fix on an existing feature.

@baje-odoo baje-odoo changed the title Chapter 2 [ADD] estate: New module to manage estates Jun 18, 2025
@baje-odoo baje-odoo force-pushed the 18.0-fix-invoices-baje branch 3 times, most recently from b008594 to 36574bb Compare June 19, 2025 08:20
Copy link

@bso-odoo bso-odoo left a comment

Choose a reason for hiding this comment

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

Here are a few comments:

@baje-odoo baje-odoo force-pushed the 18.0-fix-invoices-baje branch from a905a17 to 716c663 Compare June 19, 2025 13:36
@baje-odoo baje-odoo force-pushed the 18.0-fix-invoices-baje branch from 41b9431 to 2a00cff Compare June 20, 2025 07:06
@baje-odoo baje-odoo force-pushed the 18.0-fix-invoices-baje branch from 11b3175 to 759af29 Compare June 20, 2025 11:30
@baje-odoo baje-odoo force-pushed the 18.0-fix-invoices-baje branch from 759af29 to d5a3567 Compare June 20, 2025 12:32
Copy link

@bso-odoo bso-odoo left a comment

Choose a reason for hiding this comment

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

Not much to say.
There are remaining ' vs ", some extra unneeded files, etc... and fixes in late commits instead of "since the start".
But better continue with further training, if you have time later and want to practice more git cleanups, that's always possible too.

@api.ondelete(at_uninstall=False)
def _unlink_if_state_is_new_or_cancelled(self):
if any(record.state not in ('new', 'canceled') for record in self):
raise UserError("Only new and canceled properties can be deleted.")

Choose a reason for hiding this comment

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

Those strings are not implicitly translated, they have to be explicitly translated using _ (from odoo import _)

Oh, you actually do it in further commits. This is also a reason to include the fix inside the original commit. 😉

@@ -0,0 +1,3 @@
{
"python.languageServer": "None"

Choose a reason for hiding this comment

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

Of course, on real PRs, such files wouldn't be included.

@baje-odoo baje-odoo force-pushed the 18.0-fix-invoices-baje branch from a4cf5b7 to 374b700 Compare June 24, 2025 07:35
@baje-odoo baje-odoo force-pushed the 18.0-fix-invoices-baje branch from 7aec092 to d704def Compare June 24, 2025 14:21
@baje-odoo baje-odoo force-pushed the 18.0-fix-invoices-baje branch from 54166bc to f93b2e8 Compare June 25, 2025 08:12
Comment on lines 18 to 21
onWillUpdateProps(() => {
this.chart.destroy();
this.renderChart();
})
Copy link

@bso-odoo bso-odoo Jun 26, 2025

Choose a reason for hiding this comment

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

At this stage, this.props are the old values. onWillUpdateProps's function receives the new values as parameter.

@baje-odoo baje-odoo force-pushed the 18.0-fix-invoices-baje branch from 73f555e to cf8ca37 Compare June 26, 2025 09:26
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