[17.0][IMP] sale_invoice_plan: pending planned invoices#3823
[17.0][IMP] sale_invoice_plan: pending planned invoices#3823ChristianSantamaria wants to merge 1 commit intoOCA:17.0from
Conversation
|
Hi @kittiu, |
ChristianSantamaria
force-pushed
the
17.0-imp-sale_invoice_plan_pending
branch
2 times, most recently
from
ebb1481 to
317091a
Compare
DantePereyra
force-pushed
the
17.0-imp-sale_invoice_plan_pending
branch
from
317091a to
c57d832
Compare
ChristianSantamaria
force-pushed
the
17.0-imp-sale_invoice_plan_pending
branch
from
c57d832 to
445aaff
Compare
Shows pending planned invoices for the Sale Order according to its Invoice Plan.
ChristianSantamaria
force-pushed
the
17.0-imp-sale_invoice_plan_pending
branch
from
445aaff to
a2021eb
Compare
|
@marcelsavegnago @kittiu Can u review this improvement? |
|
@Saran440 could you review? Thanks! |
alexey-pelykh
left a comment
alexey-pelykh
left a comment
There was a problem hiding this comment.
Review: sale_invoice_plan - pending planned invoices
The core feature (stored computed invoice_plan_pending field, tree column, search filter, migration, tests) is well implemented. A few items to address:
Issues Found
| # | Severity | Finding |
|---|---|---|
| 1 | 🟡 Medium | README.rst maturity badge regression: The alt text changed from Production/Stable to Alpha, and license became licence. The manifest still says "development_status": "Production/Stable". This looks like an unintended artifact of oca-gen-addon-readme regeneration with possibly stale/wrong config. The same issue propagates to index.html. |
| 2 | 🟢 Minor | Migration: NULL vs 0 gap: The pre-migration SQL only updates orders that have pending plans. Orders with all plans invoiced (or no plans) remain NULL. The ORM compute returns 0 for those. Consider adding a final UPDATE sale_order SET invoice_plan_pending = 0 WHERE invoice_plan_pending IS NULL to ensure consistency between migrated data and freshly computed data. |
What Looks Good
store=Truewith@api.depends("invoice_plan_ids.invoiced")is the right approach for a field displayed in tree/search viewsopenupgradelibusage in the migration is correct:column_existsguard,openupgrade.add_columns,openupgrade.logged_query- Test coverage is solid: checks pending count after plan creation, after partial invoicing, and after full invoicing across multiple test methods
- Search filter and tree view integration are clean
Overall the implementation is sound. Addressing the README badge regression and the minor migration gap would make this ready to merge.
| :target: https://odoo-community.org/page/development-status | ||
| :alt: Production/Stable | ||
| .. |badge2| image:: https://img.shields.io/badge/license-AGPL--3-blue.png | ||
| :alt: Alpha |
There was a problem hiding this comment.
The alt text here changed from Production/Stable to Alpha, and license became licence (line 16). The __manifest__.py still declares "development_status": "Production/Stable". This looks like an unintended side effect of regenerating the README with possibly stale or misconfigured oca-gen-addon-readme. Could you verify and regenerate with the correct settings?
| WHERE so.id = sub.sale_id; | ||
| """, | ||
| ) | ||
| _logger.info("Updated invoice_plan_pending for existing sale orders.") |
There was a problem hiding this comment.
Minor: this UPDATE only sets invoice_plan_pending for orders that have at least one non-invoiced plan. Orders with zero pending plans (all invoiced or no plans at all) will keep invoice_plan_pending = NULL. The ORM compute method would return 0 for those.
Consider adding a follow-up statement to zero out NULLs for consistency:
openupgrade.logged_query(
env.cr,
"UPDATE sale_order SET invoice_plan_pending = 0 WHERE invoice_plan_pending IS NULL",
)This ensures migrated data matches what the compute method would produce.
5 participants
Shows pending planned invoices for the Sale Order according to its Invoice Plan.