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

feat(india): auto generate/cancel irn on invoice submit/cancel #30873

Closed
wants to merge 11 commits into from

Conversation

nextchamp-saqib
Copy link
Member

@nextchamp-saqib nextchamp-saqib commented May 2, 2022

Added a check in E Invoice Settings

CleanShot 2022-05-02 at 15 40 56@2x

On cancellation, Reason & Remark is by default set as '3' & 'Cancelled by user' respectively


Do not allow manual IRN cancellations (ref. #26638)

  • It is possible to check "IRN Cancelled" and then cancel the sales invoice while not cancelling the IRN on the portal. This creates confusion while auditing cancelled invoices on the portal

@maharshivpatel
Copy link
Contributor

@nextchamp-saqib cancel_irn_on_cancel function doesn't take into account when irn & ewaybill both are generated, this is due to logic being added on the client-side using einvoice.js instead of on the server-side inside utils.py cancel_irn function. I will also send PR for the same.

I am also working on adding all of the crucial logic on the server-side to avoid these kinds of bugs and enable rapid development.

@maharshivpatel
Copy link
Contributor

Problem: if IRN (e-invoice) cancellation is not successful due to any reason. invoice is still canceled so now the user doesn't have any way to cancel the e-invoice.

some possible errors:

  1. It's been more than 24 hours so IRN can't be canceled.
  2. Eway Bill was scanned by an officer en route ( or any other reason ) so the e-way bill can't be canceled and due to that IRN can't be canceled
  3. GSP application error.

In Case of reason 1 & 2. invoice will never be canceled and a credit note should be created against it.

Suggestions:

  1. Instead of using the on_cancel hook to cancel IRN we should use before_cancel so we can prevent invoice cancellation if IRN was not canceled.
  2. E-way API is now implemented so we can also use it to auto cancel the e-way bill ( error handling is not working as expected we may have to fix it first ).

@codecov
Copy link

codecov bot commented May 9, 2022

Codecov Report

Merging #30873 (650cef3) into develop (e3c0d01) will increase coverage by 0.29%.
The diff coverage is 20.00%.

@@             Coverage Diff             @@
##           develop   #30873      +/-   ##
===========================================
+ Coverage    62.97%   63.26%   +0.29%     
===========================================
  Files          986      986              
  Lines        67417    67523     +106     
===========================================
+ Hits         42453    42719     +266     
+ Misses       24964    24804     -160     
Impacted Files Coverage Δ
erpnext/hooks.py 100.00% <ø> (ø)
erpnext/regional/india/setup.py 94.24% <ø> (ø)
erpnext/regional/india/e_invoice/utils.py 38.84% <9.67%> (-1.43%) ⬇️
erpnext/controllers/accounts_controller.py 85.59% <100.00%> (+0.02%) ⬆️
.../report/delayed_item_report/delayed_item_report.py 60.78% <0.00%> (-35.30%) ⬇️
erpnext/accounts/doctype/bank/bank.py 71.42% <0.00%> (-14.29%) ⬇️
...rpnext/accounts/doctype/shareholder/shareholder.py 80.00% <0.00%> (-10.00%) ⬇️
...saction/incorrect_balance_qty_after_transaction.py 88.37% <0.00%> (-9.31%) ⬇️
...pnext/stock/doctype/delivery_note/delivery_note.py 66.21% <0.00%> (-2.71%) ⬇️
erpnext/assets/doctype/asset/depreciation.py 85.23% <0.00%> (-2.69%) ⬇️
... and 41 more

@nextchamp-saqib
Copy link
Member Author

nextchamp-saqib commented May 9, 2022

if IRN (e-invoice) cancellation is not successful due to any reason
3. GSP application error.

(if error handling is fixed) There will be an error thrown right? The cancellation process would rollback

  1. It's been more than 24 hours so IRN can't be canceled
  2. Eway Bill was scanned by an officer en route ( or any other reason ) so the e-way bill can't be canceled and due to that IRN can't be canceled

This problem existed even before. The way a user can cancel is to create a Credit Note or check IRN Cancelled manually and then cancel the invoice. This hasn't been documented anywhere and it is confusing.
Let me know if you have a user-friendly solution for this

Instead of using the on_cancel hook to cancel IRN we should use before_cancel so we can prevent invoice cancellation if IRN was not canceled.

If cancellation of IRN is successful and erpnext throws an error then the cancellation will be rolled backed but the IRN will be canceled on the portal.
This will again cause inconsistencies.

This was the reason before_submit or before_cancel hooks aren't used because IRN generation and cancellation must happen as the last step of submission/cancellation

@nextchamp-saqib nextchamp-saqib marked this pull request as draft May 9, 2022 12:14
@nextchamp-saqib
Copy link
Member Author

error handling is not working as expected we may have to fix it first

This should fix it frappe/frappe#16855

It is possible to check "IRN Cancelled" and cancel the sales invoice while not cancelling the IRN on the portal. This creates confusion while auditing cancelled invoices on the portal
@nextchamp-saqib nextchamp-saqib marked this pull request as ready for review May 26, 2022 13:37
@nextchamp-saqib
Copy link
Member Author

nextchamp-saqib commented May 26, 2022

Hey, @maharshivpatel can you review/test this once again and let me know if you see any problems here?

As per your previous comment, only one problem is unsolved.
If someone has already canceled IRN on the portal and then tries to cancel it in ERPNext again, then he would not be able to do so and will be stuck without being able to cancel the invoice.

@maharshivpatel
Copy link
Contributor

@nextchamp-saqib I will start testing soon meanwhile about that one problem that is unsolved I think we should do something like the below. this will increase the time limit to 72 hours and prevent accidental cancels. it should prevent all of the issues and provide better UX. what are your thoughts on this ?

extend check time to 72 hours
if status check time has passed

@maharshivpatel
Copy link
Contributor

maharshivpatel commented May 27, 2022

from #31100
We can remove the validation altogether. If IRN will be generated on submission, then modifying an invoice value after submission is not possible. So is there a need to keep the validation that some fields of the invoice cannot be modified?

This is only applicable if the user chooses to opt for "auto-generate" functionality.

If the user doesn't opt for "generate on submission" we would still need the validation.

I would also suggest giving preference to manual IRN generation for new ERPNext users.

As they are configuring the system this is the best option as this prevents most mistakes and gives a sense of control while previewing the details that are being sent to IRP.

we should also add an option to show a preview dialog for "generate on submission" and if the user clicks on the primary button then "continue submission" else "rollback the submit.

for example, we recently discovered that a user wrote the wrong item name ( Product A ) with the item code of ( Product B ) and the user added that item on SI as it showed up as ( Product B ) due to the item code.

however, when we generate IRN, the item name would be sent to IRP so the wrong item name ( Product A ) this mistake was prevented as the name ( Product A ) showed up in Preview.

moral of the story is previewing details before sending is great functionality and we should not remove it even for "generate on submission" we can add an option to enable/disable it for UX.

Copy link
Contributor

@maharshivpatel maharshivpatel left a comment

Choose a reason for hiding this comment

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

When API response is "success": false, It throws an error as expected but it doesn't roll back the submit.

erpnext/regional/india/e_invoice/utils.py Show resolved Hide resolved
@@ -885,7 +885,10 @@ def log_request(self, url, headers, data, res):
}
)
request_log.save(ignore_permissions=True)
frappe.db.commit()

