-
-
Notifications
You must be signed in to change notification settings - Fork 119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[payment-8]: Payment api refactoring with customer and shoping session logic #87
Conversation
Reyzis2021
commented
Aug 20, 2023
…ring-with-customer-and-shoping-session-logic
…otalPrice' into feature/Payment-8/payment-api-refactoring-with-customer-and-shoping-session-logic # Conflicts: # src/main/java/com/zufar/onlinestore/payment/api/PaymentApi.java # src/main/java/com/zufar/onlinestore/payment/api/dto/ProcessedPaymentDetailsDto.java # src/main/java/com/zufar/onlinestore/payment/api/impl/PaymentApiImpl.java # src/main/java/com/zufar/onlinestore/payment/api/impl/intent/PaymentCreator.java # src/main/java/com/zufar/onlinestore/payment/api/impl/intent/PaymentIntentCreator.java # src/main/java/com/zufar/onlinestore/payment/api/impl/intent/PaymentMethodCreator.java # src/main/java/com/zufar/onlinestore/payment/api/impl/intent/PaymentRetriever.java # src/main/java/com/zufar/onlinestore/payment/converter/StripePaymentIntentConverter.java # src/main/java/com/zufar/onlinestore/payment/dto/CreatePaymentDto.java # src/main/java/com/zufar/onlinestore/payment/endpoint/PaymentEndpoint.java # src/main/java/com/zufar/onlinestore/payment/entity/Payment.java # src/main/java/com/zufar/onlinestore/payment/exception/PaymentMethodProcessingException.java # src/main/resources/db/changelog/version-1.0/20.07.2023.create-payment-table.sql
…ring-with-customer-and-shoping-session-logic # Conflicts: # src/main/java/com/zufar/onlinestore/payment/converter/StripePaymentIntentConverter.java
…ring-with-customer-and-shoping-session-logic
…ring-with-customer-and-shoping-session-logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
|
||
|
||
@Mapper(componentModel = "spring", uses = ShoppingSessionItemDtoConverter.class) | ||
@Mapper(componentModel = MappingConstants.ComponentModel.SPRING, uses = {ShoppingSessionItemDtoConverter.class, ItemsTotalPriceCalculator.class}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better use "spring" in the componentModel instead of MappingConstants.ComponentModel.SPRING ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
import org.mapstruct.Named; | ||
|
||
|
||
@Mapper(componentModel = "spring", uses = ProductInfoDtoConverter.class) | ||
@Mapper(componentModel = MappingConstants.ComponentModel.SPRING, uses = ProductInfoDtoConverter.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe better use "spring" in the componentModel instead of MappingConstants.ComponentModel.SPRING ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
@Mapping(target = "itemsTotalPrice", source = "paymentIntent.amount", qualifiedByName = {"calculateForPayment"}) | ||
Payment toEntity(final PaymentIntent paymentIntent, final ShoppingSessionDto shoppingSession); | ||
|
||
@Mapping(target = "customer", source = "paymentMethod.customer") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like we can drop this annotation, because of the the names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it’s not possible to do it this way because there is an ambiguity in the target types, and the maptract will not understand how to properly map
Payment toEntity(final PaymentIntent paymentIntent, final ShoppingSessionDto shoppingSession); | ||
|
||
@Mapping(target = "customer", source = "paymentMethod.customer") | ||
@Mapping(target = "currency", source = "currency") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like we can drop this annotation, because of the the names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -72,7 +72,7 @@ private List<ShoppingSessionItem> createItems(Set<NewShoppingSessionItemDto> ite | |||
.productsQuantity(productsWithQuantity.get(productInfo.getProductId())) | |||
.productInfo(productInfo) | |||
.build()) | |||
.collect(Collectors.toList()); | |||
.toList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be careful here. Should we return an immutable list here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
@Service | ||
public class ItemsTotalPriceCalculator { | ||
|
||
public BigDecimal calculate(List<ShoppingSessionItemDto> items) { | ||
@Named("toItemsTotalPrice") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this annotation here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
@Mapper(componentModel = "spring", uses = ShoppingSessionItemDtoConverter.class) | ||
@Mapper(componentModel = MappingConstants.ComponentModel.SPRING, uses = {ShoppingSessionItemDtoConverter.class, ItemsTotalPriceCalculator.class}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
public interface ShoppingSessionDtoConverter { | ||
|
||
@Mapping(target = "items", source = "entity.items", qualifiedByName = {"toShoppingSessionItemDto"}) | ||
@Mapping(target = "itemsTotalPrice", source = "entity.items", qualifiedByName = {"toItemsTotalPrice"}) | ||
ShoppingSessionDto toDto(final ShoppingSession entity); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add new line here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
import org.mapstruct.Named; | ||
|
||
|
||
@Mapper(componentModel = "spring", uses = ProductInfoDtoConverter.class) | ||
@Mapper(componentModel = MappingConstants.ComponentModel.SPRING, uses = ProductInfoDtoConverter.class) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
return ResponseEntity.ok() | ||
|
||
ApiResponse<Void> apiResponse = ApiResponse.<Void>builder() | ||
.message("Payment event successfully processed") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably needs to be in centralized storage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -33,5 +33,6 @@ public enum PaymentStatus { | |||
|
|||
private final String status; | |||
private final String description; | |||
|
|||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove redunant empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
import com.zufar.onlinestore.payment.exception.PaymentNotFoundException; | ||
import com.zufar.onlinestore.payment.exception.PaymentMethodProcessingException; | ||
import com.zufar.onlinestore.payment.exception.PaymentEventParsingException; | ||
import com.zufar.onlinestore.payment.exception.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wildcard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it fixed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
CREATE TABLE IF NOT EXISTS payment | ||
( | ||
payment_id BIGSERIAL PRIMARY KEY, | ||
payment_intent_id VARCHAR(32) NOT NULL UNIQUE, | ||
payment_intent_id VARCHAR(64) NOT NULL UNIQUE, | ||
shopping_session_id UUID NOT NULL UNIQUE, | ||
items_total_price DECIMAL NOT NULL CHECK (items_total_price > 0), | ||
status VARCHAR(32), | ||
description TEXT | ||
|
||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess we have to define cons on table lvl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -3,6 +3,7 @@ CREATE TABLE user_details | |||
id UUID NOT NULL PRIMARY KEY, | |||
first_name VARCHAR(55) NOT NULL, | |||
last_name VARCHAR(55) NOT NULL, | |||
stripe_customer_id VARCHAR(64) UNIQUE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
…ring-with-customer-and-shoping-session-logic # Conflicts: # src/main/java/com/zufar/onlinestore/user/converter/UserDtoConverter.java
…ring-with-customer-and-shoping-session-logic # Conflicts: # src/main/java/com/zufar/onlinestore/payment/exception/handler/PaymentExceptionHandler.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
import jakarta.validation.constraints.NotBlank; | ||
import java.util.UUID; | ||
|
||
public record ProcessPaymentDto( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ProcessPaymentRequest
looks better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -3,6 +3,7 @@ CREATE TABLE user_details | |||
id UUID NOT NULL PRIMARY KEY, | |||
first_name VARCHAR(55) NOT NULL, | |||
last_name VARCHAR(55) NOT NULL, | |||
stripe_customer_token VARCHAR(64) UNIQUE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add NOT_NULL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this field must be filled after creating the user, so it needs to be able to store the null
import com.zufar.onlinestore.payment.exception.PaymentNotFoundException; | ||
import com.zufar.onlinestore.payment.exception.PaymentMethodProcessingException; | ||
import com.zufar.onlinestore.payment.exception.PaymentEventParsingException; | ||
import com.zufar.onlinestore.payment.exception.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it fixed?
|
||
ApiResponse<ProcessedPaymentWithClientSecretDto> apiResponse = ApiResponse.<ProcessedPaymentWithClientSecretDto>builder() | ||
.data(processedPayment) | ||
.messages(List.of("Payment successfully processed")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use messages only for errors. Just leave messages empty as you have data field filled by processedPayment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
.body(paymentMethodId); | ||
ApiResponse<ProcessedPaymentDetailsDto> apiResponse = ApiResponse.<ProcessedPaymentDetailsDto>builder() | ||
.data(retrievedPayment) | ||
.messages(List.of("Payment successfully retrieved")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use messages only for errors. Just leave messages empty as you have data field filled by retrievedPayment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
import lombok.Builder; | ||
|
||
@Builder | ||
public record CreateCardDetailsTokenDto( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are @notempty annotations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
@PostMapping("/card") | ||
public ResponseEntity<ApiResponse<String>> processCardDetailsToken(@RequestBody @Valid final CreateCardDetailsTokenDto createCardDetailsTokenDto) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@RequestHeader("Stripe-Signature") @NotEmpty final String stripeSignatureHeader) throws PaymentEventProcessingException, PaymentEventParsingException { | ||
public ResponseEntity<ApiResponse<Void>> paymentEventProcess(@RequestBody @NotEmpty final String paymentIntentPayload, | ||
@RequestHeader("Stripe-Signature") @NotEmpty final String stripeSignatureHeader) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
* This class responsible for Stipe customer creation. It needed in order to connect | ||
* customer with payment intent, this will ensure the correct management of payment by Stripe API | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra space
* on their payment method (stripe object). Payment method is secondary object for | ||
* creating and processing payment by Stripe API. | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra space
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
return cardDetailsToken.getId(); | ||
} | ||
|
||
private static Map<String, Object> createCardDetails(CreateCardDetailsTokenDto createCardDetailsTokenDto) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove static
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
import com.zufar.onlinestore.payment.exception.PaymentNotFoundException; | ||
import com.zufar.onlinestore.payment.exception.PaymentMethodProcessingException; | ||
import com.zufar.onlinestore.payment.exception.PaymentEventParsingException; | ||
import com.zufar.onlinestore.payment.exception.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no