SAK-49440 msgcntr modify the mark as read behaviour to mark as not read #14331
Conversation
WalkthroughRenames read-related APIs, fields, permissions, UI labels, JS identifiers, CSS, localization keys, and DB mappings from "mark as read"/MARK_AS_READ to "mark as not read"/MARK_AS_NOT_READ; updates implementations, event handling, and reader-count logic to use the new NotRead variants. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/PrivateMessageManagerImpl.java (2)
1527-1596: Critical semantic mismatch: Method name contradicts its behavior.The method
markMessageAsNotReadForUseractually marks the message as read (see line 1587:setRead(Boolean.TRUE)), which is the exact opposite of what the name implies. The name "not read" suggests the message should be marked as unread, but the implementation:
- Sets
read = TRUE(line 1587)- Decrements the unread message count (line 1593)
- Posts a
EVENT_MESSAGES_READevent (line 1594)This is especially confusing given that
markMessageAsUnreadForUser(line 1643) exists and correctly setsread = FALSE.The semantic inversion will cause significant confusion for developers maintaining this code. If the intent is to rename for UI/UX reasons, consider a name that doesn't conflict with the actual behavior, or ensure the interface contract is clearly documented.
What is the recommended naming convention for Java methods that toggle boolean states?
2402-2423: Method call behavior is correct, but naming causes confusion.The call to
markMessageAsNotReadForUseron line 2411 is functionally correct—when a user replies to a message, it makes sense to mark that message as read. However, the method name suggests the opposite action, making this code difficult to understand and maintain.A developer reading this flow would reasonably expect:
markMessageAsRepliedForUser→ marks as replied ✓markMessageAsNotReadForUser→ marks as "not read" (unread) ✗ (actually marks as read)This contradicts intuitive expectations.
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/MessageForumSynopticBean.java (1)
1563-1607: Update Javadoc/comments/log text to match “not read” and remove concatenation.The method now marks messages as not read, but the Javadoc, inline comment, and error logs still say “mark all as read,” which is misleading for support diagnostics. Also, the log messages currently concatenate literals; simplify to a single literal to align with logging guidance.
🔧 Proposed fix
- /** - * This marks all Private messages as read for a particular site - * - * `@param` ActionEvent e - */ + /** + * This marks all Private messages as not read for a particular site + * + * `@param` ActionEvent e + */ public void processNotReadAll(ActionEvent e) { @@ if (privateMessages == null) { - log.error("No messages found while attempting to mark all as read " - + "from synoptic Message Center tool."); + log.error("No messages found while attempting to mark all as not read from synoptic Message Center tool."); } @@ // Get the site id and user id and call query to - // mark them all as read + // mark them all as not read List privateMessages = pvtMessageManager.getMessagesByType( @@ if (privateMessages == null) { - log.error("No messages found while attempting to mark all as read " - + "from synoptic Message Center tool."); + log.error("No messages found while attempting to mark all as not read from synoptic Message Center tool."); }As per coding guidelines, use non-concatenated/parameterized SLF4J logging.
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/PrivateMessagesTool.java (1)
1282-1288: The method name contradicts its implementation and should be renamed.The method
markMessageAsNotReadForUseractually marks the message as read (setssetRead(Boolean.TRUE)and postsEVENT_MESSAGES_READ), but the name suggests it marks the message as "not read" (unread). This is confusing and creates a significant maintainability issue.The behavior at line 1284 is correct for viewing message details—it should mark the message as read. However, the method should be renamed to
markMessageAsReadForUserto match its actual implementation and align with the opposite methodmarkMessageAsUnreadForUser, which correctly marks as unread by settingsetRead(Boolean.FALSE).Rename
markMessageAsNotReadForUsertomarkMessageAsReadForUserthroughout the codebase for clarity and consistency.msgcntr/messageforums-app/src/webapp/js/forum.js (1)
199-212: Fix assignment bug inresetMainCheckbox(Line 210).
if (mainCheckboxEl.checked = true)uses assignment (=) instead of comparison, creating a constant condition that always evaluates to true. Replace with a proper boolean check and null guard.🐛 Proposed fix
- if (mainCheckboxEl.checked = true) { - mainCheckboxEl.checked = false; - } + if (mainCheckboxEl && mainCheckboxEl.checked) { + mainCheckboxEl.checked = false; + }msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/MessageForumsMessageManagerImpl.java (1)
1180-1234: Fix SLF4J logging to use parameterized format at line 1189.Line 1189 uses string concatenation for logging (
log.debug("set Message readers count to: " + nr)) instead of parameterized logging. Replace with:log.debug("set Message readers count to: {}", nr)to comply with SLF4J guidelines. See line 1203 for the correct pattern already used in the same method.The boolean inversion at line 1180 is correct, and all verified call sites pass appropriate argument values for the updated semantics.
🤖 Fix all issues with AI agents
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java`:
- Around line 6836-6847: The two action handlers pass the wrong boolean to the
marking helpers: processActionMarkAllAsNotRead() currently calls
markAllMessages(true) and processActionMarkAllThreadAsNotRead() calls
markAllThreadAsNotRead(true) but the parameter semantics are true = read, false
= unread; change those calls to pass false so they mark items as unread (align
with processActionMarkCheckedAsUnread() which calls markCheckedMessages(false));
update calls in DiscussionForumTool.java for markAllMessages(...) and
markAllThreadAsNotRead(...) accordingly.
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/jsf/AjaxPhaseListener.java`:
- Around line 45-58: The "markMessageAsNotRead" branch in AjaxPhaseListener
currently does nothing when messageId or topicId is null and returns an empty
response; update the branch so that when either messageId or topicId is missing
you write an explicit failure response (e.g., out.println("FAIL")) and
return/exit the handler so the client sees a clear failure; locate the code in
AjaxPhaseListener where the action equals "markMessageAsNotRead" and add the
explicit FAIL output for the missing-id case (referencing the variables
messageId, topicId and the existing out.println usage) to match the null-action
handling elsewhere.
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/PrivateMessagesTool.java`:
- Around line 4753-4759: The current branch in markCheckedMessages (invoked by
processActionMarkCheckedAsRead) calls prtMsgManager.markMessageAsNotReadForUser
when readStatus==true and decoMessage.isHasRead()==false, which is semantically
inverted; inspect the PrivateMessageManager interface implementations for
markMessageAsNotReadForUser / markMessageAsUnreadForUser /
markMessageAsReadForUser to confirm their actual behavior, then update
markCheckedMessages so that when readStatus==true it calls the method that marks
the message as read (e.g., markMessageAsReadForUser) for unread messages, and
when readStatus==false it calls the method that marks the message as unread
(e.g., markMessageAsNotReadForUser or markMessageAsUnreadForUser); ensure the
logic in processActionMarkCheckedAsRead and processActionMarkCheckedAsUnread
aligns with these corrected method calls.
In
`@msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/DiscussionTopicBean.java`:
- Around line 889-897: getIsMarkAsNotRead() currently calls
isMarkAsNotRead.booleanValue() which can throw NPE if isMarkAsNotRead is null;
change the getter to return a safe boolean (for example return
Boolean.TRUE.equals(isMarkAsNotRead) or (isMarkAsNotRead != null &&
isMarkAsNotRead.booleanValue())) so it never dereferences null; update
getIsMarkAsNotRead and consider keeping setIsMarkAsNotRead(Boolean) as-is to
allow null but ensure the getter handles that case.
In `@msgcntr/messageforums-app/src/webapp/js/forum.js`:
- Around line 366-417: The success handler is doing string concatenation instead
of numeric increment and injecting HTML unsafely; change the numeric update on
the thread count (the $('.' + thisThread).find('em').text(...) line) to parse
the current value as an integer and set text(current + 1), and replace all
direct .html()/.prepend() calls that build HTML strings with creation of safe
elements (e.g., create a span via DOM/jQuery, add classes, and set its text to
newFlag) before inserting; update references: thisThread, unread, newFlag, the
messagetitlelink .html(...) call, the prepend('<span class="messageNew">' +
newFlag + '</span>') and the dfFlatView prepend/after that concatenate newFlag
so they use safe element creation and numeric parsing instead of string
concatenation.
In `@msgcntr/messageforums-app/src/webapp/js/threadScrollEvent.js`:
- Around line 27-33: The scroll handler attaches to scroller (const scroller =
document.querySelector(...)) but elementsInViewPort and isBottomInViewport still
test against the window; update the call chain so scrollerfn passes the scroller
element into elementsInViewPort (and any intermediate functions) and change
isBottomInViewport to accept a container parameter and use
container.getBoundingClientRect() (not window) to compute visibility (compare
element.getBoundingClientRect() against the container rect and scroll bounds).
Also update any callers of isBottomInViewport/elementsInViewPort to provide
scroller so marking messages read uses the scroller's bounds.
- Around line 10-11: The code uses unqualified globals and allows
division-by-zero in updateBar; change references from showThreadChanges and
statRead to window.showThreadChanges and window.statRead to avoid implicit
globals, and update the percentage calc in updateBar (the function name
updateBar and variable totalWidth) to guard against totalWidth === 0 by
computing percent only when totalWidth > 0 (e.g., set percent = totalWidth > 0 ?
Math.round((value / totalWidth) * 100) : 0) so you never assign Infinity% to
element.style.width.
In
`@msgcntr/messageforums-app/src/webapp/jsp/discussionForum/message/dfViewSearchBar.jsp`:
- Around line 71-72: The value attribute on the h:commandLink with id
"markAsNotRead" uses a typod message key "cdfm_mark_check_as_not:read" (colon)
which won't resolve; update the EL to use the correct key
"cdfm_mark_check_as_not_read" to match the title and other usages (change the
value on the ForumTool.processActionMarkCheckedAsNotRead link).
In
`@msgcntr/messageforums-app/src/webapp/jsp/discussionForum/message/dfViewThreadBodyInclude.jsp`:
- Around line 26-27: The id attribute is invalid because it nests <h:outputText>
inside the quoted attribute and uses abbreviation prefixes; replace the nested
tag with a single attribute value that uses EL expressions (no nested JSP tags)
and switch to kebab-case for the parts. Specifically, remove the inner
<h:outputText> and set id to a kebab-case composition like
message-#{message.message.id}-topic-#{ForumTool.selectedTopic.topic.id}-forum-#{ForumTool.selectedTopic.topic.baseForum.id},
ensuring the attribute is a single quoted string and not containing another tag.
In
`@msgcntr/messageforums-hbm/src/java/org/sakaiproject/component/app/messageforums/dao/hibernate/PermissionLevelImpl.hbm.xml`:
- Line 35: The Hibernate mapping changed the property name markAsNotRead to map
to column MARK_AS_NOT_READ in PermissionLevelImpl.hbm.xml, so add a DB migration
in sakai-reference docs/conversion that renames (or creates and populates)
MFR_PERMISSION_LEVEL_T.MARK_AS_NOT_READ from the existing MARK_AS_READ values,
include a safe rollback path that restores the original column and data if
needed, and update the upgrade/conversion documentation with the exact version
steps and SQL used; ensure the migration preserves nullability/constraints and
is idempotent.
🧹 Nitpick comments (14)
msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/PrivateMessageManagerImpl.java (1)
1549-1573: Log and exception messages perpetuate the naming confusion.The updated log and exception messages (lines 1549, 1570, 1572) are consistent with the renamed method, but they will produce misleading output:
markMessageAsNotReadForUser(message: ...) has empty recipient listWhen debugging, this message suggests the system was trying to mark a message as "not read," when it was actually attempting to mark it as read. Consider adding clarifying comments or updating the messages to reflect the actual action being performed.
msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/entity/ForumMessageEntityProviderImpl.java (1)
452-475: Use parameterized logging for retry messages.String concatenation in SLF4J logs should be replaced with
{}placeholders.♻️ Proposed fix
- log.info("ForumMessageEntityProviderImpl: markAsNotRead: HibernateOptimisticLockingFailureException: attempts left: " - + numOfAttempts); + log.info("ForumMessageEntityProviderImpl: markAsNotRead: HibernateOptimisticLockingFailureException: attempts left: {}", numOfAttempts); ... - log.info("ForumMessageEntityProviderImpl: markAsNotRead: attempts left: " - + numOfAttempts); + log.info("ForumMessageEntityProviderImpl: markAsNotRead: attempts left: {}", numOfAttempts);As per coding guidelines, use SLF4J parameterized logging.
msgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/ui/PrivateMessageManager.java (1)
72-74: Clarify NotRead vs Unread API semantics.With
markMessageAsNotReadForUser(...)added alongside existingmarkMessageAsUnreadForUser(...), the distinction is ambiguous. Consider adding JavaDoc to differentiate or deprecate one to avoid misuse.msgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/MessageForumsMessageManager.java (1)
101-105: Consider renaming the boolean parameter for clarity.The method name now implies “not read” but the boolean parameter is still
read. Renaming it (and updating Javadoc) would reduce confusion for callers.msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.java (2)
1698-1707: Use parameterized logging inmarkMessageAs.Avoid string concatenation in SLF4J logs.
♻️ Proposed fix
- log.debug("markMessageAsNotRead(Message" + message + ")"); + log.debug("markMessageAsNotRead(Message {})", message);As per coding guidelines, use SLF4J parameterized logging.
1716-1725: Fix log typo and parameterize the message.The log line says
markMessageNoReadStatusForUserand uses concatenation.♻️ Proposed fix
- log.debug("markMessageNoReadStatusForUser(Message" + message + " readStatus:" + readStatus + " userId: " + userId + ")"); + log.debug("markMessageNotReadStatusForUser(Message {}, readStatus: {}, userId: {})", message, readStatus, userId);As per coding guidelines, use SLF4J parameterized logging.
msgcntr/messageforums-app/src/webapp/jsp/discussionForum/message/dfFlatView.jsp (2)
19-19: Define i18n globals beforethreadScrollEvent.jsexecutes.
Line 19 loads the script before Lines 33–37 define the new strings; if the script reads them at module load, they’ll beundefined. Please verify the load order and move the definitions above the include if needed. Also consider swapping$(document).readyforDOMContentLoadedand reducing globals. As per coding guidelines, ...♻️ Suggested re-ordering
- <script src="/messageforums-tool/js/threadScrollEvent.js"></script> + <script> + var markAsNotReadText = "<h:outputText value="#{msgs.cdfm_mark_as_not_read}"/>"; + var showThreadChanges = "<h:outputText value="#{ForumTool.showThreadChanges}"/>"; + var statRead = "<h:outputText value="#{msgs.stat_forum_read}"/>"; + var newFlag = "<h:outputText value="#{msgs.cdfm_newflag}"/>"; + var isMarkAsNotReadValue = "<h:outputText value="#{ForumTool.selectedTopic.isMarkAsNotRead}"/>"; + </script> + <script src="/messageforums-tool/js/threadScrollEvent.js"></script> ... - var markAsNotReadText = "<h:outputText value="#{msgs.cdfm_mark_as_not_read}"/>"; - var showThreadChanges = "<h:outputText value="#{ForumTool.showThreadChanges}"/>"; - var statRead = "<h:outputText value="#{msgs.stat_forum_read}"/>"; - var newFlag = "<h:outputText value="#{msgs.cdfm_newflag}"/>"; - var isMarkAsNotReadValue = "<h:outputText value="#{ForumTool.selectedTopic.isMarkAsNotRead}"/>";Also applies to: 33-37
72-77: Use Bootstrap progress markup + kebab-case for the header bar.
This new block can use Bootstrap’s progress component for built-in accessibility and styling consistency, and the new class/id names should prefer kebab-case (e.g.,header-bar,my-bar). As per coding guidelines, ...♻️ Suggested Bootstrap markup
- <div class="headerBar"> - <div class="progress-container"> - <div class="progress-bar" id="myBar"></div> - </div> - <div id="progress-value" class="p-2"></div> - </div> + <div class="header-bar"> + <div class="progress"> + <div class="progress-bar" id="my-bar" role="progressbar" + aria-valuenow="0" aria-valuemin="0" aria-valuemax="100"></div> + </div> + <div id="progress-value" class="p-2" aria-live="polite"></div> + </div>msgcntr/messageforums-app/src/webapp/jsp/synoptic/synMain.jsp (1)
78-80: Prefer kebab-case for new IDs/classes (popup + icon).
Consider renamingmarkAsNotRead/markAsNotReadIconto kebab-case and updating CSS/JS selectors accordingly for consistency with the frontend conventions. As per coding guidelines, ...Also applies to: 173-175
msgcntr/messageforums-app/src/webapp/jsp/discussionForum/message/dfViewThread.jsp (1)
81-86: Align the progress bar with Bootstrap’s component markup + a11y attributes.Consider switching to Bootstrap’s
.progress/.progress-barstructure and addingrole="progressbar"plus aria values for accessibility. If you rename IDs/classes to kebab-case, update the related JS selectors accordingly. As per coding guidelines, use Bootstrap 5.2 components and ensure responsive UI.♻️ Suggested markup update
- <div class="headerBar"> - <div class="progress-container"> - <div class="progress-bar" id="myBar"></div> - </div> - <div id="progress-value" class="p-2"></div> - </div> + <div class="header-bar"> + <div class="progress"> + <div class="progress-bar" id="thread-progress-bar" + role="progressbar" aria-valuenow="0" aria-valuemin="0" aria-valuemax="100"></div> + </div> + <div id="thread-progress-value" class="p-2"></div> + </div>msgcntr/messageforums-app/src/webapp/jsp/discussionForum/message/dfViewThreadBodyInclude.jsp (1)
41-91: Consider kebab-case for new CSS hooks.
messageNewNumReadersandmarkAsNotReadIconare newly introduced class names; consider renaming them to kebab-case (e.g.,message-new-num-readers,mark-as-not-read-icon) and update CSS/JS references accordingly. As per coding guidelines, prefer kebab-case for class and id values.msgcntr/messageforums-app/src/webapp/js/forum.js (1)
428-468: Add error handling and validateselffordoAjaxRead.The new flow sets a spinner and mutates the DOM but doesn’t handle AJAX failures, and it prepends an
<a>intoself. Please verifyselfis a container element (not an<a>/img>) at call sites and add an error path to restore UI state.You can locate call sites quickly:
#!/bin/bash # Find all doAjaxRead usages and inspect the `self` argument. rg -n "doAjaxRead\s*\(" -C3msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/MessageForumsMessageManagerImpl.java (1)
1135-1163: Use parameterized SLF4J logging and fix the typo in the new log message.Line 1135 says
markMessageReadNotForUser, and the new log lines use string concatenation. Prefer SLF4J placeholders for consistency and to avoid unnecessary string building.🛠️ Proposed fix
- log.error("markMessageReadNotForUser failed with topicId: " + topicId + ", messageId: " + messageId); + log.error("markMessageNotReadForUser failed with topicId: {}, messageId: {}", topicId, messageId); @@ - log.debug("markMessageNotReadForUser executing with topicId: " + topicId + ", messageId: " + messageId); + log.debug("markMessageNotReadForUser executing with topicId: {}, messageId: {}", topicId, messageId); @@ - log.error("markMessageNotReadForUser failed with topicId: " + topicId + ", messageId: " + messageId + ", userId: " + userId); + log.error("markMessageNotReadForUser failed with topicId: {}, messageId: {}, userId: {}", topicId, messageId, userId); @@ - log.debug("markMessageNotReadForUser executing with topicId: " + topicId + ", messageId: " + messageId); + log.debug("markMessageNotReadForUser executing with topicId: {}, messageId: {}", topicId, messageId);As per coding guidelines, use SLF4J parameterized logging.
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java (1)
2864-2870: ClarifyreadStatussemantics in the new thread-display overload.
processActionGetDisplayThread(boolean readStatus)always sets message beans to unread; consider renaming the parameter (e.g.,markAsNotRead) or documenting the boolean meaning to prevent future misuse.Also applies to: 2882-2947
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java
Show resolved
Hide resolved
...tr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/jsf/AjaxPhaseListener.java
Show resolved
Hide resolved
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/PrivateMessagesTool.java
Show resolved
Hide resolved
...r/messageforums-app/src/java/org/sakaiproject/tool/messageforums/ui/DiscussionTopicBean.java
Show resolved
Hide resolved
msgcntr/messageforums-app/src/webapp/jsp/discussionForum/message/dfViewSearchBar.jsp
Outdated
Show resolved
Hide resolved
msgcntr/messageforums-app/src/webapp/jsp/discussionForum/message/dfViewThreadBodyInclude.jsp
Show resolved
Hide resolved
.../java/org/sakaiproject/component/app/messageforums/dao/hibernate/PermissionLevelImpl.hbm.xml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.java`:
- Around line 1698-1707: Replace string-concatenated debug logging with SLF4J
parameterized logging in the markMessageAsNoRead method: change the log.debug
call that currently builds "markMessageAsNotRead(Message" + message + ")" to use
log.debug("markMessageAsNoRead(Message={})", message) (use the exact method name
markMessageAsNoRead) so the message object is passed as a parameter; do the same
for the other similar debug call around messageManager.markMessageNotReadForUser
(the second occurrence noted at lines 1716-1725) to use parameterized
placeholders instead of concatenation.
♻️ Duplicate comments (1)
msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java (1)
6836-6847: Duplicate: mark-all not-read actions still passtrue.This matches the previously flagged inversion (should pass
falseto mark unread). Please apply the same fix noted earlier.
🧹 Nitpick comments (2)
msgcntr/messageforums-api/src/java/org/sakaiproject/api/app/messageforums/ui/DiscussionForumManager.java (1)
439-447: Align NOT_READ naming for API clarity.
markMessageAsNoReadconflicts with the “NotRead” terminology used elsewhere (e.g.,markMessageNotReadStatusForUser) and the updated Javadoc, which makes the public API feel inconsistent. Consider renaming tomarkMessageAsNotReadand renaming the boolean parameter tonotRead/notReadStatusfor clarity. This is a public API change, so please verify call sites before finalizing.♻️ Proposed rename (requires updating call sites)
- public void markMessageAsNoRead(Message message, boolean readStatus); + public void markMessageAsNotRead(Message message, boolean notReadStatus); - public void markMessageNotReadStatusForUser(Message message, boolean readStatus, String userId); + public void markMessageNotReadStatusForUser(Message message, boolean notReadStatus, String userId);msgcntr/messageforums-app/src/java/org/sakaiproject/tool/messageforums/DiscussionForumTool.java (1)
2882-2947: Consider extracting shared thread-display logic.The new overload duplicates
processActionGetDisplayThread()almost entirely; a shared helper would reduce drift risk and makereadStatushandling explicit.
...mpl/src/java/org/sakaiproject/component/app/messageforums/ui/DiscussionForumManagerImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@msgcntr/messageforums-api/src/bundle/org/sakaiproject/api/app/messagecenter/bundle/Messages.properties`:
- Line 59: The locale bundles are inconsistent: some files still use the old
keys (e.g., cdfm_mark_as_read, cdfm_mark_all_as_read) while the code expects the
new keys (e.g., cdfm_mark_as_not_read); update every locale properties file to
replace the old key names with the new key names (preserve the translated
values) so all languages include cdfm_mark_as_not_read and any other renamed
keys referenced by the application; search for the old keys across the
repository, verify translations are copied to the corresponding new keys in each
locale file, and remove or alias the old keys to avoid duplicate/conflicting
entries.
♻️ Duplicate comments (3)
msgcntr/messageforums-api/src/bundle/org/sakaiproject/api/app/messagecenter/bundle/Messages.properties (3)
187-187: Covered by prior verification request for key renames.
307-307: Covered by prior verification request for key renames.
418-418: Covered by prior verification request for key renames.Also applies to: 660-660
🧹 Nitpick comments (2)
msgcntr/messageforums-api/src/bundle/org/sakaiproject/api/app/messagecenter/bundle/Messages_es.properties (1)
59-59: Align “no leído” wording with existing “sin leer” terminology.These new strings introduce “no leído/no leídos” while most of this bundle uses “sin leer” (e.g.,
stat_unread,cdfm_no_unread_messages,cdfm_unread). Consider aligning for consistency.🔤 Possible wording alignment
-cdfm_mark_as_not_read=Marcar como no leído +cdfm_mark_as_not_read=Marcar como sin leer -cdfm_mark_all_as_not_read=Marcar todos como no leídos +cdfm_mark_all_as_not_read=Marcar todos como sin leer -cdfm_no_message_mark_no_read=No se ha seleccionado ningún mensaje para marcarlo como no leído. Seleccione un mensaje. +cdfm_no_message_mark_no_read=No se ha seleccionado ningún mensaje para marcarlo como sin leer. Seleccione un mensaje. -perm_mark_as_not_read=Marcar como no leído +perm_mark_as_not_read=Marcar como sin leer -pvt_no_message_mark_no_read=No se ha seleccionado ningún mensaje para marcarlo como no leído. Seleccione un mensaje. +pvt_no_message_mark_no_read=No se ha seleccionado ningún mensaje para marcarlo como sin leer. Seleccione un mensaje.Also applies to: 187-187, 305-305, 414-414, 656-656
msgcntr/messageforums-component-impl/src/java/org/sakaiproject/component/app/messageforums/entity/ForumMessageEntityProviderImpl.java (1)
429-438: Verify the new “not read” semantics and boolean parameter.
markAsReadnow callsmarkMessageNotReadForUser(...)withfalse, while private messages still mark read. Please confirm the boolean argument still results in the intended “mark as not read” behavior for forums and that all callers of themarkreadaction now expect this inversion. Consider adding a brief comment (or renaming if feasible) to reduce confusion between the method name and its current behavior.
...ssageforums-api/src/bundle/org/sakaiproject/api/app/messagecenter/bundle/Messages.properties
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
msgcntr/messageforums-app/src/webapp/jsp/discussionForum/message/dfViewSearchBar.jsp (1)
69-85: Fix incorrect method names in commented code that would cause runtime failures if uncommented.The commented JSP code (lines 71, 79) references methods that don't exist in DiscussionForumTool:
- Line 71:
processActionMarkCheckedAsNotReaddoes not exist; should useprocessActionMarkCheckedAsUnread()- Line 79:
processActionMarkAllNotAsReadis a typo; the correct method isprocessActionMarkAllAsNotRead()(note: "AllAs" not "AllNot")Either uncomment and correct these method names to match the actual implementations in DiscussionForumTool, or remove the commented code entirely to avoid confusion.
🤖 Fix all issues with AI agents
In `@msgcntr/messageforums-app/src/webapp/js/threadScrollEvent.js`:
- Around line 3-63: The handler currently assigns to window.onload (see
window.onload = function() { ... }), which clobbers other load handlers; change
this to window.addEventListener("load", ...) to compose handlers safely and
preserve the email-link fix in forum.js; while doing so, convert the global
window.numRead usage into a local variable (e.g., let numRead = 0) and update
all references inside the file (updateBar(window.numRead, ...) ->
updateBar(numRead, ...), incrementing numRead where window.numRead++ appears)
unless external code depends on window.numRead—in that case, set window.numRead
= numRead only when you must expose it. Ensure the scrollerfn,
elementsInViewPort, doAjaxRead, and updateBar logic remain unchanged except for
using the local numRead variable.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@msgcntr/messageforums-app/src/webapp/js/threadScrollEvent.js`:
- Around line 1-6: Remove the top-level assignment of window.numRead = 0 and
instead reset window.numRead only inside the if (showThreadChanges === "true")
block where you recalculate it from DOM items (the code that queries
/^mi-\d+ti-\d+fi-\d+$/ and sets totalWidth); this prevents clobbering a
pre-populated value used by the else branch that calls updateBar() and relies on
statRead. While editing, keep references to window.onload and statRead intact
but consider encapsulating window.numRead (and other globals) into a module/IIFE
or component to avoid polluting global scope.
In
`@msgcntr/messageforums-app/src/webapp/jsp/discussionForum/message/dfViewSearchBar.jsp`:
- Around line 79-80: The commandLink in dfViewSearchBar.jsp references the wrong
action method name processActionMarkAllNotAsRead; update the action attribute to
call the existing DiscussionForumTool method processActionMarkAllAsNotRead
(i.e., replace processActionMarkAllNotAsRead with processActionMarkAllAsNotRead)
so the JSP matches the actual method name used elsewhere (e.g., dfFlatView.jsp)
and avoids a runtime missing-method error.
🧹 Nitpick comments (1)
msgcntr/messageforums-app/src/webapp/js/threadScrollEvent.js (1)
3-8: PreferaddEventListener("load", ...)to avoid clobbering other handlers.Assigning
window.onloadoverwrites any existing onload handler. UsingaddEventListeneris safer and more idiomatic in this codebase.♻️ Proposed refactor
-window.onload = function() { +window.addEventListener("load", () => { ... -} +});Also applies to: 63-63
msgcntr/messageforums-app/src/webapp/jsp/discussionForum/message/dfViewSearchBar.jsp
Outdated
Show resolved
Hide resolved
|
@ern could you take a look? thanks. |
|
This looks absolutely insane. 39 files changed for "Mark as read" to become "Mark as not read"?! Has Teaching & Learning looked at this? |
|
@ottenhoff Yes, Teaching and learning reviewed and approved the new behaviour https://sakaiproject.atlassian.net/browse/SAK-49440 |
|
Ok got it. I remove my objections ;) |
ern
changed the title
ern
changed the title
| msgsList = filterModeratedMessages(msgsList, selectedTopic.getTopic(), | ||
| (DiscussionForum) selectedTopic.getTopic().getBaseForum()); | ||
|
|
||
| List orderedList = new ArrayList(); |
| setTopicBeanAssign(); | ||
| selectedTopic = getSelectedTopic(); | ||
|
|
||
| List msgsList = selectedTopic.getMessages(); |
| } | ||
|
|
||
| // mark all as not read | ||
| for (int i = 0; i < selectedThread.size(); i++) { |
| return MAIN; | ||
| } | ||
|
|
||
| for (int i = 0; i < msgsList.size(); i++) { |
| $(self).parents('tr').removeClass('messageNewNext'); | ||
| $(self).parents("div").children("span.messageNew").remove(); | ||
| $(self).parents("div").children("div.messageMetadata").find("span.unreadMsg").removeClass('unreadMsg'); | ||
| $(self).parents("div").parents("div").children('a.messageNewAnchor').remove(); |
There was a problem hiding this comment.
does optional chaining operator make sense here?
ern
left a comment
ern
left a comment
There was a problem hiding this comment.
look pretty good some minor comments and I would be little more defensive in the JS especially in the AJAX handler
4 participants
Modify the default behaviour of the "mark as read" in the forums tool
https://sakaiproject.atlassian.net/browse/SAK-49440
Script:
https://github.com/sakaiproject/sakai-reference/pull/282/files
Summary by CodeRabbit
New Features
UI/UX Changes
Permissions
Localization
✏️ Tip: You can customize this high-level summary in your review settings.