Frappe Draw: Share dialog UI wired to sharing backend (view/comment/edit) - #13
Merged
Conversation
…nt/edit)
The Share dialog (ShareMenu.vue + useShare.js) already existed but called backend
methods that were never built (draw.api.diagram.*). Point it at the real
draw.api.share.* API, add the third "Can comment" access level end to end, and add
the user-search endpoint the invite box needs.
- api/share.py: add search_users(txt) (enabled users, name/full-name match) and
enrich get_diagram_shares to return {user, full_name, user_image, level,
can_edit} + raw read/write/comment (level = edit|comment|view).
- useShare.js: call draw.api.share.get_diagram_shares / share_diagram (idempotent
add+update) / unshare_diagram / search_users / set_public; addMember/setMemberRole
now take a level string instead of a can_edit boolean.
- ShareMenu.vue: invite + per-member selects gain "Can comment" (view/comment/edit).
Verified: build + 7 Python tests (adds a level-reporting case) + live app — opening
Share, inviting a user at "comment" creates the right DocShare
(read=1,write=0,comment=1,level=comment), the member shows with a "Can comment"
select, and the "Shared with…" toast fires (screenshot); no page errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Confidence Score: 4/5Safe to merge; the new sharing logic is correct and tested. The two issues are minor and won't break functionality. The core sharing flow—invite, level update, revoke, public toggle—is correctly wired and covered by 7 Python tests. The N+1 query in get_diagram_shares only matters at scale, and the open search_users endpoint is likely acceptable for the invite UX but worth an intentional call. draw/api/share.py — both findings live here; everything else is straightforward glue. Reviews (1): Last reviewed commit: "Frappe Draw: wire the Share dialog to th..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+62
to
+79
| shares = [] | ||
| for row in frappe.share.get_users("Draw Diagram", name): | ||
| if row.get("everyone") or not row.get("user"): | ||
| continue | ||
| info = frappe.db.get_value("User", row.user, ["full_name", "user_image"], as_dict=True) or {} | ||
| shares.append( | ||
| { | ||
| "user": row.user, | ||
| "full_name": info.get("full_name"), | ||
| "user_image": info.get("user_image"), | ||
| "read": row.read, | ||
| "write": row.write, | ||
| "comment": row.get("comment"), | ||
| "level": _level_of(row), | ||
| "can_edit": bool(row.write), | ||
| } | ||
| ) | ||
| return shares |
There was a problem hiding this comment.
frappe.db.get_value is called once per shared user. Batch this with a single frappe.get_all("User", filters={"name": ["in", user_list]}, fields=["name","full_name","user_image"]) keyed by name, then look up each row from that dict.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+82
to
+94
| @frappe.whitelist() | ||
| def search_users(txt: str = "") -> list: | ||
| """Enabled users matching `txt` (name or full name), for the invite box. | ||
| Excludes Guest/Administrator.""" | ||
| like = f"%{txt or ''}%" | ||
| return frappe.get_all( | ||
| "User", | ||
| filters={"enabled": 1, "name": ["not in", ("Administrator", "Guest")]}, | ||
| or_filters={"name": ["like", like], "full_name": ["like", like]}, | ||
| fields=["name", "full_name", "user_image"], | ||
| limit=10, | ||
| order_by="full_name asc", | ||
| ) |
There was a problem hiding this comment.
search_users has no "can share any diagram" gate — any authenticated user can call it to enumerate all enabled site users. If that's intentional for the invite UX, it should be documented; if not, add a permission check.
vibhavkatre
pushed a commit
that referenced
this pull request
Jul 31, 2026
…and sharing
Extending the audit into two untested surfaces found a shipped, user-facing bug.
**Public sharing was dead over HTTP.** `set_public(name: str, enabled)` left one
argument unannotated, and Frappe enforces that EVERY argument of a whitelisted
function is annotated — it answers 417 with
FrappeTypeError: Argument 'enabled' in 'draw.api.share.set_public' is missing
type annotation. All arguments must have type annotations when type checking
is enforced.
So switching a diagram to "Anyone with the link can view" failed every time and
the user got "Could not update sharing. Please try again." Called in-process the
same function works perfectly, which is exactly why seven Python tests covering
sharing all passed: they import and call it, so they never cross the boundary
where the enforcement lives.
An AST sweep of all 11 whitelisted endpoints found this was the only one. Rather
than fix just the instance, `test_every_whitelisted_endpoint_annotates_all_arguments`
now walks the app's whitelisted signatures and fails on any unannotated argument —
the class is guarded, not the case. Verified after the fix that a JSON number, a
numeric string and a bool all return 200, so the annotation narrows nothing.
**New E2E coverage.**
`export.spec.js` (8 tests) drives the real Export menu and reads the file the
browser actually downloaded. Export is the feature with the worst record here —
#40 found every unified document exported as block-only, and that whiteboard lines
and tables were never exported at all, because `documentToSvg` was only ever
exercised by unit tests on hand-built documents. The SVG cases assert content from
every layer; PNG is checked by decoding its IHDR (a truncated or blank file fails);
PNG 1x vs 3x pins the scale argument; PDF is checked by its magic bytes. Confirmed
non-vacuous by re-breaking the line/table branch and watching the assertion fail.
`sharing.spec.js` (10 tests) drives the share dialog and asserts on the PERSISTED
DocShare rows, never the list the dialog renders — a dialog showing a member it
failed to save is this feature's characteristic failure, and how it shipped wired
to nonexistent endpoints before #13.
**Two things the quality-code-review checklist turned up in my own diff:** the
specs addressed the dialog's dropdowns positionally, which silently targets the
wrong control as soon as the dialog gains one; and those selects carried no
accessible label at all. Both fixed together — the selects now have `aria-label`s
and the specs use `getByLabel`, so the tests are robust and the dialog is more
accessible.
Also documented, in `helpers/fixtures.js`, that the fixture titles each document
after its test — so test names become page text. That has now bitten this suite
twice in opposite directions: a false PASS in the in-frame spec and a false
FAILURE here, where every test whose name contained "export" matched the title
button.
182 vitest · 56 E2E (+18) · 11 Python (+1).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vibhavkatre
added a commit
that referenced
this pull request
Jul 31, 2026
…and sharing (#53) * fix(share): "anyone with the link" never worked, plus E2E for export and sharing Extending the audit into two untested surfaces found a shipped, user-facing bug. **Public sharing was dead over HTTP.** `set_public(name: str, enabled)` left one argument unannotated, and Frappe enforces that EVERY argument of a whitelisted function is annotated — it answers 417 with FrappeTypeError: Argument 'enabled' in 'draw.api.share.set_public' is missing type annotation. All arguments must have type annotations when type checking is enforced. So switching a diagram to "Anyone with the link can view" failed every time and the user got "Could not update sharing. Please try again." Called in-process the same function works perfectly, which is exactly why seven Python tests covering sharing all passed: they import and call it, so they never cross the boundary where the enforcement lives. An AST sweep of all 11 whitelisted endpoints found this was the only one. Rather than fix just the instance, `test_every_whitelisted_endpoint_annotates_all_arguments` now walks the app's whitelisted signatures and fails on any unannotated argument — the class is guarded, not the case. Verified after the fix that a JSON number, a numeric string and a bool all return 200, so the annotation narrows nothing. **New E2E coverage.** `export.spec.js` (8 tests) drives the real Export menu and reads the file the browser actually downloaded. Export is the feature with the worst record here — #40 found every unified document exported as block-only, and that whiteboard lines and tables were never exported at all, because `documentToSvg` was only ever exercised by unit tests on hand-built documents. The SVG cases assert content from every layer; PNG is checked by decoding its IHDR (a truncated or blank file fails); PNG 1x vs 3x pins the scale argument; PDF is checked by its magic bytes. Confirmed non-vacuous by re-breaking the line/table branch and watching the assertion fail. `sharing.spec.js` (10 tests) drives the share dialog and asserts on the PERSISTED DocShare rows, never the list the dialog renders — a dialog showing a member it failed to save is this feature's characteristic failure, and how it shipped wired to nonexistent endpoints before #13. **Two things the quality-code-review checklist turned up in my own diff:** the specs addressed the dialog's dropdowns positionally, which silently targets the wrong control as soon as the dialog gains one; and those selects carried no accessible label at all. Both fixed together — the selects now have `aria-label`s and the specs use `getByLabel`, so the tests are robust and the dialog is more accessible. Also documented, in `helpers/fixtures.js`, that the fixture titles each document after its test — so test names become page text. That has now bitten this suite twice in opposite directions: a false PASS in the in-frame spec and a false FAILURE here, where every test whose name contained "export" matched the title button. 182 vitest · 56 E2E (+18) · 11 Python (+1). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(share): a change made while the previous one is in flight was dropped Found while chasing a flaky test in this branch — the flake was real behaviour, not a bad assertion. `toggleGlobalAccess` returned early whenever `updating` was true, so switching to "anyone with the link" and straight back to "restricted" discarded the second change. Worse than losing it: the dropdown then re-read the stale `isPublic` and snapped back to "link", telling the user their click had applied when it had not. Access changes are now queued rather than dropped, and the dialog sets the state the user PICKED instead of flipping the current one — a toggle applied to a value that is itself mid-update is ambiguous, a desired state never is. `toggleGlobalAccess` stays as a shim, since it was the public name here and flipping the current value is still what a bare toggle should mean. `useShare.test.js` is new (6 tests): the mid-flight case and the ordering case both fail against the old drop-on-busy code, verified by re-introducing it. Two notes for whoever edits that file — the resource double must be `reactive()`, because `isPublic` is a Vue computed and a plain object makes it cache its first value forever; and `reload()` reflects what was last sent to the server, read off the call spy, rather than a value the test sets, which would race the queue it is trying to observe. Both of those bit me and both fail in a way that looks like the code is broken rather than the double. The E2E round-trip also now waits for the DIALOG to catch up rather than only the server: `is_public` flips as soon as `set_public` commits, but the client is still inside its `reload()` at that point, so polling the API alone and switching straight back raced the reload. That was the flake. 252 vitest (+6) · 67 E2E (+2) · 11 Python. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Vibhav Katre <vibhav@frappe.io> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The Share dialog (
ShareMenu.vue+useShare.js) already existed but called backend methods that were never built (draw.api.diagram.*). This wires it to the realdraw.api.share.*API (from the sharing-backend PR), adds the third "Can comment" level end-to-end, and adds the user-search endpoint the invite box needs. Sharing is now a complete, user-facing feature.Changes
api/share.py:search_users(txt)(enabled users, name/full-name match) + enrichedget_diagram_shares→{user, full_name, user_image, level, can_edit}+ rawread/write/comment(level = edit|comment|view).useShare.js: callsdraw.api.share.get_diagram_shares/share_diagram(idempotent add+update) /unshare_diagram/search_users/set_public;addMember/setMemberRolenow take a level string.ShareMenu.vue: invite + per-member selects gain "Can comment" (view / comment / edit).Testing
read=1, write=0, comment=1, level=comment); the member renders with a "Can comment" select and the "Shared with…" toast fires (screenshot); no page errors.Remaining roadmap
required_app.🤖 Generated with Claude Code