feature: Submit building subscription #155
Conversation
WalkthroughThis pull request introduces new DTOs, entities, enums, and service layers to support credit conversion ratios and subscription functionality. Changes include new records for data transfer, additional entity fields and relationships (including subscription and transaction enhancements), and new repository methods to support business queries. A REST controller and underlying service implementation handle subscription requests with validations, wallet operations, and error handling. Furthermore, the pull request adds SQL migration scripts for creating and updating database tables as well as front-end models and configuration enhancements for testing and logging. Changes
Sequence Diagram(s)GET /api/subscription/convert-ratio FlowsequenceDiagram
participant C as Client
participant SC as SubscriptionController
participant SS as SubscriptionService
participant M as CreditConvertRatioMapper
participant DB as Database
C->>SC: GET /api/subscription/convert-ratio
SC->>SS: getCreditConvertRatios()
SS->>DB: Query credit conversion ratios
DB-->>SS: Return entity list
SS->>M: Map entities to DTOs
M-->>SS: Return DTO list
SC-->>C: 200 OK with DTO list
POST /api/subscription FlowsequenceDiagram
participant C as Client
participant SC as SubscriptionController
participant SS as SubscriptionService
C->>SC: POST /api/subscription (SubscribeRequestDTO)
SC->>SS: subscribe(request)
SS->>SS: Validate & process subscription (check dates, devices, wallet, etc.)
SS-->>SC: Success or error outcome
SC-->>C: 201 Created (or error response)
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (16)
sep490-enterprise/src/main/java/enterprise/entities/CreditConvertRatioEntity.java (1)
20-21: Consider using BigDecimal instead of double for financial valuesUsing
doublefor financial calculations can lead to precision issues.BigDecimalis generally recommended for monetary values.@Column(name = "ratio") - private double ratio; + private BigDecimal ratio;Don't forget to add the import:
import java.math.BigDecimal;sep490-enterprise/src/main/java/enterprise/repositories/CreditConvertRatioRepository.java (1)
9-11: Consider adding a findByConvertType query methodThe repository currently only provides standard CRUD operations. Adding a specific finder method for
convertTypewould improve usability since it's a common lookup scenario.@Repository public interface CreditConvertRatioRepository extends JpaRepository<CreditConvertRatioEntity, UUID> { + Optional<CreditConvertRatioEntity> findByConvertType(CreditConvertType convertType); }Don't forget to add the import:
import enterprise.entities.CreditConvertType; import java.util.Optional;sep490-enterprise/src/main/java/enterprise/services/SubscriptionService.java (1)
9-13: Consider returning DTOs instead of entities.The
getCreditConvertRatios()method returns entity objects directly, which could leak implementation details to the presentation layer. This approach tightly couples your service layer with your data layer and might expose sensitive or unnecessary information.- List<CreditConvertRatioEntity> getCreditConvertRatios(); + List<CreditConvertRatioDTO> getCreditConvertRatios();Additionally, the
subscribe()method returns void, which doesn't provide any feedback about the result. Consider returning a response object that indicates success or provides information about the created subscription.sep490-enterprise/src/main/java/enterprise/repositories/SubscriptionRepository.java (1)
18-18: Consider pagination for potentially large result sets.Similar to the
BuildingRepository, this method returns all valid subscriptions without pagination, which could lead to performance issues if there are many subscriptions.Consider adding pagination to improve performance, especially if this method will be used in user-facing interfaces:
- List<SubscriptionEntity> findAllValidSubscriptions(@Param("now") LocalDate now); + Page<SubscriptionEntity> findAllValidSubscriptions(@Param("now") LocalDate now, Pageable pageable);sep490-enterprise/src/main/java/enterprise/dtos/SubscribeRequestDTO.java (1)
8-15: Add validation constraints for all fields.While you've added validation for
monthsandnumberOfDevices, other fields lack validation. Consider adding:public record SubscribeRequestDTO( - UUID buildingId, + @NotNull UUID buildingId, @Min(1) int months, @Min(100) int numberOfDevices, - double monthRatio, - double deviceRatio, - TransactionType type) { + @NotNull @DecimalMin("0.0") double monthRatio, + @NotNull @DecimalMin("0.0") double deviceRatio, + @NotNull TransactionType type) { }Also, consider adding validation documentation to explain the business rules behind these constraints (e.g., why 100 is the minimum number of devices).
sep490-enterprise/src/main/java/enterprise/services/impl/SubscriptionServiceImpl.java (3)
55-55: Fix typo in comment.There's a spelling error in the comment.
- // Handel wallet + // Handle wallet
62-62: Fix typo in comment.There's a spelling error in the comment.
- // Handel Subscription + // Handle Subscription
116-120: Improve variable naming for clarity.The variable name
noDevicesRatiois confusing as it's actually representing the cost per device, not the absence of devices.- CreditConvertRatioEntity noDevicesRatio = creditConvertRatios + CreditConvertRatioEntity deviceRatio = creditConvertRatios .stream() - .filter(x -> x.getConvertType().equals(CreditConvertType.MONTH)) + .filter(x -> x.getConvertType().equals(CreditConvertType.DEVICE)) .findFirst() .orElseThrow();sep490-enterprise/src/main/java/enterprise/rest/SubscriptionController.java (1)
31-35: Consider renaming method to align with endpoint path.The method name
getCreditConvertRatiodoesn't match the endpoint path/convert-ratio(singular vs. plural). For better code readability and consistency, consider renaming the method togetCreditConvertRatios()to match the plural form returned by the service.-public ResponseEntity<List<CreditConvertRatioDTO>> getCreditConvertRatio() { +public ResponseEntity<List<CreditConvertRatioDTO>> getCreditConvertRatios() {sep490-enterprise/src/main/resources/db/migration/V0.0.1.9__CreditSubscription.sql (1)
15-22: Reconsider foreign key constraint naming and column nullability.Two items to consider:
- The foreign key constraint name
pk_subscriptionsis misleading as 'pk' typically indicates a primary key, but this is a foreign key.- All new columns are marked as NOT NULL, but depending on your business logic, some fields like
amount,months, andnumber_of_devicesmight not apply to all transaction types.ALTER TABLE transactions ADD COLUMN transaction_type VARCHAR(255) NOT NULL, ADD COLUMN subscription_id UUID NOT NULL, - ADD COLUMN amount DOUBLE PRECISION, - ADD COLUMN months INTEGER, - ADD COLUMN number_of_devices INTEGER; + ADD COLUMN amount DOUBLE PRECISION NULL, + ADD COLUMN months INTEGER NULL, + ADD COLUMN number_of_devices INTEGER NULL; ALTER TABLE transactions - ADD CONSTRAINT pk_subscriptions FOREIGN KEY (subscription_id) REFERENCES subscriptions (subscription_id); + ADD CONSTRAINT fk_transaction_subscription FOREIGN KEY (subscription_id) REFERENCES subscriptions (subscription_id);sep490-enterprise/src/main/java/enterprise/entities/TransactionEntity.java (2)
42-44: Consider using BigDecimal for financial amounts.Using
doublefor financial calculations can lead to precision issues. Consider usingBigDecimalinstead for more accurate financial operations.@Column(name = "amount") -private double amount; +private BigDecimal amount;Don't forget to add the import:
import java.math.BigDecimal;
45-49: Add validation constraints for numeric fields.Consider adding validation annotations to ensure
monthsandnumberOfDevicescontain valid values (e.g., positive numbers).@Column(name = "months") +@Min(value = 1, message = "Months must be at least 1") private int months; @Column(name = "number_of_devices") +@Min(value = 1, message = "Number of devices must be at least 1") private int numberOfDevices;Don't forget to add the import:
import jakarta.validation.constraints.Min;sep490-enterprise/src/main/java/enterprise/entities/SubscriptionEntity.java (4)
33-40: Add validation for date range and device count.Consider adding validation annotations to ensure:
startDateis before or equal toendDatemaxNumberOfDeviceshas a valid positive value+@NotNull @Column(name = "start_date") private LocalDate startDate; +@NotNull @Column(name = "end_date") private LocalDate endDate; @Column(name = "max_number_of_devices") +@Min(value = 1, message = "Maximum number of devices must be at least 1") private int maxNumberOfDevices;Don't forget to add the import:
import jakarta.validation.constraints.Min;
42-45: Rename collection field to use plural form.For better code readability and to follow standard conventions, rename the collection field from
transactiontotransactions.@OneToMany(fetch = FetchType.LAZY, cascade = {CascadeType.PERSIST, CascadeType.MERGE, CascadeType.REFRESH, CascadeType.DETACH}, mappedBy = "subscription") -private List<TransactionEntity> transaction = new ArrayList<>(); +private List<TransactionEntity> transactions = new ArrayList<>();Also update the
addTransactionmethod:public void addTransaction(TransactionEntity transaction) { - this.transaction.add(transaction); + this.transactions.add(transaction); }
47-51: Make isValid() method more testable.For better testability, consider refactoring the method to accept the current date as a parameter:
-public boolean isValid() { - LocalDate now = LocalDate.now(); +public boolean isValid() { + return isValid(LocalDate.now()); +} + +public boolean isValid(LocalDate now) { return !now.isBefore(startDate) && !now.isAfter(endDate); }
53-55: Add null check in addTransaction method.Add a null check to prevent NullPointerException.
public void addTransaction(TransactionEntity transaction) { + if (transaction == null) { + return; + } this.transaction.add(transaction); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
sep490-enterprise/src/main/java/enterprise/dtos/CreditConvertRatioDTO.java(1 hunks)sep490-enterprise/src/main/java/enterprise/dtos/SubscribeRequestDTO.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/CreditConvertRatioEntity.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/CreditConvertType.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/SubscriptionEntity.java(2 hunks)sep490-enterprise/src/main/java/enterprise/entities/TransactionEntity.java(2 hunks)sep490-enterprise/src/main/java/enterprise/entities/TransactionType.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/WalletEntity.java(1 hunks)sep490-enterprise/src/main/java/enterprise/mappers/CreditConvertRatioMapper.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/BuildingRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/CreditConvertRatioRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/SubscriptionRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/rest/SubscriptionController.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/SubscriptionService.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/impl/SubscriptionServiceImpl.java(1 hunks)sep490-enterprise/src/main/resources/db/migration/V0.0.1.9__CreditSubscription.sql(1 hunks)sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts(1 hunks)sep490-idp/src/main/resources/data/temp-data.sql(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- sep490-enterprise/src/main/java/enterprise/entities/CreditConvertType.java
- sep490-enterprise/src/main/java/enterprise/entities/TransactionType.java
🔇 Additional comments (15)
sep490-enterprise/src/main/java/enterprise/dtos/CreditConvertRatioDTO.java (1)
7-8: LGTM - Well-structured DTO using Java recordsThe DTO is well-designed using Java records, which provides immutability and automatic generation of necessary methods.
sep490-enterprise/src/main/java/enterprise/entities/CreditConvertRatioEntity.java (1)
23-25: LGTM - Proper enum handling with unique constraintThe use of
EnumType.STRINGprovides better readability in the database, and the unique constraint onconvert_typeensures data integrity.sep490-enterprise/src/main/java/enterprise/repositories/BuildingRepository.java (1)
15-15:❓ Verification inconclusive
Consider pagination for potentially large result sets.
The method
findAllByEnterpriseIdreturns all buildings for an enterprise without pagination. This could lead to performance issues if an enterprise has a large number of buildings.If the number of buildings per enterprise is expected to be large, consider either:
- Using the existing paginated method
findByEnterpriseId- Adding a limit to this method
- Adding a comment explaining why a non-paginated version is needed
🏁 Script executed:
#!/bin/bash # Check if there are existing buildings and estimate potential scale echo "Checking building table structure and potential scale:" rg -A 10 "CREATE TABLE.*building" --glob="*.sql"Length of output: 4311
Action: Evaluate pagination for building retrieval
The repository method
findAllByEnterpriseId(UUID enterpriseId)currently returns all building records for an enterprise without any pagination. Given that the database migration (V0.0.1.0__EnterpriseEntityRelationships.sql) creates thebuildingstable—suggesting that enterprises might have many entries—this approach could lead to performance issues with large result sets.Please consider one of the following options:
- Use the existing paginated method (e.g.,
findByEnterpriseId) if it meets the requirements.- Add pagination (limit/offset) to this method.
- Alternatively, if returning all records is intentional due to business constraints, add a clarifying comment in the code.
Please verify that the chosen approach aligns with the expected scale for enterprise building data and addresses potential performance concerns.
sep490-enterprise/src/main/java/enterprise/repositories/SubscriptionRepository.java (1)
13-18: Well-structured JPQL query.The query is clear and follows good practices with multi-line formatting for readability.
sep490-enterprise/src/main/java/enterprise/mappers/CreditConvertRatioMapper.java (1)
1-15: Clean and well-structured mapper interface.The mapper interface is correctly defined with the
@Mapperannotation and properly configured with Spring's component model. The mapping methods are appropriately named and follow the standard pattern for bidirectional entity-DTO conversion.sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts (2)
26-34: Well-defined enum and interface for credit conversion.The
CreditConvertTypeenum andCreditConvertRatiointerface are properly defined and align well with their backend counterparts. Usingkeyof typeof CreditConvertTypefor theconvertTypeproperty ensures type safety when working with the enum values.
36-43: Comprehensive SubscribeRequest interface.The
SubscribeRequestinterface correctly extendsBaseDTOand includes all necessary properties for handling subscription requests. The typing is appropriate and consistent with the backend model.sep490-enterprise/src/main/java/enterprise/services/impl/SubscriptionServiceImpl.java (1)
28-43: Well-structured service implementation with proper annotations.The service class is properly annotated with Spring's
@Service,@Transactional,@RequiredArgsConstructor, and includes appropriate logging with@Slf4j. Dependencies are correctly injected via constructor injection.sep490-idp/src/main/resources/data/temp-data.sql (1)
23-28: Well-structured initial data for credit conversion ratios.The SQL insertion statements for the
credit_convert_ratiotable are properly formatted and include the correct data for bothMONTHandDEVICEconversion types with appropriate ratio values. This provides a good foundation for testing the subscription functionality.sep490-enterprise/src/main/java/enterprise/rest/SubscriptionController.java (2)
22-25: Good controller structure with proper annotations.The controller is well-organized with appropriate annotations for REST functionality, request mapping, and constructor dependency injection.
37-42: Appropriate security constraint and response status for subscription endpoint.Correctly restricting access to enterprise owners and returning HTTP 201 (Created) status for successful subscription.
sep490-enterprise/src/main/resources/db/migration/V0.0.1.9__CreditSubscription.sql (2)
1-13: Good table definition with unique constraint.The
credit_convert_ratiotable is properly defined with audit fields, UUID primary key, and a unique constraint onconvert_typeto prevent duplicates.
24-27: Appropriate subscription table modifications.The additions to the
subscriptionstable are properly defined with appropriate NOT NULL constraints for required fields.sep490-enterprise/src/main/java/enterprise/entities/TransactionEntity.java (2)
34-36: Proper enum mapping for transaction type.The
TransactionTypeenum is correctly mapped withEnumType.STRINGto store the enum names in the database.
38-40: Good relationship definition with subscription entity.The many-to-one relationship with
SubscriptionEntityis properly set up with appropriate fetch type and nullable constraints.
sep490-enterprise/src/main/java/enterprise/entities/WalletEntity.java
Outdated
Show resolved
Hide resolved
sep490-enterprise/src/main/java/enterprise/services/impl/SubscriptionServiceImpl.java
Outdated
Show resolved
Hide resolved
sep490-enterprise/src/main/java/enterprise/services/impl/SubscriptionServiceImpl.java
Show resolved
Hide resolved
GiaBaorr
force-pushed
the
feature/submit-subscription
branch
from
77b4fa0 to
c899364
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
sep490-enterprise/src/main/java/enterprise/entities/WalletEntity.java (2)
40-45: Consider adding validation for negative withdrawal amountsWhile the method correctly prevents withdrawing more than the available balance, it doesn't validate if the withdrawal amount is negative. A negative withdrawal would effectively become a deposit.
public void withdraw(long amount) { + if (amount < 0) { + throw new IllegalArgumentException("Withdrawal amount must be positive"); + } if (amount > balance) { throw new IllegalArgumentException("Insufficient balance"); } balance -= amount; }
40-45: Add Javadoc to document the method's behaviorConsider adding Javadoc to document the method's purpose and the exceptions it might throw, which would improve code clarity and maintainability.
+/** + * Withdraws the specified amount from the wallet. + * + * @param amount the amount to withdraw + * @throws IllegalArgumentException if the amount is greater than the current balance + */ public void withdraw(long amount) { if (amount > balance) { throw new IllegalArgumentException("Insufficient balance"); } balance -= amount; }sep490-idp/src/main/resources/data/temp-data.sql (1)
22-29: SQL Insert for Credit Conversion Ratios is Correctly StructuredThe new SQL block properly inserts seed data into the
credit_convert_ratiotable. The use ofNOW()for the timestamp fields andgen_random_uuid()for the ID is appropriate, and the two rows provide distinct values for theconvert_typefield—aligning with the unique constraint defined in the corresponding migration script.Optional Improvement: If this script may be executed multiple times (e.g., in testing or development environments), consider making the insertion idempotent by adding an
ON CONFLICT DO NOTHINGclause or similar strategy to prevent duplicate key errors.sep490-enterprise/src/main/java/enterprise/entities/SubscriptionEntity.java (2)
42-45: Consider renaming the collection field to plural form.The field name
transactionrepresents a collection of transactions, so it should follow Java naming conventions and be plural (transactions).- private List<TransactionEntity> transaction = new ArrayList<>(); + private List<TransactionEntity> transactions = new ArrayList<>();
39-40: Consider adding validation for maxNumberOfDevices.To ensure data integrity, consider adding validation to ensure maxNumberOfDevices is a positive number.
@Column(name = "max_number_of_devices") + @jakarta.validation.constraints.Positive private int maxNumberOfDevices;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
sep490-enterprise/src/main/java/enterprise/dtos/CreditConvertRatioDTO.java(1 hunks)sep490-enterprise/src/main/java/enterprise/dtos/SubscribeRequestDTO.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/CreditConvertRatioEntity.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/CreditConvertType.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/SubscriptionEntity.java(2 hunks)sep490-enterprise/src/main/java/enterprise/entities/TransactionEntity.java(2 hunks)sep490-enterprise/src/main/java/enterprise/entities/TransactionType.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/WalletEntity.java(1 hunks)sep490-enterprise/src/main/java/enterprise/mappers/CreditConvertRatioMapper.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/BuildingRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/CreditConvertRatioRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/SubscriptionRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/rest/SubscriptionController.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/SubscriptionService.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/impl/SubscriptionServiceImpl.java(1 hunks)sep490-enterprise/src/main/resources/db/migration/V0.0.1.9__CreditSubscription.sql(1 hunks)sep490-enterprise/src/test/java/enterprise/TestcontainersConfigs.java(1 hunks)sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts(1 hunks)sep490-idp/src/main/resources/data/temp-data.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- sep490-enterprise/src/main/java/enterprise/entities/CreditConvertType.java
- sep490-enterprise/src/main/java/enterprise/entities/TransactionType.java
- sep490-enterprise/src/main/java/enterprise/dtos/CreditConvertRatioDTO.java
- sep490-enterprise/src/main/java/enterprise/repositories/CreditConvertRatioRepository.java
- sep490-enterprise/src/main/java/enterprise/services/SubscriptionService.java
- sep490-enterprise/src/main/java/enterprise/entities/CreditConvertRatioEntity.java
- sep490-enterprise/src/main/java/enterprise/repositories/BuildingRepository.java
- sep490-enterprise/src/main/java/enterprise/mappers/CreditConvertRatioMapper.java
- sep490-enterprise/src/main/java/enterprise/services/impl/SubscriptionServiceImpl.java
- sep490-enterprise/src/main/java/enterprise/repositories/SubscriptionRepository.java
- sep490-enterprise/src/main/java/enterprise/rest/SubscriptionController.java
- sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts
- sep490-enterprise/src/main/java/enterprise/entities/TransactionEntity.java
- sep490-enterprise/src/main/resources/db/migration/V0.0.1.9__CreditSubscription.sql
- sep490-enterprise/src/main/java/enterprise/dtos/SubscribeRequestDTO.java
🔇 Additional comments (5)
sep490-enterprise/src/test/java/enterprise/TestcontainersConfigs.java (1)
63-63: Appropriate Flyway logging configuration added.Setting Flyway's logging level to DEBUG will provide more detailed information about database migrations during tests, which is particularly useful for troubleshooting the new credit subscription schema changes introduced in this PR.
sep490-enterprise/src/main/java/enterprise/entities/WalletEntity.java (1)
40-45: LGTM: Withdrawal method implements proper balance validationThe implementation correctly validates if there's sufficient balance before withdrawing, preventing negative balances as recommended in the previous review.
sep490-enterprise/src/main/java/enterprise/entities/SubscriptionEntity.java (3)
4-5: New imports look good.The added imports appropriately support the new fields and relationships being introduced to the subscription entity.
Also applies to: 10-10, 17-19
33-40: Date fields and device limit are well-structured.The new fields (startDate, endDate, and maxNumberOfDevices) are properly annotated with @column and mapped to appropriate database columns.
47-51: The validity check method is well-implemented.The
isValid()method effectively checks if the current date falls within the subscription period.
sep490-enterprise/src/main/java/enterprise/entities/SubscriptionEntity.java
Outdated
Show resolved
Hide resolved
GiaBaorr
force-pushed
the
feature/submit-subscription
branch
from
1b4cb5c to
5ea0875
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sep490-enterprise/src/main/java/enterprise/entities/SubscriptionEntity.java (3)
47-51: Consider renaming and improving the date validation method.The
isValid()method checks if the current date falls within the subscription period, but the name doesn't explicitly convey this temporal check. A more descriptive name likeisActive()orisCurrentlyValid()would better communicate its purpose.Additionally, the date validation logic could be simplified for better readability:
- public boolean isValid() { + public boolean isCurrentlyValid() { LocalDate now = LocalDate.now(); - return !now.isBefore(startDate) - && !now.isAfter(endDate); + return (now.isEqual(startDate) || now.isAfter(startDate)) + && (now.isEqual(endDate) || now.isBefore(endDate)); }Or even more concisely using the Java Time API:
- public boolean isValid() { + public boolean isCurrentlyValid() { LocalDate now = LocalDate.now(); - return !now.isBefore(startDate) - && !now.isAfter(endDate); + return now.compareTo(startDate) >= 0 && now.compareTo(endDate) <= 0; }
33-40: Add validation constraints to enhance data integrity.The subscription date fields and device limit would benefit from validation annotations to ensure data integrity before persistence.
@Column(name = "start_date") + @NotNull private LocalDate startDate; @Column(name = "end_date") + @NotNull private LocalDate endDate; @Column(name = "max_number_of_devices") + @Min(value = 1, message = "Number of devices must be at least 1") private int maxNumberOfDevices;This ensures that:
- Date fields cannot be null, preventing potential issues with the
isValid()method- The maximum number of devices is always a positive value
45-45: Consider renaming the field to match its plural nature.The field
transactionrepresents a collection of transactions, so renaming it totransactionswould make the code more intuitive and follow common naming conventions for collections.- private List<TransactionEntity> transaction = new ArrayList<>(); + private List<TransactionEntity> transactions = new ArrayList<>();Remember to update all references to this field throughout the class, including the
addTransactionmethod.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
sep490-enterprise/src/main/java/enterprise/dtos/CreditConvertRatioDTO.java(1 hunks)sep490-enterprise/src/main/java/enterprise/dtos/SubscribeRequestDTO.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/CreditConvertRatioEntity.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/CreditConvertType.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/SubscriptionEntity.java(2 hunks)sep490-enterprise/src/main/java/enterprise/entities/TransactionEntity.java(2 hunks)sep490-enterprise/src/main/java/enterprise/entities/TransactionType.java(1 hunks)sep490-enterprise/src/main/java/enterprise/entities/WalletEntity.java(1 hunks)sep490-enterprise/src/main/java/enterprise/mappers/CreditConvertRatioMapper.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/BuildingRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/CreditConvertRatioRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/repositories/SubscriptionRepository.java(1 hunks)sep490-enterprise/src/main/java/enterprise/rest/SubscriptionController.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/SubscriptionService.java(1 hunks)sep490-enterprise/src/main/java/enterprise/services/impl/SubscriptionServiceImpl.java(1 hunks)sep490-enterprise/src/main/resources/db/migration/V0.0.1.9__CreditSubscription.sql(1 hunks)sep490-enterprise/src/test/java/enterprise/TestcontainersConfigs.java(1 hunks)sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts(1 hunks)sep490-idp/src/main/resources/data/temp-data.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- sep490-enterprise/src/main/java/enterprise/entities/TransactionType.java
- sep490-enterprise/src/main/java/enterprise/entities/CreditConvertType.java
- sep490-enterprise/src/main/java/enterprise/entities/CreditConvertRatioEntity.java
- sep490-enterprise/src/main/java/enterprise/services/SubscriptionService.java
- sep490-enterprise/src/main/java/enterprise/dtos/CreditConvertRatioDTO.java
- sep490-enterprise/src/main/java/enterprise/entities/WalletEntity.java
- sep490-enterprise/src/main/java/enterprise/repositories/CreditConvertRatioRepository.java
- sep490-enterprise/src/main/java/enterprise/repositories/SubscriptionRepository.java
- sep490-enterprise/src/test/java/enterprise/TestcontainersConfigs.java
- sep490-enterprise/src/main/java/enterprise/repositories/BuildingRepository.java
- sep490-enterprise/src/main/java/enterprise/mappers/CreditConvertRatioMapper.java
- sep490-frontend/src/app/modules/enterprise/models/enterprise.dto.ts
- sep490-enterprise/src/main/java/enterprise/services/impl/SubscriptionServiceImpl.java
- sep490-enterprise/src/main/java/enterprise/rest/SubscriptionController.java
- sep490-enterprise/src/main/java/enterprise/dtos/SubscribeRequestDTO.java
- sep490-idp/src/main/resources/data/temp-data.sql
- sep490-enterprise/src/main/resources/db/migration/V0.0.1.9__CreditSubscription.sql
- sep490-enterprise/src/main/java/enterprise/entities/TransactionEntity.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Java Modules (sep490-enterprise)
🔇 Additional comments (1)
sep490-enterprise/src/main/java/enterprise/entities/SubscriptionEntity.java (1)
53-58: The addTransaction implementation includes necessary null check and bidirectional relationship.The method now properly checks for null and maintains the bidirectional relationship between subscription and transaction as recommended in the previous review.
sep490-enterprise/src/main/java/enterprise/entities/TransactionEntity.java
Show resolved
Hide resolved
Summary by CodeRabbit
New Features
Chores