log, transaction #20
log, transaction #20
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
|
이거 빌드 테스트한다고 잠시 하드코딩했었는데 실수로 커밋했었나봄 다시 환경변수로 넣어놨음 |
|
최종상태가 하드코딩이 아니어도 감지를 하는구나 |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces transaction management functionality following hexagonal architecture and domain-driven design principles. The implementation includes charge and payment transaction handling with repository persistence, mappers for DTO/domain conversion, and REST API endpoints.
- Implements transaction domain entities (Transaction, Charge, Pay, ItemScan) with JPA inheritance
- Adds transaction repository ports and adapters following hexagonal architecture
- Creates REST API endpoints for transaction operations (get, charge, pay)
Reviewed Changes
Copilot reviewed 17 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/resources/application.yaml | Test configuration file with database, JWT, and Toss API settings |
| src/main/kotlin/bssm/devcoop/occount/presentation/transaction/dto/PayRequestDto.kt | Request DTO for payment transactions |
| src/main/kotlin/bssm/devcoop/occount/presentation/transaction/dto/ChargeRequestDto.kt | Request DTO for charge transactions |
| src/main/kotlin/bssm/devcoop/occount/presentation/transaction/TransactionController.kt | REST controller providing transaction endpoints |
| src/main/kotlin/bssm/devcoop/occount/infrastructure/transaction/persistence/repository/TransactionJpaRepository.kt | JPA repository interface for Transaction entity |
| src/main/kotlin/bssm/devcoop/occount/infrastructure/transaction/persistence/repository/ChargeJpaRepository.kt | JPA repository with custom query for charge total calculation |
| src/main/kotlin/bssm/devcoop/occount/infrastructure/transaction/persistence/adapter/TransactionPersistenceAdapter.kt | Adapter implementing TransactionRepositoryPort |
| src/main/kotlin/bssm/devcoop/occount/infrastructure/transaction/persistence/adapter/ChargePersistenceAdapter.kt | Adapter implementing ChargeRepositoryPort |
| src/main/kotlin/bssm/devcoop/occount/domain/transaction/Transaction.kt | Abstract base entity for all transactions |
| src/main/kotlin/bssm/devcoop/occount/domain/transaction/Pay.kt | Payment transaction entity |
| src/main/kotlin/bssm/devcoop/occount/domain/transaction/ItemScan.kt | Entity for scanned items in payment transactions |
| src/main/kotlin/bssm/devcoop/occount/domain/transaction/Charge.kt | Charge transaction entity |
| src/main/kotlin/bssm/devcoop/occount/application/transaction/port/out/TransactionRepositoryPort.kt | Output port interface for transaction repository |
| src/main/kotlin/bssm/devcoop/occount/application/transaction/port/out/ChargeRepositoryPort.kt | Output port interface for charge repository |
| src/main/kotlin/bssm/devcoop/occount/application/transaction/port/in/TransactionUseCase.kt | Input port interface defining transaction use cases |
| src/main/kotlin/bssm/devcoop/occount/application/transaction/mapper/TransactionMapper.kt | Mapper for converting between DTOs and domain entities |
| src/main/kotlin/bssm/devcoop/occount/application/transaction/dto/TransactionResponseDto.kt | Response DTOs for transaction operations |
| src/main/kotlin/bssm/devcoop/occount/application/transaction/TransactionApplicationService.kt | Service implementing transaction use cases |
| GEMINI.md | Documentation of architecture and development guidelines |
| .gitignore | Added .DS_Store to ignored files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/kotlin/bssm/devcoop/occount/presentation/transaction/dto/ChargeRequestDto.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/presentation/transaction/dto/PayRequestDto.kt
Outdated
Show resolved
Hide resolved
...ssm/devcoop/occount/infrastructure/transaction/persistence/repository/ChargeJpaRepository.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/domain/transaction/ItemScan.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/domain/transaction/Charge.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/application/transaction/mapper/TransactionMapper.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/application/transaction/dto/TransactionResponseDto.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/application/transaction/TransactionApplicationService.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 18 out of 21 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...m/devcoop/occount/infrastructure/transaction/persistence/adapter/ChargePersistenceAdapter.kt
Outdated
Show resolved
Hide resolved
...m/devcoop/occount/infrastructure/transaction/persistence/adapter/ChargePersistenceAdapter.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/domain/transaction/Transaction.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/application/transaction/TransactionApplicationService.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/domain/transaction/ItemScan.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/application/transaction/dto/TransactionResponseDto.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 20 out of 23 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...m/devcoop/occount/infrastructure/transaction/persistence/adapter/ChargePersistenceAdapter.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/application/transaction/mapper/TransactionMapper.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/bssm/devcoop/occount/domain/transaction/Charge.kt
Outdated
Show resolved
Hide resolved
HyunwooKiim
force-pushed
the
Feat-Logs,-Transaction
branch
from
9080417 to
a8e7b44
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request introduces a complete transaction management feature with domain entities (Transaction, Charge, Pay, ItemScan), request/response DTOs, repository layer with QueryDSL, application services for command and query operations, REST controllers, and custom exception handling integrated with global error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CommandCtrl as TransactionCommandController
participant CmdService as TransactionCommandService
participant Mapper as TransactionMapper
participant Repo as TransactionRepository
participant DB as Database
Client->>CommandCtrl: POST /transactions/charge (ChargeRequest)
CommandCtrl->>CmdService: createCharge(ChargeRequest)
CmdService->>Mapper: toEntity(ChargeRequest)
Mapper-->>CmdService: Charge entity
CmdService->>Repo: save(Charge)
Repo->>DB: INSERT INTO tbl_charge
DB-->>Repo: Transaction saved
Repo-->>CmdService: Transaction with ID
CmdService->>Mapper: toTransactionResponse(Charge)
Mapper-->>CmdService: ChargeResponse
CmdService-->>CommandCtrl: TransactionResponse
CommandCtrl-->>Client: 201 Created (TransactionResponse)
sequenceDiagram
participant Client
participant QueryCtrl as TransactionQueryController
participant QueryService as TransactionQueryService
participant Repo as TransactionRepository
participant DB as Database
participant Mapper as TransactionMapper
Client->>QueryCtrl: GET /transactions/{userCode}
QueryCtrl->>QueryService: queryTransactionsByUserCode(userCode)
QueryService->>Repo: findByUserCode(userCode)
Repo->>DB: SELECT * FROM tbl_transaction WHERE user_code = ?
DB-->>Repo: List<Transaction>
Repo-->>QueryService: Transactions
QueryService->>Mapper: toTransactionResponse(each)
Mapper-->>QueryService: List<TransactionResponse>
QueryService-->>QueryCtrl: TransactionListResponse
QueryCtrl-->>Client: 200 OK (TransactionListResponse)
Note over QueryCtrl,DB: Alternate: GET /transactions/{userCode}/today-charge-total
Client->>QueryCtrl: GET /transactions/{userCode}/today-charge-total
QueryCtrl->>QueryService: queryTodayChargeTotal(userCode)
QueryService->>Repo: findTotalChargeByUserCodeAndDateBetween(...)
Repo->>DB: QueryDSL: SUM(chargedPoint) WHERE conditions
DB-->>Repo: Int (total)
Repo-->>QueryService: total
QueryService-->>QueryCtrl: Int
QueryCtrl-->>Client: 200 OK (Int)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/Pay.kt (1)
3-27: Missing join column causes join table mismatchWithout declaring the owning-side join column, JPA will synthesize an extra join table for this one-to-many. If your schema expects
tbl_item_scanto hold apay_idforeign key (which the mapper/service logic implies), persisting aPaywithItemScans will fail at runtime because that join table does not exist. Attach the FK explicitly.import jakarta.persistence.CascadeType import jakarta.persistence.Column import jakarta.persistence.Entity +import jakarta.persistence.JoinColumn import jakarta.persistence.OneToMany import jakarta.persistence.Table ... - @OneToMany(cascade = [CascadeType.ALL]) - private val itemScans: List<ItemScan> + @OneToMany(cascade = [CascadeType.ALL]) + @JoinColumn(name = "pay_id") + private val itemScans: List<ItemScan>
🧹 Nitpick comments (16)
prompt.md (2)
22-44: Add language identifier to fenced code block (MD040).Line 22 starts a fenced code block without a language specifier. Add
kotlinto enable syntax highlighting and satisfy linting requirements.-``` +```kotlin domain/ {domain}/
53-61: Add language identifier to fenced code block (MD040).Line 53 starts a fenced code block without a language specifier. Add
yamlortextto enable syntax highlighting and satisfy linting requirements.-``` +```yaml global/ aspect/ # AOPsrc/main/kotlin/bssm/devcoop/occount/domain/transaction/presentation/TransactionQueryController.kt (3)
17-21: Add input validation for userCode.The
userCodepath variable lacks validation, which could lead to unexpected behavior with malformed inputs (empty strings, special characters, etc.). Consider adding validation annotations such as@NotBlankor@Patternto enforce format constraints.Apply this diff to add validation:
+import jakarta.validation.constraints.NotBlank +import jakarta.validation.Valid + +@Validated @RestController @RequestMapping("/transactions") class TransactionQueryController( private val transactionQueryService: TransactionQueryService, ) { @GetMapping("/{userCode}") @ResponseStatus(HttpStatus.OK) - fun getTransactionsByUser(@PathVariable userCode: String): TransactionListResponse { + fun getTransactionsByUser(@PathVariable @NotBlank userCode: String): TransactionListResponse { return transactionQueryService.queryTransactionsByUserCode(userCode) }
23-27: Add input validation for userCode.Same validation concern as the previous endpoint. The
userCodeparameter should be validated to prevent malformed inputs.Apply this diff:
- fun getTodayChargeTotal(@PathVariable userCode: String): Int { + fun getTodayChargeTotal(@PathVariable @NotBlank userCode: String): Int { return transactionQueryService.queryTodayChargeTotal(userCode) }
17-21: Consider adding pagination support.The endpoint returns all transactions for a user without pagination, which could cause performance and memory issues for users with large transaction histories. Consider adding pagination parameters (page, size) or implementing cursor-based pagination.
src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/service/TransactionCommandService.kt (2)
15-20: Consider adding business rule validation.The method accepts
ChargeRequestwithout validating business rules such as:
- Point values are non-negative
beforePointandafterPointrelationship is correct (afterPoint = beforePoint + chargedPoint)chargeTypeis a valid enum valueuserCodeexistsWhile controller-level validation handles format, service-layer validation should enforce domain invariants.
22-27: Consider adding business rule validation.Similar to
createCharge, this method should validate:
- Point arithmetic consistency (afterPoint = beforePoint - payedPoint)
payTypeis validitemScanslist is not empty (if required by business logic)- Point values are non-negative
src/main/kotlin/bssm/devcoop/occount/domain/transaction/presentation/TransactionCommandController.kt (2)
19-23: Add validation for request body.The
ChargeRequestshould be validated at the controller level using@Validannotation. This ensures that invalid requests are rejected early with appropriate error messages.Apply this diff:
+import jakarta.validation.Valid + +@Validated @RestController @RequestMapping("/transactions") class TransactionCommandController( private val transactionCommandService: TransactionCommandService, ) { @PostMapping("/charge") @ResponseStatus(HttpStatus.CREATED) - fun createCharge(@RequestBody request: ChargeRequest): TransactionResponse { + fun createCharge(@RequestBody @Valid request: ChargeRequest): TransactionResponse { return transactionCommandService.createCharge(request) }Note: You'll also need to add validation annotations to
ChargeRequestfields.
25-29: Add validation for request body.Same concern as
createCharge. Add@Validannotation to ensurePayRequestis validated.Apply this diff:
- fun createPay(@RequestBody request: PayRequest): TransactionResponse { + fun createPay(@RequestBody @Valid request: PayRequest): TransactionResponse { return transactionCommandService.createPay(request) }src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/exception/TransactionNotFoundException.kt (1)
5-8: Consider accepting transaction identifier for better error context.The exception currently uses a generic error message. For better debugging and error reporting, consider accepting an optional transaction identifier to provide more specific error messages like "Transaction with ID X not found."
Example implementation:
class TransactionNotFoundException(transactionId: String? = null) : BusinessBaseException( TransactionApplicationErrorMessage.TRANSACTION_NOT_FOUND.httpStatus, transactionId?.let { "존재하지 않는 거래입니다: $it" } ?: TransactionApplicationErrorMessage.TRANSACTION_NOT_FOUND.message )src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/dto/request/PayRequest.kt (2)
5-14: Add validation annotations to enforce data integrity.The
PayRequestlacks validation constraints, which could allow invalid data to reach the service layer. Consider adding:
@NotBlankforuserCode,managedEmail,payType@PositiveOrZeroforbeforePoint,afterPoint@PositiveforpayedPoint@NotEmptyforitemScans@PastOrPresentfortransactionDate- Consider using an enum for
payTypeinstead of StringExample:
import jakarta.validation.constraints.* data class PayRequest( @field:NotBlank val userCode: String, @field:PastOrPresent val transactionDate: LocalDateTime, @field:PositiveOrZero val beforePoint: Int, @field:PositiveOrZero val afterPoint: Int, @field:NotBlank @field:Email val managedEmail: String, @field:NotBlank val payType: String, @field:Positive val payedPoint: Int, @field:NotEmpty @field:Valid val itemScans: List<ItemScanRequest> )
16-19: Add validation annotations to ItemScanRequest.The
ItemScanRequestshould validate:
itemIdis positivescannedAtis not in the futureExample:
data class ItemScanRequest( @field:Positive val itemId: Int, @field:PastOrPresent val scannedAt: LocalDateTime )src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/dto/request/ChargeRequest.kt (1)
5-18: Add validation annotations to enforce data integrity.The
ChargeRequestlacks validation constraints. Consider adding:
@NotBlankforuserCode,managedEmail,chargeTypemanagedEmail@PositiveOrZeroforbeforePoint,afterPoint@PositiveforchargedPoint@PastOrPresentfortransactionDateandrefundDate- Consider using an enum for
chargeTypeandrefundStateinstead of String- Add
@Sizeconstraints for String fields to prevent excessively long inputsExample:
import jakarta.validation.constraints.* data class ChargeRequest( @field:NotBlank val userCode: String, @field:PastOrPresent val transactionDate: LocalDateTime, @field:PositiveOrZero val beforePoint: Int, @field:PositiveOrZero val afterPoint: Int, @field:NotBlank @field:Email val managedEmail: String, @field:NotBlank val chargeType: String, @field:Positive val chargedPoint: Int, val reason: String?, val paymentId: String?, val refundState: String?, @field:PastOrPresent val refundDate: LocalDateTime?, val refundRequesterId: String? )src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/service/TransactionQueryService.kt (2)
16-19: Consider adding input validation for userCode.The method doesn't validate whether
userCodeis blank or empty before querying. While the repository will likely return an empty list, adding explicit validation would make the API more robust and provide clearer error messages for invalid inputs.Apply this diff to add validation:
fun queryTransactionsByUserCode(userCode: String): TransactionListResponse { + require(userCode.isNotBlank()) { "userCode must not be blank" } val transactions = transactionRepository.findByUserCode(userCode) return mapToTransactionListResponse(transactions) }
21-21: Consider potential overflow with Int return type.If charge totals can accumulate to large values (e.g., representing currency in minor units like cents, or if points can grow significantly), the
Inttype (max ~2.1 billion) could overflow. Consider whetherLongwould be more appropriate for this calculation.src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/dto/response/TransactionResponse.kt (1)
10-11: Consider Long type for point values if large accumulations are possible.All point-related fields (
beforePoint,afterPoint,chargedPoint,payedPoint) useInttype, which has a maximum value of approximately 2.1 billion. If your point system could accumulate values beyond this limit or if points represent currency in minor units (e.g., cents), consider usingLongto prevent overflow.For most point systems,
Intis sufficient, but it's worth confirming based on your business requirements.Also applies to: 14-14, 26-27, 30-30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
.gitignore(1 hunks)prompt.md(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/dto/request/ChargeRequest.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/dto/request/PayRequest.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/dto/response/TransactionResponse.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/exception/TransactionApplicationErrorMessage.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/exception/TransactionNotFoundException.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/mapper/TransactionMapper.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/service/TransactionCommandService.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/service/TransactionQueryService.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/Charge.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/ItemScan.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/Pay.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/Transaction.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/exception/InvalidTransactionTypeException.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/exception/TransactionErrorMessage.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/type/ChargeType.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/presentation/TransactionCommandController.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/presentation/TransactionQueryController.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/repository/TransactionPersistenceRepository.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/repository/TransactionRepository.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/domain/transaction/repository/TransactionRepositoryImpl.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/global/error/ErrorMessage.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/global/error/exception/BusinessBaseException.kt(1 hunks)src/main/kotlin/bssm/devcoop/occount/global/error/exception/GlobalExceptionHandler.kt(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
prompt.md
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (16)
.gitignore (1)
36-36: LGTM!Adding
.DS_Storeto exclude macOS system files is standard practice and appropriately placed.prompt.md (1)
1-297: Excellent architectural reference document aligned with PR scope.This comprehensive guide codifies the CQRS and hexagonal architecture patterns evident in the transaction domain implementation. The conventions for naming, layering, entity/VO design, repository structure (3-layer separation), and null safety are well-articulated and should serve as a strong reference for ongoing development.
The tech stack section (lines 285–297) provides clarity on the approved dependencies (Spring Boot 3.5.5, Kotlin 1.9.25, JDK 21, QueryDSL 5.1.0, etc.).
src/main/kotlin/bssm/devcoop/occount/global/error/ErrorMessage.kt (1)
16-16: Formatting improvement.The added blank line improves readability by visually separating the JWT-related errors from other error types.
src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/type/ChargeType.kt (1)
7-9: LGTM!The
fromValueimplementation usingentries.findis idiomatic Kotlin and correctly maps string values to enum constants.src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/ItemScan.kt (1)
11-23: Entity structure looks good.The JPA annotations and field mappings are correctly configured for the ItemScan entity.
src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/Transaction.kt (1)
13-43: Well-structured entity design.The abstract
Transactionentity with JOINED inheritance strategy is a solid foundation for polymorphic transaction types. The field mappings, nullable ID configuration, and accessor methods are all correctly implemented.src/main/kotlin/bssm/devcoop/occount/global/error/exception/GlobalExceptionHandler.kt (1)
12-16: LGTM!The
BusinessBaseExceptionhandler correctly extracts the HTTP status and message from the exception, providing a consistent error response structure that aligns with the existing validation handler.src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/exception/InvalidTransactionTypeException.kt (1)
5-8: LGTM!The exception class correctly delegates to
TransactionErrorMessagefor error details, following a clean and consistent error handling pattern.src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/exception/TransactionErrorMessage.kt (1)
5-10: LGTM!The error message enum is well-structured and follows the established pattern in the codebase, with appropriate HTTP status and localized message.
src/main/kotlin/bssm/devcoop/occount/domain/transaction/repository/TransactionPersistenceRepository.kt (1)
7-10: LGTM!The repository interface is correctly configured with appropriate type parameters and includes a well-named derived query method for finding transactions by user code.
src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/exception/TransactionApplicationErrorMessage.kt (1)
5-10: LGTM!The enum structure is clean and correctly integrates with the global exception handling framework. The Korean error message is appropriately domain-specific.
src/main/kotlin/bssm/devcoop/occount/global/error/exception/BusinessBaseException.kt (1)
6-11: LGTM!The refactored constructor design is clean and maintains backward compatibility. Exposing
httpStatusas a public property enables the global exception handler to access it for proper HTTP response status mapping. The secondary constructor provides a convenient way to use standardized error messages.src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/service/TransactionQueryService.kt (1)
33-36: LGTM!Clean mapping logic with proper delegation to the mapper.
src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/dto/response/TransactionResponse.kt (3)
41-43: LGTM!Clean wrapper type for the transaction list. This is a good practice for API responses as it provides flexibility to add metadata (pagination, totals, etc.) in the future without breaking the API contract.
5-20: No changes needed - nullable transactionId is correct.The
transactionIdis intentionally nullable in the domain class (line 20 of Transaction.kt) with a default value ofnull. This is standard for JPA entities with@GeneratedValue(IDENTITY)since the ID isnullbefore database persistence. The response DTO correctly reflects this by also usingLong?, and the mapper appropriately copies the value. No design changes or documentation needed.Likely an incorrect or invalid review comment.
22-32: No changes required—nullable IDs follow JPA patterns correctly.The nullable
transactionIdandscanIdin the response DTOs are intentional. Both domain entities (TransactionandItemScan) use@Id @GeneratedValuefields initialized as nullable (Long? = nullandInt? = null), which is standard JPA practice: database-generated IDs are null before persistence. The DTOs correctly reflect these types from their source domain objects. No documentation or type changes are needed.Likely an incorrect or invalid review comment.
| val todayStart = LocalDate.now().atStartOfDay() | ||
| val todayEnd = LocalDate.now().atTime(LocalTime.MAX) | ||
| val chargeTypes = listOf(ChargeType.TYPE2.value, ChargeType.TYPE3.value) |
There was a problem hiding this comment.
Inject a Clock for testability and document the ChargeType filter.
Two concerns:
-
Testability: Using
LocalDate.now()directly makes this method impossible to unit test with different dates. This prevents testing edge cases like date boundaries or different time zones. -
Magic values: The hardcoded list
listOf(ChargeType.TYPE2.value, ChargeType.TYPE3.value)lacks explanation. Why only these two types? What do TYPE2 and TYPE3 represent? This should either be documented or extracted to a well-named constant.
Apply this diff to inject a Clock for testability:
+import java.time.Clock
+
@Service
class TransactionQueryService(
private val transactionRepository: TransactionRepository,
+ private val clock: Clock = Clock.systemDefaultZone(),
) {
fun queryTransactionsByUserCode(userCode: String): TransactionListResponse {
val transactions = transactionRepository.findByUserCode(userCode)
return mapToTransactionListResponse(transactions)
}
fun queryTodayChargeTotal(userCode: String): Int {
- val todayStart = LocalDate.now().atStartOfDay()
- val todayEnd = LocalDate.now().atTime(LocalTime.MAX)
+ val todayStart = LocalDate.now(clock).atStartOfDay()
+ val todayEnd = LocalDate.now(clock).atTime(LocalTime.MAX)
val chargeTypes = listOf(ChargeType.TYPE2.value, ChargeType.TYPE3.value)For the ChargeType filter, consider extracting to a constant:
companion object {
// Document why only these types are included in today's total
private val CHARGEABLE_TYPES = listOf(ChargeType.TYPE2.value, ChargeType.TYPE3.value)
}
fun queryTodayChargeTotal(userCode: String): Int {
// ...
return transactionRepository.findTotalChargeByUserCodeAndDateBetween(
userCode,
todayStart,
todayEnd,
CHARGEABLE_TYPES
)
}🤖 Prompt for AI Agents
In
src/main/kotlin/bssm/devcoop/occount/domain/transaction/application/service/TransactionQueryService.kt
around lines 22-24, replace direct calls to LocalDate.now() and the inline list
of charge types with testable and documented alternatives: inject a
java.time.Clock into the service (constructor-injected, with a default of
Clock.systemDefaultZone() if needed) and use LocalDate.now(clock) to compute
todayStart and todayEnd so tests can control the current date/time; extract the
hardcoded listOf(ChargeType.TYPE2.value, ChargeType.TYPE3.value) into a
well-named companion object constant (with a short comment explaining why only
those types are chargeable) and pass that constant into the repository call.
| @field:Column(name = "scanned_at", nullable = false) | ||
| private val scannedAt: LocalDateTime | ||
| ) { | ||
| protected constructor() : this(null, 0, LocalDateTime.now()) |
There was a problem hiding this comment.
Problematic defaults in no-arg constructor.
The protected no-arg constructor provides defaults that could cause issues:
itemId = 0may not be a valid item IDscannedAt = LocalDateTime.now()will generate a new timestamp when JPA hydrates the entity from the database, overwriting the persisted value
For JPA entities, the no-arg constructor should typically use minimal/null defaults without business logic, as JPA will populate fields via reflection after construction.
Consider this approach:
- protected constructor() : this(null, 0, LocalDateTime.now())
+ protected constructor() : this(null, 0, LocalDateTime.MIN)Or better yet, make all fields nullable in the constructor and handle defaults at persistence time, not hydration time.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected constructor() : this(null, 0, LocalDateTime.now()) | |
| protected constructor() : this(null, 0, LocalDateTime.MIN) |
🤖 Prompt for AI Agents
In src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/ItemScan.kt
around line 25, the protected no-arg constructor currently supplies business
defaults (itemId = 0 and scannedAt = LocalDateTime.now()) which can overwrite
persisted values; change the no-arg constructor to provide minimal/null values
only (e.g. pass null for itemId and scannedAt) and, if necessary, make the
corresponding fields nullable so JPA can hydrate them without invoking business
logic; move any defaulting (like assigning a timestamp or validating an ID) into
application/persistence logic instead of the JPA constructor.
| enum class ChargeType(val value: String) { | ||
| TYPE2("2"), | ||
| TYPE3("3"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use descriptive names for enum constants.
The constants TYPE2 and TYPE3 don't convey their business meaning. Consider renaming them to reflect what they represent (e.g., CREDIT_CARD, BANK_TRANSFER, CASH, etc.) to improve code readability and maintainability.
🤖 Prompt for AI Agents
In
src/main/kotlin/bssm/devcoop/occount/domain/transaction/domain/type/ChargeType.kt
around lines 3 to 5, the enum constants TYPE2 and TYPE3 are non-descriptive;
rename them to meaningful business names (e.g., CREDIT_CARD, BANK_TRANSFER or
whatever the domain represents) and update their usages across the codebase to
the new names while preserving the underlying string values ("2" and "3") by
keeping the constructor parameter unchanged.
| override fun findTotalChargeByUserCodeAndDateBetween( | ||
| userCode: String, | ||
| startDate: LocalDateTime, | ||
| endDate: LocalDateTime, | ||
| chargeTypes: List<String> | ||
| ): Int { | ||
| return jpaQueryFactory | ||
| .select(charge.chargedPoint.sum()) | ||
| .from(charge) | ||
| .where( | ||
| charge.userCode.eq(userCode), | ||
| charge.transactionDate.between(startDate, endDate), | ||
| charge.refundState.isNull, | ||
| charge.chargeType.`in`(chargeTypes) | ||
| ) | ||
| .fetchOne() ?: 0 |
There was a problem hiding this comment.
Guard against empty chargeTypes to avoid QueryDSL failure
charge.chargeType.in(chargeTypes) throws IllegalArgumentException when the list is empty, so callers hit a 500 instead of getting “0 total.” Bail out early when no charge types are provided.
override fun findTotalChargeByUserCodeAndDateBetween(
userCode: String,
startDate: LocalDateTime,
endDate: LocalDateTime,
chargeTypes: List<String>
): Int {
+ if (chargeTypes.isEmpty()) {
+ return 0
+ }
return jpaQueryFactory
.select(charge.chargedPoint.sum())
.from(charge)🤖 Prompt for AI Agents
In
src/main/kotlin/bssm/devcoop/occount/domain/transaction/repository/TransactionRepositoryImpl.kt
around lines 22 to 37, the call charge.chargeType.`in`(chargeTypes) will throw
IllegalArgumentException for an empty list; add a guard at the start of
findTotalChargeByUserCodeAndDateBetween to return 0 if chargeTypes.isEmpty(), or
alternatively build the QueryDSL predicate only when chargeTypes is not empty
(e.g., conditionally include charge.chargeType.in(...) or use a null-safe
predicate), so the method returns 0 instead of raising an exception.
commit은 이상하게 되어있으니까 file changes로 봐야할듯
Summary by CodeRabbit
New Features
Documentation
Chores