Skip to content

[16.0][ADD] sale_force_invoice_amount#3573

Open
Wvven wants to merge 1 commit intoOCA:16.0from
Wvven:16.0-sale_force_invoice_amount
Open

[16.0][ADD] sale_force_invoice_amount#3573
Wvven wants to merge 1 commit intoOCA:16.0from
Wvven:16.0-sale_force_invoice_amount

Conversation

@Wvven
Copy link
Copy Markdown

@Wvven Wvven commented Feb 7, 2025

When both sale_force_invoiced and sale_order_invoice_amount are installed, the invoiced_amount value were not updated correctly

@Wvven Wvven force-pushed the 16.0-sale_force_invoice_amount branch 5 times, most recently from 87238de to 7c5b75f Compare February 12, 2025 09:35
if res:
for order in res:
if order.force_invoiced:
order.invoiced_amount = order.uninvoiced_amount
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

AFAIK uninvoiced_amount could be partial. In case of force_invoiced, invoiced_amount should be set to order total amount I think

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, the code now should be fine

@Wvven Wvven force-pushed the 16.0-sale_force_invoice_amount branch from 7c5b75f to 1f01119 Compare February 12, 2025 14:23
Copy link
Copy Markdown
Member

@eLBati eLBati left a comment

Choose a reason for hiding this comment

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

invoiced

I'm getting this

@Wvven Wvven force-pushed the 16.0-sale_force_invoice_amount branch from 1f01119 to 9ee5dde Compare February 17, 2025 16:22
@rousseldenis
Copy link
Copy Markdown
Contributor

@Wvven This is a module addition. You can title your PR like : [16.0][ADD] sale_force_invoice_amount

@rousseldenis
Copy link
Copy Markdown
Contributor

And your commit message should be like that too.

@rousseldenis rousseldenis added this to the 16.0 milestone Feb 18, 2025
@Wvven Wvven force-pushed the 16.0-sale_force_invoice_amount branch from 9ee5dde to 6af4860 Compare February 18, 2025 14:11
@Wvven Wvven changed the title [16.0][FIX] Sale_force_invoice_amount [16.0][ADD] sale_force_invoice_amount Feb 19, 2025
@Wvven Wvven force-pushed the 16.0-sale_force_invoice_amount branch from 6af4860 to a55bf2d Compare February 19, 2025 09:42
@Wvven
Copy link
Copy Markdown
Author

Wvven commented Feb 19, 2025

@rousseldenis i've changed the name and the commit as you suggested, thanks

Copy link
Copy Markdown
Member

@eLBati eLBati left a comment

Choose a reason for hiding this comment

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

Thanks.
LGTM for the rest

"summary": "When the Force Invoiced is checked, the invoiced amount is updated.",
"version": "16.0.1.0.0",
"author": "Innovyou, Odoo Community Association (OCA)",
"category": "sale",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would use category Hidden

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Changed, thanks!

@@ -0,0 +1 @@
Fixes the force invoice field when both sale_force_invoiced and sale_order_invoice_amount are installed No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually this fixes the invoice and uninvoiced amounts in sale order, right?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, i'll add that to the message

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Wvven thanks.
The commit message should be updated accordingly

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@eLBati Fatto, grazie

@Wvven Wvven force-pushed the 16.0-sale_force_invoice_amount branch 2 times, most recently from ec69d82 to aae1422 Compare February 20, 2025 15:20
Fixes the value od the fields invoice_amount and uninvoiced_amount when the force_invoiced is checked
@Wvven Wvven force-pushed the 16.0-sale_force_invoice_amount branch from aae1422 to c7d42fc Compare February 26, 2025 15:56
Copy link
Copy Markdown
Member

@eLBati eLBati left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link
Copy Markdown

@alexey-pelykh alexey-pelykh left a comment

Choose a reason for hiding this comment

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

Review: sale_force_invoice_amount

This is a glue module bridging sale_force_invoiced and sale_order_invoice_amount so that when force_invoiced is checked, invoiced_amount is set to the order total and uninvoiced_amount to 0. The approach is sound -- auto_install: True with both modules as dependencies is the correct OCA pattern for a glue module.

Note: This PR has already been approved by @eLBati. The observations below are supplementary and non-blocking.

Observations

1. static/description/index.html contains placeholder text

The generated HTML file has TEST as placeholder text in the description, usage, and contributors sections. This file should be regenerated with oca-gen-addon-readme after the latest readme updates. It will be visible in the Odoo Apps page / module description.

2. No tests

There are no tests for this module. A minimal test that creates a sale order, sets force_invoiced = True, and asserts invoiced_amount == amount_total and uninvoiced_amount == 0 would strengthen confidence in the glue behavior and protect against future regressions.

3. readme/DESCRIPTION.md -- typo

The description reads:

Fixes the value od the fields invoice_amount and uninvoiced_amount when the force_invoiced is checked

"od" should be "of".

4. Missing newline at end of file

readme/CONTRIBUTORS.md and readme/DESCRIPTION.md are missing a trailing newline.

5. Missing copyright/license headers

Python source files (__init__.py, models/__init__.py, models/sale_order.py) are missing the standard OCA copyright/license header.

6. Minor: "data": [] in manifest

The empty "data": [] key is unnecessary and can be removed for cleanliness.

Summary

The core logic is correct and the glue module pattern is well-applied. The items above are mostly cosmetic/quality-of-life improvements. The placeholder TEST text in index.html is the most visible issue since it would show up in the module description within Odoo.


Review posted via CorporateHub OCA review campaign

@@ -0,0 +1,426 @@
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This file contains TEST placeholder text in the description, usage, and contributors sections (lines ~382, ~390, ~397). It should be regenerated with oca-gen-addon-readme after the readme source files are finalized.

@@ -0,0 +1,22 @@
from odoo import api, models
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing the standard OCA copyright/license header. For example:

# Copyright 2025 Innovyou
# License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl).

@@ -0,0 +1 @@
Fixes the value od the fields invoice_amount and uninvoiced_amount when the force_invoiced is checked No newline at end of file
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor typo: "od" should be "of" ("Fixes the value of the fields..."). Also missing trailing newline at end of file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants