SAK-51956 motifications Add broadcast type notification #14312
Conversation
WalkthroughAdds broadcast and TTL support to notifications, introduces UserNotificationTransferBean, changes UserNotificationData shape, schedules TTL-driven expiry via a new SchedulingService and implementation, migrates components from local ScheduledExecutorService to SchedulingService, and updates many handlers/tests to the new APIs. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.js (1)
87-90: Addcredentials: "include"to maintain consistency with other endpoint requests.The GET request to fetch initial notifications was changed to omit
credentials: "include", but other operations in this file (POST requests at lines 245 and 263 to the same/api/users/me/notifications/*endpoints) explicitly include it. For authenticated endpoints, explicitly including credentials is the standard pattern and should be consistent across all requests to the same endpoint.Code context
fetch(this.url, { cache: "no-cache", headers: { "Content-Type": "application/json" }, })Other requests in the file include credentials:
fetch(url, { method: "POST", credentials: "include" })kernel/kernel-impl/src/main/java/org/sakaiproject/content/impl/FileConversionServiceImpl.java (1)
114-206: Capture and cancel the scheduled task in the destroy() method.The scheduled task started at line 114 is never captured or cancelled. When
FileConversionServiceImplis destroyed, the task continues running on the sharedSchedulingService, causing:
- Accumulation of multiple scheduled tasks if the bean is reinitialized
- Resource and memory leaks
- Unnecessary CPU usage
Store the
ScheduledFuture<?> scheduledTaskreturned byscheduleWithFixedDelay()and cancel it in thedestroy()method.Proposed fix
Add a field to store the scheduled task:
+ private ScheduledFuture<?> scheduledTask;Capture the return value at line 114:
- schedulingService.scheduleWithFixedDelay(() -> { + scheduledTask = schedulingService.scheduleWithFixedDelay(() -> {Cancel it in destroy():
public void destroy() { + if (scheduledTask != null) { + scheduledTask.cancel(false); + } workers.shutdownNow(); }microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java (1)
946-994: Critical: Remove unused ScheduledExecutorService to prevent resource leak.Line 947 creates a local
ScheduledExecutorServicethat is never used. The scheduling calls on lines 981 and 993 correctly use the injectedschedulingServiceinstead. However, the unused executor is never shut down, creating a resource leak.🔎 Proposed fix
@Override public void createTeamFromGroupAsync(String groupId) throws MicrosoftCredentialsException { - ScheduledExecutorService executor = Executors.newScheduledThreadPool(1); AtomicInteger counter = new AtomicInteger(1); final int MAX_RETRY = 5;
🤖 Fix all issues with AI Agents
In
@announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java:
- Around line 170-177: The delete CriteriaQuery uses the wrong Root: replace
references to eventQueryTable with eventDeleteQueryTable in the predicate for
eventDeleteQuery so the DELETE filters use the delete query root; specifically
update the where(...) call used with eventDeleteQuery to use
eventDeleteQueryTable.get("event") for comparisons to
AnnouncementService.SECURE_ANNC_ADD, AnnouncementService.EVENT_AVAILABLE_ANNC
and AnnouncementService.EVENT_ANNC_UPDATE_AVAILABILITY and ensure
queryBuilder.equal(eventDeleteQueryTable.get("ref"), eventResource) is used
before executing session.createQuery(eventDeleteQuery).executeUpdate().
- Around line 203-205: The code in AnnouncementsUserNotificationHandler loads
all users via userDirectoryService.getUsers() when SiteService.ADMIN_SITE_ID and
eventResource contains "motd" (setting usersToNotify), which will OOM on large
installs; instead decide whether MOTD availability changes should use the same
broadcast path as EVENT_MOTD_NEW (avoid per-user notifications) or, if per-user
processing is required for EVENT_AVAILABLE_ANNC /
EVENT_ANNC_UPDATE_AVAILABILITY, replace the eager
userDirectoryService.getUsers() call with a paged/iterator-based retrieval that
streams user IDs (or a query for only active users) so you never materialize the
entire user list into memory; update the usersToNotify assignment and relevant
logic to use the chosen broadcast mechanism or the memory-safe iterator
approach.
In
@kernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.java:
- Line 24: Remove the unused import UserNotificationData from the top of the
UserMessagingService interface; locate the line importing
org.sakaiproject.messaging.api.UserNotificationData in the UserMessagingService
file and delete that import statement so only referenced types remain.
In
@kernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.java:
- Around line 21-24: The Javadoc in SchedulingService.java contains a typo in
the property name: remove the stray double-quote from the string
`schedulingservice.poolsize"` so the documentation reads
`schedulingservice.poolsize`; update the Javadoc comment above the
SchedulingService interface/class to correct that property name.
In
@kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java:
- Around line 579-594: pushToAllUsers currently calls
userDirectoryService.getUsers() which loads all users into memory; change it to
paginate using getUsers(int first, int last) to avoid OOMs. Modify
pushToAllUsers to loop in batches (e.g., pageSize constant), request users with
getUsers(first, first+pageSize-1), process each returned user by building the
UserNotificationTransferBean (reuse existing logic: set from, to = u.getId(),
event, ref, title), call decorateNotification and push for each, and continue
until no more users are returned; keep executor.execute(...) wrapper and reuse
decorateNotification and push methods. Ensure pageSize is reasonable and handle
the case where getUsers returns an empty list to terminate the loop.
- Around line 488-498: The getNotifications method can return duplicates when a
record has broadcast=true and toUser equals the current user; update the Stream
pipeline that builds beans (which starts with
userNotificationRepository.findByToUser(userId) and
userNotificationRepository.findByBroadcast(true), maps via
UserNotificationTransferBean::of and decorateNotification) to call .distinct()
before .collect() so duplicate UserNotificationTransferBean instances are
removed; ensure UserNotificationTransferBean equality works for distinct() to
operate correctly.
🧹 Nitpick comments (12)
webapi/src/main/java/org/sakaiproject/webapi/controllers/NotificationsController.java (1)
48-88: Consider logging full exceptions instead of juste.toString()In the three
catchblocks you calllog.error("... {}", e.toString());, which loses the stack trace. Passing the exception as the last argument would make debugging much easier:Proposed logging improvement (optional)
- } catch (Exception e) { - log.error("Failed to clear notification: {}", e.toString()); + } catch (Exception e) { + log.error("Failed to clear notification {}", id, e); } @@ - } catch (Exception e) { - log.error("Failed to clear all notifications: {}", e.toString()); + } catch (Exception e) { + log.error("Failed to clear all notifications", e); } @@ - } catch (Exception e) { - log.error("Failed to mark all notifications as viewed: {}", e.toString()); + } catch (Exception e) { + log.error("Failed to mark all notifications as viewed", e); }kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.java (1)
22-42: Consider using Lombok annotations for consistency.The class imports Lombok annotations (
@AllArgsConstructor,@NoArgsConstructor,@Data) but doesn't use them. The relatedUserNotificationDataclass uses these annotations with private fields. Using public fields here is inconsistent and may lead to maintenance issues.🔎 Proposed refactor to align with UserNotificationData pattern
package org.sakaiproject.messaging.api; import org.sakaiproject.messaging.api.model.UserNotification; import java.time.Instant; import lombok.AllArgsConstructor; import lombok.NoArgsConstructor; import lombok.Data; +@AllArgsConstructor +@NoArgsConstructor +@Data public class UserNotificationTransferBean { - public String from; - public String to; - public String event; - public String ref; - public String title; - public String siteId; - public String url; - public Instant eventDate; - public boolean viewed; - public String tool; - public boolean broadcast; - - public String fromDisplayName; - public String siteTitle; - public String formattedEventDate; + private String from; + private String to; + private String event; + private String ref; + private String title; + private String siteId; + private String url; + private Instant eventDate; + private boolean viewed; + private String tool; + private boolean broadcast; + + private String fromDisplayName; + private String siteTitle; + private String formattedEventDate; public static UserNotificationTransferBean of(UserNotification un) { UserNotificationTransferBean bean = new UserNotificationTransferBean(); bean.from = un.getFromUser(); bean.to = un.getToUser(); bean.event = un.getEvent(); bean.ref = un.getRef(); bean.title = un.getTitle(); bean.siteId = un.getSiteId(); bean.url = un.getUrl(); bean.eventDate = un.getEventDate(); bean.viewed = un.getViewed(); bean.tool = un.getTool(); bean.broadcast = un.getBroadcast(); return bean; } }Note: If you use the Lombok
@Dataannotation, you can also simplify the factory method by using setters instead of direct field access.kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.java (2)
52-63: Consider injecting a Clock for testability.The method uses
Instant.now()directly, which makes unit testing difficult since you cannot control the current time in tests.💡 Consider using an injected Clock
Inject a
Clockinstance and useInstant.now(clock):At class level:
private Clock clock = Clock.systemUTC(); public void setClock(Clock clock) { this.clock = clock; }Then in the method:
- Instant now = Instant.now(); + Instant now = Instant.now(clock);This allows tests to inject a fixed clock for deterministic testing.
65-77: Consider injecting a Clock for testability.Similar to
findByBroadcast, usingInstant.now()directly limits testability. Consider injecting aClockinstance.See the recommendation in the
findByBroadcastmethod above for implementation details.kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationData.java (1)
20-26: Consider implications of changing from@Getterto@Data.The switch from
@Getterto@Datamakes this class mutable by generating setters for all fields. IfUserNotificationDatais passed between components, unintended mutations could introduce bugs.However, if this is purely a data transfer object where mutability is acceptable, this change is fine.
If immutability is desired, consider using
@Valueinstead:-@AllArgsConstructor -@NoArgsConstructor -@Data +@Value +@Builder +@WithThis would make the class immutable while still allowing flexible construction via the builder pattern. If you need a no-args constructor for frameworks, you can keep
@NoArgsConstructor(access = AccessLevel.PRIVATE)with@AllArgsConstructor.kernel/kernel-impl/src/main/java/org/sakaiproject/scheduling/impl/SchedulingServiceImpl.java (3)
34-38: Consider configuring daemon threads for the executor.The thread pool threads are not explicitly configured as daemon threads. Non-daemon threads can prevent JVM shutdown if tasks are still running.
💡 Use a custom ThreadFactory to create daemon threads
public void init() { int poolSize = serverConfigurationService.getInt("schedulingservice.poolsize", 4); - executor = Executors.newScheduledThreadPool(poolSize); + executor = Executors.newScheduledThreadPool(poolSize, r -> { + Thread t = new Thread(r); + t.setDaemon(true); + t.setName("SchedulingService-" + t.getId()); + return t; + }); }
40-43: Consider graceful shutdown instead of abrupt termination.
shutdownNow()immediately stops all executing tasks and may leave work incomplete. Consider a graceful shutdown approach that allows tasks to complete.💡 Implement graceful shutdown
public void destroy() { - executor.shutdownNow(); + executor.shutdown(); + try { + if (!executor.awaitTermination(30, TimeUnit.SECONDS)) { + executor.shutdownNow(); + if (!executor.awaitTermination(10, TimeUnit.SECONDS)) { + log.warn("Executor did not terminate"); + } + } + } catch (InterruptedException ie) { + executor.shutdownNow(); + Thread.currentThread().interrupt(); + } }This gives tasks 30 seconds to complete normally before forcing shutdown.
45-58: Add defensive checks for executor state.The methods don't check if the executor is null or has been shut down before delegating calls, which could lead to
NullPointerExceptionorRejectedExecutionException.💡 Add executor state validation
@Override public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) { + if (executor == null || executor.isShutdown()) { + throw new IllegalStateException("SchedulingService is not initialized or has been shut down"); + } return executor.schedule(command, delay, unit); } @Override public ScheduledFuture<?> scheduleWithFixedDelay(Runnable command, long initialDelay, long delay, TimeUnit unit) { + if (executor == null || executor.isShutdown()) { + throw new IllegalStateException("SchedulingService is not initialized or has been shut down"); + } return executor.scheduleWithFixedDelay(command, initialDelay, delay, unit); }samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/TestsAndQuizzesUserNotificationHandler.java (2)
195-196: Use SLF4J parameterized logging instead of string concatenation.As per coding guidelines, log messages should use parameterized placeholders rather than string concatenation.
Proposed fix
- }catch (Exception e2){ - log.error("failed to delete TestAndQuizzes userNotification alerts: " + e2.getMessage()); + } catch (Exception e2) { + log.error("failed to delete TestAndQuizzes userNotification alerts: {}", e2.getMessage());
236-237: Use SLF4J parameterized logging instead of string concatenation.Same issue as above - use parameterized logging.
Proposed fix
- }catch (Exception e3){ - log.error("failed to delete TestAndQuizzes UserNotificationData data " + e3.getMessage()); + } catch (Exception e3) { + log.error("failed to delete TestAndQuizzes UserNotificationData data: {}", e3.getMessage());kernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.java (1)
85-88: Duplicate setters: Lombok-generated and manual setters coexist.The
@Setterannotations on lines 86-88 will generate setter methods via Lombok, but manual setter methods for the same fields still exist at lines 678-688. This creates redundant code and potential confusion.Proposed fix - remove manual setters
- public void setServerConfigurationService(ServerConfigurationService serverConfigurationService) { - this.serverConfigurationService = serverConfigurationService; - } - - public void setDao(HibernateConfigItemDao dao) { - this.dao = dao; - } - - public void setTextEncryptor(PBEStringEncryptor textEncryptor) { - this.textEncryptor = textEncryptor; - }kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java (1)
137-137: Unused field detected.The
deletingExpiredNotificationsfield is declared but never referenced anywhere in the code. This appears to be dead code.Proposed fix
- private boolean deletingExpiredNotifications = false;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javaassignment/api/src/java/org/sakaiproject/assignment/api/AddAssignmentUserNotificationHandler.javaassignment/api/src/java/org/sakaiproject/assignment/api/GradeAssignmentUserNotificationHandler.javacommons/api/src/java/org/sakaiproject/commons/api/CommonsCommentUserNotificationHandler.javaconfig/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieskernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationData.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.javakernel/api/src/main/java/org/sakaiproject/messaging/api/model/UserNotification.javakernel/api/src/main/java/org/sakaiproject/messaging/api/repository/UserNotificationRepository.javakernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.javakernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/AuthzGroupServiceTest.javakernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/DbAuthzGroupService.javakernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.javakernel/kernel-impl/src/main/java/org/sakaiproject/content/impl/FileConversionServiceImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/event/impl/ClusterEventTracking.javakernel/kernel-impl/src/main/java/org/sakaiproject/event/impl/EventTrackingTest.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/scheduling/impl/SchedulingServiceImpl.javakernel/kernel-impl/src/main/webapp/WEB-INF/authz-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/scheduling-components.xmlkernel/kernel-impl/src/test/java/org/sakaiproject/content/impl/test/FileConversionServiceTestConfiguration.javakernel/kernel-impl/src/test/java/org/sakaiproject/event/impl/test/LearningResourceTestConfiguration.javakernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTestConfiguration.javakernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTests.javalessonbuilder/components/src/java/org/sakaiproject/lessonbuildertool/impl/AddLessonsCommentUserNotificationHandlerImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamsgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/PrivateMessageUserNotificationHandler.javaplus/impl/src/main/java/org/sakaiproject/plus/impl/PlusServiceImpl.javaplus/impl/src/main/webapp/WEB-INF/components.xmlplus/impl/src/test/org/sakaiproject/plus/impl/PlusTestConfiguration.javasamigo/samigo-services/src/java/org/sakaiproject/tool/assessment/TestsAndQuizzesUserNotificationHandler.javawebapi/src/main/java/org/sakaiproject/webapi/controllers/NotificationsController.javawebapi/src/main/java/org/sakaiproject/webapi/controllers/SitesController.javawebcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.js
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{html,jsp,vm,xhtml,xml}: Use Bootstrap 5.2 as the preferred UI framework for styling in Sakai frontends
Leverage Bootstrap 5 components for consistent UI/UX
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/scheduling-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlplus/impl/src/main/webapp/WEB-INF/components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/authz-components.xml
**/*.{html,jsp,vm,xhtml,xml,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Ensure all UI components work across different screen sizes (Responsive Design)
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/scheduling-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlplus/impl/src/main/webapp/WEB-INF/components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/authz-components.xml
**/*.{js,html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Use the web components in the webcomponents/ directory when possible in Sakai frontends
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlwebcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.jskernel/kernel-impl/src/main/webapp/WEB-INF/scheduling-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlplus/impl/src/main/webapp/WEB-INF/components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/authz-components.xml
**/*.{html,jsp,jspx,xml,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer kebab-case for values of HTML class and id attributes
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlwebcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.jskernel/kernel-impl/src/main/webapp/WEB-INF/scheduling-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlplus/impl/src/main/webapp/WEB-INF/components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/authz-components.xml
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
kernel/api/src/main/java/org/sakaiproject/messaging/api/repository/UserNotificationRepository.javakernel/kernel-impl/src/main/java/org/sakaiproject/content/impl/FileConversionServiceImpl.javalessonbuilder/components/src/java/org/sakaiproject/lessonbuildertool/impl/AddLessonsCommentUserNotificationHandlerImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/AuthzGroupServiceTest.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.javakernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javawebapi/src/main/java/org/sakaiproject/webapi/controllers/SitesController.javakernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTestConfiguration.javaassignment/api/src/java/org/sakaiproject/assignment/api/AddAssignmentUserNotificationHandler.javaassignment/api/src/java/org/sakaiproject/assignment/api/GradeAssignmentUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/event/impl/EventTrackingTest.javakernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTests.javaplus/impl/src/test/org/sakaiproject/plus/impl/PlusTestConfiguration.javaplus/impl/src/main/java/org/sakaiproject/plus/impl/PlusServiceImpl.javawebapi/src/main/java/org/sakaiproject/webapi/controllers/NotificationsController.javakernel/kernel-impl/src/test/java/org/sakaiproject/content/impl/test/FileConversionServiceTestConfiguration.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javacommons/api/src/java/org/sakaiproject/commons/api/CommonsCommentUserNotificationHandler.javakernel/kernel-impl/src/test/java/org/sakaiproject/event/impl/test/LearningResourceTestConfiguration.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.javakernel/api/src/main/java/org/sakaiproject/messaging/api/model/UserNotification.javasamigo/samigo-services/src/java/org/sakaiproject/tool/assessment/TestsAndQuizzesUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/scheduling/impl/SchedulingServiceImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationData.javakernel/kernel-impl/src/main/java/org/sakaiproject/event/impl/ClusterEventTracking.javamsgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/PrivateMessageUserNotificationHandler.javaannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/DbAuthzGroupService.java
**/*.js
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.js: Use clean, standard modern JavaScript in Sakai frontends
Update jQuery code to modern JavaScript when making changes, if the changes are minimal
Prefer ES6+ features (arrow functions, template literals, destructuring, etc.) in JavaScript
Write modular, reusable JavaScript components in Sakai frontends
Minimize use of global variables and functions (Avoid Global Scope) in JavaScript
Files:
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.js
webcomponents/**/*.js
📄 CodeRabbit inference engine (.cursor/rules/webcomponents-rule.mdc)
webcomponents/**/*.js: Use double quotes instead of single quotes in Sakai Webcomponents
Do not use trailing spaces in Sakai Webcomponents code
Use the i18n property for internationalization; never use an OR switch to also provide English text
Do not attempt to manipulate dates and times based on the browser's timezone; use the Sakai portal object which includes the user's preferred timezone
Prefix internal component state properties with an underscore (e.g.,_points) and expose getters if external read access is required; use Lit{ state: true }properties and prefer reactive declarative template bindings over imperative DOM access
Files:
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.js
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Target evergreen browsers; assume ES2022+ features and browser APIs likefetchkeepalive are present; avoid legacy branches, UA sniffing, or fallbacks unless a specific evergreen gap is documented
Replace jQuery with modern DOM APIs when touching code; new work should not add jQuery dependencies
Compose Lit components, ES modules, and encapsulated helpers; keep state local and explicit with modular code
Prefer module scope or class fields; expose intentional APIs instead of incidental globals; avoid global side channels
Files:
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.js
**/*/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Internal reactive state in Lit components should stay prefixed with
_and is only surfaced through getters/setters when required
Files:
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.js
{**/UserMessagingServiceImpl.java,**/sakai-service-worker.js}
📄 CodeRabbit inference engine (AGENTS.md)
Use Web Push with VAPID via
UserMessagingServiceImpl.java; rely on the service worker insakai-service-worker.js
Files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
🧠 Learnings (15)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.jskernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.javawebapi/src/main/java/org/sakaiproject/webapi/controllers/SitesController.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaconfig/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertiesannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to webcomponents/tool/src/main/frontend/**/*.js : Define custom elements for Sakai-specific functionality when building Web Components
Applied to files:
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.js
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.jskernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.javawebapi/src/main/java/org/sakaiproject/webapi/controllers/SitesController.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaconfig/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertiesannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-09-16T04:03:17.598Z
Learnt from: kunaljaykam
Repo: sakaiproject/sakai PR: 13974
File: syllabus/syllabus-impl/src/webapp/WEB-INF/components.xml:80-90
Timestamp: 2025-09-16T04:03:17.598Z
Learning: In Sakai's search infrastructure, content producers like SyllabusContentProducer don't require explicit destroy-method configuration in Spring XML. The search framework handles cleanup automatically, and adding destroy-method when no destroy() method exists would cause configuration errors.
Applied to files:
kernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlplus/impl/src/main/webapp/WEB-INF/components.xml
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
Applied to files:
webapi/src/main/java/org/sakaiproject/webapi/controllers/SitesController.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaconfig/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to {**/UserMessagingServiceImpl.java,**/sakai-service-worker.js} : Use Web Push with VAPID via `UserMessagingServiceImpl.java`; rely on the service worker in `sakai-service-worker.js`
Applied to files:
webapi/src/main/java/org/sakaiproject/webapi/controllers/SitesController.javakernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTests.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.java
📚 Learning: 2025-09-25T13:34:06.742Z
Learnt from: susowrc
Repo: sakaiproject/sakai PR: 14105
File: assignment/tool/src/java/org/sakaiproject/assignment/entityproviders/AssignmentEntityProvider.java:1736-1745
Timestamp: 2025-09-25T13:34:06.742Z
Learning: In Sakai, each site can only have one tool placement for a given common tool ID (e.g., "sakai.assignment.grades" for the Assignment tool). The method getToolForCommonId() returns a single ToolConfiguration object, not a collection, confirming this one-to-one relationship between tools and sites.
Applied to files:
webapi/src/main/java/org/sakaiproject/webapi/controllers/SitesController.java
📚 Learning: 2025-10-10T15:13:08.361Z
Learnt from: ern
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-impl/src/test/java/org/sakaiproject/accountvalidator/test/AccountValidationTestConfiguration.java:69-72
Timestamp: 2025-10-10T15:13:08.361Z
Learning: In the Sakai project, test-specific mock configurations should remain in the test class itself as part of the test logic. Only mock configurations that are common across multiple test classes should be centralized in test configuration classes.
Applied to files:
kernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTestConfiguration.javaplus/impl/src/test/org/sakaiproject/plus/impl/PlusTestConfiguration.javakernel/kernel-impl/src/test/java/org/sakaiproject/event/impl/test/LearningResourceTestConfiguration.java
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Use the Kernel's service location mechanisms to access core services such as User Management, Email Service, Authorization, Content Hosting, and Session Management
Applied to files:
kernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/authz-components.xml
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/UserMessagingServiceImpl.java : Use Web Push with VAPID via `UserMessagingServiceImpl.java` and the service worker in `sakai-service-worker.js`
Applied to files:
kernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTests.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/UserMessagingServiceImpl.java : Implement backend Web Push (VAPID) in UserMessagingServiceImpl.java
Applied to files:
kernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTests.javawebapi/src/main/java/org/sakaiproject/webapi/controllers/NotificationsController.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-service-worker.js : Handle background push in sakai-service-worker.js
Applied to files:
kernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTests.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Handle push token subscription updates and expirations properly
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-10-07T15:11:27.298Z
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
Applied to files:
samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/TestsAndQuizzesUserNotificationHandler.java
🧬 Code graph analysis (7)
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.java (1)
reset-pass/account-validator-impl/src/main/java/org/sakaiproject/accountvalidator/impl/repository/ValidationAccountRepositoryImpl.java (1)
Transactional(38-91)
webapi/src/main/java/org/sakaiproject/webapi/controllers/SitesController.java (1)
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.java (1)
UserNotificationTransferBean(26-60)
assignment/api/src/java/org/sakaiproject/assignment/api/AddAssignmentUserNotificationHandler.java (1)
assignment/api/src/java/org/sakaiproject/assignment/api/AssignmentConstants.java (1)
AssignmentConstants(31-395)
assignment/api/src/java/org/sakaiproject/assignment/api/GradeAssignmentUserNotificationHandler.java (2)
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/utils.js (1)
url(3-3)assignment/api/src/java/org/sakaiproject/assignment/api/AssignmentConstants.java (1)
AssignmentConstants(31-395)
webapi/src/main/java/org/sakaiproject/webapi/controllers/NotificationsController.java (1)
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.java (1)
UserNotificationTransferBean(26-60)
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java (1)
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.java (1)
UserNotificationTransferBean(26-60)
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.java (1)
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationData.java (1)
AllArgsConstructor(24-37)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: sakai-deploy
...api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
Outdated
Show resolved
Hide resolved
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.java
Outdated
Show resolved
Hide resolved
kernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.java
Show resolved
Hide resolved
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
Show resolved
Hide resolved
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
Show resolved
Hide resolved
https://sakaiproject.atlassian.net/browse/SAK-51956 The idea behind this change is to replace the MOTD type notifications with a single db entry rather than one for each system user. This change also creates a SchedulingService which provides a facade onto a ScheduledExecutorService and updates all the uses of scheduled executors across Sakai to use it.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java (1)
946-994: Remove unused executor variable.Line 947 declares
ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);but this variable is never used since scheduling now goes throughschedulingService. This is dead code and should be removed.🔎 Proposed fix
public void createTeamFromGroupAsync(String groupId) throws MicrosoftCredentialsException { - ScheduledExecutorService executor = Executors.newScheduledThreadPool(1); - AtomicInteger counter = new AtomicInteger(1); final int MAX_RETRY = 5;
🤖 Fix all issues with AI Agents
In
@kernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.java:
- Around line 85-88: StoredConfigService has Lombok @Setter on fields
schedulingService, serverConfigurationService, dao, and textEncryptor which
conflicts with existing manual setter methods (the duplicated setters for these
fields); to fix, remove the manual setter methods for schedulingService,
serverConfigurationService, dao, and textEncryptor so Lombok-generated setters
are the sole definitions, or alternatively remove the @Setter annotations from
those fields—prefer removing the manual setters to keep Lombok usage consistent.
In @plus/impl/src/main/webapp/WEB-INF/components.xml:
- Around line 70-74: The PlusServiceImpl bean declaration lacks the
SchedulingService bean import; open the components.xml that declares
org.sakaiproject.plus.impl.PlusServiceImpl and add an import for the
scheduling-components.xml so Spring can wire the autowired SchedulingService in
PlusServiceImpl (i.e., include the resource classpath import for
org/sakaiproject/scheduling/impl/scheduling-components.xml near the top of the
file after namespace declarations).
🧹 Nitpick comments (7)
kernel/api/src/main/java/org/sakaiproject/messaging/api/model/UserNotification.java (1)
90-91: Explicit database migration scripts are recommended for the BROADCAST column.With Sakai's default
auto.ddl=trueconfiguration, Hibernate will automatically add theBROADCASTcolumn toUSER_NOTIFICATIONS. However, organizations that disable auto-DDL in production for stability or control may need explicit migration scripts. Consider adding optional migration scripts for HSQLDB, MySQL, and Oracle if full deployment control is desired, though the feature will work out-of-the-box with the default Sakai configuration.kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.java (1)
52-63: Consider adding@Transactional(readOnly = true)for read consistency.This query method would benefit from explicit transaction boundaries with read-only semantics for better performance and consistency, similar to other repository query methods.
🔎 Proposed enhancement
+@Transactional(readOnly = true) public List<UserNotification> findByBroadcast(boolean broadcast) {kernel/kernel-impl/src/main/java/org/sakaiproject/scheduling/impl/SchedulingServiceImpl.java (2)
34-38: Consider validating pool size configuration.The pool size is read from configuration without validation. If the configured value is non-positive,
Executors.newScheduledThreadPool()will throw anIllegalArgumentException.🔎 Proposed defensive check
public void init() { int poolSize = serverConfigurationService.getInt("schedulingservice.poolsize", 4); + if (poolSize <= 0) { + log.warn("Invalid schedulingservice.poolsize ({}), using default of 4", poolSize); + poolSize = 4; + } executor = Executors.newScheduledThreadPool(poolSize); }
40-43: Consider graceful shutdown option.
shutdownNow()immediately interrupts all running tasks. Depending on the scheduled tasks' nature, you might want to consider a graceful shutdown sequence that first callsshutdown(), awaits termination with a timeout, and only then callsshutdownNow()for remaining tasks.🔎 Proposed graceful shutdown pattern
public void destroy() { - executor.shutdownNow(); + executor.shutdown(); + try { + if (!executor.awaitTermination(5, TimeUnit.SECONDS)) { + executor.shutdownNow(); + } + } catch (InterruptedException e) { + executor.shutdownNow(); + Thread.currentThread().interrupt(); + } }samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/TestsAndQuizzesUserNotificationHandler.java (2)
196-196: Use SLF4J parameterized logging instead of string concatenation.As per coding guidelines, log messages should use parameterized logging.
Proposed fix
- log.error("failed to delete TestAndQuizzes userNotification alerts: " + e2.getMessage()); + log.error("failed to delete TestAndQuizzes userNotification alerts: {}", e2.getMessage());
237-237: Use SLF4J parameterized logging instead of string concatenation.Same issue as above - use parameterized logging.
Proposed fix
- log.error("failed to delete TestAndQuizzes UserNotificationData data " + e3.getMessage()); + log.error("failed to delete TestAndQuizzes UserNotificationData data: {}", e3.getMessage());kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java (1)
137-137: Unused field:deletingExpiredNotificationsis declared but never used.This field appears to be dead code. If it was intended to track state, the implementation is missing. Otherwise, remove it.
Proposed fix
- private boolean deletingExpiredNotifications = false;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javaassignment/api/src/java/org/sakaiproject/assignment/api/AddAssignmentUserNotificationHandler.javaassignment/api/src/java/org/sakaiproject/assignment/api/GradeAssignmentUserNotificationHandler.javacommons/api/src/java/org/sakaiproject/commons/api/CommonsCommentUserNotificationHandler.javaconfig/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieskernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationData.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.javakernel/api/src/main/java/org/sakaiproject/messaging/api/model/UserNotification.javakernel/api/src/main/java/org/sakaiproject/messaging/api/repository/UserNotificationRepository.javakernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.javakernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/AuthzGroupServiceTest.javakernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/DbAuthzGroupService.javakernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.javakernel/kernel-impl/src/main/java/org/sakaiproject/content/impl/FileConversionServiceImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/event/impl/ClusterEventTracking.javakernel/kernel-impl/src/main/java/org/sakaiproject/event/impl/EventTrackingTest.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/scheduling/impl/SchedulingServiceImpl.javakernel/kernel-impl/src/main/webapp/WEB-INF/authz-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/scheduling-components.xmlkernel/kernel-impl/src/test/java/org/sakaiproject/content/impl/test/FileConversionServiceTestConfiguration.javakernel/kernel-impl/src/test/java/org/sakaiproject/event/impl/test/LearningResourceTestConfiguration.javakernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTestConfiguration.javakernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTests.javalessonbuilder/components/src/java/org/sakaiproject/lessonbuildertool/impl/AddLessonsCommentUserNotificationHandlerImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamsgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/PrivateMessageUserNotificationHandler.javaplus/impl/src/main/java/org/sakaiproject/plus/impl/PlusServiceImpl.javaplus/impl/src/main/webapp/WEB-INF/components.xmlplus/impl/src/test/org/sakaiproject/plus/impl/PlusTestConfiguration.javasamigo/samigo-services/src/java/org/sakaiproject/tool/assessment/TestsAndQuizzesUserNotificationHandler.javawebapi/src/main/java/org/sakaiproject/webapi/controllers/NotificationsController.javawebapi/src/main/java/org/sakaiproject/webapi/controllers/SitesController.javawebcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.js
🚧 Files skipped from review as they are similar to previous changes (15)
- kernel/api/src/main/java/org/sakaiproject/messaging/api/repository/UserNotificationRepository.java
- kernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTestConfiguration.java
- config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties
- webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/SakaiNotifications.js
- msgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/PrivateMessageUserNotificationHandler.java
- assignment/api/src/java/org/sakaiproject/assignment/api/GradeAssignmentUserNotificationHandler.java
- kernel/kernel-impl/src/test/java/org/sakaiproject/content/impl/test/FileConversionServiceTestConfiguration.java
- kernel/kernel-impl/src/test/java/org/sakaiproject/messaging/impl/UserMessagingServiceTests.java
- kernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/AuthzGroupServiceTest.java
- kernel/kernel-impl/src/main/webapp/WEB-INF/components.xml
- kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.java
- lessonbuilder/components/src/java/org/sakaiproject/lessonbuildertool/impl/AddLessonsCommentUserNotificationHandlerImpl.java
- kernel/kernel-impl/src/main/webapp/WEB-INF/scheduling-components.xml
- webapi/src/main/java/org/sakaiproject/webapi/controllers/SitesController.java
- kernel/kernel-impl/src/main/webapp/WEB-INF/authz-components.xml
🧰 Additional context used
📓 Path-based instructions (6)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
webapi/src/main/java/org/sakaiproject/webapi/controllers/NotificationsController.javaplus/impl/src/test/org/sakaiproject/plus/impl/PlusTestConfiguration.javakernel/kernel-impl/src/test/java/org/sakaiproject/event/impl/test/LearningResourceTestConfiguration.javakernel/api/src/main/java/org/sakaiproject/messaging/api/model/UserNotification.javaplus/impl/src/main/java/org/sakaiproject/plus/impl/PlusServiceImpl.javakernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/event/impl/EventTrackingTest.javakernel/kernel-impl/src/main/java/org/sakaiproject/event/impl/ClusterEventTracking.javacommons/api/src/java/org/sakaiproject/commons/api/CommonsCommentUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/DbAuthzGroupService.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationData.javakernel/kernel-impl/src/main/java/org/sakaiproject/content/impl/FileConversionServiceImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javaassignment/api/src/java/org/sakaiproject/assignment/api/AddAssignmentUserNotificationHandler.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.javasamigo/samigo-services/src/java/org/sakaiproject/tool/assessment/TestsAndQuizzesUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/scheduling/impl/SchedulingServiceImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.javaannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
**/*.{html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
**/*.{html,jsp,vm,xhtml,xml}: Use Bootstrap 5.2 as the preferred UI framework for styling in Sakai frontends
Leverage Bootstrap 5 components for consistent UI/UX
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlplus/impl/src/main/webapp/WEB-INF/components.xml
**/*.{html,jsp,vm,xhtml,xml,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Ensure all UI components work across different screen sizes (Responsive Design)
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlplus/impl/src/main/webapp/WEB-INF/components.xml
**/*.{js,html,jsp,vm,xhtml,xml}
📄 CodeRabbit inference engine (.cursor/rules/frontend.mdc)
Use the web components in the webcomponents/ directory when possible in Sakai frontends
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlplus/impl/src/main/webapp/WEB-INF/components.xml
**/*.{html,jsp,jspx,xml,ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
Prefer kebab-case for values of HTML class and id attributes
Files:
kernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xmlkernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xmlplus/impl/src/main/webapp/WEB-INF/components.xml
{**/UserMessagingServiceImpl.java,**/sakai-service-worker.js}
📄 CodeRabbit inference engine (AGENTS.md)
Use Web Push with VAPID via
UserMessagingServiceImpl.java; rely on the service worker insakai-service-worker.js
Files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
🧠 Learnings (12)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/UserMessagingServiceImpl.java : Implement backend Web Push (VAPID) in UserMessagingServiceImpl.java
Applied to files:
webapi/src/main/java/org/sakaiproject/webapi/controllers/NotificationsController.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.java
📚 Learning: 2025-10-10T15:13:08.361Z
Learnt from: ern
Repo: sakaiproject/sakai PR: 14152
File: reset-pass/account-validator-impl/src/test/java/org/sakaiproject/accountvalidator/test/AccountValidationTestConfiguration.java:69-72
Timestamp: 2025-10-10T15:13:08.361Z
Learning: In the Sakai project, test-specific mock configurations should remain in the test class itself as part of the test logic. Only mock configurations that are common across multiple test classes should be centralized in test configuration classes.
Applied to files:
plus/impl/src/test/org/sakaiproject/plus/impl/PlusTestConfiguration.javakernel/kernel-impl/src/test/java/org/sakaiproject/event/impl/test/LearningResourceTestConfiguration.java
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Use the Kernel's service location mechanisms to access core services such as User Management, Email Service, Authorization, Content Hosting, and Session Management
Applied to files:
kernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xml
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to {**/UserMessagingServiceImpl.java,**/sakai-service-worker.js} : Use Web Push with VAPID via `UserMessagingServiceImpl.java`; rely on the service worker in `sakai-service-worker.js`
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.java
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/UserMessagingServiceImpl.java : Use Web Push with VAPID via `UserMessagingServiceImpl.java` and the service worker in `sakai-service-worker.js`
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Handle push token subscription updates and expirations properly
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-09-16T04:03:17.598Z
Learnt from: kunaljaykam
Repo: sakaiproject/sakai PR: 13974
File: syllabus/syllabus-impl/src/webapp/WEB-INF/components.xml:80-90
Timestamp: 2025-09-16T04:03:17.598Z
Learning: In Sakai's search infrastructure, content producers like SyllabusContentProducer don't require explicit destroy-method configuration in Spring XML. The search framework handles cleanup automatically, and adding destroy-method when no destroy() method exists would cause configuration errors.
Applied to files:
plus/impl/src/main/webapp/WEB-INF/components.xml
📚 Learning: 2025-10-07T15:11:27.298Z
Learnt from: ottenhoff
Repo: sakaiproject/sakai PR: 0
File: :0-0
Timestamp: 2025-10-07T15:11:27.298Z
Learning: In samigo’s Total Scores view (samigo/samigo-app/src/webapp/jsf/evaluation/totalScores.jsp), mailto links were hidden after commit dee05746 (PR #12312, SAK-49674) added a render check requiring email.fromEmailAddress to be non-empty; PR #14154 (SAK-52058) restores visibility by checking only description.email.
Applied to files:
samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/TestsAndQuizzesUserNotificationHandler.java
🧬 Code graph analysis (3)
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.java (1)
reset-pass/account-validator-impl/src/main/java/org/sakaiproject/accountvalidator/impl/repository/ValidationAccountRepositoryImpl.java (1)
Transactional(38-91)
commons/api/src/java/org/sakaiproject/commons/api/CommonsCommentUserNotificationHandler.java (1)
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/utils.js (1)
url(3-3)
samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/TestsAndQuizzesUserNotificationHandler.java (1)
webcomponents/tool/src/main/frontend/packages/sakai-notifications/src/utils.js (1)
url(3-3)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: sakai-deploy
🔇 Additional comments (34)
kernel/kernel-impl/src/main/java/org/sakaiproject/event/impl/ClusterEventTracking.java (2)
44-44: LGTM! Clean dependency injection pattern.The SchedulingService import and abstract method follow the established pattern for collaborator dependencies in this class.
Also applies to: 105-106
226-231: LGTM! Successful migration to SchedulingService.The replacement of the local scheduler with
schedulingService().scheduleWithFixedDelay(...)correctly maintains the original scheduling behavior with a 60-second initial delay and periodic execution everym_periodseconds.kernel/kernel-impl/src/main/java/org/sakaiproject/event/impl/EventTrackingTest.java (1)
31-31: LGTM! Proper test collaborator implementation.The SchedulingService test dependency follows the same pattern as other collaborator methods in this test class, returning null to satisfy the abstract method requirement.
Also applies to: 94-101
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java (1)
113-113: LGTM! SchedulingService dependency added.The import and setter follow the established Lombok pattern used for other dependencies in this class.
Also applies to: 194-194
plus/impl/src/test/org/sakaiproject/plus/impl/PlusTestConfiguration.java (1)
31-31: LGTM! Consistent test configuration pattern.The SchedulingService mock bean follows the same structure as other mock dependencies in this test configuration class, providing a common mock that can be used across multiple tests.
Also applies to: 100-104
kernel/kernel-impl/src/main/webapp/WEB-INF/configstore-components.xml (1)
43-46: LGTM!The SchedulingService injection is correctly configured. The removal of
destroy-methodsuggests that lifecycle management (likely scheduled cleanup tasks) is now delegated to the SchedulingService facade.kernel/kernel-impl/src/test/java/org/sakaiproject/event/impl/test/LearningResourceTestConfiguration.java (1)
45-45: LGTM!The
SchedulingServicemock bean follows the established pattern in this shared test configuration. This is appropriate for a configuration class used across multiple tests.Also applies to: 200-203
webapi/src/main/java/org/sakaiproject/webapi/controllers/NotificationsController.java (1)
27-27: LGTM!The change to
UserNotificationTransferBeanaligns with the service layer API change and follows best practices by exposing a transfer object rather than a JPA entity through the REST API.Also applies to: 41-41
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.java (1)
66-66: No action needed. The API change is already complete: the interface returnsList<UserNotificationTransferBean>, the implementation correctly converts entities to transfer beans, and all callers (SitesController, NotificationsController, and tests) already handle the new type correctly.kernel/api/src/main/java/org/sakaiproject/messaging/api/model/UserNotification.java (1)
70-71: Remove claim about nullable constraint change—cannot be verified.The review claims the URL column "previously had
nullable = falsebut now uses the defaultnullable = true." However, no evidence supports this: the current code does not specifynullable = false, no database migration exists, and no prior state is accessible. All existing code always populates the url field when creating UserNotificationData objects, indicating it's treated as required in practice despite the schema default.If this change actually occurred, a critical database migration is missing. If it did not occur, the review comment is unsubstantiated.
Likely an incorrect or invalid review comment.
kernel/kernel-impl/src/main/webapp/WEB-INF/event-components.xml (1)
37-37: LGTM! SchedulingService lookup-method added correctly.The lookup-method addition follows the established pattern for service dependencies in this bean configuration. This allows
EventTrackingServiceto obtain theSchedulingServiceon-demand through Spring's method injection.commons/api/src/java/org/sakaiproject/commons/api/CommonsCommentUserNotificationHandler.java (2)
110-110: Consistent with Line 94 – verify constructor parameters.This notification for other commenters uses the same parameter values (
false, null) as the post author notification, which is correct for consistency. Both are targeted notifications rather than broadcasts.
94-94: The constructor parameters are correctly used for comment notifications.The
UserNotificationDataclass (kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationData.java) confirms that:
- The
falseparameter correctly setsbroadcast = false, indicating these are targeted (non-broadcast) notifications appropriate for individual comment notifications- The
nullparameter correctly setsttl = null(Duration field), representing no TTL expiry for comment notificationsThe hardcoded values are appropriate.
plus/impl/src/main/java/org/sakaiproject/plus/impl/PlusServiceImpl.java (3)
105-105: LGTM! SchedulingService import added.The import statement correctly references the new
SchedulingServiceAPI interface.
149-149: LGTM! SchedulingService autowired correctly.The
SchedulingServicedependency is properly autowired following Spring conventions. Ensure that the corresponding Spring configuration (components.xml) imports the scheduling-components.xml to make this bean available.
156-172: LGTM! Migration to SchedulingService completed successfully.The migration from direct
ScheduledExecutorServiceusage to the newSchedulingServiceabstraction is correctly implemented:
- Initial delay: 120 seconds (allows Sakai to start)
- Fixed delay: 60 seconds (from
refreshTaskInterval)- Explicit
TimeUnit.SECONDSparameter addedThe scheduled task (
refreshContextMembershipsTask) remains unchanged, ensuring behavioral consistency.kernel/kernel-impl/src/main/java/org/sakaiproject/authz/impl/DbAuthzGroupService.java (3)
63-63: LGTM! SchedulingService import added.The import statement correctly references the new
SchedulingServiceAPI interface.
128-129: LGTM! Abstract accessor method for SchedulingService.The abstract method declaration follows the Spring lookup-method injection pattern. This method will be implemented by Spring at runtime based on the
<lookup-method>configuration in event-components.xml (Line 37).
256-302: LGTM! Migration to SchedulingService completed successfully.The migration from direct
ScheduledExecutorServiceusage to the newSchedulingServiceabstraction is correctly implemented:
- Uses
schedulingService()method call (Spring lookup-method pattern)- Initial delay: 120 seconds (allows Sakai to start)
- Fixed delay:
refreshTaskInterval(defaults to 60 seconds)- Explicit
TimeUnit.SECONDSparameter addedThe scheduled task (
RefreshAuthzGroupTask) remains unchanged, ensuring behavioral consistency. The configuration in event-components.xml properly supports this via the lookup-method.kernel/kernel-impl/src/main/java/org/sakaiproject/content/impl/FileConversionServiceImpl.java (1)
44-44: LGTM! Clean migration to centralized SchedulingService.The replacement of the local
masterscheduler with the centralizedSchedulingServiceis well-executed. The field injection, method call, and lifecycle management are all correct.Also applies to: 58-58, 114-114
assignment/api/src/java/org/sakaiproject/assignment/api/AddAssignmentUserNotificationHandler.java (1)
130-130: LGTM! Consistent adaptation to extended constructor.All three call sites correctly pass
falsefor the broadcast flag andnullfor the extra payload, which is appropriate for individual user notifications in the assignment context.Also applies to: 167-167, 181-181
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationData.java (2)
20-26: LGTM! Appropriate Lombok annotations for a DTO.The shift to
@Datawith both@NoArgsConstructorand@AllArgsConstructoris standard for data transfer objects, providing full accessor methods and multiple construction options.
35-36: Conversion logic fromttltoendDateis properly implemented in the persistence layer.The
ttlfield (Duration) in UserNotificationData is converted toendDate(Instant) in UserNotification during entity creation. In UserMessagingServiceImpl, the conversion occurs with null-safety:if (und.getTtl() != null) { ba.setEndDate(eventDate.toInstant().plus(und.getTtl())); }. This correctly adds the TTL duration to the event timestamp to calculate the end date for persistence.kernel/kernel-impl/src/main/java/org/sakaiproject/scheduling/impl/SchedulingServiceImpl.java (1)
45-58: LGTM! Clean delegation to underlying executor.Both scheduling methods correctly delegate to the
ScheduledExecutorServiceand return theScheduledFuturehandles. The thin-wrapper approach is appropriate for this facade pattern.kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/repository/UserNotificationRepositoryImpl.java (1)
65-77: TheUserNotificationentity includes theendDatefield (mapped toEND_DATEcolumn asInstanttype), so the CriteriaDelete query indeleteExpiredNotifications()is correct.samigo/samigo-services/src/java/org/sakaiproject/tool/assessment/TestsAndQuizzesUserNotificationHandler.java (1)
168-168: LGTM! Constructor updates align with the extended UserNotificationData signature.The new parameters (
false, null) correctly indicate these are user-specific notifications (not broadcasts) with no TTL expiration, consistent with the pattern across other handlers.Also applies to: 210-210
kernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.java (1)
129-139: Migration to SchedulingService looks correct.The scheduling logic is properly migrated from a local
ScheduledExecutorServiceto the centralizedSchedulingService. The schedule parameters (initial delay, period, time unit) are preserved.kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java (3)
162-170: Scheduled cleanup of expired notifications looks correct.The task runs every minute and deletes expired notifications within a transaction. This aligns with the TTL-based expiration model.
379-400: Notification handling updated to use bean-based workflow.The flow correctly:
- Inserts the notification via
doInsert- Converts to
UserNotificationTransferBean- Decorates and pushes (broadcast vs. single-user path)
415-448: TTL handling viaendDateis correctly implemented.The
doInsertmethod properly calculates and sets theendDatefrom the TTL duration when present.announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java (4)
88-93: MOTD TTL configuration via @PostConstruct is well implemented.The configurable TTL (
announcement.motd.notification.ttlhours, default 24 hours) provides flexibility for administrators.
113-116: Correct early exit to prevent duplicate MOTD processing.The guard ensures
SECURE_ANNC_ADDevents for MOTD are ignored since they're handled by the dedicatedEVENT_MOTD_NEWpath.
123-133: MOTD broadcast notification with TTL is correctly implemented.The broadcast notification:
- Sets
toto "BROADCAST"- Sets
broadcasttotrue- Applies configurable TTL via
Duration.ofHours(motdTTLHours)This aligns with the PR objective to replace per-user MOTD entries with a single broadcast notification.
233-237: Notification creation for site announcements is correctly implemented.The filtering logic properly:
- Excludes the event originator
- Excludes superusers
- Uses the updated UserNotificationData constructor with broadcast=false and ttl=null
kernel/api/src/main/java/org/sakaiproject/messaging/api/model/UserNotification.java
Show resolved
Hide resolved
kernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI Agents
In
@announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java:
- Around line 159-171: The Predicate eventMatcher built against eventQueryTable
(used by the SELECT CriteriaQuery) must not be reused for the DELETE; create a
new Root for the DELETE and build a new Predicate from that root (or extract
predicate creation into a helper like
createEventMatcherPredicate(CriteriaBuilder cb, Root<UserNotification> root,
String eventResource) and call it for both queries). Replace reuse of
eventMatcher in the CriteriaDelete<UserNotification> eventDeleteQuery with a
predicate created from the delete root, and ensure
eventDeleteQuery.from(UserNotification.class) is used before applying the
predicate so eventDeleteQuery.where(...) uses a predicate bound to its own root.
- Around line 198-200: The code eagerly loads all users via
userDirectoryService.getUsers() when an MOTD availability event
(EVENT_AVAILABLE_ANNC or EVENT_ANNC_UPDATE_AVAILABILITY) occurs, which will OOM
at scale; replace this per-user loading with the same broadcast mechanism used
for EVENT_MOTD_NEW (see the logic around lines 124-134) so availability changes
are sent as a broadcast instead of building usersToNotify, or if broadcast is
unsuitable, change the usersToNotify population to a streaming/paged approach
(avoid userDirectoryService.getUsers() returning a full list) so you never
materialize the entire user set; update AnnouncementsUserNotificationHandler to
detect SiteService.ADMIN_SITE_ID && eventResource.contains("motd") and either
call the existing MOTD broadcast path or iterate users via a paging iterator to
send notifications.
In
@kernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.java:
- Around line 25-49: Add a scheduleAtFixedRate method to the SchedulingService
interface to match ScheduledExecutorService semantics: declare
ScheduledFuture<?> scheduleAtFixedRate(Runnable command, long initialDelay, long
period, TimeUnit unit) with Javadoc similar to scheduleWithFixedDelay; then
update every implementation of SchedulingService to implement this new method
and adjust callers (e.g., TimedAssessmentQueue, PrecachingDbUserService) to use
the injected SchedulingService.scheduleAtFixedRate instead of calling
ScheduledExecutorService.scheduleAtFixedRate directly so the abstraction is
complete and consistent with existing schedule and scheduleWithFixedDelay
methods.
🧹 Nitpick comments (3)
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.java (1)
24-24: Remove the unused import forUserNotification.After the return type change on line 65,
UserNotificationis no longer referenced in this interface. Clean up by removing this import.🔎 Proposed fix
-import org.sakaiproject.messaging.api.model.UserNotification;kernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.java (2)
27-34: Consider adding@returndocumentation.The method documentation is clear but missing a
@returntag to describe what the returnedScheduledFuture<?>represents. Consider adding documentation like:@return a ScheduledFuture representing pending completion of the task.🔎 Proposed enhancement
/** * Schedule the Runnable to be executed after delay units, that is delay number of time units. * * @param command The command to run * @param delay The time, in units, to wait until execution of command * @param unit The type of time unit to use. + * @return a ScheduledFuture representing pending completion of the task */ ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit);
36-48: Consider adding@returndocumentation.Similar to the
schedulemethod, this method is missing a@returntag. Consider documenting the returnedScheduledFuture<?>.🔎 Proposed enhancement
/** * Schedule the Runnable to be executed after initialDelay units and then every delay units * thereafter. * * @param command The command to run * @param initialDelay The time, in units, to wait until execution of command * @param delay The time, in units, to wait between each further execution of command * @param unit The type of time unit to use. + * @return a ScheduledFuture representing pending completion of the task */ ScheduledFuture<?> scheduleWithFixedDelay(Runnable command, long initialDelay, long delay, TimeUnit unit);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.javakernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.javaannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.java
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to {**/UserMessagingServiceImpl.java,**/sakai-service-worker.js} : Use Web Push with VAPID via `UserMessagingServiceImpl.java`; rely on the service worker in `sakai-service-worker.js`
Applied to files:
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.java
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (4)
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserMessagingService.java (1)
65-65: Breaking API change is fully implemented and all implementations and consumers have been updated consistently.The return type change from
List<UserNotification>toList<UserNotificationTransferBean>has been properly applied across all layers: the interface definition, the main implementation (UserMessagingServiceImpl), controller overrides, and all call sites. Tests have been updated accordingly, and the repository layer appropriately remains isolated with its own entity types.kernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.java (1)
21-24: LGTM! Typo fixed.The Javadoc is clear, and the property name typo from the previous review has been corrected.
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java (2)
87-94: LGTM: MOTD TTL configuration.The PostConstruct initialization reads the TTL configuration with a sensible default of 24 hours. The field and method follow standard conventions.
124-134: LGTM: MOTD broadcast notification.The EVENT_MOTD_NEW handling correctly creates a single broadcast notification instead of per-user entries, which addresses the scalability goal mentioned in the PR objectives. The TTL is properly configured using the initialized motdTTLHours value.
...api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
Outdated
Show resolved
Hide resolved
...api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
Outdated
Show resolved
Hide resolved
| public interface SchedulingService { | ||
|
|
||
| /** | ||
| * Schedule the Runnable to be executed after delay units, that is delay number of time units. | ||
| * | ||
| * @param command The command to run | ||
| * @param delay The time, in units, to wait until execution of command | ||
| * @param unit The type of time unit to use. | ||
| */ | ||
| ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit); | ||
|
|
||
| /** | ||
| * Schedule the Runnable to be executed after initialDelay units and then every delay units | ||
| * thereafter. | ||
| * | ||
| * @param command The command to run | ||
| * @param initialDelay The time, in units, to wait until execution of command | ||
| * @param delay The time, in units, to wait between each further execution of command | ||
| * @param unit The type of time unit to use. | ||
| */ | ||
| ScheduledFuture<?> scheduleWithFixedDelay(Runnable command, | ||
| long initialDelay, | ||
| long delay, | ||
| TimeUnit unit); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for scheduleAtFixedRate usage to determine if it's needed
rg -n --type java 'scheduleAtFixedRate' -C 3Repository: sakaiproject/sakai
Length of output: 2701
🏁 Script executed:
#!/bin/bash
# Find and examine the SchedulingServiceImpl implementation
find . -name 'SchedulingServiceImpl.java' -type fRepository: sakaiproject/sakai
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Search for any comments or documentation about the scheduling API design
rg -n --type java 'SchedulingService' | head -20Repository: sakaiproject/sakai
Length of output: 3163
🏁 Script executed:
#!/bin/bash
# Read the SchedulingServiceImpl to understand its implementation
cat -n ./kernel/kernel-impl/src/main/java/org/sakaiproject/scheduling/impl/SchedulingServiceImpl.javaRepository: sakaiproject/sakai
Length of output: 2513
Add scheduleAtFixedRate to SchedulingService interface for consistency.
The limited API surface (only schedule and scheduleWithFixedDelay) is intentional, but the codebase contains examples where components need scheduleAtFixedRate and bypass SchedulingService entirely:
TimedAssessmentQueue.java:84callsthreadPool.scheduleAtFixedRate()PrecachingDbUserService.java:143callsdailyTimer.scheduleAtFixedRate()
This creates inconsistency where some components use the injected SchedulingService (e.g., PlusServiceImpl, MicrosoftCommonServiceImpl) while others manage their own ScheduledExecutorService instances to access missing scheduling methods. Adding scheduleAtFixedRate to the interface would provide a complete scheduling abstraction and eliminate these workarounds.
🤖 Prompt for AI Agents
In
@kernel/api/src/main/java/org/sakaiproject/scheduling/api/SchedulingService.java
around lines 25 - 49, Add a scheduleAtFixedRate method to the SchedulingService
interface to match ScheduledExecutorService semantics: declare
ScheduledFuture<?> scheduleAtFixedRate(Runnable command, long initialDelay, long
period, TimeUnit unit) with Javadoc similar to scheduleWithFixedDelay; then
update every implementation of SchedulingService to implement this new method
and adjust callers (e.g., TimedAssessmentQueue, PrecachingDbUserService) to use
the injected SchedulingService.scheduleAtFixedRate instead of calling
ScheduledExecutorService.scheduleAtFixedRate directly so the abstraction is
complete and consistent with existing schedule and scheduleWithFixedDelay
methods.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java:
- Around line 158-171: The predicate eventMatcher was built against the SELECT
root eventQueryTable and then reused for the DELETE, which is invalid; create a
new root for the delete (e.g., eventDeleteRoot =
eventDeleteQuery.from(UserNotification.class)) and rebuild the predicate against
that root instead of reusing eventMatcher—use the same attribute names (e.g.,
get("event") and get("ref")) and the same AnnouncementService constants to
compose a new predicate for eventDeleteQuery.where(...), then execute the
delete; keep the original SELECT/query detach logic unchanged.
In
@kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java:
- Around line 162-170: The scheduled cleanup task created by
schedulingService.scheduleWithFixedDelay(...) is never cancelled in destroy(),
so store the returned ScheduledFuture<?> when scheduling (e.g.,
notificationExpiryTaskFuture for the deleteExpiredNotifications block and
notificationPublishTaskFuture for the other scheduled block around lines
226-237) as fields initialized in init(), and in destroy() check each future for
non-null and not done then call cancel(false) (and set to null) to stop the
recurring task; ensure you reference the existing
schedulingService.scheduleWithFixedDelay(...) invocations and the class
destroy()/init() methods when adding these fields and cancellation logic.
- Around line 595-601: When building per-user notifications, the new
UserNotificationTransferBean is only populating from, to, event, ref and title;
update the construction in the block that creates "un" (the
UserNotificationTransferBean used with decorateNotification(...) and push(...))
to also copy bean.eventDate, bean.siteId, bean.url, bean.tool and bean.broadcast
(and any other relevant metadata present on the original bean) so the per-user
notification preserves the original notification context; locate the creation of
"un" where un.from = bean.from and un.to = u.getId() and add assignments for the
missing fields before calling decorateNotification(un) and push(...).
- Around line 583-606: In pushToAllUsers fix pagination by increasing batch size
(replace pageSize = 1 with a reasonable value like 500), correct inclusive
bounds for getUsers by setting last = first + pageSize - 1 (and update the loop
progression to first = last + 1; last = first + pageSize - 1), and simplify
iteration by replacing users.stream().forEach(...) with users.forEach(...); keep
using userDirectoryService.getUsers(...), decorateNotification(...), push(...)
and UserNotificationTransferBean as in the diff.
🧹 Nitpick comments (1)
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java (1)
137-137: Unused fielddeletingExpiredNotifications.This field is declared but never read or modified elsewhere in the class. If it was intended to guard against concurrent deletion runs, consider removing it or implementing the guard logic.
🧹 Suggested removal
- private boolean deletingExpiredNotifications = false;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
{**/UserMessagingServiceImpl.java,**/sakai-service-worker.js}
📄 CodeRabbit inference engine (AGENTS.md)
Use Web Push with VAPID via
UserMessagingServiceImpl.java; rely on the service worker insakai-service-worker.js
Files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to {**/UserMessagingServiceImpl.java,**/sakai-service-worker.js} : Use Web Push with VAPID via `UserMessagingServiceImpl.java`; rely on the service worker in `sakai-service-worker.js`
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to {**/UserMessagingServiceImpl.java,**/sakai-service-worker.js} : Use Web Push with VAPID via `UserMessagingServiceImpl.java`; rely on the service worker in `sakai-service-worker.js`
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/UserMessagingServiceImpl.java : Use Web Push with VAPID via `UserMessagingServiceImpl.java` and the service worker in `sakai-service-worker.js`
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-09-25T13:34:06.742Z
Learnt from: susowrc
Repo: sakaiproject/sakai PR: 14105
File: assignment/tool/src/java/org/sakaiproject/assignment/entityproviders/AssignmentEntityProvider.java:1736-1745
Timestamp: 2025-09-25T13:34:06.742Z
Learning: In Sakai, each site can only have one tool placement for a given common tool ID (e.g., "sakai.assignment.grades" for the Assignment tool). The method getToolForCommonId() returns a single ToolConfiguration object, not a collection, confirming this one-to-one relationship between tools and sites.
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/UserMessagingServiceImpl.java : Implement backend Web Push (VAPID) in UserMessagingServiceImpl.java
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Handle push token subscription updates and expirations properly
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
🧬 Code graph analysis (1)
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java (1)
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.java (1)
UserNotificationTransferBean(26-60)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
🔇 Additional comments (14)
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java (5)
24-24: LGTM: Imports and initialization.The new imports (Duration, PostConstruct, Predicate) and MOTD TTL configuration field with
@PostConstructinitialization are well-structured and follow Spring best practices.Also applies to: 31-31, 36-36, 86-93
114-116: LGTM: Guard prevents duplicate MOTD processing.The early return for
annc.newevents withmotdin the resource correctly delegates MOTD handling to theEVENT_MOTD_NEWpath, avoiding duplicate notifications.
123-133: LGTM: MOTD broadcast notification correctly implemented.The
EVENT_MOTD_NEWhandler creates a single broadcast notification with TTL instead of creating per-user entries, which aligns with the PR objective to replace MOTD-type notifications that create one database entry per user.
176-228: LGTM: Non-MOTD notification path correctly implemented.The recipient determination logic appropriately:
- Retrieves only site/group members with read permission (no eager loading of all system users)
- Applies role-based filtering when
SELECTED_ROLES_PROPERTYis specified- Filters out the event originator and superusers
- Creates individual (non-broadcast) notifications for regular announcements
This addresses the scalability concern from previous reviews.
232-237: LGTM: Exception handling and resource cleanup.The catch block uses SLF4J parameterized logging, and the
finallyblock ensures the security advisor is properly removed regardless of success or exception.kernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.java (3)
36-36: LGTM! Imports support the refactoring to SchedulingService.The new imports for Lombok's
@SetterandSchedulingServiceare appropriate for the dependency injection and scheduling refactoring described in the PR objectives.Also applies to: 52-52
129-141: LGTM! Scheduling refactored to use SchedulingService.The migration from a local
ScheduledExecutorServiceto the injectedSchedulingServicealigns with the PR objectives to centralize scheduling across Sakai. The method signature and parameters are correct, and the scheduling logic remains sound.
85-88: @Setter annotations correctly replace the manual setters that were previously removed.The field injections via
@Setterare properly wired inconfigstore-components.xmlwithlazy-init="false", ensuring thatSchedulingServiceand other dependencies are available beforeinit()is invoked. Spring guarantees that if any required property cannot be wired, the application fails to start—not at runtime. The code pattern is correct and requires no null-safety checks.Likely an incorrect or invalid review comment.
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java (6)
379-400: LGTM!The bean-based notification flow correctly handles both broadcast and non-broadcast cases, with proper decoration before pushing.
415-448: LGTM!The refactored
doInsertmethod correctly extracts fields fromUserNotificationDataand properly calculatesendDatefrom TTL when present.
520-540: LGTM!The decoration logic correctly populates display names and site titles with appropriate fallbacks when lookups fail.
561-574: LGTM!The test notification creation is straightforward and correctly uses the new bean-based approach.
616-669: LGTM!The push method correctly handles the
UserNotificationTransferBean, includes proper null checks, and appropriately cleans up invalid subscriptions on permanent failure status codes.
310-331: LGTM!The handler registration/unregistration logic correctly uses copy-on-write semantics with atomic reference replacement for thread safety.
Also applies to: 334-353
...api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
Outdated
Show resolved
Hide resolved
| schedulingService.scheduleWithFixedDelay(() -> { | ||
|
|
||
| transactionTemplate.execute(new TransactionCallbackWithoutResult() { | ||
|
|
||
| protected void doInTransactionWithoutResult(TransactionStatus status) { | ||
| userNotificationRepository.deleteExpiredNotifications(); | ||
| } | ||
| }); | ||
| }, 0, 1, TimeUnit.MINUTES); |
There was a problem hiding this comment.
Scheduled task is never cancelled on shutdown.
The task scheduled via schedulingService.scheduleWithFixedDelay() is not cancelled in destroy(). This could cause the task to continue running after the service is destroyed, potentially leading to errors if dependencies are no longer available.
🔧 Proposed fix
Store the returned ScheduledFuture and cancel it in destroy():
+ private java.util.concurrent.ScheduledFuture<?> expiredNotificationsTask;
private boolean deletingExpiredNotifications = false;In init():
- schedulingService.scheduleWithFixedDelay(() -> {
+ expiredNotificationsTask = schedulingService.scheduleWithFixedDelay(() -> {
transactionTemplate.execute(new TransactionCallbackWithoutResult() {In destroy():
public void destroy() {
+ if (expiredNotificationsTask != null) {
+ expiredNotificationsTask.cancel(false);
+ }
+
if (executor != null) {Also applies to: 226-237
🤖 Prompt for AI Agents
In
@kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
around lines 162 - 170, The scheduled cleanup task created by
schedulingService.scheduleWithFixedDelay(...) is never cancelled in destroy(),
so store the returned ScheduledFuture<?> when scheduling (e.g.,
notificationExpiryTaskFuture for the deleteExpiredNotifications block and
notificationPublishTaskFuture for the other scheduled block around lines
226-237) as fields initialized in init(), and in destroy() check each future for
non-null and not done then call cancel(false) (and set to null) to stop the
recurring task; ensure you reference the existing
schedulingService.scheduleWithFixedDelay(...) invocations and the class
destroy()/init() methods when adding these fields and cancellation logic.
There was a problem hiding this comment.
The schedulingservice destroy method shuts down the threads and cancels all of the queued tasks. That should be adequate I would have thought.
There was a problem hiding this comment.
@adrianfish, you're absolutely right! I verified the SchedulingServiceImpl implementation, and its destroy() method does call shutdownNow() on the internal executor, which cancels all queued and scheduled tasks. Since the SchedulingService is autowired into UserMessagingServiceImpl, Spring's dependency management ensures that UserMessagingServiceImpl.destroy() is called before SchedulingService.destroy(), so the centralized cleanup is sufficient.
I also noticed that this pattern is consistent across the codebase—other services like DbAuthzGroupService, ClusterEventTracking, FileConversionServiceImpl, and PlusServiceImpl also use SchedulingService without explicitly cancelling their scheduled tasks. The centralized shutdown approach is cleaner and avoids duplicating cancellation logic in every service.
Thank you for the clarification!
🧠 Learnings used
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to {**/UserMessagingServiceImpl.java,**/sakai-service-worker.js} : Use Web Push with VAPID via `UserMessagingServiceImpl.java`; rely on the service worker in `sakai-service-worker.js`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/UserMessagingServiceImpl.java : Implement backend Web Push (VAPID) in UserMessagingServiceImpl.java
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/UserMessagingServiceImpl.java : Use Web Push with VAPID via `UserMessagingServiceImpl.java` and the service worker in `sakai-service-worker.js`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
Show resolved
Hide resolved
| UserNotificationTransferBean un = new UserNotificationTransferBean(); | ||
| un.from = bean.from; | ||
| un.to = u.getId(); | ||
| un.event = bean.event; | ||
| un.ref = bean.ref; | ||
| un.title = bean.title; | ||
| push(decorateNotification(un)); |
There was a problem hiding this comment.
Incomplete field copying when creating per-user notification beans.
The new UserNotificationTransferBean copies only from, to, event, ref, and title from the original bean. Important fields like eventDate, siteId, url, tool, and broadcast are not copied, which may cause push notifications to display incomplete or incorrect information.
🔧 Proposed fix
users.forEach(u -> {
UserNotificationTransferBean un = new UserNotificationTransferBean();
un.from = bean.from;
un.to = u.getId();
un.event = bean.event;
un.ref = bean.ref;
un.title = bean.title;
+ un.siteId = bean.siteId;
+ un.url = bean.url;
+ un.eventDate = bean.eventDate;
+ un.tool = bean.tool;
+ un.broadcast = bean.broadcast;
push(decorateNotification(un));
});🤖 Prompt for AI Agents
In
@kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
around lines 595 - 601, When building per-user notifications, the new
UserNotificationTransferBean is only populating from, to, event, ref and title;
update the construction in the block that creates "un" (the
UserNotificationTransferBean used with decorateNotification(...) and push(...))
to also copy bean.eventDate, bean.siteId, bean.url, bean.tool and bean.broadcast
(and any other relevant metadata present on the original bean) so the per-user
notification preserves the original notification context; locate the creation of
"un" where un.from = bean.from and un.to = u.getId() and add assignments for the
missing fields before calling decorateNotification(un) and push(...).
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java:
- Line 92: There's a typo in the configuration key used when reading the MOTD
TTL: update the string passed to serverConfigurationService.getInt in
AnnouncementsUserNotificationHandler (where motdTTLHours is set) from
"announcement.motod.notification.ttlhours" to the correct
"announcement.motd.notification.ttlhours" so the intended configuration value is
read; leave the motdTTLHours variable and getInt call otherwise unchanged.
- Around line 207-213: The stream filter in AnnouncementsUserNotificationHandler
that sets filteredUsersToNotify calls site.getMember(u).getRole() without
checking for null; change the filter on usersToNotify to first retrieve
SiteMember member = site.getMember(u) and guard that member != null and
member.getRole() != null before calling getId(), e.g. replace the lambda u ->
selectedRoles.contains(site.getMember(u).getRole().getId()) with a safe check
that skips users with null member/role so no NPE occurs when computing
filteredUsersToNotify.
- Around line 123-133: The MOTD branch in AnnouncementsUserNotificationHandler
is missing siteId, url and toolId on the UserNotificationData; set
userNotificationData.setSiteId(event.getContext()), set
userNotificationData.setToolId(AnnouncementService.SAKAI_ANNOUNCEMENT_TOOL_ID),
and populate userNotificationData.setUrl(...) with the same announcement
navigation URL you build for regular announcements (using the message/header
identifier from message.getHeader() or message.getId() and
event.getContext()/tool id) so broadcast notifications include site, tool and a
clickable link.
In
@kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java:
- Around line 583-606: The current loop uses a pageSize set to 1 and calls
users.stream().forEach(...), which is inefficient; change the pageSize
initialization to a larger value (e.g., 100) to reduce DB round-trips and
replace users.stream().forEach(...) with users.forEach(...) for direct
iteration; keep the logic that constructs UserNotificationTransferBean (un.from,
un.to, un.event, un.ref, un.title), calls decorateNotification(un) and then
push(...), and update the pagination math using the same first/last/done
variables around the userDirectoryService.getUsers(...) call.
🧹 Nitpick comments (2)
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java (1)
219-219: Consider consistent UserNotificationData creation pattern.Line 219 uses the
UserNotificationDataconstructor, while the MOTD path (lines 126-132) uses setters. For consistency, consider using the same pattern throughout, though the current approach works correctly.kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java (1)
137-137: Unused fielddeletingExpiredNotifications.This field is declared but never read or written anywhere in the class. Remove it or implement its intended usage.
🧹 Suggested removal
- private boolean deletingExpiredNotifications = false;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.javakernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
🧰 Additional context used
📓 Path-based instructions (2)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.javakernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.java
{**/UserMessagingServiceImpl.java,**/sakai-service-worker.js}
📄 CodeRabbit inference engine (AGENTS.md)
Use Web Push with VAPID via
UserMessagingServiceImpl.java; rely on the service worker insakai-service-worker.js
Files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
🧠 Learnings (9)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to {**/UserMessagingServiceImpl.java,**/sakai-service-worker.js} : Use Web Push with VAPID via `UserMessagingServiceImpl.java`; rely on the service worker in `sakai-service-worker.js`
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/UserMessagingServiceImpl.java : Implement backend Web Push (VAPID) in UserMessagingServiceImpl.java
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/UserMessagingServiceImpl.java : Use Web Push with VAPID via `UserMessagingServiceImpl.java` and the service worker in `sakai-service-worker.js`
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaannouncement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-10-07T16:11:33.008Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Handle push token subscription updates and expirations properly
Applied to files:
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
📚 Learning: 2025-09-25T13:34:06.742Z
Learnt from: susowrc
Repo: sakaiproject/sakai PR: 14105
File: assignment/tool/src/java/org/sakaiproject/assignment/entityproviders/AssignmentEntityProvider.java:1736-1745
Timestamp: 2025-09-25T13:34:06.742Z
Learning: In Sakai, each site can only have one tool placement for a given common tool ID (e.g., "sakai.assignment.grades" for the Assignment tool). The method getToolForCommonId() returns a single ToolConfiguration object, not a collection, confirming this one-to-one relationship between tools and sites.
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
🧬 Code graph analysis (1)
kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java (2)
kernel/api/src/main/java/org/sakaiproject/messaging/api/UserNotificationTransferBean.java (1)
UserNotificationTransferBean(26-60)kernel/api/src/main/java/org/sakaiproject/serialization/MapperFactory.java (1)
MapperFactory(36-427)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (13)
kernel/kernel-impl/src/main/java/org/sakaiproject/config/impl/StoredConfigService.java (2)
85-88: LGTM - Lombok @Setter annotations are correctly applied.The @Setter annotations properly generate setters for Spring dependency injection. The previous concern about duplicate manual setters has been resolved.
129-139: LGTM - Migration to SchedulingService is correct.The replacement of the local
ScheduledExecutorServicewith the centralizedSchedulingServiceis implemented correctly:
- The
scheduleWithFixedDelaycall signature and parameters are correct- The anonymous
Runnablewith an instance fieldpollDateappropriately maintains state between polling iterations- The minimum 120-second initial delay is preserved for Sakai startup
- Lifecycle management is now centralized in
SchedulingService(the removeddestroy()method is no longer needed)announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java (4)
113-116: LGTM: Early exit prevents duplicate MOTD processing.The guard correctly prevents "annc.new" events with MOTD resources from being processed twice, since they're handled by the dedicated "motd.new" event path.
135-141: LGTM: Early initialization enables draft/future visibility logic.Moving the draft and future message flag initialization before the removal logic allows them to influence the removal conditions at line 146.
143-169: LGTM: Removal logic correctly fixed.The refactored removal logic addresses the previously flagged issues:
- Uses
buildWherehelper with appropriate table roots for both SELECT and DELETE queries- Properly detaches notifications before bulk delete to maintain persistence context consistency
- Includes transaction handling with error logging
236-245: LGTM: Helper method correctly encapsulates predicate logic.The
buildWherehelper properly accepts the query-specificRootparameter, allowing safe reuse across SELECT and DELETE queries without the previous predicate-sharing issues.kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java (7)
162-170: Scheduled cleanup task looks good.The expired notification cleanup is properly scheduled with transaction handling. The 1-minute interval is reasonable for this type of housekeeping task.
378-400: Bean-based notification flow is well-structured.The refactoring to use
UserNotificationDataandUserNotificationTransferBeanimproves clarity. The conditional branching for broadcast vs. per-user push is correct.
415-448: Clean refactoring ofdoInsert()to useUserNotificationData.The method now properly extracts all fields from the data bean, including TTL-based end date calculation. The null check on
getTtl()before settingendDateis correct.
520-540: Decoration logic is correctly adapted for the bean-based model.Exception handling provides sensible fallbacks when user or site lookups fail.
616-669: Push notification handling is well-implemented.The method correctly validates inputs, handles subscription errors with appropriate cleanup for permanent failures (410, 404, 400), and logs using parameterized format as per coding guidelines.
561-574: Test notification implementation is clean.Correctly creates and decorates a test notification using the new bean-based model.
493-495: Add.distinct()to prevent duplicate notifications.The stream concatenates user-specific and broadcast notifications without deduplication. If a record has both
broadcast=trueandtoUsermatching the current user, it would appear twice.🔧 Suggested fix
List<UserNotificationTransferBean> beans = Stream.concat(userNotificationRepository.findByToUser(userId).stream(), userNotificationRepository.findByBroadcast(true).stream()) - .map(UserNotificationTransferBean::of).map(this::decorateNotification).collect(Collectors.toList()); + .distinct() + .map(UserNotificationTransferBean::of).map(this::decorateNotification).collect(Collectors.toList());Note: Apply
.distinct()onUserNotificationbefore mapping, and ensureUserNotificationhas properequals/hashCodeimplementation based on its ID.Likely an incorrect or invalid review comment.
|
|
||
| @PostConstruct | ||
| private void init() { | ||
| motdTTLHours = serverConfigurationService.getInt("announcement.motd.notification.ttlhours", 24); |
There was a problem hiding this comment.
Fix typo in configuration property name.
The configuration property name has a typo: "announcement.motod.notification.ttlhours" should be "announcement.motd.notification.ttlhours" (motod → motd).
🔧 Proposed fix
- motdTTLHours = serverConfigurationService.getInt("announcement.motod.notification.ttlhours", 24);
+ motdTTLHours = serverConfigurationService.getInt("announcement.motd.notification.ttlhours", 24);🤖 Prompt for AI Agents
In
@announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
at line 92, There's a typo in the configuration key used when reading the MOTD
TTL: update the string passed to serverConfigurationService.getInt in
AnnouncementsUserNotificationHandler (where motdTTLHours is set) from
"announcement.motod.notification.ttlhours" to the correct
"announcement.motd.notification.ttlhours" so the intended configuration value is
read; leave the motdTTLHours variable and getInt call otherwise unchanged.
...api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
Show resolved
Hide resolved
| if (selectedRoles != null && !selectedRoles.isEmpty()) { | ||
| filteredUsersToNotify = usersToNotify.stream() | ||
| .filter(u -> selectedRoles.contains(site.getMember(u).getRole().getId())) | ||
| .collect(Collectors.toSet()); | ||
| } else { | ||
| filteredUsersToNotify = usersToNotify; | ||
| } |
There was a problem hiding this comment.
Guard against NPE when filtering by role.
Line 209 calls site.getMember(u).getRole() without null-checking the member. If a user is no longer a site member (race condition or async site update), getMember returns null, causing a NullPointerException.
🛡️ Proposed fix
Set<String> filteredUsersToNotify;
// filter users to notify based on their role
List<String> selectedRoles = message.getProperties().getPropertyList(SELECTED_ROLES_PROPERTY);
if (selectedRoles != null && !selectedRoles.isEmpty()) {
filteredUsersToNotify = usersToNotify.stream()
- .filter(u -> selectedRoles.contains(site.getMember(u).getRole().getId()))
+ .filter(u -> {
+ var member = site.getMember(u);
+ return member != null && selectedRoles.contains(member.getRole().getId());
+ })
.collect(Collectors.toSet());
} else {
filteredUsersToNotify = usersToNotify;
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
around lines 207 - 213, The stream filter in
AnnouncementsUserNotificationHandler that sets filteredUsersToNotify calls
site.getMember(u).getRole() without checking for null; change the filter on
usersToNotify to first retrieve SiteMember member = site.getMember(u) and guard
that member != null and member.getRole() != null before calling getId(), e.g.
replace the lambda u ->
selectedRoles.contains(site.getMember(u).getRole().getId()) with a safe check
that skips users with null member/role so no NPE occurs when computing
filteredUsersToNotify.
| int pageSize = 1; | ||
| int first = 1; | ||
| int last = first + pageSize; | ||
| int total = userDirectoryService.countUsers(); | ||
| int done = 0; | ||
|
|
||
| while (done < total) { | ||
|
|
||
| List<User> users = userDirectoryService.getUsers(first, last); | ||
|
|
||
| users.stream().forEach(u -> { | ||
|
|
||
| UserNotificationTransferBean un = new UserNotificationTransferBean(); | ||
| un.from = bean.from; | ||
| un.to = u.getId(); | ||
| un.event = bean.event; | ||
| un.ref = bean.ref; | ||
| un.title = bean.title; | ||
| push(decorateNotification(un)); | ||
| }); | ||
|
|
||
| first = last + pageSize; | ||
| last = first + pageSize; | ||
| done += users.size(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find UserDirectoryService interface/class
fd -t f -name "*.java" | xargs rg "interface UserDirectoryService|class UserDirectoryService" -lRepository: sakaiproject/sakai
Length of output: 563
🏁 Script executed:
# Search for getUsers method definition with documentation
rg -n "getUsers\s*\(\s*int\s*,\s*int\s*\)" --type java -B 5 -A 15 | head -150Repository: sakaiproject/sakai
Length of output: 44
🏁 Script executed:
# Look for any javadoc or comments around getUsers in UserDirectoryService
rg -n "getUsers" --type java -B 3 -A 10 | grep -A 10 "int.*int" | head -100Repository: sakaiproject/sakai
Length of output: 11582
🏁 Script executed:
# Get the UserDirectoryService interface with JavaDoc
cat -n kernel/api/src/main/java/org/sakaiproject/user/api/UserDirectoryService.java | sed -n '415,435p'Repository: sakaiproject/sakai
Length of output: 856
🏁 Script executed:
# Also check the test file to see how getUsers is actually used
cat -n kernel/kernel-impl/src/test/java/org/sakaiproject/user/impl/test/UserDirectoryServiceGetTest.java | head -150Repository: sakaiproject/sakai
Length of output: 6959
🏁 Script executed:
# Find actual usages of getUsers with int parameters
rg "getUsers\s*\(\s*\d+\s*,\s*\d+" --type java -B 2 -A 2Repository: sakaiproject/sakai
Length of output: 44
🏁 Script executed:
# Also search for pagination patterns similar to the reviewed code
rg -A 5 "getUsers\s*\(\s*first\s*,\s*last\s*\)" --type javaRepository: sakaiproject/sakai
Length of output: 2177
🏁 Script executed:
# Check the implementation in BaseUserDirectoryService to see what m_storage.getAll does
cat -n kernel/kernel-impl/src/main/java/org/sakaiproject/user/impl/BaseUserDirectoryService.java | sed -n '1375,1382p'Repository: sakaiproject/sakai
Length of output: 258
🏁 Script executed:
# Find the storage interface and implementation for getAll
rg "interface.*Storage|class.*Storage" --type java kernel/kernel-impl/src/main/java/org/sakaiproject/user/impl/ | grep -i userRepository: sakaiproject/sakai
Length of output: 801
🏁 Script executed:
# Search for getAll method implementation
rg "getAll\s*\(\s*int\s+\w+\s*,\s*int" --type java -A 15 kernel/kernel-impl/src/main/java/org/sakaiproject/user/impl/Repository: sakaiproject/sakai
Length of output: 3465
🏁 Script executed:
# Find getAllResources implementation in BaseDbFlatStorage
rg "getAllResources\s*\(\s*int\s+\w+\s*,\s*int" --type java -A 20Repository: sakaiproject/sakai
Length of output: 9844
🏁 Script executed:
# Check how the indices are used in the SQL or pagination logic
rg "LIMIT|OFFSET|first.*last" --type java -B 3 -A 3 kernel/kernel-impl/src/main/java/org/sakaiproject/user/impl/DbUserService.java | head -80Repository: sakaiproject/sakai
Length of output: 1192
🏁 Script executed:
# Check the BaseDbFlatStorage to confirm the offset/limit calculation
cat -n kernel/kernel-storage-util/src/main/java/org/sakaiproject/util/BaseDbFlatStorage.java | sed -n '260,285p'Repository: sakaiproject/sakai
Length of output: 606
🏁 Script executed:
# Also look at how other code uses pagination with getUsers
cat -n entitybroker/core-providers/src/java/org/sakaiproject/entitybroker/providers/UserEntityProvider.java | sed -n '100,150p'Repository: sakaiproject/sakai
Length of output: 2701
🏁 Script executed:
# Find the exact line with the offset/limit calculation
cat -n kernel/kernel-storage-util/src/main/java/org/sakaiproject/util/BaseDbFlatStorage.java | sed -n '175,220p'Repository: sakaiproject/sakai
Length of output: 1951
🏁 Script executed:
# Let me also check the actual getUsers pagination in UserMessagingServiceImpl more carefully
cat -n kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java | sed -n '575,610p'Repository: sakaiproject/sakai
Length of output: 1387
🏁 Script executed:
# Let me verify by manually computing what happens with the current code
cat > /tmp/test_pagination.py << 'EOF'
# Current code with pageSize = 1
pageSize = 1
first = 1
last = first + pageSize # = 2
total = 10
done = 0
print("Current code with pageSize=1:")
iteration = 1
while done < total:
# Calculate OFFSET and LIMIT as per BaseDbFlatStorage
offset = first - 1
limit = last - first + 1
print(f" Iteration {iteration}: first={first}, last={last}, OFFSET={offset}, LIMIT={limit}, fetches positions {first} to {last}")
# Simulate fetching users
users_fetched = min(limit, total - done)
done += users_fetched
# Next iteration
first = last + pageSize # = last + 1
last = first + pageSize # = first + 1
iteration += 1
if iteration > 15:
break
print("\n" + "="*60 + "\n")
# With suggested fix (pageSize=100)
pageSize = 100
first = 1
last = first + pageSize - 1 # = 100
total = 10
done = 0
print("Suggested fix with pageSize=100:")
iteration = 1
while done < total:
offset = first - 1
limit = last - first + 1
print(f" Iteration {iteration}: first={first}, last={last}, OFFSET={offset}, LIMIT={limit}, fetches positions {first} to {last}")
users_fetched = min(limit, total - done)
done += users_fetched
first = last + 1
last = first + pageSize - 1
iteration += 1
if iteration > 15:
break
print("\n" + "="*60 + "\n")
# Original code with pageSize=100 (unfixed)
pageSize = 100
first = 1
last = first + pageSize # = 101
total = 1000
done = 0
print("Original code with pageSize=100 (buggy):")
iteration = 1
while done < total and iteration <= 5:
offset = first - 1
limit = last - first + 1
print(f" Iteration {iteration}: first={first}, last={last}, OFFSET={offset}, LIMIT={limit}, fetches positions {first} to {last}")
users_fetched = min(limit, total - done)
done += users_fetched
first = last + pageSize # = last + 100
last = first + pageSize # = first + 100
iteration += 1
EOF
python /tmp/test_pagination.pyRepository: sakaiproject/sakai
Length of output: 148
🏁 Script executed:
# From the earlier output, the key line was:
# (first - 1), (last - first + 1)
# This means: offset = first - 1, limit = last - first + 1
cat > /tmp/analyze_pagination.txt << 'EOF'
Based on BaseDbFlatStorage.java line: (first - 1), (last - first + 1)
This means: offset = first - 1, limit = last - first + 1
Current pagination code analysis:
- Iteration 1: first=1, last=1+1=2
→ offset=0, limit=2-1+1=2
→ fetches records at positions 0-1 (users 1-2 in 1-based indexing) ✓
- Iteration 2: first=2+1=3, last=3+1=4
→ offset=2, limit=4-3+1=2
→ fetches records at positions 2-3 (users 3-4 in 1-based indexing) ✓
Conclusion: NO GAP in pagination! Records are consecutive.
Other issues verified:
1. pageSize=1 is inefficient - causes many DB calls ✓
2. users.stream().forEach() - need to verify if this is considered bad practice
EOF
cat /tmp/analyze_pagination.txtRepository: sakaiproject/sakai
Length of output: 714
🏁 Script executed:
# Verify the stream().forEach() claim in Java best practices and this codebase
rg "\.forEach\(" --type java kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.javaRepository: sakaiproject/sakai
Length of output: 322
🏁 Script executed:
# Check line 604-605 more carefully with context
cat -n kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java | sed -n '589,608p'Repository: sakaiproject/sakai
Length of output: 879
Reduce page size and simplify stream usage.
-
Inefficient page size:
pageSize = 1causes excessive database queries for large installations. Increase to at least 100. -
Unnecessary stream: Use
users.forEach()instead ofusers.stream().forEach()for simpler, more direct iteration.
🔧 Suggested fix
private void pushToAllUsers(UserNotificationTransferBean bean) {
executor.execute(() -> {
- int pageSize = 1;
+ int pageSize = 100;
int first = 1;
int last = first + pageSize;
int total = userDirectoryService.countUsers();
int done = 0;
while (done < total) {
List<User> users = userDirectoryService.getUsers(first, last);
- users.stream().forEach(u -> {
+ users.forEach(u -> {
UserNotificationTransferBean un = new UserNotificationTransferBean();
un.from = bean.from;
un.to = u.getId();
un.event = bean.event;
un.ref = bean.ref;
un.title = bean.title;
push(decorateNotification(un));
});
first = last + pageSize;
last = first + pageSize;
done += users.size();
}
});
}🤖 Prompt for AI Agents
In
@kernel/kernel-impl/src/main/java/org/sakaiproject/messaging/impl/UserMessagingServiceImpl.java
around lines 583 - 606, The current loop uses a pageSize set to 1 and calls
users.stream().forEach(...), which is inefficient; change the pageSize
initialization to a larger value (e.g., 100) to reduce DB round-trips and
replace users.stream().forEach(...) with users.forEach(...) for direct
iteration; keep the logic that constructs UserNotificationTransferBean (un.from,
un.to, un.event, un.ref, un.title), calls decorateNotification(un) and then
push(...), and update the pagination math using the same first/last/done
variables around the userDirectoryService.getUsers(...) call.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
@announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java:
- Around line 123-133: The MOTD branch handling
AnnouncementService.EVENT_MOTD_NEW builds a UserNotificationData but omits
siteId, url, and toolId; update the MOTD path (in
AnnouncementsUserNotificationHandler where UserNotificationData is constructed)
to mirror the regular notification path by setting
userNotificationData.setSiteId(event.getContext()),
userNotificationData.setUrl(...) using the same URL construction used for
non‑MOTD notifications (e.g., build the announcement link from the
message/header or AnnouncementReference), and set
userNotificationData.setToolId(...) to the same tool id value used in the normal
notification flow so broadcast notifications include site context, a
clickthrough URL, and tool association.
- Around line 206-213: The role-filtering stream can NPE because
site.getMember(u) or getRole() may be null; update the block that sets
filteredUsersToNotify (using selectedRoles, usersToNotify, site.getMember,
getRole, getId) to defensively check for nulls before calling
getRole().getId()—e.g., map each user to SiteMember, skip if null, get role and
skip if null, then test role.getId() against selectedRoles; ensure you use
explicit types (no var) per guidelines when adding any local variables.
🧹 Nitpick comments (1)
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java (1)
176-176: Consider using constant for tool common ID.The hardcoded string
"sakai.announcements"should ideally be a constant fromAnnouncementService(similar toSAKAI_ANNOUNCEMENT_TOOL_IDused at line 219) for consistency and maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T16:11:33.008Z
Learning: Applies to **/sakai-notifications.properties : Use sakai-notifications.properties for internationalized PWA installation messages
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/*.{java,properties} : Reconcile push notification subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to **/sakai-notifications.properties : Reconcile subscriptions proactively and keep localization strings in `sakai-notifications.properties`
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-11-24T19:12:27.663Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:27.663Z
Learning: Applies to {**/UserMessagingServiceImpl.java,**/sakai-service-worker.js} : Use Web Push with VAPID via `UserMessagingServiceImpl.java`; rely on the service worker in `sakai-service-worker.js`
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-11-24T19:12:20.537Z
Learnt from: CR
Repo: sakaiproject/sakai PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T19:12:20.537Z
Learning: Applies to **/UserMessagingServiceImpl.java : Use Web Push with VAPID via `UserMessagingServiceImpl.java` and the service worker in `sakai-service-worker.js`
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
📚 Learning: 2025-09-25T13:34:06.742Z
Learnt from: susowrc
Repo: sakaiproject/sakai PR: 14105
File: assignment/tool/src/java/org/sakaiproject/assignment/entityproviders/AssignmentEntityProvider.java:1736-1745
Timestamp: 2025-09-25T13:34:06.742Z
Learning: In Sakai, each site can only have one tool placement for a given common tool ID (e.g., "sakai.assignment.grades" for the Assignment tool). The method getToolForCommonId() returns a single ToolConfiguration object, not a collection, confirming this one-to-one relationship between tools and sites.
Applied to files:
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (5)
announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java (5)
86-93: LGTM: Clean configuration initialization.The constant and PostConstruct initialization for MOTD TTL configuration follows best practices.
113-116: LGTM: Correct delegation to MOTD_NEW.The early return prevents duplicate processing by delegating MOTD announcements to the dedicated EVENT_MOTD_NEW handler.
135-141: LGTM: Good refactoring.Early initialization of
isDraftMessageandisFutureMessageimproves readability and avoids repeated message header access.
143-169: LGTM: Correct transaction handling.The deletion logic properly:
- Queries and detaches entities before bulk delete
- Uses separate roots for SELECT and DELETE queries via
buildWherehelper- Handles transaction exceptions with appropriate logging
236-245: LGTM: Excellent refactoring.The
buildWherehelper method eliminates the previous predicate reuse issue by allowing each query to build its own predicate from its own root. This is a clean solution that makes the code more maintainable.
| if (eventName.equals(AnnouncementService.EVENT_MOTD_NEW)) { | ||
|
|
||
| String title = ((AnnouncementMessageHeader) message.getHeader()).getSubject(); | ||
| UserNotificationData userNotificationData = new UserNotificationData(); | ||
| userNotificationData.setFrom(event.getUserId()); | ||
| userNotificationData.setTo("BROADCAST"); | ||
| userNotificationData.setTitle(title); | ||
| userNotificationData.setBroadcast(true); | ||
| userNotificationData.setTtl(Duration.ofHours(motdTTLHours)); | ||
| return Optional.of(List.of(userNotificationData)); | ||
| } |
There was a problem hiding this comment.
Missing required fields in MOTD broadcast notification.
The MOTD broadcast notification is missing siteId, url, and toolId that are set in the regular notification path (lines 179-184, 187, 219). Without these fields:
- Users cannot click through to the announcement (no URL)
- Site context is missing for downstream processing
- Tool association is absent for proper categorization
🔧 Proposed fix
if (eventName.equals(AnnouncementService.EVENT_MOTD_NEW)) {
String title = ((AnnouncementMessageHeader) message.getHeader()).getSubject();
+ String eventContext = event.getContext();
+
+ // Build URL similar to regular notification flow
+ Site site = siteService.getSite(eventContext);
+ ToolConfiguration toolConfig = site.getToolForCommonId("sakai.announcements");
+ String url = "";
+ if (toolConfig != null) {
+ url = serverConfigurationService.getPortalUrl()
+ + "/directtool/"
+ + toolConfig.getId()
+ + "?itemReference="
+ + eventResource
+ + "&sakai_action=doShowmetadata";
+ }
+
UserNotificationData userNotificationData = new UserNotificationData();
userNotificationData.setFrom(event.getUserId());
userNotificationData.setTo("BROADCAST");
+ userNotificationData.setSiteId(eventContext);
userNotificationData.setTitle(title);
+ userNotificationData.setUrl(url);
+ userNotificationData.setToolId(AnnouncementService.SAKAI_ANNOUNCEMENT_TOOL_ID);
userNotificationData.setBroadcast(true);
userNotificationData.setTtl(Duration.ofHours(motdTTLHours));
return Optional.of(List.of(userNotificationData));
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
@announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
around lines 123 - 133, The MOTD branch handling
AnnouncementService.EVENT_MOTD_NEW builds a UserNotificationData but omits
siteId, url, and toolId; update the MOTD path (in
AnnouncementsUserNotificationHandler where UserNotificationData is constructed)
to mirror the regular notification path by setting
userNotificationData.setSiteId(event.getContext()),
userNotificationData.setUrl(...) using the same URL construction used for
non‑MOTD notifications (e.g., build the announcement link from the
message/header or AnnouncementReference), and set
userNotificationData.setToolId(...) to the same tool id value used in the normal
notification flow so broadcast notifications include site context, a
clickthrough URL, and tool association.
| List<String> selectedRoles = message.getProperties().getPropertyList(SELECTED_ROLES_PROPERTY); | ||
| if (selectedRoles != null && !selectedRoles.isEmpty()) { | ||
| filteredUsersToNotify = usersToNotify.stream() | ||
| .filter(u -> selectedRoles.contains(site.getMember(u).getRole().getId())) | ||
| .collect(Collectors.toSet()); | ||
| } else { | ||
| filteredUsersToNotify = usersToNotify; | ||
| } |
There was a problem hiding this comment.
Potential NPE in role-based filtering.
The role filtering at lines 208-209 calls site.getMember(u).getRole().getId() without null checks. If a user's membership changes between getUsersIsAllowed() (lines 195, 200) and this filter, or if getRole() returns null, this will throw a NullPointerException.
🛡️ Proposed defensive fix
if (selectedRoles != null && !selectedRoles.isEmpty()) {
filteredUsersToNotify = usersToNotify.stream()
- .filter(u -> selectedRoles.contains(site.getMember(u).getRole().getId()))
+ .filter(u -> {
+ var member = site.getMember(u);
+ return member != null && member.getRole() != null
+ && selectedRoles.contains(member.getRole().getId());
+ })
.collect(Collectors.toSet());
} else {Note: Replace var with explicit type per coding guidelines:
if (selectedRoles != null && !selectedRoles.isEmpty()) {
filteredUsersToNotify = usersToNotify.stream()
- .filter(u -> selectedRoles.contains(site.getMember(u).getRole().getId()))
+ .filter(u -> {
+ org.sakaiproject.site.api.Member member = site.getMember(u);
+ return member != null && member.getRole() != null
+ && selectedRoles.contains(member.getRole().getId());
+ })
.collect(Collectors.toSet());
} else {📝 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.
| List<String> selectedRoles = message.getProperties().getPropertyList(SELECTED_ROLES_PROPERTY); | |
| if (selectedRoles != null && !selectedRoles.isEmpty()) { | |
| filteredUsersToNotify = usersToNotify.stream() | |
| .filter(u -> selectedRoles.contains(site.getMember(u).getRole().getId())) | |
| .collect(Collectors.toSet()); | |
| } else { | |
| filteredUsersToNotify = usersToNotify; | |
| } | |
| List<String> selectedRoles = message.getProperties().getPropertyList(SELECTED_ROLES_PROPERTY); | |
| if (selectedRoles != null && !selectedRoles.isEmpty()) { | |
| filteredUsersToNotify = usersToNotify.stream() | |
| .filter(u -> { | |
| org.sakaiproject.site.api.Member member = site.getMember(u); | |
| return member != null && member.getRole() != null | |
| && selectedRoles.contains(member.getRole().getId()); | |
| }) | |
| .collect(Collectors.toSet()); | |
| } else { | |
| filteredUsersToNotify = usersToNotify; | |
| } |
🤖 Prompt for AI Agents
In
@announcement/announcement-api/api/src/java/org/sakaiproject/announcement/api/AnnouncementsUserNotificationHandler.java
around lines 206 - 213, The role-filtering stream can NPE because
site.getMember(u) or getRole() may be null; update the block that sets
filteredUsersToNotify (using selectedRoles, usersToNotify, site.getMember,
getRole, getId) to defensively check for nulls before calling
getRole().getId()—e.g., map each user to SiteMember, skip if null, get role and
skip if null, then test role.getId() against selectedRoles; ensure you use
explicit types (no var) per guidelines when adding any local variables.
1 participant
https://sakaiproject.atlassian.net/browse/SAK-51956
The idea behind this change is to replace the MOTD type notifications with a single db entry rather than one for each system user.
This change also creates a SchedulingService which provides a facade onto a ScheduledExecutorService and updates all the uses of scheduled executors across Sakai to use it.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.