From 6a8a8fc1f8f746cef3560c824e05525a84b1767e Mon Sep 17 00:00:00 2001 From: Vibhav Katre Date: Fri, 17 Jul 2026 12:14:01 +0530 Subject: [PATCH 1/3] =?UTF-8?q?Frappe=20Draw:=20Writer-style=20diagram=20s?= =?UTF-8?q?haring=20(view=20/=20comment=20/=20edit)=20=E2=80=94=20backend?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Roadmap item: share a diagram with view / comment / edit access like Frappe Writer. Built on Frappe core DocShare + a custom "comment" permission type — NO Frappe Drive dependency. (Drive file-tree registration and Yjs realtime are separately-scoped follow-ups; see research notes.) - patch register_comment_permission_type: registers a "comment" Permission Type for Draw Diagram (adds a `comment` Check to DocShare + the doctype's perm rules). Idempotent; skips gracefully on Frappe versions without Permission Type. - draw/api/share.py: whitelisted share_diagram(name,user,level) / unshare_diagram / get_diagram_shares / set_public. Levels map to DocShare flags — view={read}, comment={read,comment}, edit={read,comment,write,share}. Public reuses the diagram's is_public flag. All gated on the caller holding `share`. - draw/api/permission.py + hooks permission_query_conditions: list visibility now includes diagrams shared with the user (DocShare) and public ones, on top of owner-scoped access. Document-level read/write come from DocShare; `comment` is checked via frappe.has_permission("Draw Diagram","comment",doc=...). Verified: `bench run-tests --app draw` → 6 pass, incl. new tests that a shared user gets read+write+comment on "edit", read-only on "view", and nothing after unshare (all asserted through frappe.has_permission as that user). Next: the Share dialog UI (port Drive's ShareDialog — user picker + per-user view/comment/edit + public link). Realtime collab (Yjs, per Writer) is a separate milestone. Co-Authored-By: Claude Opus 4.8 (1M context) --- draw/api/permission.py | 28 ++++++++ draw/api/share.py | 60 +++++++++++++++++ .../doctype/draw_diagram/test_draw_diagram.py | 64 +++++++++++++++++++ draw/hooks.py | 15 ++--- draw/patches.txt | 3 +- .../register_comment_permission_type.py | 18 ++++++ 6 files changed, 178 insertions(+), 10 deletions(-) create mode 100644 draw/api/permission.py create mode 100644 draw/api/share.py create mode 100644 draw/patches/register_comment_permission_type.py diff --git a/draw/api/permission.py b/draw/api/permission.py new file mode 100644 index 0000000..6fe4367 --- /dev/null +++ b/draw/api/permission.py @@ -0,0 +1,28 @@ +# Permission helpers for Draw Diagram sharing (Writer-style view/edit/comment). +# Diagrams are owner-scoped by default (the Draw User role's if_owner perms); this +# widens list visibility to also include diagrams shared with the user (Frappe +# core DocShare) and public ones. Document-level read/write come from DocShare +# automatically; the custom "comment" permission type is checked via +# frappe.has_permission("Draw Diagram", "comment", doc=name). + +import frappe + + +def query_conditions(user: str | None = None) -> str: + """SQL clause limiting Draw Diagram list queries to what `user` may see: + their own diagrams, ones shared with them, and public ones. System Managers + (full access) get no restriction.""" + user = user or frappe.session.user + if "System Manager" in frappe.get_roles(user): + return "" + + table = "`tabDraw Diagram`" + conditions = [ + f"{table}.owner = {frappe.db.escape(user)}", + f"{table}.is_public = 1", + ] + shared = frappe.share.get_shared("Draw Diagram", user) + if shared: + names = ", ".join(frappe.db.escape(name) for name in shared) + conditions.append(f"{table}.name in ({names})") + return "(" + " or ".join(conditions) + ")" diff --git a/draw/api/share.py b/draw/api/share.py new file mode 100644 index 0000000..049e9dc --- /dev/null +++ b/draw/api/share.py @@ -0,0 +1,60 @@ +# Writer-style sharing for Draw Diagram — view / comment / edit access levels, +# built on Frappe core DocShare (frappe.share) plus a custom "comment" permission +# type (registered by draw.patches.register_comment_permission_type). No Frappe +# Drive dependency. Public access reuses the diagram's own `is_public` flag, which +# draw.api.permission.query_conditions already honours. + +import frappe +from frappe import _ +from frappe.utils import cint + +# Access level -> DocShare flags. "edit" also grants share so collaborators can +# re-share, matching the Drive/Writer "editor" tier. +LEVEL_FLAGS = { + "view": {"read": 1, "comment": 0, "write": 0, "share": 0}, + "comment": {"read": 1, "comment": 1, "write": 0, "share": 0}, + "edit": {"read": 1, "comment": 1, "write": 1, "share": 1}, +} + + +def _check_can_share(name: str) -> None: + if not frappe.has_permission("Draw Diagram", "share", doc=name): + frappe.throw(_("You are not permitted to share this diagram."), frappe.PermissionError) + + +@frappe.whitelist() +def share_diagram(name: str, user: str, level: str = "view") -> list: + """Share a diagram with a user at view / comment / edit level (idempotent — + re-sharing updates the level). Returns the current share list.""" + _check_can_share(name) + flags = LEVEL_FLAGS.get(level) + if not flags: + frappe.throw(_("Unknown access level: {0}").format(level)) + # Clear any prior grant first so lowering a level actually removes flags. + frappe.share.remove("Draw Diagram", name, user) + frappe.share.add("Draw Diagram", name, user=user, notify=0, **flags) + return get_diagram_shares(name) + + +@frappe.whitelist() +def unshare_diagram(name: str, user: str) -> list: + """Revoke a user's access. Returns the current share list.""" + _check_can_share(name) + frappe.share.remove("Draw Diagram", name, user) + return get_diagram_shares(name) + + +@frappe.whitelist() +def get_diagram_shares(name: str) -> list: + """The users this diagram is shared with, each with their access flags.""" + if not frappe.has_permission("Draw Diagram", "read", doc=name): + frappe.throw(_("Not permitted."), frappe.PermissionError) + return frappe.share.get_users("Draw Diagram", name) + + +@frappe.whitelist() +def set_public(name: str, enabled) -> None: + """Toggle "anyone in this site can view" via the diagram's is_public flag + (honoured by draw.api.permission.query_conditions).""" + _check_can_share(name) + frappe.db.set_value("Draw Diagram", name, "is_public", 1 if cint(enabled) else 0) diff --git a/draw/draw/doctype/draw_diagram/test_draw_diagram.py b/draw/draw/doctype/draw_diagram/test_draw_diagram.py index 1106eca..957062f 100644 --- a/draw/draw/doctype/draw_diagram/test_draw_diagram.py +++ b/draw/draw/doctype/draw_diagram/test_draw_diagram.py @@ -38,3 +38,67 @@ def test_legacy_single_type_still_valid(self): for t in ("block", "mindmap", "flowchart", "whiteboard"): doc = self._make(t, {"schemaVersion": 1, "diagramType": t}) self.assertEqual(doc.diagram_type, t) + + # ----- Writer-style sharing (view / comment / edit) ----- + + def _user(self, email): + if not frappe.db.exists("User", email): + frappe.get_doc( + { + "doctype": "User", + "email": email, + "first_name": email.split("@")[0], + "send_welcome_email": 0, + "roles": [{"role": "Draw User"}], + } + ).insert(ignore_permissions=True) + self.addCleanup(lambda: frappe.delete_doc("User", email, force=True, ignore_permissions=True)) + return email + + def test_share_edit_grants_read_write_comment(self): + from draw.api.share import get_diagram_shares, share_diagram + + user = self._user("draw-editor@example.com") + doc = self._make("unified", {"schemaVersion": 1, "diagramType": "unified"}) + share_diagram(doc.name, user, "edit") + + shares = {s["user"]: s for s in get_diagram_shares(doc.name)} + self.assertIn(user, shares) + self.assertTrue(shares[user]["read"] and shares[user]["write"] and shares[user].get("comment")) + + frappe.set_user(user) + try: + self.assertTrue(frappe.has_permission("Draw Diagram", "read", doc=doc.name)) + self.assertTrue(frappe.has_permission("Draw Diagram", "write", doc=doc.name)) + self.assertTrue(frappe.has_permission("Draw Diagram", "comment", doc=doc.name)) + finally: + frappe.set_user("Administrator") + + def test_share_view_is_read_only(self): + from draw.api.share import share_diagram + + user = self._user("draw-viewer@example.com") + doc = self._make("block", {"schemaVersion": 1, "diagramType": "block"}) + share_diagram(doc.name, user, "view") + + frappe.set_user(user) + try: + self.assertTrue(frappe.has_permission("Draw Diagram", "read", doc=doc.name)) + self.assertFalse(frappe.has_permission("Draw Diagram", "write", doc=doc.name)) + finally: + frappe.set_user("Administrator") + + def test_unshare_revokes_access(self): + from draw.api.share import get_diagram_shares, share_diagram, unshare_diagram + + user = self._user("draw-revoke@example.com") + doc = self._make("block", {"schemaVersion": 1, "diagramType": "block"}) + share_diagram(doc.name, user, "edit") + unshare_diagram(doc.name, user) + + self.assertEqual(get_diagram_shares(doc.name), []) + frappe.set_user(user) + try: + self.assertFalse(frappe.has_permission("Draw Diagram", "read", doc=doc.name)) + finally: + frappe.set_user("Administrator") diff --git a/draw/hooks.py b/draw/hooks.py index 5d11f42..9b6215d 100644 --- a/draw/hooks.py +++ b/draw/hooks.py @@ -129,15 +129,12 @@ # Permissions # ----------- -# Permissions evaluated in scripted ways - -# permission_query_conditions = { -# "Event": "frappe.desk.doctype.event.event.get_permission_query_conditions", -# } -# -# has_permission = { -# "Event": "frappe.desk.doctype.event.event.has_permission", -# } +# Diagrams are owner-scoped by default; widen list visibility to also include +# diagrams shared with the user (DocShare) and public ones. Document-level +# read/write/comment come from DocShare + the custom "comment" permission type. +permission_query_conditions = { + "Draw Diagram": "draw.api.permission.query_conditions", +} # Document Events # --------------- diff --git a/draw/patches.txt b/draw/patches.txt index f298651..f839faf 100644 --- a/draw/patches.txt +++ b/draw/patches.txt @@ -4,4 +4,5 @@ [post_model_sync] # Patches added in this section will be executed after doctypes are migrated -draw.patches.add_draw_user_role_and_permissions \ No newline at end of file +draw.patches.add_draw_user_role_and_permissions +draw.patches.register_comment_permission_type \ No newline at end of file diff --git a/draw/patches/register_comment_permission_type.py b/draw/patches/register_comment_permission_type.py new file mode 100644 index 0000000..f3c1c81 --- /dev/null +++ b/draw/patches/register_comment_permission_type.py @@ -0,0 +1,18 @@ +# Register a custom "comment" permission type for Draw Diagram so diagrams can be +# shared at a view / comment / edit level (Writer-style). Creating a Permission +# Type is gated to developer-mode/migrate/install, so it must run from a patch. +# Adds a `comment` Check to DocShare + the DocType's permission rules. Re-runnable. + +import frappe + + +def execute() -> None: + # Older Frappe versions have no Permission Type doctype — skip gracefully. + if not frappe.db.exists("DocType", "Permission Type"): + return + if frappe.db.exists("Permission Type", {"doc_type": "Draw Diagram", "perm_type": "comment"}): + return + frappe.get_doc( + {"doctype": "Permission Type", "doc_type": "Draw Diagram", "perm_type": "comment"} + ).insert(ignore_permissions=True) + frappe.clear_cache() From 7bcc758d165120fa1d7aab891cf3e75ec808b439 Mon Sep 17 00:00:00 2001 From: Vibhav Katre Date: Fri, 17 Jul 2026 12:17:15 +0530 Subject: [PATCH 2/3] test: drop Draw User role from sharing test user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The role isn't guaranteed to exist on a fresh CI site at test time, and sharing must NOT depend on it — DocShare grants access on its own. Removing it fixes the CI LinkValidationError and makes the test a stronger, role-independent check. Co-Authored-By: Claude Opus 4.8 (1M context) --- draw/draw/doctype/draw_diagram/test_draw_diagram.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/draw/draw/doctype/draw_diagram/test_draw_diagram.py b/draw/draw/doctype/draw_diagram/test_draw_diagram.py index 957062f..378c409 100644 --- a/draw/draw/doctype/draw_diagram/test_draw_diagram.py +++ b/draw/draw/doctype/draw_diagram/test_draw_diagram.py @@ -42,6 +42,8 @@ def test_legacy_single_type_still_valid(self): # ----- Writer-style sharing (view / comment / edit) ----- def _user(self, email): + # Deliberately NO Draw-specific role — this proves DocShare alone grants + # access to a shared diagram, independent of any role permission. if not frappe.db.exists("User", email): frappe.get_doc( { @@ -49,7 +51,6 @@ def _user(self, email): "email": email, "first_name": email.split("@")[0], "send_welcome_email": 0, - "roles": [{"role": "Draw User"}], } ).insert(ignore_permissions=True) self.addCleanup(lambda: frappe.delete_doc("User", email, force=True, ignore_permissions=True)) From df4d9598b8ca4ce331944267f05d0544d6a3f9d1 Mon Sep 17 00:00:00 2001 From: Vibhav Katre Date: Fri, 17 Jul 2026 12:25:00 +0530 Subject: [PATCH 3/3] fix: run app setup on fresh install too (after_install/after_migrate hooks) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Patches in patches.txt are marked complete WITHOUT executing on a fresh install, so a fresh Frappe Cloud install never got the Draw User role or the "comment" permission type — only migrates of existing sites did. (This is why the sharing tests failed on CI's fresh site but passed locally.) Consolidate all idempotent setup into draw/setup.py::ensure_setup (Draw User role + owner perms + register the diagram "comment" permission type) and run it from BOTH after_install (fresh) and after_migrate (upgrades). Remove the two now- superseded patches. Also assert the "comment" grant functionally via frappe.has_permission rather than the raw DocShare column. Verified locally: bench migrate runs the after_migrate hook cleanly; run-tests --app draw → 6 pass. CI's fresh-site install now exercises the after_install path. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../doctype/draw_diagram/test_draw_diagram.py | 5 +- draw/hooks.py | 6 +- draw/patches.txt | 5 +- .../add_draw_user_role_and_permissions.py | 55 ---------------- .../register_comment_permission_type.py | 18 ----- draw/setup.py | 65 +++++++++++++++++++ 6 files changed, 77 insertions(+), 77 deletions(-) delete mode 100644 draw/patches/add_draw_user_role_and_permissions.py delete mode 100644 draw/patches/register_comment_permission_type.py create mode 100644 draw/setup.py diff --git a/draw/draw/doctype/draw_diagram/test_draw_diagram.py b/draw/draw/doctype/draw_diagram/test_draw_diagram.py index 378c409..20c88ef 100644 --- a/draw/draw/doctype/draw_diagram/test_draw_diagram.py +++ b/draw/draw/doctype/draw_diagram/test_draw_diagram.py @@ -63,10 +63,13 @@ def test_share_edit_grants_read_write_comment(self): doc = self._make("unified", {"schemaVersion": 1, "diagramType": "unified"}) share_diagram(doc.name, user, "edit") + # Core flags on the share row are reliable everywhere. shares = {s["user"]: s for s in get_diagram_shares(doc.name)} self.assertIn(user, shares) - self.assertTrue(shares[user]["read"] and shares[user]["write"] and shares[user].get("comment")) + self.assertTrue(shares[user]["read"] and shares[user]["write"]) + # The contract that matters is enforcement — check it functionally, incl. + # the custom "comment" permission type. frappe.set_user(user) try: self.assertTrue(frappe.has_permission("Draw Diagram", "read", doc=doc.name)) diff --git a/draw/hooks.py b/draw/hooks.py index 9b6215d..a1c6a58 100644 --- a/draw/hooks.py +++ b/draw/hooks.py @@ -91,7 +91,11 @@ # ------------ # before_install = "draw.install.before_install" -# after_install = "draw.install.after_install" +# Idempotent setup (Draw User role + owner perms + diagram "comment" permission +# type). Run on fresh install AND on every migrate, because patches.txt patches do +# NOT execute on a fresh install. +after_install = "draw.setup.ensure_setup" +after_migrate = "draw.setup.ensure_setup" # Uninstallation # ------------ diff --git a/draw/patches.txt b/draw/patches.txt index f839faf..098d177 100644 --- a/draw/patches.txt +++ b/draw/patches.txt @@ -4,5 +4,6 @@ [post_model_sync] # Patches added in this section will be executed after doctypes are migrated -draw.patches.add_draw_user_role_and_permissions -draw.patches.register_comment_permission_type \ No newline at end of file +# (App setup — Draw User role, permissions, "comment" permission type — now lives +# in draw.setup.ensure_setup, run from after_install + after_migrate so it also +# applies on fresh installs, which skip patches.) diff --git a/draw/patches/add_draw_user_role_and_permissions.py b/draw/patches/add_draw_user_role_and_permissions.py deleted file mode 100644 index 5c0f8e8..0000000 --- a/draw/patches/add_draw_user_role_and_permissions.py +++ /dev/null @@ -1,55 +0,0 @@ -# Copyright (c) 2026, Frappe and contributors -# For license information, please see license.txt - -# Create the "Draw User" role that SPA users get, and give Draw Diagram / Draw -# Folder owner-scoped permissions (read/write/create/delete own) via if_owner. -# System Manager keeps full access (defined in the DocType JSON). Re-runnable. - -import frappe - -ROLE = "Draw User" -OWNED_DOCTYPES = ("Draw Diagram", "Draw Folder") - - -def execute() -> None: - _ensure_role() - for doctype in OWNED_DOCTYPES: - _ensure_owner_permission(doctype) - frappe.clear_cache() - - -def _ensure_role() -> None: - """Create the Draw User role idempotently (desk-enabled, normal users get it).""" - if frappe.db.exists("Role", ROLE): - return - frappe.get_doc( - { - "doctype": "Role", - "role_name": ROLE, - "desk_access": 1, - } - ).insert(ignore_permissions=True) - - -def _ensure_owner_permission(doctype: str) -> None: - """Add an if_owner perm row for Draw User on the given doctype if missing.""" - exists = frappe.db.exists( - "Custom DocPerm", {"parent": doctype, "role": ROLE, "if_owner": 1} - ) - if exists: - return - frappe.get_doc( - { - "doctype": "Custom DocPerm", - "parent": doctype, - "parenttype": "DocType", - "parentfield": "permissions", - "role": ROLE, - "if_owner": 1, - "read": 1, - "write": 1, - "create": 1, - "delete": 1, - "share": 1, - } - ).insert(ignore_permissions=True) diff --git a/draw/patches/register_comment_permission_type.py b/draw/patches/register_comment_permission_type.py deleted file mode 100644 index f3c1c81..0000000 --- a/draw/patches/register_comment_permission_type.py +++ /dev/null @@ -1,18 +0,0 @@ -# Register a custom "comment" permission type for Draw Diagram so diagrams can be -# shared at a view / comment / edit level (Writer-style). Creating a Permission -# Type is gated to developer-mode/migrate/install, so it must run from a patch. -# Adds a `comment` Check to DocShare + the DocType's permission rules. Re-runnable. - -import frappe - - -def execute() -> None: - # Older Frappe versions have no Permission Type doctype — skip gracefully. - if not frappe.db.exists("DocType", "Permission Type"): - return - if frappe.db.exists("Permission Type", {"doc_type": "Draw Diagram", "perm_type": "comment"}): - return - frappe.get_doc( - {"doctype": "Permission Type", "doc_type": "Draw Diagram", "perm_type": "comment"} - ).insert(ignore_permissions=True) - frappe.clear_cache() diff --git a/draw/setup.py b/draw/setup.py new file mode 100644 index 0000000..ce7bfcb --- /dev/null +++ b/draw/setup.py @@ -0,0 +1,65 @@ +# Idempotent app setup, run from BOTH the after_install and after_migrate hooks. +# +# Why hooks and not patches: patches in patches.txt are marked complete WITHOUT +# executing on a fresh install (they exist to migrate existing data), so a fresh +# Frappe Cloud install would otherwise never get the Draw User role or the custom +# permission type. Running an idempotent setup from after_install (fresh) + +# after_migrate (upgrades) covers both. + +import frappe + +ROLE = "Draw User" +OWNED_DOCTYPES = ("Draw Diagram", "Draw Folder") + + +def ensure_setup(*args, **kwargs) -> None: + """Create the Draw User role + owner-scoped perms and register the diagram + "comment" permission type. Safe to run repeatedly.""" + _ensure_role() + for doctype in OWNED_DOCTYPES: + _ensure_owner_permission(doctype) + _ensure_comment_permission_type() + frappe.clear_cache() + + +def _ensure_role() -> None: + """Create the Draw User role idempotently (desk-enabled, normal users get it).""" + if frappe.db.exists("Role", ROLE): + return + frappe.get_doc({"doctype": "Role", "role_name": ROLE, "desk_access": 1}).insert( + ignore_permissions=True + ) + + +def _ensure_owner_permission(doctype: str) -> None: + """Add an if_owner perm row for Draw User on the given doctype if missing.""" + if frappe.db.exists("Custom DocPerm", {"parent": doctype, "role": ROLE, "if_owner": 1}): + return + frappe.get_doc( + { + "doctype": "Custom DocPerm", + "parent": doctype, + "parenttype": "DocType", + "parentfield": "permissions", + "role": ROLE, + "if_owner": 1, + "read": 1, + "write": 1, + "create": 1, + "delete": 1, + "share": 1, + } + ).insert(ignore_permissions=True) + + +def _ensure_comment_permission_type() -> None: + """Register a "comment" permission type for Draw Diagram so diagrams can be + shared at a view / comment / edit level. Adds a `comment` Check to DocShare + + the doctype's perm rules. No-ops on Frappe versions without Permission Type.""" + if not frappe.db.exists("DocType", "Permission Type"): + return + if frappe.db.exists("Permission Type", {"doc_type": "Draw Diagram", "perm_type": "comment"}): + return + frappe.get_doc( + {"doctype": "Permission Type", "doc_type": "Draw Diagram", "perm_type": "comment"} + ).insert(ignore_permissions=True)