Open
Conversation
Author
Author
jnc-odoo
reviewed
Sep 30, 2025
jnc-odoo
left a comment
jnc-odoo
left a comment
There was a problem hiding this comment.
In general, really good code, just comments about the good practice that you can find here:
https://www.odoo.com/documentation/19.0/contributing/development/coding_guidelines.html
| </field> | ||
| </record> | ||
| </data> | ||
| </odoo> No newline at end of file |
There was a problem hiding this comment.
As a good practice, we usually leave a blank line at the end of the file
| <record id="categorie_chocolade" model="bakker_koeken_categorie"> | ||
| <field name="name">Chocolade</field> | ||
| <field name="beschrijving">Koeken met chocolade</field> | ||
| <field name="active">True</field> |
There was a problem hiding this comment.
active True is not necessary as you set to True by default in your python definition
|
|
||
| @api.depends('verkoop_ids.aantal', 'verkoop_ids.totaal_bedrag', 'verkoop_ids.status') | ||
| def _compute_verkoop_stats(self): | ||
| for record in self: |
There was a problem hiding this comment.
I would have used something like "for bakker in self:"
to have something more visual than record
| totaal_omzet = fields.Float(string='Totaal Omzet', compute='_compute_verkoop_stats', store=True) | ||
| verkoop_count = fields.Integer(string='Aantal Verkopen', compute='_compute_verkoop_count') | ||
|
|
||
| @api.depends('verkoop_ids.aantal', 'verkoop_ids.totaal_bedrag', 'verkoop_ids.status') |
There was a problem hiding this comment.
We tend to use everywhere the same quote notation. So single or double quote everywhere and not a mix
Comment on lines
+38
to
+56
| @api.onchange('prijs_koek', 'voorraad_koek') | ||
| def _onchange_prijs_koek(self): | ||
| self._compute_totaal_inventarisatie() | ||
|
|
||
| @api.onchange('totaal_inventarisatie') | ||
| def _onchange_totaal_inventarisatie(self): | ||
| self._inverse_totaal_inventarisatie() | ||
|
|
||
| @api.depends('prijs_koek', 'voorraad_koek') | ||
| def _compute_totaal_inventarisatie(self): | ||
| for record in self: | ||
| record.totaal_inventarisatie = record.prijs_koek * record.voorraad_koek | ||
|
|
||
| def _inverse_totaal_inventarisatie(self): | ||
| for record in self: | ||
| if record.prijs_koek != 0: | ||
| record.voorraad_koek = record.totaal_inventarisatie / record.prijs_koek | ||
| else: | ||
| record.voorraad_koek = 0 |
There was a problem hiding this comment.
I think this could be rewritten just with a compute and an inverse or just with onchange ?
In my opinion, calling a compute in a onchange is not a good idea
| raise ValidationError("Geen voorraad beschikbaar!") | ||
|
|
||
| # Zoek of maak een standaard walk-in klant | ||
| walk_in_klant = self.env['res.partner'].search([('name', '=', 'Walk-in klant')], limit=1) |
There was a problem hiding this comment.
using a name for a search is never a good idea, it could change, another way would be to add a checkbox on the res.partner model and then search for the record that has this checkbox set
Suggested change
| walk_in_klant = self.env['res.partner'].search([('name', '=', 'Walk-in klant')], limit=1) | |
| walk_in_klant = self.env['res.partner'].search([('is_walk_in_client', '=', True)], limit=1) |
| from odoo.exceptions import ValidationError | ||
|
|
||
| class BakkerKoeken(models.Model): | ||
| _name = "bakker_koeken" |
There was a problem hiding this comment.
Suggested change
| _name = "bakker_koeken" | |
| _name = "bakker.koeken" |
Use of . for the name of the models
| klant_telefoon = fields.Char(string="Telefoon", related='partner_id.phone', readonly=True) | ||
| aantal = fields.Integer(string="Aantal", required=True, default=1, tracking=True) | ||
| prijs_per_stuk = fields.Float(string="Prijs per stuk", required=True, tracking=True) | ||
| korting_percentage = fields.Float(string="Korting (%)", default=0.0, tracking=True) |
There was a problem hiding this comment.
Suggested change
| korting_percentage = fields.Float(string="Korting (%)", default=0.0, tracking=True) | |
| korting_percentage = fields.Float(string="Korting (%)", tracking=True) |
If I am not mistaken in odoo, a float field has a default value of 0.0
| } | ||
|
|
||
|
|
||
| class BakkerVerkoopWizard(models.TransientModel): |
There was a problem hiding this comment.
For wizard mode, you can add a new folder called wizards where you put all the transient models
| ], string="Betaal Methode", default='cash') | ||
|
|
||
| # Gerelateerde velden van de klant | ||
| klant_email = fields.Char(string="Email", related='partner_id.email', readonly=True) |
There was a problem hiding this comment.
Suggested change
| klant_email = fields.Char(string="Email", related='partner_id.email', readonly=True) | |
| klant_email = fields.Char(string="Email", related='partner_id.email') |
A related field should be readonly=True by default if I remember well
Author
|
Hey
Thanks a lot for the effort of reviewing the code! I will take the good practices and tips with me in the future : ) !!
Met vriendelijke groeten | Best regards | Salutations
Wout Hostens
ERP Support
***@***.***<http://www.energyvision.be/>
M: +32 472 06 56 62 - T: +32 9 38 38 296 - E: ***@***.***
HQ Ghent: Bijenstraat 28 - 9051 Ghent
Office Brussels: Avenue du Laerbeek 74 - 1090 Brussels
Follow EnergyVision:
***@***.******@***.******@***.***<https://www.linkedin.com/company/energyvisionbe>
***@***.***<https://campaigns.signature365.com/eu-KLRaU5L6yq12UiPC-soP2O732ruKWH9YJ/gen_KAz38JNAT0thL8Ys/go/Gq6>
Please consider the environment before printing this email
From: jnc-odoo ***@***.***>
Date: Tuesday, 30 September 2025 at 12:34
To: odoo/technical-training ***@***.***>
Cc: Wout Hostens ***@***.***>, Author ***@***.***>
Subject: Re: [odoo/technical-training] Wout Hostens (PR #74)
U ontvangt niet vaak e-mail van ***@***.*** Ontdek waarom dit belangrijk is<https://aka.ms/LearnAboutSenderIdentification>
@jnc-odoo commented on this pull request.
In general, really good code, just comments about the good practice that you can find here:
https://www.odoo.com/documentation/19.0/contributing/development/coding_guidelines.html
________________________________
In Bakker/data/bakker_email_template.xml<#74 (comment)>:
+ <p>Dank u wel voor uw bezoek aan onze bakkerij! We hopen u snel weer te zien.</p>
+
+ <div style="margin-top: 30px; text-align: center; color: #666;">
+ <p>Met vriendelijke groet,<br/>
+ <strong>Het Bakkerij Team</strong></p>
+ </div>
+ </div>
+
+ <footer style="background-color: #8B4513; color: white; padding: 15px; text-align: center;">
+ <p style="margin: 0;">🍪 Uw lokale bakkerij - Vers gebakken elke dag!</p>
+ </footer>
+</div>
+ </field>
+ </record>
+ </data>
+</odoo>
As a good practice, we usually leave a blank line at the end of the file
________________________________
In Bakker/data/bakker_koeken_categorie_data.xml<#74 (comment)>:
@@ -0,0 +1,35 @@
+<?xml version="1.0" encoding="utf-8"?>
+<odoo>
+ <data>
+ <!-- Categorieën voor koeken -->
+ <record id="categorie_chocolade" model="bakker_koeken_categorie">
+ <field name="name">Chocolade</field>
+ <field name="beschrijving">Koeken met chocolade</field>
+ <field name="active">True</field>
active True is not necessary as you set to True by default in your python definition
________________________________
In Bakker/models/bakker_koeken.py<#74 (comment)>:
+ vervaldatum_koek = fields.Date(string="Vervaldatum van de koek", required=True, help="Vul hier de vervaldatum van de koek in", default=lambda self: fields.Date.today() + timedelta(days=30), readonly=True)
+ aankoopdatum_koek = fields.Date(string="Aankoopdatum van de koek", default=fields.Date.today, help="Vul hier de aankoopdatum van de koek in", readonly=True)
+ categorie_koek_id = fields.Many2one('bakker_koeken_categorie', string="Categorie van de koek", required=True, help="Selecteer hier de categorie van de koek")
+ pudding_koek = fields.Boolean(string="Bevat pudding", default=False, help="Vink dit aan als de koek pudding bevat")
+ totaal_inventarisatie = fields.Float(string="Totale inventarisatie waarde", compute="_compute_totaal_inventarisatie", store=True, inverse="_inverse_totaal_inventarisatie", help="Totale waarde van de koek in inventarisatie (prijs * voorraad)")
+ tags_ids = fields.Many2many('bakker_koeken_tags', string="Tags", help="Selecteer hier de tags voor de koek")
+
+ # Nieuwe verkoop gerelateerde velden
+ verkoop_ids = fields.One2many('bakker_verkoop', 'koek_id', string='Verkopen')
+ totaal_verkocht = fields.Integer(string='Totaal Verkocht', compute='_compute_verkoop_stats', store=True)
+ totaal_omzet = fields.Float(string='Totaal Omzet', compute='_compute_verkoop_stats', store=True)
+ verkoop_count = fields.Integer(string='Aantal Verkopen', compute='_compute_verkoop_count')
+
+ @api.depends('verkoop_ids.aantal', 'verkoop_ids.totaal_bedrag', 'verkoop_ids.status')
+ def _compute_verkoop_stats(self):
+ for record in self:
I would have used something like "for bakker in self:"
to have something more visual than record
________________________________
In Bakker/models/bakker_koeken.py<#74 (comment)>:
+ prijs_koek = fields.Float(string="Prijs van de koek", required=True, help="Vul hier de prijs van de koek in")
+ voorraad_koek = fields.Integer(string="Voorraad van de koek", required=True , default=10)
+ vervaldatum_koek = fields.Date(string="Vervaldatum van de koek", required=True, help="Vul hier de vervaldatum van de koek in", default=lambda self: fields.Date.today() + timedelta(days=30), readonly=True)
+ aankoopdatum_koek = fields.Date(string="Aankoopdatum van de koek", default=fields.Date.today, help="Vul hier de aankoopdatum van de koek in", readonly=True)
+ categorie_koek_id = fields.Many2one('bakker_koeken_categorie', string="Categorie van de koek", required=True, help="Selecteer hier de categorie van de koek")
+ pudding_koek = fields.Boolean(string="Bevat pudding", default=False, help="Vink dit aan als de koek pudding bevat")
+ totaal_inventarisatie = fields.Float(string="Totale inventarisatie waarde", compute="_compute_totaal_inventarisatie", store=True, inverse="_inverse_totaal_inventarisatie", help="Totale waarde van de koek in inventarisatie (prijs * voorraad)")
+ tags_ids = fields.Many2many('bakker_koeken_tags', string="Tags", help="Selecteer hier de tags voor de koek")
+
+ # Nieuwe verkoop gerelateerde velden
+ verkoop_ids = fields.One2many('bakker_verkoop', 'koek_id', string='Verkopen')
+ totaal_verkocht = fields.Integer(string='Totaal Verkocht', compute='_compute_verkoop_stats', store=True)
+ totaal_omzet = fields.Float(string='Totaal Omzet', compute='_compute_verkoop_stats', store=True)
+ verkoop_count = fields.Integer(string='Aantal Verkopen', compute='_compute_verkoop_count')
+
+ @api.depends('verkoop_ids.aantal', 'verkoop_ids.totaal_bedrag', 'verkoop_ids.status')
We tend to use everywhere the same quote notation. So single or double quote everywhere and not a mix
________________________________
In Bakker/models/bakker_koeken.py<#74 (comment)>:
+ @api.onchange('prijs_koek', 'voorraad_koek')
+ def _onchange_prijs_koek(self):
+ self._compute_totaal_inventarisatie()
+
+ @api.onchange('totaal_inventarisatie')
+ def _onchange_totaal_inventarisatie(self):
+ self._inverse_totaal_inventarisatie()
+
+ @api.depends('prijs_koek', 'voorraad_koek')
+ def _compute_totaal_inventarisatie(self):
+ for record in self:
+ record.totaal_inventarisatie = record.prijs_koek * record.voorraad_koek
+
+ def _inverse_totaal_inventarisatie(self):
+ for record in self:
+ if record.prijs_koek != 0:
+ record.voorraad_koek = record.totaal_inventarisatie / record.prijs_koek
+ else:
+ record.voorraad_koek = 0
I think this could be rewritten just with a compute and an inverse or just with onchange ?
In my opinion, calling a compute in a onchange is not a good idea
________________________________
In Bakker/models/bakker_koeken.py<#74 (comment)>:
+ for record in self:
+ # Maak een kopie van het huidige record
+ new_batch = record.copy({
+ 'voorraad_koek': 30,
+ 'aankoopdatum_koek': fields.Date.today(),
+ 'vervaldatum_koek': fields.Date.today() + timedelta(days=30),
+ 'name_koek': f"{record.name_koek} - Verse Batch",
+ })
+
+ vers_tag = self.env['bakker_koeken_tags'].search([('name', '=', 'Vers')], limit=1)
+ if vers_tag:
+ new_batch.tags_ids = [(4, vers_tag.id)]
⬇️ Suggested change
- for record in self:
- # Maak een kopie van het huidige record
- new_batch = record.copy({
- 'voorraad_koek': 30,
- 'aankoopdatum_koek': fields.Date.today(),
- 'vervaldatum_koek': fields.Date.today() + timedelta(days=30),
- 'name_koek': f"{record.name_koek} - Verse Batch",
- })
-
- vers_tag = self.env['bakker_koeken_tags'].search([('name', '=', 'Vers')], limit=1)
- if vers_tag:
- new_batch.tags_ids = [(4, vers_tag.id)]
+ vers_tag = self.env['bakker_koeken_tags'].search([('name', '=', 'Vers')], limit=1)
+ for record in self:
+ # Maak een kopie van het huidige record
+ new_batch = record.copy({
+ 'voorraad_koek': 30,
+ 'aankoopdatum_koek': fields.Date.today(),
+ 'vervaldatum_koek': fields.Date.today() + timedelta(days=30),
+ 'name_koek': f"{record.name_koek} - Verse Batch",
+ })
+
+ if vers_tag:
+ new_batch.tags_ids = [(4, vers_tag.id)]
As you are searching with a hardcoded 'Vers' the search can be outside the loop and called once only
________________________________
In Bakker/models/bakker_koeken.py<#74 (comment)>:
+ if populair_tag:
+ record.tags_ids = [(4, populair_tag.id)]
+ return True
+
+ def action_seizoen_special(self):
+ """Markeer als seizoensspecial"""
+ seizoen_tag = self.env['bakker_koeken_tags'].search([('name', '=', 'Seizoen')], limit=1)
+ for record in self:
+ if seizoen_tag:
+ record.tags_ids = [(4, seizoen_tag.id)]
+ record.prijs_koek = record.prijs_koek * 1.15 # 15% prijsverhoging
+ return True
+
+ def action_kwaliteitscontrole(self):
+ """Doe kwaliteitscontrole"""
+ import random
can be imported at the beginning of the file
________________________________
In Bakker/models/bakker_koeken.py<#74 (comment)>:
+ def action_seizoen_special(self):
+ """Markeer als seizoensspecial"""
+ seizoen_tag = self.env['bakker_koeken_tags'].search([('name', '=', 'Seizoen')], limit=1)
+ for record in self:
+ if seizoen_tag:
+ record.tags_ids = [(4, seizoen_tag.id)]
+ record.prijs_koek = record.prijs_koek * 1.15 # 15% prijsverhoging
+ return True
+
+ def action_kwaliteitscontrole(self):
+ """Doe kwaliteitscontrole"""
+ import random
+ for record in self:
+ if random.choice([True, False]): # 50% kans
+
+ vers_tag = self.env['bakker_koeken_tags'].search([('name', '=', 'Vers')], limit=1)
can be outside the loop
________________________________
In Bakker/models/bakker_koeken.py<#74 (comment)>:
+ return {
+ 'name': f'Verkopen voor {self.name_koek}',
+ 'type': 'ir.actions.act_window',
+ 'res_model': 'bakker_verkoop',
+ 'view_mode': 'list,form',
+ 'domain': [('koek_id', '=', self.id)],
+ 'context': {'default_koek_id': self.id}
+ }
+
+ def action_snelle_verkoop(self):
+ """Snelle verkoop - direct 1 koek verkopen"""
+ if self.voorraad_koek <= 0:
+ raise ValidationError("Geen voorraad beschikbaar!")
+
+ # Zoek of maak een standaard walk-in klant
+ walk_in_klant = self.env['res.partner'].search([('name', '=', 'Walk-in klant')], limit=1)
using a name for a search is never a good idea, it could change, another way would be to add a checkbox on the res.partner model and then search for the record that has this checkbox set
⬇️ Suggested change
- walk_in_klant = self.env['res.partner'].search([('name', '=', 'Walk-in klant')], limit=1)
+ walk_in_klant = self.env['res.partner'].search([('is_walk_in_client', '=', True)], limit=1)
________________________________
In Bakker/models/bakker_koeken.py<#74 (comment)>:
@@ -0,0 +1,233 @@
+from odoo import models, fields, api
+from datetime import timedelta
+from odoo.exceptions import ValidationError
+
+class BakkerKoeken(models.Model):
+ _name = "bakker_koeken"
⬇️ Suggested change
- _name = "bakker_koeken"
+ _name = "bakker.koeken"
Use of . for the name of the models
________________________________
In Bakker/models/bakker_verkoop.py<#74 (comment)>:
+
+class BakkerVerkoop(models.Model):
+ _name = "bakker_verkoop"
+ _description = "Verkoop van bakkerij koeken"
+ _inherit = ['mail.thread', 'mail.activity.mixin']
+ _order = "verkoop_datum desc"
+
+
+ name = fields.Char(string="Verkoop Nummer", required=True, default="Nieuw", tracking=True)
+ koek_id = fields.Many2one('bakker_koeken', string="Koek", required=True, tracking=True)
+ partner_id = fields.Many2one('res.partner', string="Klant", required=True, tracking=True)
+ klant_email = fields.Char(string="Email", related='partner_id.email', readonly=True)
+ klant_telefoon = fields.Char(string="Telefoon", related='partner_id.phone', readonly=True)
+ aantal = fields.Integer(string="Aantal", required=True, default=1, tracking=True)
+ prijs_per_stuk = fields.Float(string="Prijs per stuk", required=True, tracking=True)
+ korting_percentage = fields.Float(string="Korting (%)", default=0.0, tracking=True)
⬇️ Suggested change
- korting_percentage = fields.Float(string="Korting (%)", default=0.0, tracking=True)
+ korting_percentage = fields.Float(string="Korting (%)", tracking=True)
If I am not mistaken in odoo, a float field has a default value of 0.0
________________________________
In Bakker/models/bakker_verkoop.py<#74 (comment)>:
+ 'res_model': 'base.document.layout',
+ 'view_mode': 'form',
+ 'target': 'new',
+ 'context': {
+ 'default_report_layout': 'web.external_layout_standard',
+ 'default_logo': True,
+ 'default_company_details': True,
+ 'active_ids': self.ids,
+ 'active_model': 'bakker_verkoop',
+ 'report_action': self.env.ref('Bakker.report_bakker_factuur').report_action(self),
+ 'close_on_report_download': True,
+ }
+ }
+
+
+class BakkerVerkoopWizard(models.TransientModel):
For wizard mode, you can add a new folder called wizards where you put all the transient models
________________________________
In Bakker/models/bakker_verkoop.py<#74 (comment)>:
+ partner_id = fields.Many2one('res.partner', string='Klant', required=True)
+ aantal = fields.Integer(string='Aantal', required=True, default=1)
+ prijs_per_stuk = fields.Float(string='Prijs per stuk', required=True)
+ korting_percentage = fields.Float(string='Korting (%)', default=0.0)
+ finale_prijs = fields.Float(string='Finale Prijs per stuk', compute='_compute_finale_prijs')
+ totaal_bedrag = fields.Float(string='Totaal Bedrag', compute='_compute_totaal_bedrag')
+ beschikbare_voorraad = fields.Integer(related='koek_id.voorraad_koek', string='Beschikbare Voorraad')
+ direct_betaald = fields.Boolean(string='Direct Betaald', default=True)
+ betaal_methode = fields.Selection([
+ ('cash', 'Contant'),
+ ('card', 'Bankkaart'),
+ ('digital', 'Digitaal')
+ ], string="Betaal Methode", default='cash')
+
+ # Gerelateerde velden van de klant
+ klant_email = fields.Char(string="Email", related='partner_id.email', readonly=True)
⬇️ Suggested change
- klant_email = fields.Char(string="Email", related='partner_id.email', readonly=True)
+ klant_email = fields.Char(string="Email", related='partner_id.email')
A related field should be readonly=True by default if I remember well
—
Reply to this email directly, view it on GitHub<#74 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/BLALSE4MXR4OA23CNKMKP2D3VJMEJAVCNFSM6AAAAACGYP35UGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTEOBTGU2TEOJZGI>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.