eops-370-fix(cli): add patch command so partial updates stop clearing fields - #21
Closed
mykhaylob-de wants to merge 1 commit into
Closed
mykhaylob-de wants to merge 1 commit into
mykhaylob-de wants to merge 1 commit into
Conversation
… fields `update` sends PUT, which is a full replace, so any field missing from the JSON file was silently blanked. The natural CLI workflow - write a small file, apply it - destroyed data. Twelve /public/v2 resources expose PATCH and the API already merges correctly under test, so add a `patch` command rather than having `update` fetch and merge client-side; reimplementing the merge in the CLI would be both weaker and racy. - add a `patch` command on the twelve resources whose API exposes PATCH - `update` keeps PUT semantics but now reads the record first and warns which populated fields the payload would clear, confirming when a terminal is attached. --yes skips the warning and the read it needs, so batch runs do not pay an extra GET per record - warn when omitting record_status would post a draft: the update schema defaults record_status to posted, so a partial PUT silently promoted drafts - replace the shared IJE-shaped `post` whitelist with a per-resource PostWritableFields, so enabling post elsewhere cannot wipe fields outside a list written for intercompany journal entries - cover both paths end to end by running the built CLI against a stub v2 API over a real socket, which also reproduces the original PUT data loss
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.
Summary
updatesends PUT, which is a full replace, so any field missing from the JSON file was silently blanked. The natural CLI workflow — write a small file, apply it — destroyed data, and on invoices it also flipped drafts to posted because the update schema defaultsrecord_statustoposted.Twelve
/public/v2resources expose PATCH and the API already merges correctly under test (11 tests, includingpatch_items_absent_keeps_existing_lines), so this adds apatchcommand rather than havingupdatefetch and merge client-side. Reimplementing the merge in the CLI would be both weaker and racy.Changes
patchcommand on the twelve resources whose API exposes PATCH: invoices, bills, customer payments/credits/prepayments/prepayment-applications, customers, vendors, items, classifications, fixed assets, contractsupdatekeeps PUT semantics but reads the record first and warns which populated fields the payload would clear, confirming when a terminal is attached. Without a terminal it warns and continues, so existing scripted updates do not break on a prompt they cannot answer.--yesskips the warning and the read it needs, so batch runs do not pay an extra GET per recordupdatewarns when omittingrecord_statuswould post a draftpostwhitelist becomes a per-resourcePostWritableFields(record fields, line fields, line key).postnow only exists where a shape is declared, so enabling it on another resource cannot wipe fields outside a list written for intercompany journal entriesid,number,created_at, totals…), otherwise every update would warn and the warning would stop meaning anythingTest plan
uv run pytest) — 180 passed, 125 skipped (integration tests needX_API_KEY)uv run ruff check .) — clean, andruff format --checkcleandualentry invoices patch/dualentry invoices updateNew coverage:
tests/test_e2e_partial_update.pyruns the builtdualentrybinary as a subprocess over a real socket against a stub v2 API. It proves a two-fieldpatchleaves every other field byte-identical, and reproduces the original data loss: afterupdatewith onlymemo,customer_idanditemsarenull,reference_numberis"", andrecord_statushas flippeddraft → postedtests/test_partial_updates.pycovers the warning contents, the confirm/abort path,--yes, advisory-probe failures (API error, connect error, timeout), and thatpatchis present on exactly the resources whose API supports itReviewed independently with Cursor (
gpt-5.3-codex-high); its two Important findings — the pre-read only caughtAPIErrorrather than transport failures, and the unconditional extra GET on every update — are fixed here.