Optimize reminder scheduling using dedicated Reminder entity #113
Conversation
📝 WalkthroughWalkthroughAdds a reminder scheduling feature: new Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as ReminderScheduler
participant Repo as ReminderRepository
participant DB as Database
participant Notifier as NotificationService
Note over Scheduler, Repo: Scheduled daily job (09:00)
Scheduler->>Repo: findByReminderDateAndSentFalse(today)
Repo->>DB: SELECT ... WHERE reminder_date = today AND sent = false
DB-->>Repo: pending reminders
Repo-->>Scheduler: list of reminders
loop for each reminder
Scheduler->>Notifier: sendReminder(reminder)
alt success
Notifier-->>Scheduler: success
Scheduler->>Scheduler: set reminder.sent = true
else failure
Notifier-->>Scheduler: error
Scheduler-->>Scheduler: log error (leave sent=false)
end
end
Scheduler->>Repo: saveAll(updatedReminders)
Repo->>DB: UPDATE reminders SET sent = true ...
DB-->>Repo: ack
Repo-->>Scheduler: persisted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @divyanshkande for your contribution! Your pull request has been submitted successfully, and a maintainer will review it soon. We’re excited to have you on board! Remember to star the repo 🌟 to help us grow, and follow @ajaynegi45 to stay in the loop and increase the visibility of your contributions!
![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 (1)
src/main/java/com/libraryman_api/entity/Reminder.java (1)
68-68: Consider using a proper JPA relationship instead of storing just the ID.The
borrowingIdfield stores only a Long reference. Using a@ManyToOnerelationship with the Borrowing entity would provide referential integrity, enable cascade operations, and allow easier navigation between entities.💡 Example with JPA relationship
+ @ManyToOne(fetch = FetchType.LAZY) + @JoinColumn(name = "borrowing_id", nullable = false) + private Borrowing borrowing; - private Long borrowingId;Then update getters/setters accordingly. This approach ensures database-level foreign key constraints and leverages JPA's relationship management.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.mdsrc/main/java/com/libraryman_api/entity/Reminder.javasrc/main/java/com/libraryman_api/entity/ReminderType.javasrc/main/java/com/libraryman_api/repository/ReminderRepository.javasrc/main/java/com/libraryman_api/scheduler/ReminderScheduler.java
🔇 Additional comments (4)
src/main/java/com/libraryman_api/entity/ReminderType.java (1)
1-11: LGTM! Clean and well-documented enum.The enum is properly structured with clear documentation describing the purpose of each reminder type.
README.md (1)
89-114: Documentation clearly explains the optimization approach.The added section provides a clear overview of the reminder scheduling optimization, covering the problem, solution, and edge case handling.
src/main/java/com/libraryman_api/repository/ReminderRepository.java (1)
16-18: LGTM! Repository follows Spring Data JPA conventions.The custom query method leverages Spring Data JPA's method name derivation to efficiently fetch pending reminders for a specific date.
src/main/java/com/libraryman_api/scheduler/ReminderScheduler.java (1)
37-37: Verify timezone consistency between cron schedule and date calculation.The cron expression runs at 09:00 (which uses server timezone), while
LocalDate.now()also uses the system default timezone. Ensure that both align with your intended timezone (e.g., UTC) to avoid processing reminders for the wrong date.Consider explicitly using a specific timezone:
LocalDate today = LocalDate.now(ZoneId.of("UTC"));Or configure the scheduler's timezone in the cron expression:
@Scheduled(cron = "0 0 9 * * ?", zone = "UTC")
|
Hi maintainer, I’ve reviewed the feedback and I’m currently working on implementing proper notification logic along with error handling and transaction management for the reminder scheduler. I’ll update this PR shortly. Thanks! |
|
I’ve addressed the review comments and pushed the updates. Please take another look. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pom.xml (1)
123-128: Consider removing the hardcoded version and fix indentation.Spring Boot's parent POM manages Lombok's version. You can omit
<version>to use the managed version, which ensures compatibility with the Spring Boot version in use. Additionally, the indentation here uses spaces while the rest of the file uses tabs.🔎 Proposed fix
- <dependency> - <groupId>org.projectlombok</groupId> - <artifactId>lombok</artifactId> - <version>1.18.30</version> - <scope>provided</scope> -</dependency> + <dependency> + <groupId>org.projectlombok</groupId> + <artifactId>lombok</artifactId> + <scope>provided</scope> + </dependency>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pom.xmlsrc/main/java/com/libraryman_api/entity/Reminder.javasrc/main/java/com/libraryman_api/scheduler/ReminderScheduler.javasrc/main/java/com/libraryman_api/service/NotificationService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/libraryman_api/entity/Reminder.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/libraryman_api/service/NotificationService.java (1)
src/main/java/com/libraryman_api/email/EmailService.java (1)
Service(20-101)
🔇 Additional comments (2)
src/main/java/com/libraryman_api/scheduler/ReminderScheduler.java (2)
42-64: Previous review concerns have been addressed.The implementation now correctly includes
@Transactional, per-reminder error handling, and proper integration withNotificationService. Reminders are only marked as sent after successful notification delivery, and failures are logged without blocking other reminders.
21-34: Good use of constructor injection and documentation.The class follows Spring best practices with constructor injection for dependencies and includes appropriate Javadoc explaining the scheduler's purpose and behavior.
| public void sendReminder(Reminder reminder) { | ||
|
|
||
| System.out.println( | ||
| "Reminder sent for borrowingId: " + reminder.getBorrowingId() + | ||
| " Type: " + reminder.getType() | ||
| ); | ||
| } |
There was a problem hiding this comment.
Replace System.out.println with proper logging and implement actual notification.
This method currently only prints to stdout, which is not suitable for production. Use SLF4J logger for consistency with the rest of the codebase (e.g., ReminderScheduler). Also, consider integrating with the existing EmailService to send actual email notifications, following the established pattern in this codebase.
🔎 Proposed logging fix and integration approach
package com.libraryman_api.service;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Service;
import com.libraryman_api.entity.Reminder;
@Service
public class NotificationService {
+ private static final Logger log = LoggerFactory.getLogger(NotificationService.class);
+
public void sendReminder(Reminder reminder) {
-
- System.out.println(
- "Reminder sent for borrowingId: " + reminder.getBorrowingId() +
- " Type: " + reminder.getType()
+ log.info(
+ "Sending reminder for borrowingId: {}, Type: {}",
+ reminder.getBorrowingId(),
+ reminder.getType()
);
+ // TODO: Integrate with EmailService to send actual email notifications
}
}🤖 Prompt for AI Agents
In src/main/java/com/libraryman_api/service/NotificationService.java around
lines 10 to 16, replace the System.out.println call with SLF4J logging and
integrate the existing EmailService to send real notifications: inject or obtain
EmailService in this class, create an email payload from Reminder
(subject/body/recipient based on borrowingId/type), call
emailService.sendEmail(...) and log at info on success and error on exceptions;
initialize a private static final Logger via
LoggerFactory.getLogger(NotificationService.class) and wrap the email send in
try/catch to log failures with the exception.
…ned Reminder entity
|
Replaced System.out.println with SLF4J logging as suggested. Logging is now production-ready and consistent with the rest of the codebase. Left a TODO for EmailService integration to keep this change focused. Also aligned Reminder entity |
| </dependency> | ||
| <dependency> | ||
| <groupId>org.projectlombok</groupId> | ||
| <artifactId>lombok</artifactId> |
| reminder.getBorrowingId(), | ||
| reminder.getType() | ||
| ); | ||
| // TODO: Integrate with EmailService to send actual email notifications |
2 participants
Summary
This PR optimizes the reminder notification workflow by introducing a
dedicated
Reminderentity that schedules reminders at the time of bookborrowing instead of scanning all borrow records daily.
Problem
The existing implementation queried all borrowings every day to determine upcoming due dates, leading to unnecessary database load and performance inefficiencies as the dataset grows.
Solution
Reminderentity to store scheduled remindersKey Improvements
Edge Case Handling
Documentation
Please let me know if any adjustments or improvements are required.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.