refactor(api): add response schemas to the thin domains - #296
Merged
Conversation
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
harshtandiya
force-pushed
the
refactor/api-thin
branch
from
July 28, 2026 18:14
380d128 to
2589893
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Collaborator
Author
|
Rebased onto updated #295 — the |
auth, account, campaigns and proposals hand-built dicts with no declared contract. Each now returns a pydantic model extending APIResponse, so the response shape is the type signature. get_user_info keeps two models rather than one: a guest sees 2 keys and a logged-in user 10, and a merged model would have added null fields to the guest payload. Settings flags stay typed int because they travel as 0/1. ProposalListItem moves to proposals/schemas.py and extends APIResponse, which drops its manual model_dump() call. Invalid-language and disabled-campaign errors now carry real status codes (400, 409) instead of the generic 417. Payloads were captured before and after and are byte-identical. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each error condition in these two domains now has a class holding its own
title and message, instead of a string at the throw site:
UnknownLanguage.throw(language_code=language_code)
AlreadyRegistered.throw()
CRMNotAvailable stays in the 4xx range on purpose. A missing app is a site
configuration gap, and a 5xx would write an Error Log on every click.
Login-required and campaign-not-found keep using frappe.AuthenticationError
and frappe.DoesNotExistError — both already carry the right status and a
clear message, so wrapping them would add classes without adding meaning.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
harshtandiya
force-pushed
the
refactor/api-thin
branch
from
July 29, 2026 16:38
2589893 to
f2d3413
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
Contributor
|
Backport failed for Please cherry-pick the changes locally and resolve any conflicts. git fetch origin main
git worktree add -d .worktree/backport-296-to-main origin/main
cd .worktree/backport-296-to-main
git switch --create backport-296-to-main
git cherry-pick -x 11e7d6feeb7c1c25f8ba9b2fd54914ecd8e2ba6d |
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
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 #295 — read that first. Diff here is only the four thin domains.
Endpoints now return pydantic models and throw named errors instead of hand-built dicts and generic 417s.
Three modelling calls, all made to keep the wire byte-identical:
get_user_infoneeds two models. Guest gets 2 keys, logged-in user gets 10. One model with optionals would add"name": null, "roles": null, …to the guest payload.int, notbool. They're Check fields travelling as0/1.rolesstays an untypedlist. It holds child-table documents that serialize as full docs.Errors carry their own copy (base class is in #295):
Two things I deliberately left alone:
CRMNotAvailableis 400, not 5xx —frappe/app.py:415writes an Error Log for anything ≥ 500, and a missing app would log on every click.frappe.AuthenticationError/frappe.DoesNotExistError. Right status, clear message, wrapping adds nothing.register_campaign_intereststill doesn't checkcampaign.enabled(onlyget_campaign_detailsdoes). Extracting a sharedget_campaign()made adding it tempting; kept behaviour-neutral. Probably a real gap — separate call.Checks: 22 tests green (2 campaign cases skip,
Buzz Campaignwon't insert without thecrmapp). Payloads captured before/after viaorjson_dumps(..., default=json_handler)are byte-identical, includingdatetimeas2026-07-08 00:30:45.683135rather than RFC-3339.ruff/pre-commit/ fullbench run-tests --app buzzclean.🤖 Generated with Claude Code