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: assigning values to rows in sales register reports #26546

Merged
merged 31 commits into from
Aug 19, 2021

Conversation

AfshanKhan
Copy link
Contributor

@AfshanKhan AfshanKhan commented Jul 19, 2021

Problem:

Suppose user permission are set on account. The value for unrealized profit and loss account is getting updated when making a Sales Invoice.
This shows wrong values in Sales Register Report and Item-Wise Sales Register Report.

Solution:

Adding _unrealized to the fieldname for Unrealized Profit Loss Account in Sale Register Report so the accounts are distinguishable.
Add ignore user permission check for unrelized profit and loss account in Sales Invoice
Don't add that column for Unrealized Profit Loss Account if its not an internal transaction

@coveralls
Copy link

coveralls commented Jul 19, 2021

Coverage Status

Coverage increased (+0.02%) to 42.944% when pulling 7c91856 on AfshanKhan:fix-sales-register-report into 8790248 on frappe:develop.

@deepeshgarg007 deepeshgarg007 self-assigned this Jul 19, 2021
@@ -84,7 +84,7 @@ def _execute(filters, additional_table_columns=None, additional_query_columns=No
# Add amount in unrealized account
for account in unrealized_profit_loss_accounts:
row.update({
frappe.scrub(account): flt(internal_invoice_map.get((inv.name, account)))
frappe.scrub(account+"_unrealized"): flt(internal_invoice_map.get((inv.name, account)))
Copy link
Member

Choose a reason for hiding this comment

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

If unrealized profit and loss account is used then it means its an internal transaction and no income is being booked so doesn't matter even if income account is being overridden

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deepeshgarg007 Exactly. So suppose there is an account let's say income_account which for a transaction has value 100. Now this account was also part of internal transaction and that is why it is in unrealized profit and loss ka list too. With a value 0. Now according to the code there are 2 columns with the same account. One for income and another for unrealized profit and loss. First the value will be assigned to income_account 100. Then later on as the fieldname is same so instead of assigning 0 to that 2nd column it is being overwritten to the 1st column which had 100. Now it has value 0. Similarly for every row the value for income_account will be 0.

Copy link
Member

Choose a reason for hiding this comment

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

@deepeshgarg007 Exactly. So suppose there is an account let's say income_account which for a transaction has value 100. Now this account was also part of internal transaction and that is why it is in unrealized profit and loss ka list too. With a value 0. Now according to the code there are 2 columns with the same account. One for income and another for unrealized profit and loss. First the value will be assigned to income_account 100. Then later on as the fieldname is same so instead of assigning 0 to that 2nd column it is being overwritten to the 1st column which had 100. Now it has value 0. Similarly for every row the value for income_account will be 0.

For an internal transaction there won't be any income booked, so doesn't matter, right? Or is it affecting any other transactions? Please add screenshots before/after for this so that there is clear understanding

@AfshanKhan AfshanKhan reopened this Aug 12, 2021
@AfshanKhan
Copy link
Contributor Author

@deepeshgarg007 Added the changes

@AfshanKhan AfshanKhan merged commit ecd6584 into frappe:develop Aug 19, 2021
frappe-pr-bot pushed a commit to frappe-pr-bot/erpnext that referenced this pull request Aug 19, 2021
* fix: assigning values to rows in sales register reports

* fix: check for is_internal_customer for unrealized_profit_loss_account

(cherry picked from commit ecd6584)
AfshanKhan added a commit that referenced this pull request Aug 19, 2021
)

* fix: assigning values to rows in sales register reports

* fix: check for is_internal_customer for unrealized_profit_loss_account

(cherry picked from commit ecd6584)

Co-authored-by: Afshan <33727827+AfshanKhan@users.noreply.github.com>
asoral pushed a commit to asoral/erpnext that referenced this pull request Nov 12, 2021
* fix: assigning values to rows in sales register reports

* fix: check for is_internal_customer for unrealized_profit_loss_account
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants