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

Allow to cancel draft transactions #21515

Closed
barredterra opened this issue Jun 28, 2023 · 7 comments · Fixed by #25971
Closed

Allow to cancel draft transactions #21515

barredterra opened this issue Jun 28, 2023 · 7 comments · Fixed by #25971
Assignees

Comments

@barredterra
Copy link
Collaborator

barredterra commented Jun 28, 2023

Is your feature request related to a problem? Please describe.
For sales transactions, many companies prefer consecutive numbering, e.g. Sales Invoice 1, 2, 3, 4, 5 without any gaps.

Gaps arise when draft or cancelled transactions get deleted. A simple solution is to disallow deletion of transactions. Cancelled transactions are clearly marked as such and do not confuse the user.
However, draft transactions appear to be open forever. We don't want to submit them, we cannot cancel them and we disallowed deletion to achieve consecutive numbering. Unused drafts are accumulating and there is no way to distinguish them from drafts that are being actively worked on.

Describe the solution you'd like
Allow to cancel draft transactions. There are no GL Entries or similar to take care of yet. So this might be as simple as doc.db_set("docstatus", 2). The only effect this should have is to mark the transaction as cancelled, no longer needed.

Describe alternatives you've considered
Use hash naming for the transactions and store the transaction number as per naming series in a separate field that is populated on submit. Draft transactions don't get a number yet, so they cannot cause gaps.

Alternative suggested by @ankush: have an additional status "Discarded" (docstatus stays at 0) at the application layer (erpnext or equivalent) to avoid confusion with the current cancel functionality

Related: #14708

@barredterra barredterra changed the title Cancel draft transaction Allow to cancel draft transactions Jun 28, 2023
@ankush
Copy link
Member

ankush commented Jul 28, 2023

Let's figure something out in Framework only 😔

Probable design:

  • A new action called "Discard" in form UI. Only appears on draft submittable documents.
  • Reuse docstatus 2 (?) and just set it with db_set (thus bypassing transition rules 🥴 ?)
  • Have an option to un-discard from UI (draft)
  • Hooks for these actions before_discard / after_discard etc.

Alternates:

  • docstatus -1 -> there are queries that do dosstatus with < operator. This will break and likely show flawed results. Eg. picklist business logic applies on draft documents too. So the assumption that only submitted documents have real impact kinda breaks down.
  • docstatus 3 similar concern as before.

Basically, the problem is that apps have treated these docstatus numbers as if they have some ordinality. In reality, it's just an enum. In the worst case we can treat this as breaking change and push in next version only if required.

@deepeshgarg007 @surajshetty3416

@barredterra
Copy link
Collaborator Author

What made you change your mind from favouring the application-level approach?

@ankush
Copy link
Member

ankush commented Jul 28, 2023

15 upvotes lol

@cogk
Copy link
Contributor

cogk commented Mar 6, 2024

For sales transactions, many companies prefer consecutive numbering, e.g. Sales Invoice 1, 2, 3, 4, 5 without any gaps.

Use hash naming for the transactions and store the transaction number as per naming series in a separate field that is populated on submit. Draft transactions don't get a number yet, so they cannot cause gaps.

That's what we have in Dokos (French fork of ERPNext): a name after submit feature.
This means that the Naming Series is not used until the document is submitted:

DocStatus Name Comment
unsaved new-sales-invoice-qsdf client side
0 (ec27f5ec591ed) random draft name, doc can deleted
1 SAL-INV-2024-00001 name after submit, doc cannot be deleted anymore
2 SAL-INV-2024-00001 name is kept for legal reasons, doc cannot be deleted
class Document:
	...
	def _save(self):
		...
		if self._action == "submit" and self.meta.name_after_submit and not self.flags.name_set:
			self._draft_name = self.name  # Client-side rename
			self.set_new_name(set_child_names=False)  # Magic happens here tooo
			from frappe.model.rename_doc import rename_doc

			rename_doc(  # Destroys cache 😢
				self.doctype,
				self._draft_name,
				self.name,
				ignore_permissions=True,
				force=True,
				show_alert=False,
			)

Gaps arise when draft or cancelled transactions get deleted

With Dokos' system, drafts don't lead to gaps because they don't use the Naming Series, and cancelled transactions can't be deleted (for legal reasons).


We also have an archiving feature thanks to an Archived Document doctype (basically a JSON snapshot of the document), which means that it's not possible to delete a submitted document. And we ensure "immutability/fraud detection" by using a chain of hashes (what we call a seal).

class ArchivedDocument(Document):
	...
	data: DF.JSON
	hash: DF.Data
	reference_docname: DF.DynamicLink
	reference_doctype: DF.Link  # Link to document -> prevents deleting linked doc!

	def on_trash(self):
		raise frappe.PermissionError

	def on_cancel(self):
		raise frappe.PermissionError

	def before_insert(self):
		...
		self.hash = chained_seal
		self.data = frappe.as_json(sealed_doc)

src 1 src 2

@ankush
Copy link
Member

ankush commented Mar 7, 2024

I've considered rename_doc before, rename doc is dangerous on large databases because it tries to rename every link field everywhere on submit. It's a big no no from performance POV. One unindexed field on large table is all it takes to severly slow down submissions.

We'd have to write a stripped down variant of rename doc to use it for draft->submit documents.

@cogk
Copy link
Contributor

cogk commented Mar 7, 2024

big no no from performance POV

Yes, so maybe we can't touch the primary key.

Use hash naming for the transactions and store the transaction number as per naming series in a separate field that is populated on submit. Draft transactions don't get a number yet, so they cannot cause gaps.

Could be shown in place of the name everywhere in the UI, just like the title field. Thanks to hide_name_column and stuff, the name could stop being prominent in the UI.

DocStatus Name (primary key) Transaction ID (new field) Comment
unsaved new-sales-invoice-qsdf client side
0 (sal-inv-2024-ec27f5e) random name (PK)
1 (sal-inv-2024-ec27f5e) SAL-INV-2024-00001 fill the tx_id field on submit
2 (sal-inv-2024-ec27f5e) SAL-INV-2024-00001 tx_id is set only once

@cogk
Copy link
Contributor

cogk commented Apr 11, 2024

Maybe a backwards compatible way of handling this dual naming scheme would be to have the "Transaction ID" field be an alternate name for the document?

There would be two ways to access a document on the desk:

  • /app/sales-invoice/v48ec27f5e (using primary key name = hash or uuid)
  • /app/sales-invoice/SAL-INV-2024-00001 (using alt name)

Listview and frappe.desk.form.load.getdoc would need to handle alt names, and transparently return the right document.


Pros: nicer for the users / Cons: …

@rutwikhdev rutwikhdev self-assigned this Apr 16, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants