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(asset): cannot create asset if cwip disabled and account not set #23580

Merged
merged 4 commits into from
Oct 12, 2020

Conversation

gwhitney
Copy link
Contributor

I am submitting this PR as encouraged to do so in issue #23462. The problem is that ERPNext is requiring the specification of a Capital Work in Progress account in order to Submit an Asset, even if the Asset Category does not have Capital Work In Progress accounting enabled.

The rationale for this change is that in the file erpnext/doctype/asset/asset.py, currently the method make_gl_entries() only ever creates entries that credit a Capital Work in Progress account. Hence, it seems to stand to reason that if Capital Work in Progress is not enabled for a given Asset (because of its Asset Category), make_gl_entries() should never be called. From that, it seems logical that validate_make_gl_entry(), which controls whether make_gl_entries() is called, should always return false if CWIP is not enabled. So that's exactly what this PR does, is enforce this last principle.

However, I should point out that this modification doesn't really make complete sense, in that there currently are explicitly coded logic paths in validate_make_gl_entry() that return True when CWIP is not enabled. So with this change, those code paths could never possibly be used, defeating whatever their purpose is. Since I cannot understand their purpose (since after all, they would, if anything, result in a GL entry that involves the Current Work in Progress even though current work in progress accounting is disabled), I am not able to suggest an improvement to this PR. But my guess is that it will need some modification to make it appropriate to merge. I expressed these concerns in the discussion to #23462, but was nevertheless encouraged to submit a PR. So here it is. If this PR is accepted, it would have the effect that it:

Closes #23462

@nextchamp-saqib
Copy link
Member

nextchamp-saqib commented Oct 11, 2020

@gwhitney
The purpose of making gl entries even if CWIP is disabled is to handle the case such as:

  • Set CWIP disabled
  • Made Purchase invoice, now fixed asset account has been debited
  • Set CWIP enabled
  • On submission of asset, it debits fixed asset account again.

Hence the conditions checks if expense has already been booked or CWIP entry has been made regardless of CWIP being enabled or disabled

@nextchamp-saqib
Copy link
Member

nextchamp-saqib commented Oct 11, 2020

Asset Category: Allow enabling cwip only of cwip account is set in category or company

Screenshot 2020-10-11 at 1 11 55 PM

Fixed following cases

  • Case 1
    • PI created with cwip disabled and no cwip account set
    • Asset submitted with cwip disabled and no cwip account set
    • GL entry should not be created
  • Case 2
    • PR created with cwip disabled
    • Asset submitted with cwip enabled
    • GL entry should not be created
    • PI should book expense of the Asset
  • Case 3
    • PR created with cwip enabled (cwip booked)
    • Asset submitted with cwip disabled
    • GL entry should be created
  • Case 4
    • PI created with update stock with cwip disabled (expense booked)
    • Asset submitted with cwip enabled
    • GL entry should not be created
  • Case 5
    • PI created with update stock with cwip enabled (cwip booked)
    • Asset submitted with cwip disabled
    • GL entry should be created

@nextchamp-saqib nextchamp-saqib changed the title fix(Asset): Never add an asset GL entry if CWIP is not enabled fix(asset): cannot create asset if cwip disabled and account not set Oct 11, 2020
@nextchamp-saqib
Copy link
Member

@gwhitney Please test the changes and let me know if it fixes your issue.

@gwhitney
Copy link
Contributor Author

OK, with the branch as updated here, I was just able to successfully create and submit an existing asset with no CWIP account set (and CWIP not enabled). So yes, it fixes the problem I was having. Clearly, the changes go well beyond that in scope, and I can't comment on whether the other aspects of the change work properly or not, but the difficulty I was having, of simply not being able to submit an existing asset without a CWIP account, seems resolved. Thank you!

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.

Can't create asset without CWIP
2 participants