[Order] - Feat: Add Patch API for update checkout entity #8
Conversation
Changelist by BitoThis pull request implements the following key changes.
|
Interaction Diagram by BitosequenceDiagram
participant API as REST API
participant Ctrl as CheckoutController<br/>🔄 Updated | ●●● High
participant Svc as CheckoutService<br/>🔄 Updated | ●●● High
participant Mapper as AddressMapper<br/>🟩 Added | ●●● High
participant Repo as CheckoutAddressRepository<br/>🟩 Added | ●●● High
participant PaySvc as PaymentService<br/>🟩 Added | ●●○ Medium
participant CustSvc as CustomerService<br/>🔄 Updated | ●●○ Medium
participant TaxSvc as TaxService<br/>🔄 Updated | ●●○ Medium
API->>Ctrl: PATCH /storefront/checkouts/{id}
Ctrl->>Svc: updateCheckoutByFields(id, CheckoutPatchVm)
Svc->>Repo: findByCheckoutIdAndType()
Svc->>Mapper: toModel or updateModel
Svc->>PaySvc: getPaymentProviders()
Svc->>CustSvc: getUserAddresses()
Svc->>TaxSvc: getTaxRate(taxClassIds, location)
Svc->>Svc: reCalculateCheckoutAmount()
Svc-->>Ctrl: CheckoutVm
Ctrl-->>API: ResponseEntity<CheckoutVm>
Note over Svc: Validates expiry-handles payment-address-tax
Critical path: REST API->CheckoutController->CheckoutService->AddressMapper->CheckoutAddressRepository->PaymentService->CustomerService->TaxService
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #f96c13
Actionable Suggestions - 10
-
java_codebase/order/src/main/java/com/yas/order/model/Checkout.java - 1
- Incorrect JPA OneToOne mapping · Line 71-71
-
java_codebase/order/src/main/java/com/yas/order/service/CheckoutService.java - 2
- Null checkoutAddress handling · Line 94-96
- Mock data in shipment provider update · Line 314-325
-
java_codebase/order/src/main/java/com/yas/order/viewmodel/checkout/CheckoutVm.java - 1
- Inconsistent shippingAddressDetail population across CheckoutVm builders · Line 28-28
-
java_codebase/customer/src/main/java/com/yas/customer/viewmodel/address/AddressDetailVm.java - 1
- Missing addressLine2 field in record · Line 9-10
-
java_codebase/product/src/main/java/com/yas/product/service/ProductService.java - 1
- Unsafe JPA entity mutation in read operation · Line 1176-1176
-
java_codebase/order/src/main/java/com/yas/order/service/PromotionService.java - 1
- Unsafe null return from circuit breaker fallback · Line 76-77
-
java_codebase/order/src/main/java/com/yas/order/config/ServiceUrlConfig.java - 1
- Missing payment service URL config · Line 7-7
-
java_codebase/order/src/main/java/com/yas/order/viewmodel/payment/PaymentProviderVm.java - 1
- Incompatible PaymentProviderVm fields for API deserialization · Line 6-11
-
java_codebase/order/src/main/java/com/yas/order/controller/CheckoutController.java - 1
- Missing input validation for shipping address ID · Line 74-74
Additional Suggestions - 1
-
java_codebase/order/src/main/java/com/yas/order/viewmodel/enumeration/DiscountType.java - 1
-
Duplicate DiscountType enum definition · Line 3-3The addition of this DiscountType enum introduces semantic duplication, as identical enums already exist in `com.yas.promotion.model.enumeration.DiscountType` and `com.yas.order.viewmodel.promotion.DiscountType`. This can lead to maintenance issues and potential inconsistencies if the enums diverge in the future, affecting downstream components like `CheckoutService` and `PromotionVerifyResultDto` that rely on discount type logic. Remove this duplicate and consolidate usage to a single enum definition to ensure consistency across the codebase.
Code suggestion
@@ -1,5 +1,0 @@ -package com.yas.order.viewmodel.enumeration; - -public enum DiscountType { - PERCENTAGE, FIXED; -}
-
Review Details
-
Files reviewed - 43 · Commit Range:
a467e9c..a467e9c- java_codebase/customer/src/main/java/com/yas/customer/service/UserAddressService.java
- java_codebase/customer/src/main/java/com/yas/customer/viewmodel/address/ActiveAddressVm.java
- java_codebase/customer/src/main/java/com/yas/customer/viewmodel/address/AddressDetailVm.java
- java_codebase/customer/src/test/java/com/yas/customer/controller/UserAddressControllerTest.java
- java_codebase/customer/src/test/java/com/yas/customer/service/LocationServiceTest.java
- java_codebase/location/src/main/java/com/yas/location/viewmodel/address/AddressDetailVm.java
- java_codebase/order/src/main/java/com/yas/order/OrderApplication.java
- java_codebase/order/src/main/java/com/yas/order/config/ServiceUrlConfig.java
- java_codebase/order/src/main/java/com/yas/order/controller/CheckoutController.java
- java_codebase/order/src/main/java/com/yas/order/mapper/AddressMapper.java
- java_codebase/order/src/main/java/com/yas/order/mapper/CheckoutMapper.java
- java_codebase/order/src/main/java/com/yas/order/model/Checkout.java
- java_codebase/order/src/main/java/com/yas/order/model/CheckoutAddress.java
- java_codebase/order/src/main/java/com/yas/order/model/CheckoutItem.java
- java_codebase/order/src/main/java/com/yas/order/repository/CheckoutAddressRepository.java
- java_codebase/order/src/main/java/com/yas/order/service/CheckoutService.java
- java_codebase/order/src/main/java/com/yas/order/service/CustomerService.java
- java_codebase/order/src/main/java/com/yas/order/service/PaymentService.java
- java_codebase/order/src/main/java/com/yas/order/service/PromotionService.java
- java_codebase/order/src/main/java/com/yas/order/service/ShipmentProviderService.java
- java_codebase/order/src/main/java/com/yas/order/service/TaxService.java
- java_codebase/order/src/main/java/com/yas/order/utils/Constants.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/checkout/CheckoutItemPostVm.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/checkout/CheckoutItemVm.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/checkout/CheckoutPatchVm.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/checkout/CheckoutPostVm.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/checkout/CheckoutVm.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/customer/ActiveAddressVm.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/enumeration/CheckoutAddressType.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/enumeration/DimensionUnit.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/enumeration/DiscountType.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/orderaddress/OrderAddressVm.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/payment/PaymentProviderVm.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/product/ProductCheckoutListVm.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/promotion/DiscountType.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/promotion/PromotionVerifyResultDto.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/promotion/PromotionVerifyVm.java
- java_codebase/order/src/main/java/com/yas/order/viewmodel/tax/TaxRateVm.java
- java_codebase/order/src/main/resources/db/changelog/ddl/changelog-0017.sql
- java_codebase/order/src/test/java/com/yas/order/controller/CheckoutControllerTest.java
- java_codebase/order/src/test/java/com/yas/order/service/CheckoutServiceTest.java
- java_codebase/product/src/main/java/com/yas/product/service/ProductService.java
- java_codebase/product/src/main/java/com/yas/product/viewmodel/product/ProductCheckoutListVm.java
-
Files skipped - 1
- java_codebase/order/src/main/resources/messages/messages.properties - Reason: Filter setting
-
Tools
- Java-google-format (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at muhammad.furqan@bito.ai.
Documentation & Help
AI Code Review powered by
| @SuppressWarnings("unused") | ||
| private Long shippingAddressId; | ||
|
|
||
| @OneToOne(fetch = FetchType.EAGER, cascade = CascadeType.PERSIST) |
There was a problem hiding this comment.
The newly added @OnetoOne relationship to CheckoutAddress lacks the mappedBy attribute, causing both entities to attempt owning the foreign key. This invalid JPA mapping will result in a MappingException at runtime when the application tries to persist or load entities, breaking checkout functionality in CheckoutService.createCheckout and related operations. Add mappedBy = "checkout" to the annotation since CheckoutAddress already owns the relationship via @joincolumn.
Code suggestion
Check the AI-generated fix before applying
| @OneToOne(fetch = FetchType.EAGER, cascade = CascadeType.PERSIST) | |
| @OneToOne(fetch = FetchType.EAGER, cascade = CascadeType.PERSIST, mappedBy = "checkout") |
Code Review Run #f96c13
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| return checkoutVm.toBuilder() | ||
| .checkoutItemVms(checkoutItemVms) | ||
| .shippingAddressDetail(addressMapper.toVm(checkout.getCheckoutAddress())) |
There was a problem hiding this comment.
The createCheckout method attempts to map checkout.getCheckoutAddress() to ActiveAddressVm without checking if it's null, which will throw a NullPointerException when no address is provided. This breaks checkout creation in production. Add a null check to safely handle cases where the address hasn't been set yet, as addresses are initialized later via update operations.
Code suggestion
Check the AI-generated fix before applying
| return checkoutVm.toBuilder() | |
| .checkoutItemVms(checkoutItemVms) | |
| .shippingAddressDetail(addressMapper.toVm(checkout.getCheckoutAddress())) | |
| return checkoutVm.toBuilder() | |
| .checkoutItemVms(checkoutItemVms) | |
| .shippingAddressDetail(checkout.getCheckoutAddress() != null ? | |
| addressMapper.toVm(checkout.getCheckoutAddress()) : null) |
Code Review Run #f96c13
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| public void updateShipmentProvider(Checkout existingCheckout, String value) { | ||
| if (shipmentProviderService.checkShipmentProviderAvailable(value)) { | ||
| existingCheckout.setShipmentMethodId(value); | ||
| } | ||
| if (existingCheckout.getShippingAddressId() != null) { | ||
| // Mock data | ||
| existingCheckout.getCheckoutItems().forEach(item -> { | ||
| item.setShipmentFee(new BigDecimal(5000)); | ||
| item.setShipmentTax(new BigDecimal(500)); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
The updateShipmentProvider method includes mock data for shipment fees and taxes, which will cause incorrect calculations in production. Additionally, it doesn't properly validate if the shipment provider is available, unlike other update methods. This impacts updateCheckoutByFields which is called from the PATCH endpoint in CheckoutController, and downstream affects the total amount calculation in reCalculateCheckoutAmount. Remove the mock logic and add proper validation to throw an exception if the shipment provider is not available.
Code suggestion
Check the AI-generated fix before applying
| public void updateShipmentProvider(Checkout existingCheckout, String value) { | |
| if (shipmentProviderService.checkShipmentProviderAvailable(value)) { | |
| existingCheckout.setShipmentMethodId(value); | |
| } | |
| if (existingCheckout.getShippingAddressId() != null) { | |
| // Mock data | |
| existingCheckout.getCheckoutItems().forEach(item -> { | |
| item.setShipmentFee(new BigDecimal(5000)); | |
| item.setShipmentTax(new BigDecimal(500)); | |
| }); | |
| } | |
| } | |
| public void updateShipmentProvider(Checkout existingCheckout, String value) { | |
| if (!shipmentProviderService.checkShipmentProviderAvailable(value)) { | |
| throw new NotFoundException("SHIPMENT_PROVIDER_NOT_AVAILABLE", value); | |
| } | |
| existingCheckout.setShipmentMethodId(value); | |
| } |
Code Review Run #f96c13
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| String shipmentServiceId, | ||
| String paymentMethodId, | ||
| Long shippingAddressId, | ||
| ActiveAddressVm shippingAddressDetail, |
There was a problem hiding this comment.
Adding the shippingAddressDetail field to CheckoutVm requires ensuring it's populated in all service methods that construct CheckoutVm instances. Currently, getCheckoutPendingStateWithItemsById in CheckoutService does not set this field, leading to null values in API responses, which can break downstream consumers like the frontend expecting complete address data. This inconsistency with methods like createCheckout and updateCheckoutByFields could cause runtime issues or incorrect behavior in checkout retrieval flows.
Code suggestion
Check the AI-generated fix before applying
- if (CollectionUtils.isEmpty(checkoutItems)) {
- return checkoutVm;
+ if (CollectionUtils.isEmpty(checkoutItems)) {
+ return checkoutVm.toBuilder().shippingAddressDetail(addressMapper.toVm(checkout.getCheckoutAddress())).build();
@@ -167,1 +167,1 @@
- return checkoutVm.toBuilder().checkoutItemVms(checkoutItemVms).build();
+ return checkoutVm.toBuilder().checkoutItemVms(checkoutItemVms).shippingAddressDetail(addressMapper.toVm(checkout.getCheckoutAddress())).build();
Code Review Run #f96c13
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| String addressLine1, | ||
| String city, |
There was a problem hiding this comment.
The AddressDetailVm record is missing the addressLine2 field, which is included in the data returned by the locationService.getAddressById method. This will cause deserialization failures when the customer module receives AddressDetailVm instances from the location service that include this field. Add String addressLine2 after addressLine1 to match the expected data structure.
Code suggestion
Check the AI-generated fix before applying
| String addressLine1, | |
| String city, | |
| String addressLine1, | |
| String addressLine2, | |
| String city, |
Code Review Run #f96c13
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| List<ProductCheckoutListVm> productCheckoutListVms = productPage.getContent() | ||
| .stream().map(product -> { | ||
| String thumbnailUrl = mediaService.getMedia(product.getThumbnailMediaId()).url(); | ||
| if (product.getParent() != null) { |
There was a problem hiding this comment.
Modifying JPA entity properties in a read operation can lead to accidental database updates since the method is transactional. Instead, modify ProductCheckoutListVm.fromModel in ProductCheckoutListVm.java to inherit these properties from the parent product if it exists, avoiding mutation of the entity. This resolves the issue comprehensively by handling inheritance in the view model layer without altering the entity.
Code suggestion
Check the AI-generated fix before applying
- product.getBrand().getId(),
- product.getPrice(),
- product.getTaxClassId(),
- "",
- product.getCreatedOn(),
- product.getCreatedBy(),
- product.getLastModifiedOn(),
- product.getLastModifiedBy(),
- product.getWeight(),
- product.getDimensionUnit(),
- product.getLength(),
- product.getWidth(),
- product.getHeight()
+ Objects.isNull(product.getParent()) ? product.getBrand().getId() : product.getParent().getBrand().getId(),
+ product.getPrice(),
+ Objects.isNull(product.getParent()) ? product.getTaxClassId() : product.getParent().getTaxClassId(),
+ "",
+ product.getCreatedOn(),
+ product.getCreatedBy(),
+ product.getLastModifiedOn(),
+ product.getLastModifiedBy(),
+ Objects.isNull(product.getParent()) ? product.getWeight() : product.getParent().getWeight(),
+ Objects.isNull(product.getParent()) ? product.getDimensionUnit() : product.getParent().getDimensionUnit(),
+ Objects.isNull(product.getParent()) ? product.getLength() : product.getParent().getLength(),
+ Objects.isNull(product.getParent()) ? product.getWidth() : product.getParent().getWidth(),
+ Objects.isNull(product.getParent()) ? product.getHeight() : product.getParent().getHeight()
Code Review Run #f96c13
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| protected PromotionVerifyResultDto handleVerifyFallback(Throwable throwable) throws Throwable { | ||
| return handleTypedFallback(throwable); |
There was a problem hiding this comment.
The handleVerifyFallback method returns null when the circuit breaker activates due to promotion service unavailability, but the caller in CheckoutService.updatePromotionCode assumes the PromotionVerifyResultDto is not null and accesses its fields directly, leading to a NullPointerException. This breaks the checkout process in production when the promotion service fails. Change the fallback to return a safe default PromotionVerifyResultDto(false, null, null, null, null) indicating an invalid promotion instead of null.
Code suggestion
Check the AI-generated fix before applying
| protected PromotionVerifyResultDto handleVerifyFallback(Throwable throwable) throws Throwable { | |
| return handleTypedFallback(throwable); | |
| protected PromotionVerifyResultDto handleVerifyFallback(Throwable throwable) throws Throwable { | |
| return new PromotionVerifyResultDto(false, null, null, null, null); |
Code Review Run #f96c13
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| public record ServiceUrlConfig( | ||
| String cart, String customer, String product, String tax, String promotion) { | ||
| } | ||
| String cart, String customer, String product, String tax, String promotion, String payment) { |
There was a problem hiding this comment.
Adding 'String payment' to the ServiceUrlConfig record introduces a new required configuration property. The PaymentService in the order module uses serviceUrlConfig.payment() to construct URLs for payment provider calls, but without the corresponding 'yas.services.payment' property in application.properties, Spring Boot will fail to bind the properties at startup, causing the entire order service to crash. This breaks the payment integration functionality that relies on this configuration.
Code suggestion
Check the AI-generated fix before applying
-yas.services.promotion=http://api.yas.local/promotion
+yas.services.promotion=http://api.yas.local/promotion
+yas.services.payment=http://api.yas.local/payment
Code Review Run #f96c13
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| public record PaymentProviderVm ( | ||
| String id, | ||
| String name, | ||
| String configureUrl, | ||
| String additionalSettings | ||
| ) { |
There was a problem hiding this comment.
The PaymentProviderVm record's fields do not align with the JSON structure returned by the payment service's /storefront/payment-providers endpoint, which will cause deserialization failures in PaymentService.getPaymentProviders and downstream in CheckoutService. The API returns fields like version, mediaId, and iconUrl, but this record only has additionalSettings. Update the record to include the correct fields to ensure compatibility.
Code suggestion
Check the AI-generated fix before applying
| public record PaymentProviderVm ( | |
| String id, | |
| String name, | |
| String configureUrl, | |
| String additionalSettings | |
| ) { | |
| public record PaymentProviderVm ( | |
| String id, | |
| String name, | |
| String configureUrl, | |
| int version, | |
| Long mediaId, | |
| String iconUrl | |
| ) { |
Code Review Run #f96c13
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| public ResponseEntity<CheckoutVm> updateCheckout(@PathVariable final String id, | ||
| @Valid @RequestBody | ||
| CheckoutPatchVm checkoutPatchVm) { | ||
| return ResponseEntity.ok(checkoutService.updateCheckoutByFields(id, checkoutPatchVm)); |
There was a problem hiding this comment.
The new PATCH endpoint for updating checkout fields calls checkoutService.updateCheckoutByFields, which in turn invokes updateAddress in the CheckoutService. This method attempts to parse the shippingAddressId string to a Long without proper error handling, risking a NumberFormatException that would cause a 500 error instead of a proper 400 Bad Request. This breaks the API's error handling for invalid input, potentially crashing the update operation in production.
Code suggestion
Check the AI-generated fix before applying
- public void updateAddress(Checkout existingCheckout, String value) {
- Optional<ActiveAddressVm> address = customerService.getUserAddresses()
- .stream().filter(add -> Objects.equals(add.id(), Long.valueOf(value)))
- .findFirst();
- if (address.isEmpty()) {
- throw new NotFoundException(ADDRESS_NOT_AVAILABLE, value);
- }
- existingCheckout.setShippingAddressId(Long.parseLong(value));
+ public void updateAddress(Checkout existingCheckout, String value) {
+ Long addressId;
+ try {
+ addressId = Long.parseLong(value);
+ } catch (NumberFormatException e) {
+ throw new BadRequestException("Invalid shipping address id: " + value);
+ }
+ Optional<ActiveAddressVm> address = customerService.getUserAddresses()
+ .stream().filter(add -> Objects.equals(add.id(), addressId))
+ .findFirst();
+ if (address.isEmpty()) {
+ throw new NotFoundException(ADDRESS_NOT_AVAILABLE, value);
+ }
+ existingCheckout.setShippingAddressId(addressId);
Code Review Run #f96c13
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
trulyfurqan
changed the title
Summary by Bito