Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 37 additions & 16 deletions ExpenseAPI/Controllers/ExpenseController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,32 +78,53 @@ public async Task<ActionResult<Expense>> PostExpense(Expense expense)
[HttpPut("{id}")]
public async Task<IActionResult> PutExpense(int id, Expense expense)
{
if (id != expense.Id)
if (expense.Id != 0 && id != expense.Id)
{
_logger.LogWarning("Mismatched expense id for update. Route id: {RouteId}, Expense id: {ExpenseId}", id, expense.Id);
return BadRequest();
}

_context.Entry(expense).State = EntityState.Modified;
var existingExpense = await _context.Expenses.FindAsync(id);
if (existingExpense == null)
{
_logger.LogWarning("Expense with id: {ExpenseId} not found during update", id);
return NotFound();
}

if (expense.Date != default)
{
existingExpense.Date = expense.Date;
}

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

existingExpense.Amount = expense.Amount;
existingExpense.Category = expense.Category;
existingExpense.Title = expense.Title;
Comment thread
linjinhsien marked this conversation as resolved.
Comment on lines +104 to +106

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;
            }


try
if (string.IsNullOrWhiteSpace(existingExpense.Description))
{
await _context.SaveChangesAsync();
_logger.LogInformation("Updated expense with id: {ExpenseId}", id);
return BadRequest("Description is required");
}
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.

{
_logger.LogWarning("Lunch expense with amount over 400 is not allowed");
return BadRequest("午餐費用不能超過400元");
}

if (existingExpense.Amount < 0)
{
if (!ExpenseExists(id))
{
_logger.LogWarning("Expense with id: {ExpenseId} not found during update", id);
return NotFound();
}
else
{
throw;
}
_logger.LogWarning("數量不能為負的");
return BadRequest("數量不能為負的");
}

await _context.SaveChangesAsync();
Comment thread
linjinhsien marked this conversation as resolved.
_logger.LogInformation("Updated expense with id: {ExpenseId}", id);

return NoContent();
}

Expand All @@ -130,4 +151,4 @@ private bool ExpenseExists(int id)
return _context.Expenses.Any(e => e.Id == id);
}
}
}
}
54 changes: 54 additions & 0 deletions ExpenseApiTest/ExpenseApiTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,58 @@ public async Task PostExpense_BoundaryExpense_ReturnsCreatedExpense()
Assert.Equal(newExpense.Date, createdExpense.Date);
Assert.Equal(newExpense.Title, createdExpense.Title);
}

[Fact]
public async Task PutExpense_PartialUpdateWithoutDate_KeepsOriginalDate()
{
// Arrange
var existingExpense = new Expense
{
Category = "食",
Description = "早餐",
Amount = 80,
Date = DateTime.Parse("2026-01-15"),
Title = "早餐"
};

_context.Expenses.Add(existingExpense);
await _context.SaveChangesAsync();

var updateRequest = new Expense
{
Description = "午餐",
Amount = 120
};

// Act
var result = await _controller.PutExpense(existingExpense.Id, updateRequest);
var updatedExpense = await _context.Expenses.FindAsync(existingExpense.Id);

// Assert
Assert.IsType<NoContentResult>(result);
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);

}

[Fact]
public async Task PutExpense_MismatchedId_ReturnsBadRequest()
{
// Arrange
var updateRequest = new Expense
{
Id = 99,
Description = "晚餐",
Amount = 100,
Date = DateTime.Parse("2026-01-10")
};

// Act
var result = await _controller.PutExpense(1, updateRequest);

// Assert
Assert.IsType<BadRequestResult>(result);
}

}