refactor: drop superseded API endpoints from draw.api.diagram - #26
Merged
Conversation
diagram.py accumulated two kinds of endpoint nothing calls. Both are removed (359 -> 210 lines); no live code path changes. 1. A SECOND sharing implementation. list_shares / add_share / update_share / remove_share / search_users / set_public_access / _get_shareable_diagram predate draw/api/share.py and were left behind when the Share dialog was repointed at the real endpoints. They are worse than merely unused: they only know view/edit, so a caller reaching one would grant access under the old two-level model while the live path (and the permission query conditions) assume the three-level view/comment/edit one. search_users existed in both modules with different implementations. 2. CRUD superseded by frappe-ui resources. list_diagrams, trash_diagram, restore_diagram and delete_diagram duplicate what the frontend already does through createListResource / createDocumentResource (see frontend/src/data/diagrams.js) — TrashView and TileGrid set is_trashed via setValue and delete via the resource. list_diagrams was also stale in its own right: it filters on owner == session.user, so it predates sharing and would never have listed a shared diagram. Every whitelisted method is remotely reachable, so removing eleven that nothing calls shrinks the attack surface as well as the file. Kept deliberately: - get_diagram / get_public_diagram / save_diagram / save_thumbnail — the endpoints the editor actually calls. - purge_old_trashed_diagrams — the daily scheduler event in hooks.py. - duplicate_diagram — unused, but there is no "Duplicate" action in the UI for it to be wired to. That is a missing affordance, not dead code, so it stays with a docstring saying so. The module header now records where listing, trash and sharing actually live, so the next reader doesn't recreate a parallel set. Tests: 10 Frappe integration tests pass (bench run-tests --app draw); no test referenced any removed endpoint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (2): Last reviewed commit: "refactor: drop duplicate_diagram too (su..." | Re-trigger Greptile |
Correction to the previous commit's reasoning. duplicate_diagram was kept on the grounds that no UI called it and there was no "Duplicate" action for it to be wired to — a missing affordance rather than dead code. That was wrong. Duplicate DOES exist and work: DiagramTile's menu has the item, and TileGrid.duplicate handles it by inserting a copy through the list resource (which loads `document`, so the copy carries real content) — exactly the pattern trash and delete already use. So this endpoint belongs in the same bucket as list_diagrams / trash_diagram / restore_diagram / delete_diagram: a second implementation of something the frontend already does. Removed, and the module header now says so. diagram.py is now 359 -> 191 lines, and every remaining whitelisted method has a live caller. Tests: 10 Frappe integration tests pass. 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.
Third of three cleanup passes (backlog #9). Python-only and independent of #24/#25 — no file overlap, so this can merge in any order.
diagram.pyhad accumulated two kinds of endpoint that nothing calls. Both are removed: 359 → 210 lines, no live code path changed.1. A second sharing implementation
list_shares/add_share/update_share/remove_share/search_users/set_public_access/_get_shareable_diagrampredate draw/api/share.py and were left behind when the Share dialog was repointed at the real endpoints.These are worse than merely unused: they only know view/edit. A caller reaching one would grant access under the old two-level model, while the live path — and
permission_query_conditions— assume the three-level view/comment/edit one.search_usersexisted in both modules with different implementations.2. CRUD superseded by frappe-ui resources
list_diagrams,trash_diagram,restore_diagram,delete_diagramduplicate what the frontend already does throughcreateListResource/createDocumentResource(see frontend/src/data/diagrams.js) —TrashViewandTileGridsetis_trashedviasetValueand delete through the resource.list_diagramswas also stale in its own right: it filters onowner == session.user, so it predates sharing and would never have listed a shared diagram.Every
@frappe.whitelist()method is remotely reachable, so removing eleven that nothing calls shrinks the attack surface as well as the file.Kept deliberately
get_diagram/get_public_diagram/save_diagram/save_thumbnail— what the editor actually callspurge_old_trashed_diagrams— the dailyscheduler_eventsentry inhooks.pyduplicate_diagram— unused, but there is no "Duplicate" action in the UI for it to be wired to. A missing affordance, not dead code, so it stays with a docstring saying so.The module header now records where listing, trash and sharing actually live, so the next reader doesn't recreate a parallel set.
Testing
bench run-tests --app draw)🤖 Generated with Claude Code