mirror of
https://github.com/s-frick/effigenix.git
synced 2026-03-28 13:49:36 +01:00
fix(production): Review-Findings aus US-P17 beheben
RescheduleNotAllowed Error-Typ statt InvalidStatusTransition(status, status), Null-Guard für newDate, Controller-Parameter als Enum statt String (verhindert 500 bei ungültigem Status), partielle Datumsangaben als 400 ablehnen, ORDER BY in findAll() konsistent mit gefilterten Queries.
This commit is contained in:
parent
ad33eed2f4
commit
19f1cf16a1
8 changed files with 46 additions and 23 deletions
|
|
@ -25,7 +25,7 @@ import java.time.ZoneOffset;
|
|||
* 10. Only IN_PROGRESS → COMPLETED transition allowed via complete() (Batch must be COMPLETED – enforced by Use Case)
|
||||
* 11. Only PLANNED or RELEASED → CANCELLED transition allowed via cancel(reason)
|
||||
* 12. CancelledReason is set exactly once during cancel() and must not be blank
|
||||
* 13. Reschedule only in PLANNED or RELEASED, new date must not be in the past
|
||||
* 13. Reschedule only in PLANNED or RELEASED, new date must not be null or in the past
|
||||
*/
|
||||
public class ProductionOrder {
|
||||
|
||||
|
|
@ -196,7 +196,10 @@ public class ProductionOrder {
|
|||
|
||||
public Result<ProductionOrderError, Void> reschedule(LocalDate newDate) {
|
||||
if (status != ProductionOrderStatus.PLANNED && status != ProductionOrderStatus.RELEASED) {
|
||||
return Result.failure(new ProductionOrderError.InvalidStatusTransition(status, status));
|
||||
return Result.failure(new ProductionOrderError.RescheduleNotAllowed(status));
|
||||
}
|
||||
if (newDate == null) {
|
||||
return Result.failure(new ProductionOrderError.ValidationFailure("newDate must not be null"));
|
||||
}
|
||||
if (newDate.isBefore(LocalDate.now())) {
|
||||
return Result.failure(new ProductionOrderError.PlannedDateInPast(newDate));
|
||||
|
|
|
|||
|
|
@ -50,6 +50,11 @@ public sealed interface ProductionOrderError {
|
|||
@Override public String message() { return "Batch '" + batchId.value() + "' is not in COMPLETED status"; }
|
||||
}
|
||||
|
||||
record RescheduleNotAllowed(ProductionOrderStatus current) implements ProductionOrderError {
|
||||
@Override public String code() { return "PRODUCTION_ORDER_RESCHEDULE_NOT_ALLOWED"; }
|
||||
@Override public String message() { return "Cannot reschedule production order in status " + current; }
|
||||
}
|
||||
|
||||
record ValidationFailure(String message) implements ProductionOrderError {
|
||||
@Override public String code() { return "PRODUCTION_ORDER_VALIDATION_ERROR"; }
|
||||
}
|
||||
|
|
|
|||
|
|
@ -47,7 +47,7 @@ public class JdbcProductionOrderRepository implements ProductionOrderRepository
|
|||
@Override
|
||||
public Result<RepositoryError, List<ProductionOrder>> findAll() {
|
||||
try {
|
||||
var orders = jdbc.sql("SELECT * FROM production_orders ORDER BY created_at DESC")
|
||||
var orders = jdbc.sql("SELECT * FROM production_orders ORDER BY planned_date, created_at DESC")
|
||||
.query(this::mapRow)
|
||||
.list();
|
||||
return Result.success(orders);
|
||||
|
|
|
|||
|
|
@ -73,20 +73,26 @@ public class ProductionOrderController {
|
|||
public ResponseEntity<List<ProductionOrderResponse>> listProductionOrders(
|
||||
@RequestParam(required = false) @DateTimeFormat(iso = DateTimeFormat.ISO.DATE) LocalDate dateFrom,
|
||||
@RequestParam(required = false) @DateTimeFormat(iso = DateTimeFormat.ISO.DATE) LocalDate dateTo,
|
||||
@RequestParam(required = false) String status,
|
||||
@RequestParam(required = false) ProductionOrderStatus status,
|
||||
Authentication authentication
|
||||
) {
|
||||
logger.info("Listing production orders by actor: {}", authentication.getName());
|
||||
|
||||
var actor = ActorId.of(authentication.getName());
|
||||
ProductionOrderStatus statusEnum = status != null ? ProductionOrderStatus.valueOf(status) : null;
|
||||
|
||||
var result = (dateFrom != null && dateTo != null && statusEnum != null)
|
||||
? listProductionOrders.executeByDateRangeAndStatus(dateFrom, dateTo, statusEnum, actor)
|
||||
: (dateFrom != null && dateTo != null)
|
||||
boolean hasDateRange = dateFrom != null && dateTo != null;
|
||||
boolean hasPartialDate = (dateFrom != null) != (dateTo != null);
|
||||
|
||||
if (hasPartialDate) {
|
||||
return ResponseEntity.badRequest().build();
|
||||
}
|
||||
|
||||
var result = hasDateRange && status != null
|
||||
? listProductionOrders.executeByDateRangeAndStatus(dateFrom, dateTo, status, actor)
|
||||
: hasDateRange
|
||||
? listProductionOrders.executeByDateRange(dateFrom, dateTo, actor)
|
||||
: (statusEnum != null)
|
||||
? listProductionOrders.executeByStatus(statusEnum, actor)
|
||||
: status != null
|
||||
? listProductionOrders.executeByStatus(status, actor)
|
||||
: listProductionOrders.execute(actor);
|
||||
|
||||
if (result.isFailure()) {
|
||||
|
|
|
|||
|
|
@ -58,6 +58,7 @@ public final class ProductionErrorHttpStatusMapper {
|
|||
case ProductionOrderError.RecipeNotFound e -> 404;
|
||||
case ProductionOrderError.RecipeNotActive e -> 409;
|
||||
case ProductionOrderError.InvalidStatusTransition e -> 409;
|
||||
case ProductionOrderError.RescheduleNotAllowed e -> 409;
|
||||
case ProductionOrderError.BatchAlreadyAssigned e -> 409;
|
||||
case ProductionOrderError.BatchNotCompleted e -> 409;
|
||||
case ProductionOrderError.ValidationFailure e -> 400;
|
||||
|
|
|
|||
|
|
@ -109,7 +109,7 @@ class RescheduleProductionOrderTest {
|
|||
var result = rescheduleProductionOrder.execute(validCommand(), performedBy);
|
||||
|
||||
assertThat(result.isFailure()).isTrue();
|
||||
assertThat(result.unsafeGetError()).isInstanceOf(ProductionOrderError.InvalidStatusTransition.class);
|
||||
assertThat(result.unsafeGetError()).isInstanceOf(ProductionOrderError.RescheduleNotAllowed.class);
|
||||
verify(productionOrderRepository, never()).save(any());
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -778,7 +778,7 @@ class ProductionOrderTest {
|
|||
var result = order.reschedule(LocalDate.now().plusDays(14));
|
||||
|
||||
assertThat(result.isFailure()).isTrue();
|
||||
var err = (ProductionOrderError.InvalidStatusTransition) result.unsafeGetError();
|
||||
var err = (ProductionOrderError.RescheduleNotAllowed) result.unsafeGetError();
|
||||
assertThat(err.current()).isEqualTo(ProductionOrderStatus.IN_PROGRESS);
|
||||
}
|
||||
|
||||
|
|
@ -790,7 +790,7 @@ class ProductionOrderTest {
|
|||
var result = order.reschedule(LocalDate.now().plusDays(14));
|
||||
|
||||
assertThat(result.isFailure()).isTrue();
|
||||
var err = (ProductionOrderError.InvalidStatusTransition) result.unsafeGetError();
|
||||
var err = (ProductionOrderError.RescheduleNotAllowed) result.unsafeGetError();
|
||||
assertThat(err.current()).isEqualTo(ProductionOrderStatus.COMPLETED);
|
||||
}
|
||||
|
||||
|
|
@ -802,10 +802,21 @@ class ProductionOrderTest {
|
|||
var result = order.reschedule(LocalDate.now().plusDays(14));
|
||||
|
||||
assertThat(result.isFailure()).isTrue();
|
||||
var err = (ProductionOrderError.InvalidStatusTransition) result.unsafeGetError();
|
||||
var err = (ProductionOrderError.RescheduleNotAllowed) result.unsafeGetError();
|
||||
assertThat(err.current()).isEqualTo(ProductionOrderStatus.CANCELLED);
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("should fail when newDate is null")
|
||||
void should_Fail_When_NewDateNull() {
|
||||
var order = orderWithStatus(ProductionOrderStatus.PLANNED);
|
||||
|
||||
var result = order.reschedule(null);
|
||||
|
||||
assertThat(result.isFailure()).isTrue();
|
||||
assertThat(result.unsafeGetError()).isInstanceOf(ProductionOrderError.ValidationFailure.class);
|
||||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("should fail when new date is in the past")
|
||||
void should_Fail_When_DateInPast() {
|
||||
|
|
|
|||
|
|
@ -887,7 +887,7 @@ class ProductionOrderControllerIntegrationTest extends AbstractIntegrationTest {
|
|||
.contentType(MediaType.APPLICATION_JSON)
|
||||
.content(json))
|
||||
.andExpect(status().isConflict())
|
||||
.andExpect(jsonPath("$.code").value("PRODUCTION_ORDER_INVALID_STATUS_TRANSITION"));
|
||||
.andExpect(jsonPath("$.code").value("PRODUCTION_ORDER_RESCHEDULE_NOT_ALLOWED"));
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -928,7 +928,7 @@ class ProductionOrderControllerIntegrationTest extends AbstractIntegrationTest {
|
|||
.contentType(MediaType.APPLICATION_JSON)
|
||||
.content(json))
|
||||
.andExpect(status().isConflict())
|
||||
.andExpect(jsonPath("$.code").value("PRODUCTION_ORDER_INVALID_STATUS_TRANSITION"));
|
||||
.andExpect(jsonPath("$.code").value("PRODUCTION_ORDER_RESCHEDULE_NOT_ALLOWED"));
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -955,7 +955,7 @@ class ProductionOrderControllerIntegrationTest extends AbstractIntegrationTest {
|
|||
.contentType(MediaType.APPLICATION_JSON)
|
||||
.content(json))
|
||||
.andExpect(status().isConflict())
|
||||
.andExpect(jsonPath("$.code").value("PRODUCTION_ORDER_INVALID_STATUS_TRANSITION"));
|
||||
.andExpect(jsonPath("$.code").value("PRODUCTION_ORDER_RESCHEDULE_NOT_ALLOWED"));
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -1078,15 +1078,12 @@ class ProductionOrderControllerIntegrationTest extends AbstractIntegrationTest {
|
|||
}
|
||||
|
||||
@Test
|
||||
@DisplayName("Nur dateFrom ohne dateTo → alle auflisten (kein Range-Filter)")
|
||||
void listWithOnlyDateFrom_returnsAll() throws Exception {
|
||||
createPlannedOrder();
|
||||
|
||||
@DisplayName("Nur dateFrom ohne dateTo → 400 Bad Request")
|
||||
void listWithOnlyDateFrom_returns400() throws Exception {
|
||||
mockMvc.perform(get("/api/production/production-orders")
|
||||
.param("dateFrom", LocalDate.now().toString())
|
||||
.header("Authorization", "Bearer " + adminToken))
|
||||
.andExpect(status().isOk())
|
||||
.andExpect(jsonPath("$.length()").value(org.hamcrest.Matchers.greaterThanOrEqualTo(1)));
|
||||
.andExpect(status().isBadRequest());
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue