Skip to content

feat: add accounting dimensions to create journal entry#365

Open
0xD0M1M0 wants to merge 6 commits into
alyf-de:version-15-hotfixfrom
0xD0M1M0:accounting-dimensions
Open

feat: add accounting dimensions to create journal entry#365
0xD0M1M0 wants to merge 6 commits into
alyf-de:version-15-hotfixfrom
0xD0M1M0:accounting-dimensions

Conversation

@0xD0M1M0
Copy link
Copy Markdown
Contributor

@0xD0M1M0 0xD0M1M0 commented May 15, 2026

Solves #362 and #360

Ref: LKP-76

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 15, 2026

Confidence Score: 5/5

Safe to merge. The accounting dimension helpers are well-guarded with an ERPNext allowlist, the JE bank-GL-skip is correct, and unit tests cover the key invariants.

All core paths — Payment Entry dimension stamping, Journal Entry non-bank row stamping, and the frontend caching logic — look correct. The only finding is a localStorage key that is never written, which means the section expanded-state preference is not persisted across transaction switches, but this causes no data loss or reconciliation error.

banking/public/js/bank_reconciliation_beta/actions_panel/create_tab.js — the localStorage persistence logic for the dimensions section state is incomplete.

Sequence Diagram

sequenceDiagram
    participant PM as PanelManager
    participant APM as ActionsPanelManager
    participant CT as CreateTab
    participant BE as Backend (Python)

    PM->>BE: get_bank_transactions()
    PM->>BE: "get_dimensions(with_cost_center_and_project=false)"
    BE-->>PM: "[custom_dimensions[], defaults_map{}]"
    PM->>PM: "cache accounting_dimensions & accounting_dimension_defaults"

    Note over PM: On transaction select
    PM->>APM: "new ActionsPanelManager({ accounting_dimensions, accounting_dimension_defaults })"
    APM->>CT: "new CreateTab({ accounting_dimensions, accounting_dimension_defaults })"
    CT->>CT: build_field_group() - renders base fields synchronously
    CT->>CT: append_accounting_dimensions() - injects collapsible section

    Note over CT: On Create click (Payment Entry)
    CT->>BE: "create_payment_entry_bts(project, cost_center, accounting_dimensions=custom_json)"
    BE->>BE: _merge_accounting_dimensions_into_payment_entry()
    BE-->>CT: Payment Entry doc / reconciled BT

    Note over CT: On Create click (Journal Entry)
    CT->>BE: "create_journal_entry_bts(accounting_dimensions=all_dims_json)"
    BE->>BE: _merge_accounting_dimensions_into_je_accounts() - skips bank GL row
    BE-->>CT: Journal Entry doc / reconciled BT
Loading

Fix All in Cursor

Reviews (3): Last reviewed commit: "chore: move project and cost center to f..." | Re-trigger Greptile

Copy link
Copy Markdown
Member

@barredterra barredterra left a comment

Choose a reason for hiding this comment

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

Tested, functionality works as expected. Some comments on consistency, style and performance.

  • I think the "standard" Accounting Dimensions Project and Cost Center should also be visible for "Create Journal Entry" (they are already for "Create Payment Entry"). Then we can always show the collapsible section (maybe add it in Create Payment Entry" as well, for consistency?)
  • The frontend calls get_dimensions for each Bank Transaction. Calling it once when the reco tool is loaded should be enough.
  • I think adding short docstrings to the methods would help, at least where the content is long and not obvious from the name.
  • I don't mind merging allow_on_submit with this PR, but if it had been a separate PR it would have been merged already. ;)

Comment thread banking/public/js/bank_reconciliation_beta/actions_panel/create_tab.js Outdated
Comment thread banking/public/js/bank_reconciliation_beta/actions_panel/create_tab.js Outdated
Copy link
Copy Markdown
Member

@barredterra barredterra left a comment

Choose a reason for hiding this comment

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

Non-blocking design thought: should we hard-wire project and cost_center in the create form and reconciliation flow instead of loading them dynamically via accounting dimension metadata?

They are standard ERPNext fields and are already special-cased in the backend allowlist, while Payment Entry also has explicit project / cost_center parameters. A clearer split might be:

  • render/pass project and cost_center explicitly
  • load dynamic accounting dimensions only for actual custom dimensions
  • keep the backend allowlist as standard fields plus get_accounting_dimensions(...)
  • keep applying dimensions to non-bank Journal Entry account rows, and to Payment Entry header fields

That would make the standard fields explicit and reduce coupling to the exact shape returned by get_dimensions(with_cost_center_and_project=True).

@0xD0M1M0 0xD0M1M0 requested a review from barredterra May 29, 2026 19:34
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.

2 participants