Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: link cash flow rows and fix summary linking #42524

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

blaggacao
Copy link
Collaborator

Context

  • Cashflow rows link into nowhere useful
  • Summary lines ditto

Solutions

  • Avoid linking summary lines (through consequent quoting)
  • Link cashflow lines to GL report with the filter set to each row's (multiple) its constituent accounts

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Jul 29, 2024
@rmehta
Copy link
Member

rmehta commented Aug 26, 2024

@ruthra-kumar merging

@rmehta rmehta merged commit 0aae621 into frappe:develop Aug 26, 2024
19 checks passed
@blaggacao blaggacao deleted the fix/financial-statement-linking branch August 26, 2024 11:12
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 2024
@@ -33,7 +33,7 @@ erpnext.financial_statements = {

return value;
} else if (frappe.query_report.get_filter_value("selected_view") == "Margin" && data) {
if (column.fieldname == "account" && data.account_name == __("Income")) {
if (column.fieldname == "stub" && data.account_name == __("Income")) {
Copy link
Member

@ruthra-kumar ruthra-kumar Nov 26, 2024

Choose a reason for hiding this comment

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

Changing fieldname to 'stub' breaks export as the statements still generates with 'account' as fieldname. In hindsight, I don't see any benefit to changing this.

Copy link
Collaborator Author

@blaggacao blaggacao Nov 26, 2024

Choose a reason for hiding this comment

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

Is there a way to fix this via a pre-export hook, where necessary?

In the javascript code account is misleading, at best: it's the table stub column which may contain anything, including, but not exclusively, an account.

I can't remember if there was a reason exceeding the purpose of type clarification, but that's a strong reason, imo.

Copy link
Member

Choose a reason for hiding this comment

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

In the javascript code account is misleading

Not sure how this is misleading. Any Examples?

Copy link
Collaborator Author

@blaggacao blaggacao Nov 29, 2024

Choose a reason for hiding this comment

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

account is misleading in that its contents are not limited to account strings. In reality, though, it's the table's stub column, which indicates what the row to the right contains, including:

  • accounts
  • summarizing lines (e.g. "Net Operating Capital Gain")
  • component descriptions / title / section separators

That is true for prepared display data.

The origin source data may still be based off accounts. When that transition happens, it should be reflected by the correct name.

I don't quite understand the export use case, but as far as I can see, if it works with origin data, it should handle an account column, however if it works with prepared data it should handle the stub column. If it works with the prepared data and still needs the account column for other reasons that looks like a type mismatch, maybe it should work with the origin data instead.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-15-hotfix needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants