Skip to content

[T3314] Donations linked to events silently lose their event association since the analytic multi-plan migration#2112

Open
danpa32 wants to merge 1 commit into
18.0from
T3314-fix-event-analytic-plan-event-id
Open

[T3314] Donations linked to events silently lose their event association since the analytic multi-plan migration#2112
danpa32 wants to merge 1 commit into
18.0from
T3314-fix-event-analytic-plan-event-id

Conversation

@danpa32

@danpa32 danpa32 commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

T3314 — Donations linked to events silently lose their event association

Found while working T0632.

The whole event-thank-you pipeline couldn't be exercised on real data because event_id never resolved on account.move.line.

Root cause

Migration commit e43ef938 [MIG] T2713 crm_compassion: migration to 18.0 introduced a dedicated analytic plan for events (crm_compassion.plan_events — v14 had no concept of analytic plans, a v17+ Odoo accounting feature), and event_compassion.py's _get_analytic_vals() assigns every new event's own analytic account to this plan. But account.move.line._compute_event() was carried over from the v14 code unchanged — it only ever reads the generic analytic_line_ids.account_id field.

Under Odoo 18's multi-plan analytics, each plan stores its account reference in its own dedicated column on account.analytic.line (x_plan<N>_id, N = that plan's id — resolved dynamically via account.analytic.plan._column_name(), addons/analytic/models/ analytic_plan.py:115-121); the generic account_id column is reserved for the root/project plan only. Since every event's own analytic account lives specifically in the "Events" plan, _compute_event() could never find it.

This does not fire an error anywhere, just a quietly empty field.

What changed

  • models/account_move_line.py: _compute_event() now also checks the "Events" plan's dynamic column (resolved via _column_name(), not hardcoded — robust to the plan having a different id in another database), in addition to the generic account_id.

No backfill migration was added for historical lines affected since the regression window (2026-06-04 onward in this DB) — worth a follow-up if that missing historical data matters.

How to test manually

  1. Upgrade the module (-u crm_compassion).
  2. Create/find an event, set its analytic distribution on a donation invoice line to that event's own analytic account (event.analytic_id).
  3. Post the invoice.
  4. Check account.move.line.event_id — should now resolve to the event (previously always empty for any event created after 2026-06-04 in this DB, or after whichever date production hit the same migration commit).

Video for testing manually for thank you letters

Screencast.From.2026-07-16.09-50-16.mp4

…tic plans

account.move.line._compute_event() only checked the generic account_id
column, but event analytic accounts live under the dedicated
crm_compassion.plan_events analytic plan, whose accounts are stored in
that plan's own dynamic x_plan<N>_id column instead. Resolve the
correct column via account.analytic.plan._column_name() rather than
hardcoding it.
@danpa32
danpa32 requested review from NoeBerdoz and ecino July 16, 2026 07:54

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the _compute_event method in account_move_line.py to correctly retrieve event analytic accounts from the custom crm_compassion.plan_events analytic plan column. The review feedback points out two critical issues: a potential traceback during module installation/upgrade if the XML reference is not found, and incorrect compute logic that prevents event_id from being cleared or updated when dependencies change. A simplified and safer implementation is suggested to resolve these issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +32 to 41
events_column = self.env.ref("crm_compassion.plan_events")._column_name()
for line in self:
if not line.event_id and line.analytic_line_ids.account_id.event_id:
line.event_id = line.analytic_line_ids.account_id.event_id
analytic_accounts = line.analytic_line_ids.account_id | (
line.analytic_line_ids.mapped(events_column)
)
event = analytic_accounts.mapped("event_id")[:1]
if not line.event_id and event:
line.event_id = event
else:
line.event_id = line.event_id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

There are two issues with the current implementation of _compute_event:

  1. Robustness during installation/upgrade: Calling self.env.ref("crm_compassion.plan_events") without raise_if_not_found=False can cause a traceback during module installation or upgrade if the compute method is triggered before the XML data is loaded into the database.
  2. Correctness of the compute logic: The conditional check if not line.event_id and event: prevents the field from being updated or cleared if the analytic distribution is changed or removed on an existing line. In Odoo, a compute method should always assign a value to the field for all records in self to ensure correctness when dependencies change.

We can simplify and fix both issues by retrieving the plan safely and directly assigning the resolved event (or False if none is found) to line.event_id.

        plan = self.env.ref("crm_compassion.plan_events", raise_if_not_found=False)
        events_column = plan._column_name() if plan else None
        for line in self:
            analytic_accounts = line.analytic_line_ids.account_id
            if events_column:
                analytic_accounts |= line.analytic_line_ids.mapped(events_column)
            line.event_id = analytic_accounts.mapped("event_id")[:1]

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.

  1. I think it is optional, not a real bug here. I could take it or leave it.
  2. This is a pre-existing behavior outside of this PR's scope. I do not know if it wrong or right and it might blast me away for another day as it is a bigger analysis of behavior.

I will not touch it if senior do not tell me to go for it first

@greptile-apps

greptile-apps Bot commented Jul 16, 2026

Copy link
Copy Markdown

Confidence Score: 5/5

Safe to merge with minimal risk.

The change is narrowly scoped to event resolution in a stored compute method. It uses the analytic plan helper instead of hardcoding the dynamic column, and it preserves the previous generic analytic-account path.

No files require special attention.

T-Rex T-Rex Logs

What T-Rex did

  • The proof documents a transition from the account_id-only path to using the dynamic Events plan column via _column_name() to resolve event_id.
  • The updated logic maps event_id to Compassion Event, validating the intended data mapping after the change.
  • A syntax check was run as part of the validation and completed with exit code 0.
  • Artifacts were prepared to support reviewer inspection, including logs and a Python source file implementing the change.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
crm_compassion/models/account_move_line.py Updates account.move.line.event_id computation to include analytic accounts stored in the Events analytic plan dynamic column; no blocking issues found.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant AML as account.move.line
participant AAL as account.analytic.line
participant Plan as crm_compassion.plan_events
participant Event as crm.event.compassion

AML->>Plan: _column_name()
Plan-->>AML: Events dynamic column name
AML->>AAL: read analytic_line_ids.account_id
AML->>AAL: mapped(events_column)
AAL-->>AML: analytic accounts
AML->>Event: mapped("event_id")
Event-->>AML: first matching event
AML->>AML: set event_id when empty
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant AML as account.move.line
participant AAL as account.analytic.line
participant Plan as crm_compassion.plan_events
participant Event as crm.event.compassion

AML->>Plan: _column_name()
Plan-->>AML: Events dynamic column name
AML->>AAL: read analytic_line_ids.account_id
AML->>AAL: mapped(events_column)
AAL-->>AML: analytic accounts
AML->>Event: mapped("event_id")
Event-->>AML: first matching event
AML->>AML: set event_id when empty
Loading

Reviews (1): Last reviewed commit: "[T3314] crm_compassion: fix event_id not..." | Re-trigger Greptile

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