Uh oh!
There was an error while loading. Please reload this page.
refactor(api): split forms into fields, services and typed responses - #300
Merged
Conversation
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
harshtandiyaforce-pushed
the
refactor/api-forms
branch
from
July 29, 2026 16:38
09ba65c to
6bdca6fCompareharshtandiyaforce-pushed
the
refactor/api-sponsorships
branch
from
July 29, 2026 16:38
1ae05a8 to
414e4f7CompareContributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
harshtandiyaforce-pushed
the
refactor/api-forms
branch
from
July 29, 2026 16:45
6bdca6f to
f56fad3Compareharshtandiyaforce-pushed
the
refactor/api-sponsorships
branch
from
July 29, 2026 16:45
414e4f7 to
acea93fCompareContributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
harshtandiyaforce-pushed
the
refactor/api-sponsorships
branch
from
July 29, 2026 16:53
acea93f to
272c648Compareharshtandiyaforce-pushed
the
refactor/api-forms
branch
from
July 29, 2026 16:53
f56fad3 to
381c7ebCompareContributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
forms/__init__.py held 416 lines: five endpoints, nine meta-introspection helpers, and a get_custom_form_data that built its response dict inline across three branches. Every refusal was a bare frappe.throw, so a closed form and a missing event came back as the same generic error. The module is now four files. fields.py owns everything that reads doctype meta — form field construction, link options, the exclude-field sets and their validation. services.py owns CustomFormService, which resolves the event and its form row as plain properties over get_cached_doc and answers form_data() or submit(). The event proposal endpoints stay module-level functions; there is no per-request state there to hang a class on. __init__.py is 36 lines of endpoints that validate by annotation, delegate and return. Responses are pydantic models. form_fields stays list[dict] on purpose: its shape is built from the target doctype's meta at runtime and varies per field type, so only the envelope is modelled. custom_fields comes from a fixed get_all field list and is typed. event.name is sent as a string — Buzz Event autonames to integers, and nothing in the dashboard reads the key, so the cast costs nothing and survives a naming change. Errors are seven named classes carrying their own status and copy: three 404s, a 401, a 409 and two 400s for the excluded-field validation that runs on Buzz Event save. The 401 is the one deliberate contract change. BaseCustomEventForm.vue and EventProposalForm.vue branch on exc_type to swap the form for a login prompt, so the four frappe.AuthenticationError throws becoming LoginRequired means both comparisons move in this commit. Nothing else depended on that class: frappe/app.py clears cookies on an exact class match, not a subclass, and only ever did so for guests. The two "log in" / "login" copy variants collapsed into one string. Two dead things went with it. The Table re-copy loop in submit_custom_form re-assigned what the allowlist loop above it had already written, and the same loop in the proposal path targeted fields Event Proposal does not have. buzz_event.py imports validate_excluded_fields from its new home in fields.py. No endpoint renamed. Both read endpoints were diffed against a copy of the pre-change module across six seeded forms, the closed branch and a form carrying Buzz Custom Field rows, and came out byte-identical apart from event.name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither login_required nor auto_close_at had any end-to-end coverage, which is exactly the pair that decides whether a custom form renders at all. The login gate is also what the dashboard's exc_type branch reads, so a rename on the server could have silently stopped the login prompt from appearing and no test would have noticed. custom-forms.setup.ts seeds two more forms on the shared event: one behind login_required, one already past its auto_close_at with its own closed copy. The guest cases run under a storageState override inside the existing project rather than a new one — they need no session, only the absence of the shared one. The closed and not-found panels share bg-surface-amber-1, so expectClosed and expectNotFound now tell them apart by heading text. expectClosed was matching bg-surface-orange-1, which renders nowhere; it had no caller, so nothing failed. The event proposal spec gains the same guest pair. Its settings flip needs the shared admin session, since that block runs signed out, so it goes through newApiContext and restores in a finally. Seeded identifiers live in e2e/data/custom-forms.ts — Playwright refuses to let a spec import a setup file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both submit endpoints are allow_guest and both insert a document, but neither declared its HTTP methods, so both were reachable over GET. frappe/auth.py:85 skips CSRF validation for anything outside UNSAFE_HTTP_METHODS, so a GET reached them without a token — and since frappe only auto-commits on POST/PUT, the insert it triggered was silently rolled back. Either way it is a path that should not exist. Both callers already POST: frappe-ui defaults to it (frappeRequest.ts:83, `options.method || 'POST'`) and the e2e callMethod helper sets it explicitly. The read endpoints are left unrestricted — they mutate nothing, so pinning their verb would only risk a caller. Also moves the two field predicates to the bottom of fields.py and get_dial_code_list to the bottom of services.py, so each file opens with the function it exists for rather than its helpers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adding methods=["POST"] pushed the two submit decorators past the 110 column limit, so ruff wrapped them and carried the trailing nosemgrep comment down to the closing paren — off the line semgrep matches. The guest-whitelisted-method rule is blocking, so the linter job failed. Moved to a preceding-line comment, which semgrep honours and ruff cannot reflow. Verified against frappe/semgrep-rules locally: the file scans clean with the comment and reports the finding without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
harshtandiyaforce-pushed
the
refactor/api-forms
branch
from
July 29, 2026 17:00
381c7eb to
5b3381fCompareContributor
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Uh oh!
There was an error while loading. Please reload this page.
Contributor
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin main
git worktree add -d .worktree/backport-300-to-main origin/main
cd .worktree/backport-300-to-main
git switch --create backport-300-to-main
git cherry-pick -x 8f7653d194ef94561a666ced552727a84381206d |
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Stacked on #298 — read that first. Base
refactor/api-sponsorships, repoint todeveloponce that merges.PR 4 of the
buzz/apirestructure.forms/__init__.py416 lines → 36, overfields.py(doctype meta reads),services.py(CustomFormService+ the proposal functions),schemas.py,exceptions.py.form_fieldsstayslist[dict]on purpose — built from the target doctype's meta at runtime, shape varies per field type, so only the envelope is modelled.custom_fieldshas a fixedget_allfield list and is typed.The four
frappe.AuthenticationErrorthrows became oneLoginRequired(401). That changesexc_type, whichBaseCustomEventForm.vue:297andEventProposalForm.vue:122branch on to show the login prompt — both moved in the same commit, checked in the browser. Safe becausefrappe/app.py:411clears cookies one.__class__ == frappe.AuthenticationError, exact class not subclass, and only ever fired for guests.Both submit endpoints are now
methods=["POST"]. They areallow_guestand they insert;frappe/auth.py:85skips CSRF outsideUNSAFE_HTTP_METHODS, so a GET reached them tokenless and then rolled back uncommitted. frappe-ui already POSTs (frappeRequest.ts:83). Reads left unrestricted — they mutate nothing. The rest ofbuzz/apihas the same gap;process_bookingandcheckin_ticketare worth a look in PRs 5–6.event.namenow travels as a string —Buzz Eventautonames to ints and nothing in the dashboard reads the key.Not changed: no endpoint renamed;
buzz_event.py:10still importsvalidate_excluded_fields, fromfields.pynow.Deleted: the
Tablere-copy loop in both submit paths — the allowlist loop above it already wrote the same value, andEvent Proposalhas noTablefields at all.Deferred, both noted in the plan:
create_event_proposalsetssubmitted_bybutEvent Proposalhas no such field, so the proposer is only everowner. Andget_link_field_optionsreturns every row of a Link target to guests unfiltered — harmless today only becauseTalk Proposal.submitted_byis excluded by fieldname, so a new Link-to-Userunder another name would leak emails.Checks: 48 Python tests (was 23), 31 custom-forms + 14 event-proposal Playwright green headed on :8005, ruff and
vue-tscclean. Both read endpoints byte-identical against a copy of the pre-change module across six seeded forms, the closed branch, and one carryingBuzz Custom Fieldrows — barevent.name. Other E2E projects touch no file here; the 4 failures there are unrelated (login-modaldrives the Buzz modal as Administrator, which cannot work).🤖 Generated with Claude Code