feat: add recipe entities + refactor codebase#113
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 67 out of 70 changed files in this pull request and generated 6 comments.
Files not reviewed (3)
- CulinaryCommandApp/Migrations/20260227192322_RecipeAndLocationUnitSupport.Designer.cs: Language not supported
- CulinaryCommandApp/Migrations/20260305191150_AddRecipeRowVersion.Designer.cs: Language not supported
- CulinaryCommandApp/Migrations/20260306015138_SeedUnits.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 68 out of 71 changed files in this pull request and generated 3 comments.
Files not reviewed (3)
- CulinaryCommandApp/Migrations/20260227192322_RecipeAndLocationUnitSupport.Designer.cs: Language not supported
- CulinaryCommandApp/Migrations/20260305191150_AddRecipeRowVersion.Designer.cs: Language not supported
- CulinaryCommandApp/Migrations/20260306015138_SeedUnits.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| public async Task<DateTime?> GetRowVersionAsync(int id) | ||
| { | ||
| return await _db.Recipes | ||
| .AsNoTracking() | ||
| .Where(r => r.RecipeId == id) | ||
| .Select(r => r.RowVersion) | ||
| .FirstOrDefaultAsync(); | ||
| } |
There was a problem hiding this comment.
GetRowVersionAsync() projects a non-nullable DateTime and uses FirstOrDefaultAsync; when the recipe isn’t found this returns default(DateTime) (0001-01-01) rather than null, which contradicts the method’s contract and breaks callers that check for null (e.g., ForceOverwrite). Project RowVersion as nullable (cast to DateTime?) and use SingleOrDefault/FirstOrDefault on the nullable projection so missing recipes return null.
| // Validate all lines: UnitId is non-nullable in the schema and required | ||
| // for inventory transactions, so every line (ingredient or sub-recipe) must | ||
| // have a unit selected before saving. Also ensure each line has exactly one | ||
| // of IngredientId / SubRecipeId set so no orphaned lines reach the database. | ||
| var missingUnit = _ingredientLines | ||
| .Where(l => l.Source.UnitId == 0) | ||
| .ToList(); | ||
| if (missingUnit.Any()) | ||
| { | ||
| _errorMessage = $"{missingUnit.Count} line(s) are missing a unit. Please select a unit for every ingredient and sub-recipe line."; | ||
| _saving = false; | ||
| return; | ||
| } | ||
|
|
||
| var missingReference = _ingredientLines | ||
| .Where(l => l.Source.IngredientId is null && l.Source.SubRecipeId is null) | ||
| .ToList(); | ||
| if (missingReference.Any()) | ||
| { | ||
| _errorMessage = $"{missingReference.Count} line(s) have no ingredient or sub-recipe selected. Please complete or remove them before saving."; | ||
| _saving = false; | ||
| return; | ||
| } |
There was a problem hiding this comment.
RecipeForm’s save validation checks for lines where both IngredientId and SubRecipeId are null, but it doesn’t enforce the “exactly one” requirement described in the comment. A line with both fields set would pass validation and persist an ambiguous reference. Add a validation that rejects lines where both IngredientId and SubRecipeId are set (and ideally ensure the UI clears the other field whenever one is chosen).
| <div class="col-md-2"> | ||
| <label class="form-label form-label-sm">Unit</label> | ||
| <select class="form-select form-select-sm" | ||
| value="@line.UnitId" | ||
| @onchange="(e) => OnLineUnitChanged(line, e.Value?.ToString())"> | ||
| <option value="0">-- Unit --</option> | ||
| @foreach (var unit in _locationUnits) | ||
| { | ||
| <option value="@unit.Id">@unit.Abbreviation</option> | ||
| } | ||
| </select> | ||
| </div> |
There was a problem hiding this comment.
For ingredient lines, the Unit dropdown is populated from all location units, allowing users to pick a unit that differs from the ingredient’s stock unit. InventoryTransactionService updates Ingredients.StockQuantity by adding StockChange without any unit conversion, so permitting arbitrary units here can lead to incorrect inventory deductions. Either constrain the selectable units to the ingredient’s unit (or a known convertible set) and normalize StockChange, or implement conversion before recording transactions.
antphan12
left a comment
antphan12
left a comment
There was a problem hiding this comment.
A lot of changes and looks good, will take a look at prod in the morning!
No description provided.