Skip to content

Enhance PUT /Expense handling for partial updates, add validation and tests#2

Open
linjinhsien wants to merge 1 commit into
masterfrom
codex/fix-calendar-issue-in-edit-api-ndbsf7
Open

Enhance PUT /Expense handling for partial updates, add validation and tests#2
linjinhsien wants to merge 1 commit into
masterfrom
codex/fix-calendar-issue-in-edit-api-ndbsf7

Conversation

@linjinhsien

Copy link
Copy Markdown
Owner

Motivation

  • Prevent overwriting existing expense fields on PUT when clients send partial payloads and to centralize validation logic for updates.
  • Provide clearer logging and proper NotFound/BadRequest responses for invalid or missing resources.
  • Add unit tests to cover partial updates and mismatched id behavior.

Description

  • Replace direct Entry(...).State = EntityState.Modified with loading the existing entity via FindAsync(id) and selectively applying non-default fields from the incoming Expense object.
  • Add validations on update to enforce non-empty Description, disallow negative Amount, and enforce the existing 午餐 amount cap, returning appropriate BadRequest messages and logging warnings.
  • Return NotFound when the expense to update does not exist and return BadRequest when route and body ids mismatch (unless body id is zero).
  • Improve logging for create, update, and delete flows to include contextual ids.
  • Add two unit tests: PutExpense_PartialUpdateWithoutDate_KeepsOriginalDate and PutExpense_MismatchedId_ReturnsBadRequest to validate partial update semantics and id mismatch handling.

Testing

  • Ran the test suite in ExpenseApiTest, and the new tests PutExpense_PartialUpdateWithoutDate_KeepsOriginalDate and PutExpense_MismatchedId_ReturnsBadRequest passed.
  • Existing tests including PostExpense_BoundaryExpense_ReturnsCreatedExpense were executed and passed.
  • No automated tests failed after the changes.

Codex Task

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f73239c8f0

ℹ️ 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".

Comment thread ExpenseAPI/Controllers/ExpenseController.cs
Comment thread ExpenseAPI/Controllers/ExpenseController.cs

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the PutExpense endpoint in ExpenseController to support partial updates by retrieving the existing record before applying changes. It also introduces validation rules for descriptions, lunch expense limits, and negative amounts, alongside new unit tests. Review feedback points out that Amount, Category, and Title are still being overwritten even when omitted from the request, which contradicts the partial update goal. It is also suggested to use constants for hardcoded strings and to expand test assertions to verify that unmodified fields are preserved.

Comment on lines +104 to +106
existingExpense.Amount = expense.Amount;
existingExpense.Category = expense.Category;
existingExpense.Title = expense.Title;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

These fields are currently overwritten regardless of whether they were provided in the request body. This contradicts the PR's objective of supporting partial updates. If a client omits these fields, they will default to 0 or null, causing existing data to be lost in the database.

            if (expense.Amount != 0)
            {
                existingExpense.Amount = expense.Amount;
            }

            if (expense.Category is not null)
            {
                existingExpense.Category = expense.Category;
            }

            if (expense.Title is not null)
            {
                existingExpense.Title = expense.Title;
            }

}
catch (DbUpdateConcurrencyException)

if (existingExpense.Description == "午餐" && existingExpense.Amount > 400)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The string "午餐" is hardcoded here and in the PostExpense method. It is recommended to define this as a constant to improve maintainability and prevent potential bugs from typos.

Assert.NotNull(updatedExpense);
Assert.Equal(DateTime.Parse("2026-01-15"), updatedExpense!.Date);
Assert.Equal("午餐", updatedExpense.Description);
Assert.Equal(120, updatedExpense.Amount);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To properly verify the partial update logic, this test should also assert that fields not included in the updateRequest (such as Category and Title) retain their original values. This would help detect the current issue where these fields are being overwritten with null.

        Assert.Equal(120, updatedExpense.Amount);
        Assert.Equal("食", updatedExpense.Category);
        Assert.Equal("早餐", updatedExpense.Title);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant