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

refactor: ignore disabled account while selecting Income Accounts #37860

Merged
merged 1 commit into from
Nov 9, 2023

Conversation

ruthra-kumar
Copy link
Member

Filter disabled accounts while selecting Income Account.

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Nov 2, 2023
@@ -568,7 +568,7 @@ cur_frm.fields_dict.write_off_cost_center.get_query = function(doc) {
cur_frm.set_query("income_account", "items", function(doc) {
return{
query: "erpnext.controllers.queries.get_income_account",
filters: {'company': doc.company}
filters: {'company': doc.company, "disabled": 0}
Copy link
Member

@deepeshgarg007 deepeshgarg007 Nov 2, 2023

Choose a reason for hiding this comment

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

While you are doing this, maybe remove usage of cur_frm and do the standard way using frm triggers?

@bosue
Copy link
Contributor

bosue commented Nov 2, 2023

Interesting.

While the right one, Expense Account, automatically discards disabled accounts, the left one doesn‘t. Would have to investigate why, but adding it manually should normally be superfluous.

Also, the right one at least filters for Profit and Loss accounts.

Neither one is configured to properly filter for only the relevant Account Type – Income Account and Expense Account, respectively.
One could say esisting sites may not have their accounts properly categorized, okay, so let‘s not backport this addition. But in develop, the proper filters should be set and enforced.

7D3D3100-26FE-493E-B336-A00579E753DD

@ruthra-kumar
Copy link
Member Author

ruthra-kumar commented Nov 3, 2023

@bosue
Framework implicitly adds disabled or enabled filter conditions based on doctype meta, granted the search has no custom query in it. https://github.com/frappe/frappe/blob/6ad2e762d325f0b62c2cf9dc4659497a0df9b3dd/frappe/desk/search.py#L154-L157

In this case, Income Account uses a custom query but Expense Account doesn't. This explains the different behaviour on the two link fields.

@bosue
Copy link
Contributor

bosue commented Nov 3, 2023

@ruthra-kumar: Thanks and sorry, I was a bit tired…
If we don‘t need a custom query here, and I don’t think we do, we should avoid it.
We should also filter for Profit and Loss accounts, at the least.
Filtering for Account Type may be a followup not being backported.

@ruthra-kumar ruthra-kumar force-pushed the filter_disable_accounts branch from e01bd9b to 3f668db Compare November 5, 2023 02:14
Copy link

codecov bot commented Nov 5, 2023

Codecov Report

Merging #37860 (6e3e094) into develop (10b9570) will decrease coverage by 0.02%.
Report is 33 commits behind head on develop.
The diff coverage is 84.61%.

@@             Coverage Diff             @@
##           develop   #37860      +/-   ##
===========================================
- Coverage    67.41%   67.40%   -0.02%     
===========================================
  Files          757      757              
  Lines        60327    60337      +10     
===========================================
  Hits         40668    40668              
- Misses       19659    19669      +10     
Files Coverage Δ
...xt/accounts/doctype/sales_invoice/sales_invoice.py 83.91% <100.00%> (ø)
.../report/accounts_receivable/accounts_receivable.py 89.96% <100.00%> (ø)
erpnext/accounts/utils.py 73.16% <100.00%> (ø)
erpnext/controllers/queries.py 57.56% <100.00%> (+0.12%) ⬆️
...ext/manufacturing/doctype/work_order/work_order.py 80.66% <100.00%> (ø)
...cturing/doctype/production_plan/production_plan.py 84.40% <94.44%> (ø)
erpnext/stock/doctype/bin/bin.py 96.20% <91.66%> (ø)
erpnext/assets/doctype/asset/depreciation.py 83.68% <50.00%> (ø)
erpnext/controllers/buying_controller.py 83.94% <20.00%> (ø)
...ent/doctype/quality_procedure/quality_procedure.py 79.71% <80.64%> (ø)

... and 10 files with indirect coverage changes

@ruthra-kumar ruthra-kumar force-pushed the filter_disable_accounts branch from 3f668db to 6e3e094 Compare November 9, 2023 04:11
@ruthra-kumar
Copy link
Member Author

@ruthra-kumar: Thanks and sorry, I was a bit tired… If we don‘t need a custom query here, and I don’t think we do, we should avoid it. We should also filter for Profit and Loss accounts, at the least. Filtering for Account Type may be a followup not being backported.

Can't avoid Custom query in this case. Check: https://github.com/frappe/erpnext/blob/9a171db97f67f88fad1589016afb4e808571d04c/erpnext/controllers/queries.py#L603C23-L605

@ruthra-kumar ruthra-kumar merged commit 3502c01 into frappe:develop Nov 9, 2023
13 checks passed
ruthra-kumar added a commit that referenced this pull request Nov 9, 2023
…-37860

refactor: ignore disabled account while selecting Income Accounts (backport #37860)
ruthra-kumar added a commit that referenced this pull request Nov 9, 2023
…-37860

refactor: ignore disabled account while selecting Income Accounts (backport #37860)
@ruthra-kumar
Copy link
Member Author

@mergify backport version-14

Copy link
Contributor

mergify bot commented Nov 9, 2023

backport version-14

✅ Backports have been created

ruthra-kumar added a commit that referenced this pull request Nov 9, 2023
refactor: ignore disabled account while selecting Income Accounts (backport #37860)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport version-14-hotfix backport to version 14 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