if (self.invoice.doctype, self.invoice.name) not in frappe.flags.currently_saving:
Copy link
Contributor

Choose a reason for hiding this comment

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

if (self.invoice.doctype, self.invoice.name) not in frappe.flags.currently_saving:
This Prevents E-invoice Request Log ( logging ).
It should log failed request even if it was made using hooks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I was aware of this.
The problem is we can't run frappe.db.commit as it will commit all the previous changes to the db and lead to partial submissions in case

If IRN is to be generated on submission and submission is just one single process, then we'll have to prevent commit unless the submission is successful

Do you have any solution for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am sorry but I don't have an elegant way to share this. You can find relevant code by looking at the line number.

@nextchamp-saqib I think something like this will work instead of preventing commit what if we rollback the changes and then commit.

what I have done is as follows

if success is true it will run log_request else save variables [ url, header, and data ].

log_request_1

if success is false then RequestFailed will be raised and it will call set_failed_status which will rollback changes and then run log_request if It is run from the hook it will commit (with changes rollbacked )

log_request_3

If it was ran by hook get the current committed docstatus from db and set it to invoice docstatus then commit changes. In this case, submit is failed so it won't commit anything (rollback) so we do want to commit . It is safe to commit log_request after set_failed_status committed (rollbacked)

log_request_2

what do you think of this? are there any downsides?

erpnext/regional/india/e_invoice/utils.py Show resolved Hide resolved
@maharshivpatel
Copy link
Contributor

When API response is "success": false, It throws an error as expected but it doesn't roll back the submit.

If you want to test this manually add IGST & SGST both taxes and then try to submit you will receive an error

[ For Sl. No 1, GST rate of tax is incorrect or not as notified ] but the invoice will still be submitted.

@nextchamp-saqib
Copy link
Member Author

nextchamp-saqib commented May 31, 2022

There's one more problem here.
The user may not know if the invoice is eligible for IRN generation. On submission, the user might expect IRN to be generated but due to a field like the GST category not being valid, the IRN won't be generated and the invoice will be submitted.

The solution to this is to have a confirmation dialog if the invoice is eligible for e-invoicing. But editing/customizing the default submit confirmation is not possible as of now
OR have a custom banner message on the top of the form to display e-invoice eligibility.

@maharshivpatel Let me know if you see any other fix for this.

@maharshivpatel
Copy link
Contributor

maharshivpatel commented Jun 1, 2022

There's one more problem here. The user may not know if the invoice is eligible for IRN generation. On submission, the user might expect IRN to be generated but due to a field like the GST category not being valid, the IRN won't be generated and the invoice will be submitted.

The solution to this is to have a confirmation dialog if the invoice is eligible for e-invoicing. But editing/customizing the default submit confirmation is not possible as of now OR have a custom banner message on the top of the form to display e-invoice eligibility.

@maharshivpatel Let me know if you see any other fix for this.

@nextchamp-saqib how do you feel about unbinding submit function using jquery and using our own function like as below?

einvoice_submit_button

can you give write access to the PR branch? It will be easy to share and compare code changes.

@@ -13,13 +13,15 @@ erpnext.setup_einvoice_actions = (doctype) => {

const { doctype, irn, irn_cancelled, ewaybill, eway_bill_cancelled, name, __unsaved } = frm.doc;

const automate_irns = await frappe.db.get_single_value('E Invoice Settings', 'automate_irns');
Copy link
Contributor

Choose a reason for hiding this comment

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

if automate_irns is true user doesn't get to use generate e-way bill dialog to generate the e-way bill after submitting the doc which is possible behavior and is currently being used in the case of the manual IRN generation process.

@stale
Copy link

stale bot commented Jun 21, 2022

This pull request has been automatically marked as inactive because it has not had recent activity. It will be closed within 3 days if no further activity occurs, but it only takes a comment to keep a contribution alive :) Also, even if it is closed, you can always reopen the PR when you're ready. Thank you for contributing.

@stale stale bot added the inactive label Jun 21, 2022
@stale stale bot closed this Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accounts inactive needs-tests This PR needs automated unit-tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants