Skip to content

[T3487] Allow invoicing the contract line amount of pricelist products - #289

Open
danpa32 wants to merge 1 commit into
18.0from
T3487-write-and-pray-fixes
Open

danpa32 wants to merge 1 commit into
18.0from
T3487-write-and-pray-fixes

Conversation

@danpa32

@danpa32 danpa32 commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

T3487 — Fix failing quality test: Create a Write&Pray Sponsorship

  • compassion-accounting — module recurring_contract
  • compassion-modules — modules sponsorship_compassion, thankyou_letters
  • compassion-switzerland — module partner_communication_switzerland (version 18.0.1.1.6)

Merge order: compassion-accounting first. sponsorship_compassion overrides the new recurring.contract.line._compute_amount_from_pricelist.

Original report

Quality test "Create a Write&Pray Sponsorship" failed in three places:

  1. After setting the type to Write&Pray, a "no mobile phone" warning appears. Adding the number from the partner then fails with "champ invalide", and the user is stuck on the form.
  2. The amount of the W&P sponsorship can't be changed, and invoices are wrong.
  3. Communications: the welcome is not created, the sponsor isn't greeted by their preferred name, and the photo communication is shown in red.

Root causes

1. Mobile warning

  • The warning (partner_communication_switzerland/models/contracts.py) ran on every change of type, even before a correspondent was chosen.
  • In v18, the arrow next to the partner opens the partner page instead of a popup. That first auto-saves the incomplete sponsorship (no child, origin, …), which fails with "champ invalide".

2. Amount and invoices

  • The Sponsorship product has a fixed pricelist price of 42 CHF, so the line price was read-only (readonly="pricelist_item_count > 0").
  • Both invoice price computations always used the pricelist price for such products (contract_group.build_inv_line_data, move_line._update_invoice_lines_from_contract).
  • Effect: W&P contracts created from MyCompassion with a contribution (e.g. 48602 with a total of 20 CHF, 48415 with 10 CHF) were invoiced at 42 CHF.

3. Communications

  • Welcome not created (French/Italian): the fr/it texts of "Write&Pray Onboarding - Welcome" (template 330, database-only translations) read survey.public_url, a v14 field that doesn't exist in v18. Rendering failed and the welcome was skipped (the error is only logged).
  • Welcome not created (no email): in _send_new_dossier, "no email → printed dossier" was checked before the W&P case. W&P sponsors with only a mobile got the printed dossier.
  • Preferred name: every _get_salutation_* uses firstname, never preferred_name (same in v14).
  • Photo communication in red: reproduced locally as "Error in attachments creation". Since T3409 (ade0fc72, merged 17.09), thankyou_letters _compute_address only assigns name_line for partners with a title, so computing the address crashes (UnboundLocalError) for any partner without one. In production, every printed communication for a partner without a title fails this way, not only W&P.

Changes

compassion-accounting

  • aedec6c — Allow invoicing the contract line amount of pricelist products: new computed amount_from_pricelist on contract lines (default: product has pricelist items). It's used by the line list view (readonly) and by both invoice price computations, so other modules can decide when the line amount wins.

How to test

  1. Mobile:
    • Sponsorship → Create → type Write&Pray: no warning yet.
    • Choose a partner without a mobile: the warning appears and the Correspondent mobile field is shown.
    • Enter a number and save: you stay on the sponsorship, and the partner now has the mobile.
  2. Contribution:
    • In Contract lines, set the Sponsorship price to 10: it can be edited and the quantity becomes 1.
    • Validate: Related invoice lines show 10 CHF, not 42, and the sponsorship waits for payment.
    • On a normal sponsorship, the price is still read-only.
  3. Welcome:
    • Use a French or Italian partner with a title, a preferred name, and a mobile but no email.
    • Create and validate a W&P.
    • Partner → Communications: there is a Write&Pray Onboarding - Welcome in SMS mode greeting them by their preferred name, plus the photo by post.
  4. Partner without title: the photo-by-post communication of a title-less partner no longer fails in attachment creation.

Add amount_from_pricelist on contract lines, used by the line view and both
invoice price computations, so that other modules can decide when the line
amount wins over the pricelist price.
@greptile-apps

greptile-apps Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

[Medium risk] Refactors pricelist detection logic in contract invoicing.

The price-choice dependency needs to be resolved before merging because an old choice could affect invoice prices. The missing regression tests are a non-blocking concern.

Findings

  1. P1 Price choice can go stale ▶
  2. P2 New price choice lacks tests ▶

Reviews (1) · Last reviewed commit: "[T3487] Allow invoicing the contract lin..."

Comment on lines +74 to +77
@api.depends("product_id")
def _compute_amount_from_pricelist(self):
for line in self:
line.amount_from_pricelist = line.pricelist_item_count > 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Price choice can go stale

If a pricelist item is added or removed after a contract line’s price-choice flag has been read in the same request, and that flag is not invalidated, invoice creation or updates can use the old price source. The flag reads pricelist_item_count but declares only product_id as a dependency. Track the count as a dependency before merging so the price choice updates with it.

Knowledge Base Used: Recurring contracts and invoicing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In theory: if a pricelist item is added or removed earlier in the same request, the cached value could be outdated. But its fix wouldn't help. Odoo's own pricelist_item_count has no dependencies at all (it just counts records), so declaring it as a dependency triggers nothing. It isn't a regression either: the old code read the same pricelist_item_count with exactly the same caching. In practice, a pricelist rule is never changed and a contract invoiced in the same request.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You're right. Adding @api.depends("pricelist_item_count") would not provide meaningful invalidation because the upstream product.pricelist_item_count itself declares no dependencies. This also preserves the same cached read used by the previous invoice computations, so it is not a regression. Since pricelist rules are not changed while an invoice is being created, this is theoretical rather than actionable. I'll withdraw the finding.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

Comment on lines +56 to +59
amount_from_pricelist = fields.Boolean(
compute="_compute_amount_from_pricelist",
help="The invoiced price comes from the pricelist instead of the line price.",
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 New price choice lacks tests

There is no direct invoice test for a product-specific pricelist item or an override that uses the contract line amount. The existing fixture uses a global item and defines no test of its own. This is a non-blocking coverage gap, but a later pricing change could restore the wrong sponsorship charge without a test catching it. Add creation and update tests for both price choices.

Knowledge Base Used: Recurring contracts and invoicing

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Artifacts

Invoice price-choice verification script

  • This authored script extracts and executes the pricing methods from either revision with lightweight records, allowing the same scenarios to be compared.

Invoice pricing before the change

  • Running the script against HEAD^ showed that a priced product used 42 for both invoice paths, including the override scenario.

Invoice pricing after the change

  • Running the same script against HEAD showed that the overridden choice used the line amount of 17 for both invoice paths.

Targeted Odoo test collection failure

  • Running the existing test file with pytest exited 2 during import because `dateutil` is missing, so an Odoo transaction run was not possible.

View artifacts

T-Rex Ran code and verified through T-Rex

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.

1 participant