Ask BC3 for presence-aware card step updates - #608
Conversation
`steps#update` builds a fresh `Step` from the request and calls `replace_assignees` unconditionally, so an omitted `due_on` and an omitted `assignee_ids` are both cleared by the server. Omission and explicit clear are indistinguishable on the wire, which puts the fix out of reach of any client change — including pointerizing the SDK request, whose nil is omitted from the JSON either way. `cards#update` already has the mechanism: `update_assignees?` tests `has_key?`, and `recordable.changing` preserves unmentioned attributes. Steps need both. Refs #604
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b52a45fb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| A deployed BC3 fix stops the data loss for every already-installed v0.8.x | ||
| client, with no CLI release, no SDK release, and nothing for users to upgrade. | ||
| The reverse is not true: no client release can fix it at all. |
There was a problem hiding this comment.
Do not conclude that no client release can address this active data loss. The two reported paths already fetch the current step: validateStep supplies the full step to doAssignStep, and the due-only update path calls CardSteps().Get; those values can be echoed into UpdateStepRequest to preserve DueOn and AssigneeIDs, following the repository's existing read-modify-write approach for todos. This remains vulnerable to concurrent edits and does not replace the proposed server fix, but it prevents the deterministic wipe without even adding a GET to these paths, so ruling it out leaves users exposed while waiting for BC3 deployment.
Useful? React with 👍 / 👎.
Files a communiqué asking BC3 Rails for presence-aware
steps#update. No CLIcode changes — the whole point is that there is no CLI fix.
Why
#604 reports live data loss on released v0.8.0/v0.8.1:
assign --stepwipesdue_on, andcards step update --duewipes assignees.I originally read this as CLI-owned, then SDK-owned. Both were wrong.
app/controllers/steps_controller.rbsettles it:new_stepisStep.new step_params— a fresh Step, so an omitteddue_onyields one without a due date.
replace_assigneesruns unconditionally, andwith no
assignee_idskeyassignee_idsfalls through to"".split(",")→[], so every assignee is removed. Both clears are server-side, driven by theabsence of a key.
That means omission and explicit-clear are the same request on the wire.
Pointerizing
UpdateStepRequest.DueOndoes not help: a nil*stringisomitted from the JSON, and the controller clears on that omission. The server
has to become presence-aware before any client change can matter.
What it asks for
cards#updatealready carries half the answer —update_assignees?guards onhas_key?, andrecordable.changing(...)preserves unmentioned attributes.The communiqué asks for both in
steps#update, plus tests for due-onlypreservation, assignee-only preservation, and explicit clear (a fix that
guards on blankness rather than key presence passes the first two and breaks
the third).
It also raises a question rather than assuming:
card_update_paramsnilsdue_onon omission deliberately, per its own comment — so cards have thesame
due_onbehaviour intentionally. We ask whether that intent extends toJSON API callers.
Why server-side is the right lane
A deployed BC3 fix stops the bleeding for every already-installed v0.8.x client
with no CLI release, no SDK release, and nothing for users to upgrade. No
client release can fix it at all.
Not in this PR
(
internal/commands/cards.go,internal/commands/assign.go).UpdateStepRequestis a dependent follow-up — whatlets a client express omit-vs-clear once the server honours it. Inert on
its own.
#604's body has been corrected to say Rails-first.
Refs #604
Summary by cubic
Adds a communiqué requesting presence-aware
steps#updatein BC3 Rails to prevent wipingdue_onand assignees when those fields are omitted. Proposes usingrecordable.changingand guarding assignee updates likecards#update; no CLI or SDK changes in this PR (addresses #604).Written for commit 4b52a45. Summary will update on new commits.