[Fix] Persist field removals with full PUT form updates - #9
Conversation
…rm-update # Conflicts: # backend/Forms/FormsService.cs # frontend/src/components/form-editor/AworkIntegrationSettings.tsx # frontend/src/lib/api.ts
| form.AworkProjectId = dto.AworkProjectId; | ||
| form.AworkProjectTypeId = dto.AworkProjectTypeId; | ||
| form.AworkTaskListId = dto.AworkTaskListId; | ||
| form.AworkTaskStatusId = dto.AworkTaskStatusId; | ||
| form.AworkTypeOfWorkId = dto.AworkTypeOfWorkId; | ||
| form.AworkAssigneeId = dto.AworkAssigneeId; |
There was a problem hiding this comment.
Bug: The UpdateForm method unconditionally overwrites fields with null when a partial payload is sent, which can silently delete Awork integration settings.
Severity: CRITICAL
Suggested Fix
To prevent data loss from partial updates, apply null-coalescing to all property assignments in the UpdateForm method. For example, change form.AworkProjectId = dto.AworkProjectId; to form.AworkProjectId = dto.AworkProjectId ?? form.AworkProjectId;. This ensures that fields not present in the request payload retain their existing values in the database.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: backend/Forms/FormsService.cs#L196-L201
Potential issue: The `UpdateForm` endpoint accepts partial JSON payloads. When a client
sends an update for only a subset of fields (e.g., just the form `name`), the other
properties in the `UpdateFormDto` deserialize to `null`. The service then
unconditionally assigns these `null` values to the database entity, for example, via
`form.AworkProjectId = dto.AworkProjectId;`. This silently wipes out existing Awork
integration configurations, causing permanent data loss. While some properties use
null-coalescing to prevent this, the Awork-related fields do not, creating a dangerous
inconsistency.
Did we get this right? 👍 / 👎 to inform future reviews.
| form.LogoUrl = string.IsNullOrWhiteSpace(dto.LogoUrl) ? null : dto.LogoUrl; | ||
| form.IsSharedWithWorkspace = dto.IsSharedWithWorkspace ?? form.IsSharedWithWorkspace; |
There was a problem hiding this comment.
Bug: The UpdateForm method incorrectly uses PATCH semantics for IsSharedWithWorkspace and IsActive, preserving nulls instead of applying the intended full PUT semantics.
Severity: MEDIUM
Suggested Fix
In backend/Forms/FormsService.cs, change the assignments for form.IsSharedWithWorkspace and form.IsActive to use direct assignment from the DTO. Additionally, update the backend UpdateFormDto to use non-nullable bool for these properties to match the frontend contract.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: backend/Forms/FormsService.cs#L207-L208
Potential issue: The `UpdateForm` method implements inconsistent update semantics. While
most fields are updated using full PUT semantics (direct assignment), the
`IsSharedWithWorkspace` and `IsActive` properties use PATCH-style semantics via the
null-coalescing operator (`??`). This means if a client sends `null` for these boolean
fields, their existing values are silently preserved instead of being updated. This
contradicts the PR's stated goal of applying full replacement semantics for all incoming
DTO values and can lead to silent data inconsistencies, as a client's intent to clear or
reset a value is ignored without error.
Reference: n/a
Description
Form editor removals (e.g. task tag, awork mappings/select fields, description) were not always persisted because update payloads omitted cleared values.
Solution
nullfor cleared optional values.Preview (screenshots/screencasts)
n/a (backend + payload behavior change)
How has this been tested?
Commands:
npm run testnpm run lintnpm run builddotnet test backend.Tests/backend.Tests.csprojawork Runner config
Default