SAK-52355 Microsoft - Add the posibility to enable a new forced synchronization in Microsoft Team for the site#14514
Closed
jesusmmp wants to merge 3 commits intosakaiproject:masterfrom
Closed
SAK-52355 Microsoft - Add the posibility to enable a new forced synchronization in Microsoft Team for the site#14514jesusmmp wants to merge 3 commits intosakaiproject:masterfrom
jesusmmp wants to merge 3 commits intosakaiproject:masterfrom
Conversation
…ronization in Microsoft Team for the site
Contributor
WalkthroughAdds site-level Microsoft Teams sync enable/disable UI and persistence, admin API and client toggle to flip synchronization disabled state, archive/unarchive team APIs and Graph/site lock handling, filters synchronization operations to enabled-only, plus i18n, config placeholders, styling, and small logging tweaks. Changes
Possibly related PRs
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)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (2)
site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (2)
263-269: Redundant null check:teamIdis guaranteed non-null here.The
teamId != null ? teamId : ""ternary on line 265 is unnecessary—lines 258-261 already return early ifteamId == null.Proposed simplification
final SiteSynchronization ss = SiteSynchronization.builder() .siteId(site.getId()) - .teamId(teamId != null ? teamId : "") + .teamId(teamId) .forced(true) .syncDateFrom(syncDateFrom) .syncDateTo(syncDateTo) .build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 263 - 269, The ternary check on teamId when building SiteSynchronization is redundant because teamId is already guaranteed non-null earlier in MicrosoftSynchronizationEnabler (the early return guards it); update the SiteSynchronization.builder() call to pass teamId directly (use .teamId(teamId)) and remove the unnecessary ? : fallback to an empty string to simplify the code and avoid dead branches.
327-336: Inconsistent whitespace: extra space before.method calls.Lines 329, 332, 333, and 335 have a trailing space after
configValuebefore the method call (e.g.,configValue .isBlank()). While syntactically valid, this is inconsistent and appears to be a copy-paste artifact.Proposed fix
private static String[] getSitePropertyNameAndOptionalValueForSynchronization() { final String configValue = ServerConfigurationService.getString(SAKAI_PROPERTY_SITE_PROPERTY_NAME_AND_OPTIONAL_VALUE, null); - if (configValue == null || configValue .isBlank()) { + if (configValue == null || configValue.isBlank()) { return new String[0]; } - if (configValue .contains("=")) { - return configValue .split("=", 2); + if (configValue.contains("=")) { + return configValue.split("=", 2); } - return new String[] { configValue .trim(), "" }; + return new String[] { configValue.trim(), "" }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 327 - 336, In getSitePropertyNameAndOptionalValueForSynchronization update the inconsistent whitespace by removing the stray space between the variable and the method call on configValue (e.g., change "configValue .isBlank()", "configValue .contains(...)", "configValue .split(...)" and "configValue .trim()" to "configValue.isBlank()", "configValue.contains(...)", "configValue.split(...)" and "configValue.trim()"); keep all other logic unchanged and ensure the method compiles and passes formatting checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java`:
- Around line 121-123: The AutoConfigController currently calls
microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false), which
only returns enabled synchronizations and allows disabled records to be
re-proposed; change the call to retrieve all synchronizations including disabled
(e.g., use a new or existing method like getAllSiteSynchronizations(true) or add
an includeDisabled parameter to microsoftSynchronizationService and call that)
so the construction of candidate site/team maps excludes any existing
synchronization regardless of disabled state; update usages in
AutoConfigController (the ssList variable and any downstream logic that expects
only enabled records) to handle the full set accordingly.
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java`:
- Around line 339-353: The controller methods that mutate state
(updateSiteSynchronizationDisabled -> mapped at
"/setDisabled-siteSynchronization/{id}" and the similar endpoints for forced and
date updates "/setForced-siteSynchronization/{id" and
"/setDate-siteSynchronization/{id}") must use POST, not GET: change `@GetMapping`
to `@PostMapping` on updateSiteSynchronizationDisabled and the corresponding
handler methods (identify by method names updateSiteSynchronizationDisabled and
the handlers for forced/date site synchronization), and keep `@ResponseBody` and
parameter handling the same; update the frontend fetch calls to include the CSRF
token in headers (e.g., X-CSRF-TOKEN) or via a form field and ensure Spring
Security CSRF protection is enabled so requests will be validated.
In `@microsoft-integration/admin-tool/src/main/resources/Messages_es.properties`:
- Line 225: The message key error.set_disabled_synchronization is specific to
“deshabilitar” but the endpoint toggles both enable/disable; update the
Messages_es.properties value to a neutral Spanish phrase (e.g., "Error al
cambiar el estado de la sincronización" or similar) so failures for either
enabling or disabling are correctly described—edit the value for
error.set_disabled_synchronization accordingly.
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html`:
- Around line 95-98: The enable/disable anchor currently uses only an icon and
title, so update the toggle control (the anchor that calls toggleDisabled and
its icon <i>) to include an accessible name: add an aria-label and/or a
visually-hidden text node whose text is bound with the same Thymeleaf expression
used for the title (e.g. use the ternary ${row.disabled} ? #{index.enable} :
#{index.disable}) so screen readers get an explicit label while preserving the
existing icon and onclick behavior.
In
`@microsoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.java`:
- Around line 89-90: The disabled field in SiteSynchronization should be
non-nullable and default to false to avoid NULL rows being skipped by
Restrictions.eq("disabled", false): update the `@Column` on the disabled field to
include nullable = false and add a `@Builder.Default` boolean disabled = false
(matching the pattern used for status/creationStatus); do the same for the
forced field (add nullable = false to its `@Column` and `@Builder.Default` boolean
forced = false) so code paths like findAllEnabled() and the equality checks in
MicrosoftSiteSynchronizationRepositoryImpl that use Restrictions.eq will behave
predictably.
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java`:
- Around line 1013-1041: archiveTeam(...) and unarchiveTeam(...) currently
swallow generic exceptions and return false, which callers in
MicrosoftSynchronizationEnabler.java ignore; change the behavior to fail loudly:
remove the broad catch(Exception) blocks in
MicrosoftCommonServiceImpl.archiveTeam and .unarchiveTeam so failures propagate
(keep rethrowing MicrosoftCredentialsException), and change the method
signatures to void (or keep boolean but throw an explicit
RuntimeException/MicrosoftOperationException on error) so callers in
MicrosoftSynchronizationEnabler.java (the call sites at the previously
referenced locations) must handle exceptions or check results; update those
callers to catch and handle the propagated exception (or check the returned
value) and only mutate synchronization state when the archive/unarchive
completes successfully.
In `@site-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.properties`:
- Around line 1130-1132: Normalize the Spanish wording for the two keys to use a
consistent, neutral UI tone: replace
sinfo.synchronization.microsoft.allowForSite value to remove "una nueva..." and
read as a concise action like "Habilitar una sincronización forzada con
Microsoft Teams para este sitio." and update
sinfo.synchronization.microsoft.confirmEnabled to a neutral confirmation
phrasing (e.g., "Se ha activado la sincronización forzada con Microsoft Teams
para este sitio.") so both lines match tone and avoid awkward phrasing.
In `@site-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.properties`:
- Around line 1158-1159: Replace the awkward phrase "a new forced
synchronization in Microsoft Teams" in the properties
sinfo.synchronization.microsoft.allowForSite and
sinfo.synchronization.microsoft.confirmEnabled with a clearer, neutral, durable
phrase (for example "a forced Microsoft Teams synchronization" or "forced
synchronization with Microsoft Teams") so the user-facing copy reads naturally;
update both keys' values to the chosen wording while preserving surrounding
punctuation and context.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 274-278: Replace the incorrect exception logging in
MicrosoftSynchronizationEnabler: in both catch blocks (catch
(MicrosoftCredentialsException e) and catch (Exception e)) stop using
e.toString() and instead pass the Throwable to SLF4J so the stacktrace is
preserved, e.g. change log.error("... for site: {}", site.getId(), e.toString())
to log.error("Microsoft credentials error while creating team for site: {}",
site.getId(), e) and similarly for the general exception handler.
- Around line 244-246: The catch block in MicrosoftSynchronizationEnabler
currently logs the exception using e.toString(), which loses the stack trace;
update the log.warn call that references site.getId() and existingSs.getTeamId()
to pass the exception object as the last parameter (i.e., keep the parameterized
message and placeholders but replace e.toString() with the exception variable e)
so SLF4J logs include the full stack trace.
- Around line 171-173: The log.warn call in MicrosoftSynchronizationEnabler's
catch block is passing e.toString() as a placeholder argument and losing the
stack trace; change the call to use the two placeholders for site.getId() and
ss.getTeamId() and pass the exception object e as the final argument (without a
placeholder) so SLF4J will log the full stack trace—update the log.warn
invocation around site.getId() and ss.getTeamId() to pass e instead of
e.toString().
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Line 2593: doEdit_site_info's "continue" handler currently calls
MathJaxEnabler.applySettingsToState() but does not invoke
MicrosoftSynchronizationEnabler.applySettingsToState(), so the Microsoft sync
toggle shown via MicrosoftSynchronizationEnabler.addToContext(...) is not
applied to state before prepareSiteForSave() runs. Update the continue handler
inside doEdit_site_info to call
MicrosoftSynchronizationEnabler.applySettingsToState() alongside the existing
MathJaxEnabler call (so both applySettingsToState() methods run before
prepareSiteForSave()), ensuring the user's Microsoft toggle is captured.
---
Nitpick comments:
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 263-269: The ternary check on teamId when building
SiteSynchronization is redundant because teamId is already guaranteed non-null
earlier in MicrosoftSynchronizationEnabler (the early return guards it); update
the SiteSynchronization.builder() call to pass teamId directly (use
.teamId(teamId)) and remove the unnecessary ? : fallback to an empty string to
simplify the code and avoid dead branches.
- Around line 327-336: In getSitePropertyNameAndOptionalValueForSynchronization
update the inconsistent whitespace by removing the stray space between the
variable and the method call on configValue (e.g., change "configValue
.isBlank()", "configValue .contains(...)", "configValue .split(...)" and
"configValue .trim()" to "configValue.isBlank()", "configValue.contains(...)",
"configValue.split(...)" and "configValue.trim()"); keep all other logic
unchanged and ensure the method compiles and passes formatting checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ccb020c-0d52-49aa-8b3d-dd386e95bf19
📒 Files selected for processing (24)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieslibrary/src/skins/default/src/sass/modules/tool/sitemanage/_sitemanage.scssmicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.javamicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.htmlmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftCommonService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftSynchronizationService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/persistence/MicrosoftSiteSynchronizationRepository.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/jobs/RunSynchronizationsJob.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/persistence/MicrosoftSiteSynchronizationRepositoryImpl.javasite-manage/site-manage-tool/tool/pom.xmlsite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javasite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-addRemoveFeatureConfirm.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm
Comment on lines
+121
to
123
| //get all enabled synchronizations | ||
| List<SiteSynchronization> ssList = microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false); | ||
|
|
Contributor
There was a problem hiding this comment.
Auto-config should exclude existing synchronizations regardless of disabled state.
This code now filters out only enabled synchronizations before building candidate site/team maps. Disabled synchronizations still exist in persistence, so they can be re-proposed and create duplicate/conflicting relationships.
🛠️ Suggested direction
-List<SiteSynchronization> ssList = microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false);
+// Use a method that returns all persisted synchronizations (enabled + disabled)
+List<SiteSynchronization> ssList = microsoftSynchronizationService.getAllSiteSynchronizations(false);If getAllSiteSynchronizations(false) no longer exists, add an explicit “includeDisabled” API path and use it here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java`
around lines 121 - 123, The AutoConfigController currently calls
microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false), which
only returns enabled synchronizations and allows disabled records to be
re-proposed; change the call to retrieve all synchronizations including disabled
(e.g., use a new or existing method like getAllSiteSynchronizations(true) or add
an includeDisabled parameter to microsoftSynchronizationService and call that)
so the construction of candidate site/team maps excludes any existing
synchronization regardless of disabled state; update usages in
AutoConfigController (the ssList variable and any downstream logic that expects
only enabled records) to handle the full set accordingly.
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (2)
243-245: Silent failure whenunarchiveTeamreturnsfalse.Same pattern as
archiveTeam—if unarchiving fails silently, there's no indication in the logs.Proposed improvement
try { - if (microsoftCommonService.unarchiveTeam(existingSs.getTeamId())) { + boolean unarchived = microsoftCommonService.unarchiveTeam(existingSs.getTeamId()); + if (unarchived) { log.info("Microsoft Team unarchived for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); + } else { + log.warn("Microsoft Team could not be unarchived (returned false) for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 243 - 245, The unarchiveTeam call in MicrosoftSynchronizationEnabler currently logs only on success, so failures are silent; update the code around microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) (mirroring how archiveTeam failures are handled) to check the false return and emit a warning or error log including site.getId() and existingSs.getTeamId() (and a brief context message) so unarchive failures are recorded in the logs.
163-165: Silent failure whenarchiveTeamreturnsfalse.If
archiveTeam()returnsfalse(indicating the team was not archived), no warning is logged. Consider logging at warn level when the operation doesn't succeed, to aid troubleshooting.Proposed improvement
try { - if (microsoftCommonService.archiveTeam(ss.getTeamId())) { + boolean archived = microsoftCommonService.archiveTeam(ss.getTeamId()); + if (archived) { log.info("Microsoft Team archived for site: {}, teamId: {}", site.getId(), ss.getTeamId()); + } else { + log.warn("Microsoft Team could not be archived (returned false) for site: {}, teamId: {}", site.getId(), ss.getTeamId()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 163 - 165, The current call to microsoftCommonService.archiveTeam(ss.getTeamId()) only logs on success; add an else branch to log a warning when archiveTeam(...) returns false so failures are visible — e.g. in MicrosoftSynchronizationEnabler where you call microsoftCommonService.archiveTeam(ss.getTeamId()), add an else that calls log.warn(...) including contextual identifiers like site.getId() and ss.getTeamId() and a short message indicating the team was not archived.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 167-171: The site property removal
(props.removeProperty(nameAndValueForSynchronization[0])) is currently inside
the loop and exception handler that iterates SiteSynchronization records and
calls archiveTeam(), causing the property to be skipped if the first
archiveTeam() throws; move the removal out of the loop and outside any try/catch
so that after processing all SiteSynchronization entries (the loop that invokes
archiveTeam()) you always call props.removeProperty(...) and log the removal
(using nameAndValueForSynchronization[0] and site.getId()) once, ensuring it
runs regardless of per-team exceptions.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Around line 15944-15945: The continue handler currently calls
getStateSite(state) and
MicrosoftSynchronizationEnabler.applySettingsToState(state, params) but omits
applying SubNav changes, causing SubNav modifications to be lost before save;
add a call to SubNavEnabler.applySettingsToState(state, params) in the same
handler (near where MicrosoftSynchronizationEnabler.applySettingsToState is
invoked) so that SubNav state is updated on the state object before
SubNavEnabler.prepareSiteForSave(...) runs during save.
---
Nitpick comments:
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 243-245: The unarchiveTeam call in MicrosoftSynchronizationEnabler
currently logs only on success, so failures are silent; update the code around
microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) (mirroring how
archiveTeam failures are handled) to check the false return and emit a warning
or error log including site.getId() and existingSs.getTeamId() (and a brief
context message) so unarchive failures are recorded in the logs.
- Around line 163-165: The current call to
microsoftCommonService.archiveTeam(ss.getTeamId()) only logs on success; add an
else branch to log a warning when archiveTeam(...) returns false so failures are
visible — e.g. in MicrosoftSynchronizationEnabler where you call
microsoftCommonService.archiveTeam(ss.getTeamId()), add an else that calls
log.warn(...) including contextual identifiers like site.getId() and
ss.getTeamId() and a short message indicating the team was not archived.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 207c707c-0ba5-4c95-a7d6-25201b1f55f8
📒 Files selected for processing (6)
microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javasite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java
✅ Files skipped from review due to trivial changes (2)
- site-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.properties
- site-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java
Comment on lines
+167
to
+171
| // Remove site property defined in microsoft.forced.synchronization.propertynameandoptionalvalue | ||
| if (nameAndValueForSynchronization.length > 0) { | ||
| props.removeProperty(nameAndValueForSynchronization[0]); | ||
| log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); | ||
| } |
Contributor
There was a problem hiding this comment.
Property removal logic is inside the loop and exception handler, causing inconsistent behavior.
If there are multiple SiteSynchronization records and the first archiveTeam() call throws an exception, the site property is never removed even though other teams might be successfully archived. The property removal should be moved outside the loop and exception handling to ensure consistency.
Proposed fix: move property removal outside the loop
if (ssList != null) {
+ // Remove site property outside of loop to ensure consistency
+ if (nameAndValueForSynchronization.length > 0) {
+ props.removeProperty(nameAndValueForSynchronization[0]);
+ log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId());
+ }
for (SiteSynchronization ss : ssList) {
if (ss.getTeamId() != null && !ss.getTeamId().isEmpty()) {
try {
if (microsoftCommonService.archiveTeam(ss.getTeamId())) {
log.info("Microsoft Team archived for site: {}, teamId: {}", site.getId(), ss.getTeamId());
}
-
- // Remove site property defined in microsoft.forced.synchronization.propertynameandoptionalvalue
- if (nameAndValueForSynchronization.length > 0) {
- props.removeProperty(nameAndValueForSynchronization[0]);
- log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId());
- }
} catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`
around lines 167 - 171, The site property removal
(props.removeProperty(nameAndValueForSynchronization[0])) is currently inside
the loop and exception handler that iterates SiteSynchronization records and
calls archiveTeam(), causing the property to be skipped if the first
archiveTeam() throws; move the removal out of the loop and outside any try/catch
so that after processing all SiteSynchronization entries (the loop that invokes
archiveTeam()) you always call props.removeProperty(...) and log the removal
(using nameAndValueForSynchronization[0] and site.getId()) once, ensuring it
runs regardless of per-team exceptions.
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java (1)
1013-1041:⚠️ Potential issue | 🟠 MajorFail loudly on archive/unarchive errors to prevent synchronization state drift.
This still swallows non-credential exceptions and returns
false, which can be ignored by callers and leave Sakai state out of sync with actual Teams archive state.Proposed fix
public boolean archiveTeam(String teamId) throws MicrosoftCredentialsException { try { getGraphClient().teams(teamId) .archive(null) .buildRequest() .post(); return true; } catch(MicrosoftCredentialsException e) { throw e; - } catch(Exception ex){ - log.debug("Error archiving Microsoft team: id={}", teamId); + } catch(Exception ex){ + log.error("Error archiving Microsoft team: id={}", teamId, ex); + throw new IllegalStateException("Failed to archive Microsoft team: " + teamId, ex); } - return false; } public boolean unarchiveTeam(String teamId) throws MicrosoftCredentialsException { try { getGraphClient().teams(teamId) .unarchive() .buildRequest() .post(); return true; } catch (MicrosoftCredentialsException e) { throw e; } catch (Exception ex) { - log.debug("Error unarchiving Microsoft team: id={}", teamId); + log.error("Error unarchiving Microsoft team: id={}", teamId, ex); + throw new IllegalStateException("Failed to unarchive Microsoft team: " + teamId, ex); } - return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java` around lines 1013 - 1041, The archiveTeam and unarchiveTeam methods currently catch generic Exception, log at debug, and return false which hides failures; instead remove the generic swallow and rethrow the unexpected exceptions (or wrap them) so callers cannot ignore failures: in archiveTeam and unarchiveTeam keep the MicrosoftCredentialsException catch as-is, but replace the generic catch(Exception ex) blocks with throwing a RuntimeException (or a specific unchecked wrapper) that includes the original exception and teamId and log it at error level via log.error(..., ex) before rethrowing; update both methods (archiveTeam, unarchiveTeam) to ensure the original exception is propagated rather than returning false.microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java (1)
121-123:⚠️ Potential issue | 🟠 MajorExclude disabled synchronizations when building auto-config candidates.
Line 122 now loads only enabled synchronizations, so disabled-but-existing links are not removed from
sitesMap/teamsMapand can be proposed again in auto-config.🛠️ Suggested fix
-//get all enabled synchronizations -List<SiteSynchronization> ssList = microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false); +// get all persisted synchronizations (enabled + disabled) to avoid reproposing existing links +List<SiteSynchronization> ssList = microsoftSynchronizationService.getAllSiteSynchronizations(false);If
getAllSiteSynchronizations(false)no longer exists, add an API variant that includes disabled rows and use it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java` around lines 121 - 123, Auto-config currently calls microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false) in AutoConfigController which omits disabled synchronizations so sitesMap/teamsMap still contain disabled links and they are re-proposed; fix by calling a service method that returns all synchronizations (enabled+disabled) when building auto-config candidates—either use an existing getAllSiteSynchronizations(includeDisabled) variant or add one on microsoftSynchronizationService (e.g., getAllSiteSynchronizations(true)) and replace the getAllEnabledSiteSynchronizations call in AutoConfigController so disabled rows are considered and removed from sitesMap/teamsMap before proposing candidates.microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html (1)
95-98:⚠️ Potential issue | 🟠 MajorAdd an explicit accessible name for the enable/disable action.
This is an icon-only control and currently depends on
title; addaria-label(and hidden text) so assistive technologies can announce the action reliably.♿ Suggested fix
-<a href="#" onclick="toggleDisabled(this)" th:title="${row.disabled} ? #{index.enable} : #{index.disable}"> - <i th:class="${row.disabled} ? 'fa fa-toggle-off' : 'fa fa-toggle-on'"></i> +<a href="#" + onclick="toggleDisabled(this); return false;" + th:title="${row.disabled} ? #{index.enable} : #{index.disable}" + th:aria-label="${row.disabled} ? #{index.enable} : #{index.disable}"> + <i th:class="${row.disabled} ? 'fa fa-toggle-off' : 'fa fa-toggle-on'" aria-hidden="true"></i> + <span class="sr-only" th:text="${row.disabled} ? #{index.enable} : #{index.disable}"></span> </a>As per coding guidelines, "
**/*.{js,html,jsp,vm,xhtml,xml}: Follow accessibility best practices".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html` around lines 95 - 98, The enable/disable anchor (onclick="toggleDisabled(this)") is an icon-only control and must have an explicit accessible name; add an aria-label on the <a> using the same Thymeleaf conditional as the title (e.g. th:attr="aria-label=${row.disabled} ? #{index.enable} : #{index.disable}") and also include a visually-hidden span inside the anchor with th:text="${row.disabled} ? #{index.enable} : #{index.disable}" so assistive tech announces the action reliably while preserving the existing <i> icon and toggleDisabled(this) handler.microsoft-integration/admin-tool/src/main/resources/Messages_es.properties (1)
225-225:⚠️ Potential issue | 🟡 MinorUse a neutral error message for toggle failures.
This message is used for both enable and disable flows, so wording focused only on “deshabilitar” can be misleading.
💡 Proposed wording update
-error.set_disabled_synchronization=Error al deshabilitar la sincronización +error.set_disabled_synchronization=Error al cambiar el estado de la sincronización🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/resources/Messages_es.properties` at line 225, The message value for the key error.set_disabled_synchronization is specific to disabling but is used for both enable and disable flows; update the value in Messages_es.properties to a neutral, toggle-agnostic Spanish string (e.g., "Error al cambiar el estado de sincronización" or similar) so it accurately reflects failures for both enabling and disabling synchronization while retaining the same property key.site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java (1)
15943-15945:⚠️ Potential issue | 🟠 MajorSubNav changes are still dropped in this continue handler.
SubNavEnabler.addToContext(...)renders this setting on the edit page, andSubNavEnabler.prepareSiteForSave(...)persists from session state, but this handler still never copies the submitted SubNav value intostate. A SubNav change made on this screen will be lost before save unlessSubNavEnabler.applySettingsToState(state, params)is added here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java` around lines 15943 - 15945, The continue handler in SiteAction is not copying submitted SubNav into state, so add a call to SubNavEnabler.applySettingsToState(state, params) alongside the other enabler calls; locate the block where MathJaxEnabler.applySettingsToState(state, params) and MicrosoftSynchronizationEnabler.applySettingsToState(state, params, site) are invoked and insert SubNavEnabler.applySettingsToState(state, params) (e.g., immediately after the MathJax call) so SubNavEnabler.prepareSiteForSave(...) will have the updated value to persist.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`:
- Around line 122-125: The fetch currently builds a GET URL (variables url,
rowId, isDisabled) and calls fetch(url) which mutates state; change this to send
a POST: use fetch(urlBase + rowId, { method: "POST", headers: { "Content-Type":
"application/json" }, body: JSON.stringify({ disabled: !isDisabled }) }) (or
application/x-www-form-urlencoded if server expects form data) and update any
controller expectations; also ensure you include the app's CSRF token/header if
one is required by the backend when sending the POST.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 260-263: The null check for teamId in
MicrosoftSynchronizationEnabler around the createTeam() result only checks for
null but must also reject empty strings; update the conditional that currently
tests "teamId == null" to treat both null and empty (e.g. "teamId == null ||
teamId.isEmpty()" or use StringUtils.isBlank(teamId)) so an empty teamId from
createTeam() is handled the same way (log the warning and return) and is not
saved later.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Line 1865: The MicrosoftSynchronizationEnabler.addToContext(context, site)
call renders the toggle from the persistent Site object, which causes validation
redisplay or the confirm screen to show the pre-change value instead of the
user's in-flight choice saved in SessionState; update the editable/confirm
screen rendering to read the Microsoft synchronization value from SessionState
(the same SessionState attribute used when capturing the user's choice) and add
that value into the context instead of (or overriding) the value taken from
site—follow the same state-backed context helper pattern used on the
confirmation path where the SessionState-backed value is injected into the
context before rendering.
---
Duplicate comments:
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java`:
- Around line 121-123: Auto-config currently calls
microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false) in
AutoConfigController which omits disabled synchronizations so sitesMap/teamsMap
still contain disabled links and they are re-proposed; fix by calling a service
method that returns all synchronizations (enabled+disabled) when building
auto-config candidates—either use an existing
getAllSiteSynchronizations(includeDisabled) variant or add one on
microsoftSynchronizationService (e.g., getAllSiteSynchronizations(true)) and
replace the getAllEnabledSiteSynchronizations call in AutoConfigController so
disabled rows are considered and removed from sitesMap/teamsMap before proposing
candidates.
In `@microsoft-integration/admin-tool/src/main/resources/Messages_es.properties`:
- Line 225: The message value for the key error.set_disabled_synchronization is
specific to disabling but is used for both enable and disable flows; update the
value in Messages_es.properties to a neutral, toggle-agnostic Spanish string
(e.g., "Error al cambiar el estado de sincronización" or similar) so it
accurately reflects failures for both enabling and disabling synchronization
while retaining the same property key.
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html`:
- Around line 95-98: The enable/disable anchor (onclick="toggleDisabled(this)")
is an icon-only control and must have an explicit accessible name; add an
aria-label on the <a> using the same Thymeleaf conditional as the title (e.g.
th:attr="aria-label=${row.disabled} ? #{index.enable} : #{index.disable}") and
also include a visually-hidden span inside the anchor with
th:text="${row.disabled} ? #{index.enable} : #{index.disable}" so assistive tech
announces the action reliably while preserving the existing <i> icon and
toggleDisabled(this) handler.
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java`:
- Around line 1013-1041: The archiveTeam and unarchiveTeam methods currently
catch generic Exception, log at debug, and return false which hides failures;
instead remove the generic swallow and rethrow the unexpected exceptions (or
wrap them) so callers cannot ignore failures: in archiveTeam and unarchiveTeam
keep the MicrosoftCredentialsException catch as-is, but replace the generic
catch(Exception ex) blocks with throwing a RuntimeException (or a specific
unchecked wrapper) that includes the original exception and teamId and log it at
error level via log.error(..., ex) before rethrowing; update both methods
(archiveTeam, unarchiveTeam) to ensure the original exception is propagated
rather than returning false.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Around line 15943-15945: The continue handler in SiteAction is not copying
submitted SubNav into state, so add a call to
SubNavEnabler.applySettingsToState(state, params) alongside the other enabler
calls; locate the block where MathJaxEnabler.applySettingsToState(state, params)
and MicrosoftSynchronizationEnabler.applySettingsToState(state, params, site)
are invoked and insert SubNavEnabler.applySettingsToState(state, params) (e.g.,
immediately after the MathJax call) so SubNavEnabler.prepareSiteForSave(...)
will have the updated value to persist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19e4e61b-a5e7-44b7-b97f-42dc9d1f7422
📒 Files selected for processing (24)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieslibrary/src/skins/default/src/sass/modules/tool/sitemanage/_sitemanage.scssmicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.javamicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.htmlmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftCommonService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftSynchronizationService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/persistence/MicrosoftSiteSynchronizationRepository.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/jobs/RunSynchronizationsJob.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/persistence/MicrosoftSiteSynchronizationRepositoryImpl.javasite-manage/site-manage-tool/tool/pom.xmlsite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javasite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-addRemoveFeatureConfirm.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/MicrosoftSynchronizationServiceImpl.java (1)
199-209:⚠️ Potential issue | 🟠 MajorDisabled rows still participate in live synchronization paths.
This enabled-only lookup only helps callers that use
getAllEnabledSiteSynchronizations(). In the same class, hook-driven flows likegroupCreated,userAddedToSite,userRemovedFromSite, and the group membership handlers still queryfindBySite(...)/findByGroup(...)without checkingss.isDisabled(), so turning a synchronization off will still create channels and push membership updates. Please gate those paths on!ss.isDisabled()or introduce enabled-only site/group lookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.java` around lines 199 - 209, The hook-driven handlers (groupCreated, userAddedToSite, userRemovedFromSite and the group membership handlers) currently call microsoftSiteSynchronizationRepository.findBySite(...) / findByGroup(...) and do not ignore disabled synchronizations, so disabled SiteSynchronization rows still create channels and push updates; update those handlers to filter out disabled syncs (check !ss.isDisabled()) after fetching or change/introduce repository methods like findEnabledBySite(...) / findEnabledByGroup(...) and use those instead (or apply a stream().filter(ss -> !ss.isDisabled()) before processing), ensuring the code paths that create channels or push membership updates only operate on enabled SiteSynchronization instances and reuse the existing ss.isDisabled() flag.
♻️ Duplicate comments (5)
microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html (1)
122-125:⚠️ Potential issue | 🔴 CriticalSend this toggle request as POST or the new action will fail.
updateSiteSynchronizationDisabled(...)is now a@PostMapping, but Line 124 still uses the Fetch defaultGET. That means Line 125 can receive a 405/non-JSON response andresponse.json()will throw before the UI can recover.🔧 Minimal fix
- let response = await fetch(url); + let response = await fetch(url, { + method: 'POST', + headers: { + 'Accept': 'application/json', + }, + });Verify the verb mismatch by comparing the frontend call with the controller mapping. Expected result:
index.htmlshowsfetch(url)with nomethod, whileMainController.javashows@PostMappingfor the same endpoint.#!/bin/bash set -euo pipefail echo "== toggleDisabled frontend call ==" sed -n '116,126p' microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html echo echo "== disabled endpoint mapping ==" sed -n '338,345p' microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html` around lines 122 - 125, The frontend is sending a GET to updateSiteSynchronizationDisabled (now a `@PostMapping`), causing a 405 and response.json() to throw; update the fetch call in index.html (the let response = await fetch(url) / let data = await response.json() sequence) to use POST by calling fetch(url, { method: 'POST' }) and optionally include appropriate headers/body if the controller expects form or JSON payload, and add a safety check on response.ok before calling response.json() to avoid throwing on non-JSON error responses.microsoft-integration/admin-tool/src/main/resources/Messages_es.properties (1)
225-225:⚠️ Potential issue | 🟡 MinorMake this error message neutral for both directions.
This key is returned for both enable and disable failures, so “deshabilitar” is misleading when the action that failed was enabling the synchronization.
💬 Suggested wording
-error.set_disabled_synchronization=Error al deshabilitar la sincronización +error.set_disabled_synchronization=Error al cambiar el estado de la sincronización🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/resources/Messages_es.properties` at line 225, The property value for key error.set_disabled_synchronization is misleading for enable failures; update the Spanish message to a neutral phrase (e.g., "Error al cambiar el estado de la sincronización" or "Error al actualizar la sincronización") so it applies to both enable and disable failures, leaving the key name error.set_disabled_synchronization unchanged; locate this entry in Messages_es.properties and replace the current value "Error al deshabilitar la sincronizaci\u00f3n" with the chosen neutral wording.site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (3)
172-173:⚠️ Potential issue | 🟡 MinorPass the
Throwableto SLF4J directly.Using
e.toString()here drops the stack trace, and thewarncalls add an extra{}just to print the exception text.As per coding guidelines, "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))".Suggested fix
- log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), ss.getTeamId(), e.toString()); + log.warn("Could not archive Microsoft Team for site: {}, teamId: {}", site.getId(), ss.getTeamId(), e); - log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), existingSs.getTeamId(), e.toString()); + log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}", site.getId(), existingSs.getTeamId(), e); - log.error("Microsoft credentials error while creating team for site: {}, {}", site.getId(), e.toString()); + log.error("Microsoft credentials error while creating team for site: {}", site.getId(), e); - log.error("Unexpected error while creating Microsoft Team for site: {}, {}", site.getId(), e.toString()); + log.error("Unexpected error while creating Microsoft Team for site: {}", site.getId(), e);Also applies to: 246-247, 276-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 172 - 173, The warning log in MicrosoftSynchronizationEnabler's catch blocks currently uses e.toString() and an extra {} placeholder; change each log.warn call to pass the Throwable directly as the last parameter (e.g., replace the call inside the catch(Exception e) in the archive block and the similar occurrences around lines referenced 246-247 and 276-279) so SLF4J receives the Throwable and can print the stack trace: keep the same message and parameter order for site.getId() and ss.getTeamId(), but pass e (the Throwable) instead of e.toString() or an extra "{}" placeholder.
156-180:⚠️ Potential issue | 🟠 MajorAlways clear the extra site property when disabling.
props.removeProperty(nameAndValueForSynchronization[0])only runs inside the per-teamtry. If the site has no synchronizations, a row has noteamId, orarchiveTeam()throws, the forced-sync property is left behind even though the toggle was turned off. Move that removal outside the loop/tryso disable is idempotent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 156 - 180, The removal of the forced-sync site property is only done inside the per-team try block, so props.removeProperty(nameAndValueForSynchronization[0]) can be skipped when there are no teams, no teamId, or archiveTeam() throws; move the property removal so it always runs when disabling synchronization: after iterating and disabling SiteSynchronization (ss.setDisabled(true) and microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss)) and/or after the ssList loop, call props.removeProperty(nameAndValueForSynchronization[0]) (guarded by nameAndValueForSynchronization.length > 0) so the property is cleared regardless of archiveTeam() success and the toggle is idempotent.
260-263:⚠️ Potential issue | 🟡 MinorReject empty team IDs from
createTeam().This guard only checks
null, but every other branch in this class treats emptyteamIdas invalid. IfcreateTeam()returns"", this will persist aSiteSynchronizationthat later cannot be archived or restored.Suggested fix
- if (teamId == null) { + if (teamId == null || teamId.isEmpty()) { log.warn("Microsoft Team creation returned null teamId for site: {}", site.getId()); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 260 - 263, The null-only check for teamId in MicrosoftSynchronizationEnabler allows empty strings through; update the guard after createTeam() to reject both null and empty team IDs (e.g., if (teamId == null || teamId.isEmpty()) ), log a warn including site.getId() and the problematic teamId, and return without persisting a SiteSynchronization so empty team IDs are not stored.
🧹 Nitpick comments (2)
microsoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.java (1)
77-78: Consider applying the same fix to theforcedfield.The past review noted that the
forcedfield (line 77-78) has the same potential issue—it lacksnullable = falseand@Builder.Default, and is also queried with equality checks. Consider adding these annotations for consistency:- `@Column`(name = "forced") - private boolean forced; + `@Column`(name = "forced", nullable = false) + `@Builder.Default` + private boolean forced = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.java` around lines 77 - 78, The `forced` field in SiteSynchronization should be made non-nullable and given a builder default so equality queries behave consistently; update the field declaration in class SiteSynchronization to add `@Column`(name = "forced", nullable = false) and `@Builder.Default`, initializing it to false (e.g., `@Builder.Default` private boolean forced = false) so Lombok's builder preserves the default and the DB column is non-nullable.config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties (1)
5349-5349: Minor doc wording fix: “sites type” → “site types”.Please update the comment text for readability and consistency in admin-facing configuration docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties` at line 5349, Update the inline comment that reads '#DEFAULT: none, use "all" for all sites or specify a list of sites type to allow. Comma-separated list of sitetype: course,project,...' by changing "sites type" to "site types" (and optionally "sitetype" to "site type" for consistency) so the comment reads clearly for admins; locate and edit the comment string in the default.sakai.properties block where that property is defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 258-279: The code logs but swallows exceptions after calling
microsoftCommonService.createTeam(...), which can leave an external Team created
while saveOrUpdateSiteSynchronization(ss) fails and later retries recreate a
duplicate Team; update MicrosoftSynchronizationEnabler so that after
createTeam() succeeds you either (a) make saveOrUpdateSiteSynchronization(ss)
atomic by propagating exceptions (rethrow after logging) to abort the outer save
and trigger compensating logic, or (b) perform compensating rollback on failure
by calling a cleanup method (e.g., microsoftCommonService.deleteTeam(teamId))
inside the catch before rethrowing; reference createTeam,
saveOrUpdateSiteSynchronization, microsoftCommonService and SiteSynchronization
to locate the code and ensure any caught Exception is not silently swallowed but
handled with rollback and/or rethrown.
- Around line 240-253: The loop currently re-enables and saves
SiteSynchronization rows even when there is no teamId, unarchiveTeam returns
false, or an exception occurs; change the logic in the for-loop over
existingSsList so that existingSs.setDisabled(false) and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs) are
only executed when microsoftCommonService.unarchiveTeam(existingSs.getTeamId())
succeeds (returns true), and do not touch rows that have null/empty teamId or
when unarchiveTeam throws or returns false; keep logging for failures but avoid
updating/saving the synchronization unless unarchive succeeded.
- Around line 133-146: The code always calls
createOrRestoreMicrosoftTeamForSite(...) whenever STATE_KEY is true, causing
re-provisioning on every save; change the logic to only invoke
createOrRestoreMicrosoftTeamForSite when Microsoft sync is being turned on
(i.e., a transition from not-enabled to enabled). Concretely, before calling
createOrRestoreMicrosoftTeamForSite, check the previous enabled state (either
from the site property SITE_PROPERTY via site.getProperties() or from the
previous STATE_KEY value) and only call createOrRestoreMicrosoftTeamForSite when
previousEnabled is false/null and isEnabled is true; otherwise just update
props.addProperty(SITE_PROPERTY, ...) without re-provisioning. Ensure you
reference STATE_KEY, SITE_PROPERTY, createOrRestoreMicrosoftTeamForSite,
site.getProperties()/site.getPropertiesEdit(), and state.getAttribute(...) to
implement the check.
- Around line 156-180: The code is toggling/archiving every SiteSynchronization
but must only affect the ones created as forced synchronizations; update the
loops in MicrosoftSynchronizationEnabler that iterate ssList so they first check
the forced flag on each SiteSynchronization (e.g., ss.isForced() or
ss.getForced() == true) and only then call
microsoftCommonService.archiveTeam(ss.getTeamId()), props.removeProperty(...),
set ss.setDisabled(true), and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); ensure this
same forced-only guard is applied in all related blocks (the ones that currently
call archiveTeam, removeProperty, and saveOrUpdateSiteSynchronization and the
block that creates the new SiteSynchronization with forced(true)) so
manual/non-forced rows are left untouched and the forced record creation logic
still runs.
---
Outside diff comments:
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.java`:
- Around line 199-209: The hook-driven handlers (groupCreated, userAddedToSite,
userRemovedFromSite and the group membership handlers) currently call
microsoftSiteSynchronizationRepository.findBySite(...) / findByGroup(...) and do
not ignore disabled synchronizations, so disabled SiteSynchronization rows still
create channels and push updates; update those handlers to filter out disabled
syncs (check !ss.isDisabled()) after fetching or change/introduce repository
methods like findEnabledBySite(...) / findEnabledByGroup(...) and use those
instead (or apply a stream().filter(ss -> !ss.isDisabled()) before processing),
ensuring the code paths that create channels or push membership updates only
operate on enabled SiteSynchronization instances and reuse the existing
ss.isDisabled() flag.
---
Duplicate comments:
In `@microsoft-integration/admin-tool/src/main/resources/Messages_es.properties`:
- Line 225: The property value for key error.set_disabled_synchronization is
misleading for enable failures; update the Spanish message to a neutral phrase
(e.g., "Error al cambiar el estado de la sincronización" or "Error al actualizar
la sincronización") so it applies to both enable and disable failures, leaving
the key name error.set_disabled_synchronization unchanged; locate this entry in
Messages_es.properties and replace the current value "Error al deshabilitar la
sincronizaci\u00f3n" with the chosen neutral wording.
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`:
- Around line 122-125: The frontend is sending a GET to
updateSiteSynchronizationDisabled (now a `@PostMapping`), causing a 405 and
response.json() to throw; update the fetch call in index.html (the let response
= await fetch(url) / let data = await response.json() sequence) to use POST by
calling fetch(url, { method: 'POST' }) and optionally include appropriate
headers/body if the controller expects form or JSON payload, and add a safety
check on response.ok before calling response.json() to avoid throwing on
non-JSON error responses.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 172-173: The warning log in MicrosoftSynchronizationEnabler's
catch blocks currently uses e.toString() and an extra {} placeholder; change
each log.warn call to pass the Throwable directly as the last parameter (e.g.,
replace the call inside the catch(Exception e) in the archive block and the
similar occurrences around lines referenced 246-247 and 276-279) so SLF4J
receives the Throwable and can print the stack trace: keep the same message and
parameter order for site.getId() and ss.getTeamId(), but pass e (the Throwable)
instead of e.toString() or an extra "{}" placeholder.
- Around line 156-180: The removal of the forced-sync site property is only done
inside the per-team try block, so
props.removeProperty(nameAndValueForSynchronization[0]) can be skipped when
there are no teams, no teamId, or archiveTeam() throws; move the property
removal so it always runs when disabling synchronization: after iterating and
disabling SiteSynchronization (ss.setDisabled(true) and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss)) and/or
after the ssList loop, call
props.removeProperty(nameAndValueForSynchronization[0]) (guarded by
nameAndValueForSynchronization.length > 0) so the property is cleared regardless
of archiveTeam() success and the toggle is idempotent.
- Around line 260-263: The null-only check for teamId in
MicrosoftSynchronizationEnabler allows empty strings through; update the guard
after createTeam() to reject both null and empty team IDs (e.g., if (teamId ==
null || teamId.isEmpty()) ), log a warn including site.getId() and the
problematic teamId, and return without persisting a SiteSynchronization so empty
team IDs are not stored.
---
Nitpick comments:
In
`@config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties`:
- Line 5349: Update the inline comment that reads '#DEFAULT: none, use "all" for
all sites or specify a list of sites type to allow. Comma-separated list of
sitetype: course,project,...' by changing "sites type" to "site types" (and
optionally "sitetype" to "site type" for consistency) so the comment reads
clearly for admins; locate and edit the comment string in the
default.sakai.properties block where that property is defined.
In
`@microsoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.java`:
- Around line 77-78: The `forced` field in SiteSynchronization should be made
non-nullable and given a builder default so equality queries behave
consistently; update the field declaration in class SiteSynchronization to add
`@Column`(name = "forced", nullable = false) and `@Builder.Default`, initializing it
to false (e.g., `@Builder.Default` private boolean forced = false) so Lombok's
builder preserves the default and the DB column is non-nullable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb291dd8-2014-4c54-89dc-eb0f9bfb5335
📒 Files selected for processing (24)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieslibrary/src/skins/default/src/sass/modules/tool/sitemanage/_sitemanage.scssmicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.javamicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.htmlmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftCommonService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftSynchronizationService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/persistence/MicrosoftSiteSynchronizationRepository.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/jobs/RunSynchronizationsJob.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/persistence/MicrosoftSiteSynchronizationRepositoryImpl.javasite-manage/site-manage-tool/tool/pom.xmlsite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javasite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-addRemoveFeatureConfirm.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm
Comment on lines
+133
to
+146
| if (state.getAttribute(STATE_KEY) != null) { | ||
| final boolean isEnabled = (Boolean) state.getAttribute(STATE_KEY); | ||
| final ResourcePropertiesEdit props = site.getPropertiesEdit(); | ||
| final String[] nameAndValueForSynchronization = getSitePropertyNameAndOptionalValueForSynchronization(); | ||
| MicrosoftCommonService microsoftCommonService = (MicrosoftCommonService) ComponentManager.get(MicrosoftCommonService.class); | ||
| MicrosoftSynchronizationService microsoftSynchronizationService = (MicrosoftSynchronizationService) ComponentManager.get(MicrosoftSynchronizationService.class); | ||
|
|
||
| if (isEnabled) { | ||
| props.addProperty(SITE_PROPERTY, Boolean.TRUE.toString()); | ||
|
|
||
| MicrosoftConfigurationService microsoftConfigurationService = (MicrosoftConfigurationService) ComponentManager.get(MicrosoftConfigurationService.class); | ||
|
|
||
| // Check if MicrosoftSynchronization was already enabled for the site before creating/restoring the Team | ||
| createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService); |
Contributor
There was a problem hiding this comment.
Avoid re-provisioning on every save.
STATE_KEY only captures the current checkbox value, so this branch reruns createOrRestoreMicrosoftTeamForSite(...) every time an already-enabled site is saved. That keeps reissuing unarchive calls and resetting syncDateFrom/syncDateTo for existing synchronizations even when Microsoft sync did not actually change.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`
around lines 133 - 146, The code always calls
createOrRestoreMicrosoftTeamForSite(...) whenever STATE_KEY is true, causing
re-provisioning on every save; change the logic to only invoke
createOrRestoreMicrosoftTeamForSite when Microsoft sync is being turned on
(i.e., a transition from not-enabled to enabled). Concretely, before calling
createOrRestoreMicrosoftTeamForSite, check the previous enabled state (either
from the site property SITE_PROPERTY via site.getProperties() or from the
previous STATE_KEY value) and only call createOrRestoreMicrosoftTeamForSite when
previousEnabled is false/null and isEnabled is true; otherwise just update
props.addProperty(SITE_PROPERTY, ...) without re-provisioning. Ensure you
reference STATE_KEY, SITE_PROPERTY, createOrRestoreMicrosoftTeamForSite,
site.getProperties()/site.getPropertiesEdit(), and state.getAttribute(...) to
implement the check.
Comment on lines
+156
to
+180
| // If a SiteSynchronization exists with a Team, archive it and delete it | ||
| if (microsoftSynchronizationService != null && microsoftCommonService != null) { | ||
| List<SiteSynchronization> ssList = microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()); | ||
| if (ssList != null) { | ||
| for (SiteSynchronization ss : ssList) { | ||
| if (ss.getTeamId() != null && !ss.getTeamId().isEmpty()) { | ||
| try { | ||
| if (microsoftCommonService.archiveTeam(ss.getTeamId())) { | ||
| log.info("Microsoft Team archived for site: {}, teamId: {}", site.getId(), ss.getTeamId()); | ||
| } | ||
|
|
||
| // Remove site property defined in microsoft.forced.synchronization.propertynameandoptionalvalue | ||
| if (nameAndValueForSynchronization.length > 0) { | ||
| props.removeProperty(nameAndValueForSynchronization[0]); | ||
| log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); | ||
| } | ||
| } catch (Exception e) { | ||
| log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), ss.getTeamId(), e.toString()); | ||
| } | ||
| } | ||
|
|
||
| ss.setDisabled(true); | ||
| microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); | ||
| log.info("SiteSynchronization disabled for site: {}", site.getId()); | ||
| } |
Contributor
There was a problem hiding this comment.
Scope this toggle to forced synchronizations only.
Both branches operate on every SiteSynchronization for the site, but the record created here is explicitly marked forced(true) on Lines 265-271. As written, toggling the site-level option can archive/restore unrelated manual Teams and can skip creating the new forced synchronization when manual rows already exist.
Also applies to: 237-255, 265-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`
around lines 156 - 180, The code is toggling/archiving every SiteSynchronization
but must only affect the ones created as forced synchronizations; update the
loops in MicrosoftSynchronizationEnabler that iterate ssList so they first check
the forced flag on each SiteSynchronization (e.g., ss.isForced() or
ss.getForced() == true) and only then call
microsoftCommonService.archiveTeam(ss.getTeamId()), props.removeProperty(...),
set ss.setDisabled(true), and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); ensure this
same forced-only guard is applied in all related blocks (the ones that currently
call archiveTeam, removeProperty, and saveOrUpdateSiteSynchronization and the
block that creates the new SiteSynchronization with forced(true)) so
manual/non-forced rows are left untouched and the forced record creation logic
still runs.
Comment on lines
+258
to
+279
| final String teamId = microsoftCommonService.createTeam(site.getTitle(), credentials.getEmail()); | ||
|
|
||
| if (teamId == null) { | ||
| log.warn("Microsoft Team creation returned null teamId for site: {}", site.getId()); | ||
| return; | ||
| } | ||
|
|
||
| final SiteSynchronization ss = SiteSynchronization.builder() | ||
| .siteId(site.getId()) | ||
| .teamId(teamId) | ||
| .forced(true) | ||
| .syncDateFrom(syncDateFrom) | ||
| .syncDateTo(syncDateTo) | ||
| .build(); | ||
|
|
||
| microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); | ||
| log.info("Microsoft Team created and SiteSynchronization saved: teamId={}, siteId={}", teamId, site.getId()); | ||
| } | ||
| } catch (MicrosoftCredentialsException e) { | ||
| log.error("Microsoft credentials error while creating team for site: {}, {}", site.getId(), e.toString()); | ||
| } catch (Exception e) { | ||
| log.error("Unexpected error while creating Microsoft Team for site: {}, {}", site.getId(), e.toString()); |
Contributor
There was a problem hiding this comment.
Don't swallow post-createTeam() failures.
createTeam() is a non-idempotent external write, but any later exception in this branch is only logged. If saveOrUpdateSiteSynchronization(ss) fails after the Team is created, the site save continues with no local record and the next retry can create a second Team for the same site.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`
around lines 258 - 279, The code logs but swallows exceptions after calling
microsoftCommonService.createTeam(...), which can leave an external Team created
while saveOrUpdateSiteSynchronization(ss) fails and later retries recreate a
duplicate Team; update MicrosoftSynchronizationEnabler so that after
createTeam() succeeds you either (a) make saveOrUpdateSiteSynchronization(ss)
atomic by propagating exceptions (rethrow after logging) to abort the outer save
and trigger compensating logic, or (b) perform compensating rollback on failure
by calling a cleanup method (e.g., microsoftCommonService.deleteTeam(teamId))
inside the catch before rethrowing; reference createTeam,
saveOrUpdateSiteSynchronization, microsoftCommonService and SiteSynchronization
to locate the code and ensure any caught Exception is not silently swallowed but
handled with rollback and/or rethrown.
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (4)
279-285:⚠️ Potential issue | 🔴 CriticalDon't log-and-return after the external create succeeds.
If Line 260 creates the Team and Line 279 fails, Lines 284-285 swallow the error and the next retry can create a second Team for the same site. Once
createTeam()succeeds, abort the save or do compensating cleanup instead of just logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 279 - 285, The code in MicrosoftSynchronizationEnabler currently logs and returns on exceptions after createTeam() succeeds, which can leave an external Team created but internal SiteSynchronization not saved leading to duplicate Teams on retry; update the try/catch around the createTeam() and microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss) so that if createTeam() succeeds but saveOrUpdateSiteSynchronization(ss) fails you either perform compensating cleanup by calling the Microsoft API to delete the newly created Team (using the teamId returned by createTeam()), or propagate the exception so the caller can handle rollback; modify the catch blocks around createTeam() and saveOrUpdateSiteSynchronization(ss) to access the teamId and invoke the deletion/cleanup via the Microsoft client or rethrow the exception instead of merely logging (reference methods: createTeam(), microsoftSynchronizationService.saveOrUpdateSiteSynchronization, and class MicrosoftSynchronizationEnabler).
133-146:⚠️ Potential issue | 🟠 MajorOnly provision on the disabled→enabled transition.
This still calls
createOrRestoreMicrosoftTeamForSite(...)on every save while the checkbox remains checked, so existing sites keep reissuing restore calls and resetting sync windows. Capture the previous site property before Line 141 and only provision when the site is moving from disabled to enabled.Suggested change
+ final boolean wasEnabled = isEnabledForSite(site); if (isEnabled) { props.addProperty(SITE_PROPERTY, Boolean.TRUE.toString()); @@ - createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService); + if (!wasEnabled) { + createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 133 - 146, Capture the site's prior sync property and only call createOrRestoreMicrosoftTeamForSite(...) when transitioning from disabled to enabled: read the existing property (e.g., via site.getProperties().getProperty(SITE_PROPERTY) or props.getProperty(SITE_PROPERTY)) into a boolean prevEnabled before you add/update the property, then if (!prevEnabled && isEnabled) invoke createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService); otherwise skip provisioning and still persist the SITE_PROPERTY via props.addProperty as currently done.
237-255:⚠️ Potential issue | 🟠 MajorRestore only the forced synchronization, and only after the Team is actually back.
Any existing row sends the code down this branch, so manual rows can prevent the create path from ever running. Then Lines 250-253 re-enable/save the row even when
teamIdis missing orunarchiveTeam()returnsfalse—andmicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javaalready normalizes generic failures tofalse. FilterexistingSsListto forced rows first; if none exist, fall through to create a new forced synchronization, and only save after a successful unarchive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 237 - 255, Filter existingSsList to prioritize forced SiteSynchronization rows (e.g., check a forced flag on SiteSynchronization) and only restore (set syncDateFrom/syncDateTo, setDisabled(false) and call microsoftSynchronizationService.saveOrUpdateSiteSynchronization) after confirming the Team is actually back: require existingSs.getTeamId() non-empty and microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) returned true; if no forced row succeeds, do not re-enable any manual rows and fall through to the create-new-forced-synchronization path so creation can proceed. Ensure you only call saveOrUpdateSiteSynchronization for the specific SiteSynchronization that was successfully unarchived.
156-180:⚠️ Potential issue | 🟠 MajorRestrict the disable path to forced rows, and remove the aux property once.
This branch currently archives/disables every
SiteSynchronizationon the site, even though this feature only provisions forced rows. Also, the extra site property is only removed if a per-team archive reaches Lines 167-170. Remove that property once in the disable flow regardless of team archival, and leave non-forced rows untouched.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 156 - 180, Only disable and update SiteSynchronization rows that are the "forced" ones instead of all rows: before iterating ssList, if nameAndValueForSynchronization.length > 0 remove the aux site property once via props.removeProperty(nameAndValueForSynchronization[0]) and log it, then in the loop only operate on a SiteSynchronization ss when it matches the forced criterion (e.g., contains the property nameAndValueForSynchronization[0] and, if a value is provided in nameAndValueForSynchronization[1], the value matches); for matching rows perform the team archive via microsoftCommonService.archiveTeam(ss.getTeamId()) and then call ss.setDisabled(true) and microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); leave non-forced ss rows untouched and do not call props.removeProperty inside the per-ss try/catch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 279-285: The code in MicrosoftSynchronizationEnabler currently
logs and returns on exceptions after createTeam() succeeds, which can leave an
external Team created but internal SiteSynchronization not saved leading to
duplicate Teams on retry; update the try/catch around the createTeam() and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss) so that if
createTeam() succeeds but saveOrUpdateSiteSynchronization(ss) fails you either
perform compensating cleanup by calling the Microsoft API to delete the newly
created Team (using the teamId returned by createTeam()), or propagate the
exception so the caller can handle rollback; modify the catch blocks around
createTeam() and saveOrUpdateSiteSynchronization(ss) to access the teamId and
invoke the deletion/cleanup via the Microsoft client or rethrow the exception
instead of merely logging (reference methods: createTeam(),
microsoftSynchronizationService.saveOrUpdateSiteSynchronization, and class
MicrosoftSynchronizationEnabler).
- Around line 133-146: Capture the site's prior sync property and only call
createOrRestoreMicrosoftTeamForSite(...) when transitioning from disabled to
enabled: read the existing property (e.g., via
site.getProperties().getProperty(SITE_PROPERTY) or
props.getProperty(SITE_PROPERTY)) into a boolean prevEnabled before you
add/update the property, then if (!prevEnabled && isEnabled) invoke
createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService,
microsoftConfigurationService, microsoftSynchronizationService); otherwise skip
provisioning and still persist the SITE_PROPERTY via props.addProperty as
currently done.
- Around line 237-255: Filter existingSsList to prioritize forced
SiteSynchronization rows (e.g., check a forced flag on SiteSynchronization) and
only restore (set syncDateFrom/syncDateTo, setDisabled(false) and call
microsoftSynchronizationService.saveOrUpdateSiteSynchronization) after
confirming the Team is actually back: require existingSs.getTeamId() non-empty
and microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) returned true;
if no forced row succeeds, do not re-enable any manual rows and fall through to
the create-new-forced-synchronization path so creation can proceed. Ensure you
only call saveOrUpdateSiteSynchronization for the specific SiteSynchronization
that was successfully unarchived.
- Around line 156-180: Only disable and update SiteSynchronization rows that are
the "forced" ones instead of all rows: before iterating ssList, if
nameAndValueForSynchronization.length > 0 remove the aux site property once via
props.removeProperty(nameAndValueForSynchronization[0]) and log it, then in the
loop only operate on a SiteSynchronization ss when it matches the forced
criterion (e.g., contains the property nameAndValueForSynchronization[0] and, if
a value is provided in nameAndValueForSynchronization[1], the value matches);
for matching rows perform the team archive via
microsoftCommonService.archiveTeam(ss.getTeamId()) and then call
ss.setDisabled(true) and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); leave
non-forced ss rows untouched and do not call props.removeProperty inside the
per-ss try/catch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5444aa7b-7c57-4988-a7a1-fcbe1c1d247c
📒 Files selected for processing (7)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java
✅ Files skipped from review due to trivial changes (2)
- microsoft-integration/admin-tool/src/main/resources/Messages.properties
- config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- microsoft-integration/admin-tool/src/main/resources/Messages_es.properties
Contributor
✅ Actions performedFull review triggered. |
Contributor
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)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.java (1)
200-209:⚠️ Potential issue | 🟠 MajorThe disabled flag is only enforced on the batch-listing path.
getAllEnabledSiteSynchronizations()protects callers that use this method, but the live hook handlers in this class still load synchronizations via unfiltered site/group lookups and never checkss.isDisabled(). Disabled synchronizations will therefore keep reacting to membership/site events. Please gate those paths on!ss.isDisabled()or add enabled-only repository lookups for the hook use cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.java` around lines 200 - 209, The batch-listing method getAllEnabledSiteSynchronizations correctly filters disabled SiteSynchronization objects, but the live event/hook handlers in MicrosoftSynchronizationServiceImpl still load synchronizations without checking the disabled flag; update those hook paths to either use the enabled-only repository lookup (microsoftSiteSynchronizationRepository.findAllEnabled()) or explicitly gate processing with if (!ss.isDisabled()) before acting on a SiteSynchronization (class SiteSynchronization) — locate the live handlers in MicrosoftSynchronizationServiceImpl and add the check or swap to the enabled repository call so disabled synchronizations no longer respond to membership/site events.
♻️ Duplicate comments (10)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java (1)
1013-1041:⚠️ Potential issue | 🟠 MajorDon't turn archive/unarchive failures into soft booleans.
These methods drive the new enable/disable flow, but they swallow non-credential exceptions and return
false. That makes it easy for callers to persist the Sakai-side state change while the Team stays in the old archive state. Let the failure propagate, or throw a dedicated exception, so the toggle cannot succeed partially.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java` around lines 1013 - 1041, The archiveTeam and unarchiveTeam methods currently swallow all non-credential exceptions and return false; instead remove or change the generic catch(Exception) blocks so failures propagate (or wrap and throw a dedicated runtime/checked exception) rather than returning false. Specifically, in archiveTeam and unarchiveTeam (methods named exactly) stop catching and dropping Exception: either let the exception bubble up after optionally logging the error with the exception details (e.g., log.debug("Error archiving/unarchiving Microsoft team: id={}", teamId, ex)) or wrap it in a dedicated exception type and throw that so callers cannot treat the operation as a silent boolean failure. Ensure MicrosoftCredentialsException is still rethrown as before.microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java (1)
121-123:⚠️ Potential issue | 🟠 MajorKeep exclusion based on all persisted synchronizations (enabled + disabled).
Using only enabled synchronizations here allows disabled records to be re-proposed during auto-config, which can create duplicate/conflicting site-team relationships.
🛠️ Suggested direction
-List<SiteSynchronization> ssList = microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false); +List<SiteSynchronization> ssList = microsoftSynchronizationService.getAllSiteSynchronizations(false);If
getAllSiteSynchronizations(false)is no longer available, add an explicit include-disabled service path and use it here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java` around lines 121 - 123, The current AutoConfigController uses microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false) to build ssList, which only returns enabled records and allows disabled synchronizations to be re-proposed; change the call to fetch all persisted synchronizations (enabled + disabled) instead — either invoke an existing service method that includes disabled entries (e.g., microsoftSynchronizationService.getAllSiteSynchronizations(true) or microsoftSynchronizationService.getAllSiteSynchronizationsIncludeDisabled()) or add a new service method that returns both enabled and disabled records and call that from AutoConfigController so ssList contains all persisted SiteSynchronization entries (referencing getAllEnabledSiteSynchronizations, microsoftSynchronizationService, AutoConfigController, and ssList).site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (8)
156-180:⚠️ Potential issue | 🟠 MajorScope archive/disable logic to forced synchronizations only.
Both the archive and disable branches operate on every
SiteSynchronizationfor the site. Since this feature creates records withforced(true), toggling the site-level option could unintentionally archive or restore manually-created Teams.Filter by
ss.isForced()(or equivalent getter) before archiving and disabling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 156 - 180, The current loop in MicrosoftSynchronizationEnabler that iterates microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()) archives and disables every SiteSynchronization; restrict this to only forced synchronizations by checking ss.isForced() (or the equivalent getter) before calling microsoftCommonService.archiveTeam(...), removing the site property, setting ss.setDisabled(true), and calling microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); ensure non-forced records are skipped so manually-created Teams are not affected.
133-146:⚠️ Potential issue | 🟠 MajorRe-provisioning issue persists: team creation/restoration runs on every save when enabled.
The code calls
createOrRestoreMicrosoftTeamForSite()unconditionally wheneverisEnabledis true, without checking if synchronization was already enabled for this site. This causes repeated unarchive calls and resetssyncDateFrom/syncDateToon every site save.Consider checking the previous enabled state (via
isEnabledForSite(site)) before calling the provisioning method:Proposed fix
if (isEnabled) { + boolean wasAlreadyEnabled = isEnabledForSite(site); props.addProperty(SITE_PROPERTY, Boolean.TRUE.toString()); MicrosoftConfigurationService microsoftConfigurationService = (MicrosoftConfigurationService) ComponentManager.get(MicrosoftConfigurationService.class); - // Check if MicrosoftSynchronization was already enabled for the site before creating/restoring the Team - createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService); + // Only create/restore when transitioning from disabled to enabled + if (!wasAlreadyEnabled) { + createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 133 - 146, When handling the enabled branch in MicrosoftSynchronizationEnabler, avoid unconditionally calling createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService); first check the previous/actual enabled state for the site (use isEnabledForSite(site) or equivalent) and only call createOrRestoreMicrosoftTeamForSite when the site was not already enabled (transition from disabled→enabled), so that repeated saves while already enabled do not re-provision or reset syncDateFrom/syncDateTo; update the logic around STATE_KEY/SITE_PROPERTY accordingly to perform the provisioning only on the enabling transition.
261-263:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().Proposed fix
} catch (Exception e) { - log.error("Microsoft Team creation failed for site: {}, {}", site.getId(), e.toString()); + log.error("Microsoft Team creation failed for site: {}", site.getId(), e); return; }As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 261 - 263, The catch block in MicrosoftSynchronizationEnabler currently logs the exception using e.toString(), which prevents SLF4J from recording the stacktrace; update the log call in the catch within the method where Microsoft Team creation is handled (class MicrosoftSynchronizationEnabler) to pass the Exception object as the last parameter (e) using SLF4J parameterized logging so the logger.error("Microsoft Team creation failed for site: {}, {}", site.getId(), ???, e) pattern is replaced with a two-parameter message that includes site.getId() and the exception as the throwable (e) so the full stack trace is captured.
167-171:⚠️ Potential issue | 🟡 MinorProperty removal is inside the loop, causing redundant operations.
The removal of the site property runs for each
SiteSynchronizationentry. Move it outside the loop for efficiency and consistency.Proposed fix
if (microsoftSynchronizationService != null && microsoftCommonService != null) { List<SiteSynchronization> ssList = microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()); if (ssList != null) { + // Remove site property once, outside the loop + if (nameAndValueForSynchronization.length > 0) { + props.removeProperty(nameAndValueForSynchronization[0]); + log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); + } for (SiteSynchronization ss : ssList) { if (ss.getTeamId() != null && !ss.getTeamId().isEmpty()) { try { if (microsoftCommonService.archiveTeam(ss.getTeamId())) { log.info("Microsoft Team archived for site: {}, teamId: {}", site.getId(), ss.getTeamId()); } - - // Remove site property defined in microsoft.forced.synchronization.propertynameandoptionalvalue - if (nameAndValueForSynchronization.length > 0) { - props.removeProperty(nameAndValueForSynchronization[0]); - log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); - } } catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 167 - 171, The site property removal (props.removeProperty(nameAndValueForSynchronization[0])) is being executed inside the loop over SiteSynchronization entries, causing redundant removals; move the removal so it runs once after the loop that iterates SiteSynchronization instances in MicrosoftSynchronizationEnabler.java: iterate through the SiteSynchronization processing as before but remove the call to props.removeProperty(...) from inside the loop and instead perform props.removeProperty(nameAndValueForSynchronization[0]) once after the loop (checking nameAndValueForSynchronization.length > 0), and preserve the debug log (log.debug(...)) adjacent to that single removal for site.getId().
172-174:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().SLF4J expects the exception as the final argument without a placeholder to preserve the stack trace.
Proposed fix
- } catch (Exception e) { - log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), ss.getTeamId(), e.toString()); + } catch (Exception e) { + log.warn("Could not archive Microsoft Team for site: {}, teamId: {}", site.getId(), ss.getTeamId(), e);As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 172 - 174, The catch block in MicrosoftSynchronizationEnabler is logging the exception using e.toString() which prevents SLF4J from recording the stack trace; update the catch in the method containing the block that currently calls log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), ss.getTeamId(), e.toString()) to instead pass the exception object as the last argument without a placeholder (e.g., keep the two {} placeholders for site.getId() and ss.getTeamId() and pass e as the final vararg) so SLF4J can render the message and include the full stack trace.
246-248:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().Same issue—pass the exception directly to preserve the stack trace.
Proposed fix
- } catch (Exception e) { - log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), existingSs.getTeamId(), e.toString()); + } catch (Exception e) { + log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}", site.getId(), existingSs.getTeamId(), e);As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 246 - 248, The catch block in MicrosoftSynchronizationEnabler currently logs the exception via e.toString(), losing the stack trace; update the log.warn call in that catch (Exception e) to pass the exception object as the last parameter (e) instead of e.toString(), e.g. log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}", site.getId(), existingSs.getTeamId(), e) so SLF4J preserves the stack trace and uses parameterized logging.
240-255:⚠️ Potential issue | 🟠 MajorDon't re-enable synchronization until Team restore succeeds.
Lines 250-253 unconditionally set
disabled=falseand save even whenunarchiveTeam()returnsfalse, throws an exception, or the entry has noteamId. The sync job will pick up rows pointing to archived or non-existent Teams.Only re-enable when unarchive succeeds:
Proposed fix
for (SiteSynchronization existingSs : existingSsList) { + boolean restored = false; if (existingSs.getTeamId() != null && !existingSs.getTeamId().isEmpty()) { try { if (microsoftCommonService.unarchiveTeam(existingSs.getTeamId())) { log.info("Microsoft Team unarchived for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); + restored = true; } } catch (Exception e) { log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}", site.getId(), existingSs.getTeamId(), e); } } - existingSs.setSyncDateFrom(syncDateFrom); - existingSs.setSyncDateTo(syncDateTo); - existingSs.setDisabled(false); - microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs); - log.info("SiteSynchronization restored for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); + if (restored) { + existingSs.setSyncDateFrom(syncDateFrom); + existingSs.setSyncDateTo(syncDateTo); + existingSs.setDisabled(false); + microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs); + log.info("SiteSynchronization restored for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 240 - 255, The loop currently sets existingSs.setDisabled(false) and calls microsoftSynchronizationService.saveOrUpdateSiteSynchronization(...) regardless of whether a Team was successfully unarchived; change the logic so that existingSs.setDisabled(false) and the saveOrUpdateSiteSynchronization call only occur when a valid teamId exists and microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) returns true (and do not re-enable on false or when an exception is thrown); keep the existing logging and exception handling around microsoftCommonService.unarchiveTeam(...) and ensure entries with null/empty teamId are not re-enabled.
271-284:⚠️ Potential issue | 🔴 CriticalDon't silently swallow failures after
createTeam()succeeds.
createTeam()is a non-idempotent external write. IfsaveOrUpdateSiteSynchronization(ss)fails after the Team is created, the exception at line 282 is only logged. No local record is saved, and the next retry may create a duplicate Team.Either propagate the exception to abort the site save, or implement compensating rollback (e.g., delete the orphaned Team).
Proposed fix (propagate exception)
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); log.info("Microsoft Team created and SiteSynchronization saved: teamId={}, siteId={}", teamId, site.getId()); } - } catch (Exception e) { - log.error("Unexpected error while creating Microsoft Team for site: {}, {}", site.getId(), e.toString()); - } + } catch (Exception e) { + log.error("Unexpected error while creating Microsoft Team for site: {}", site.getId(), e); + throw new RuntimeException("Failed to complete Microsoft Team setup for site: " + site.getId(), e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 271 - 284, The catch block for createTeam()/saveOrUpdateSiteSynchronization in MicrosoftSynchronizationEnabler.java currently swallows failures, risking orphaned Teams; change it to not silently swallow: after microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss) fails, either rethrow the exception (remove the silent catch) so the site save aborts, or implement a compensating delete of the created Team (call the appropriate Microsoft Graph client method, e.g., microsoftGraphClient.deleteTeam(teamId) or similar) inside the catch before logging; ensure the delete’s failures are logged but the original exception is propagated so retries do not create duplicate Teams, and reference createTeam(), saveOrUpdateSiteSynchronization(ss), microsoftSynchronizationService and the teamId/SiteSynchronization variables when making the change.
🧹 Nitpick comments (2)
site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm (1)
203-209: Use kebab-case for the new CSS class name.The new class value uses camelCase (
microsoftSynchronizationArea). Please switch to kebab-case to match project conventions.♻️ Suggested rename
- <div class="microsoftSynchronizationArea"> + <div class="microsoft-synchronization-area">And update the matching SCSS selector accordingly:
-.mathJaxToggleArea, .subPageNavToggleArea, .microsoftSynchronizationArea { +.mathJaxToggleArea, .subPageNavToggleArea, .microsoft-synchronization-area {As per coding guidelines, "Prefer kebab-case for values of HTML class and id attributes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm` around lines 203 - 209, Change the new HTML class microsoftSynchronizationArea to kebab-case (e.g., microsoft-synchronization-area) in the VM template and update any related selectors (SCSS) to match; also search for and update any JavaScript/CSS references that select ".microsoftSynchronizationArea" (or the camelCase form) so they use ".microsoft-synchronization-area" and keep the input id/name/isMicrosoftSynchronizationEnabled references unchanged.microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html (1)
95-100: Consider using Bootstrap 5.2'svisually-hiddenclass instead ofsr-onlyfor consistency.The
.sr-onlyclass is defined in the theme and will function correctly for screen readers. However, per Bootstrap 5.2 guidelines,visually-hiddenis the preferred utility class. The codebase shows mixed usage of both classes, suggesting a gradual migration to Bootstrap 5.2 conventions. If migrating other similar patterns in this file, align withvisually-hiddenfor consistency with the preferred framework.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html` around lines 95 - 100, The span using the legacy "sr-only" utility should be updated to Bootstrap 5.2's "visually-hidden" for consistency: in the synchronization row fragment replace the span with class "sr-only" (inside the anchor that calls toggleDisabled(this)) to use class "visually-hidden" instead, and search for other occurrences of "sr-only" in this template to migrate them similarly so the UI uses the preferred Bootstrap utility consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`:
- Around line 135-141: The toggle handler only updates elem.title and the icon
classes when isDisabled changes, leaving the element's accessible name stale;
update the element's aria-label in the same branches (and also update any
associated hidden label text if present) so assistive tech reflects the new
action. In the branches that call icon.classList.replace(...) and set
elem.title, also set elem.setAttribute('aria-label', /*[[#{index.enable}]]*/ or
/*[[#{index.disable}]]*/ as appropriate and update the hidden label node (e.g.,
a sibling with a screen-reader-only class) to the same localized string so the
visible title, aria-label, and hidden label remain consistent with isDisabled.
---
Outside diff comments:
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.java`:
- Around line 200-209: The batch-listing method
getAllEnabledSiteSynchronizations correctly filters disabled SiteSynchronization
objects, but the live event/hook handlers in MicrosoftSynchronizationServiceImpl
still load synchronizations without checking the disabled flag; update those
hook paths to either use the enabled-only repository lookup
(microsoftSiteSynchronizationRepository.findAllEnabled()) or explicitly gate
processing with if (!ss.isDisabled()) before acting on a SiteSynchronization
(class SiteSynchronization) — locate the live handlers in
MicrosoftSynchronizationServiceImpl and add the check or swap to the enabled
repository call so disabled synchronizations no longer respond to
membership/site events.
---
Duplicate comments:
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java`:
- Around line 121-123: The current AutoConfigController uses
microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false) to
build ssList, which only returns enabled records and allows disabled
synchronizations to be re-proposed; change the call to fetch all persisted
synchronizations (enabled + disabled) instead — either invoke an existing
service method that includes disabled entries (e.g.,
microsoftSynchronizationService.getAllSiteSynchronizations(true) or
microsoftSynchronizationService.getAllSiteSynchronizationsIncludeDisabled()) or
add a new service method that returns both enabled and disabled records and call
that from AutoConfigController so ssList contains all persisted
SiteSynchronization entries (referencing getAllEnabledSiteSynchronizations,
microsoftSynchronizationService, AutoConfigController, and ssList).
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java`:
- Around line 1013-1041: The archiveTeam and unarchiveTeam methods currently
swallow all non-credential exceptions and return false; instead remove or change
the generic catch(Exception) blocks so failures propagate (or wrap and throw a
dedicated runtime/checked exception) rather than returning false. Specifically,
in archiveTeam and unarchiveTeam (methods named exactly) stop catching and
dropping Exception: either let the exception bubble up after optionally logging
the error with the exception details (e.g., log.debug("Error
archiving/unarchiving Microsoft team: id={}", teamId, ex)) or wrap it in a
dedicated exception type and throw that so callers cannot treat the operation as
a silent boolean failure. Ensure MicrosoftCredentialsException is still rethrown
as before.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 156-180: The current loop in MicrosoftSynchronizationEnabler that
iterates
microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId())
archives and disables every SiteSynchronization; restrict this to only forced
synchronizations by checking ss.isForced() (or the equivalent getter) before
calling microsoftCommonService.archiveTeam(...), removing the site property,
setting ss.setDisabled(true), and calling
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); ensure
non-forced records are skipped so manually-created Teams are not affected.
- Around line 133-146: When handling the enabled branch in
MicrosoftSynchronizationEnabler, avoid unconditionally calling
createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService,
microsoftConfigurationService, microsoftSynchronizationService); first check the
previous/actual enabled state for the site (use isEnabledForSite(site) or
equivalent) and only call createOrRestoreMicrosoftTeamForSite when the site was
not already enabled (transition from disabled→enabled), so that repeated saves
while already enabled do not re-provision or reset syncDateFrom/syncDateTo;
update the logic around STATE_KEY/SITE_PROPERTY accordingly to perform the
provisioning only on the enabling transition.
- Around line 261-263: The catch block in MicrosoftSynchronizationEnabler
currently logs the exception using e.toString(), which prevents SLF4J from
recording the stacktrace; update the log call in the catch within the method
where Microsoft Team creation is handled (class MicrosoftSynchronizationEnabler)
to pass the Exception object as the last parameter (e) using SLF4J parameterized
logging so the logger.error("Microsoft Team creation failed for site: {}, {}",
site.getId(), ???, e) pattern is replaced with a two-parameter message that
includes site.getId() and the exception as the throwable (e) so the full stack
trace is captured.
- Around line 167-171: The site property removal
(props.removeProperty(nameAndValueForSynchronization[0])) is being executed
inside the loop over SiteSynchronization entries, causing redundant removals;
move the removal so it runs once after the loop that iterates
SiteSynchronization instances in MicrosoftSynchronizationEnabler.java: iterate
through the SiteSynchronization processing as before but remove the call to
props.removeProperty(...) from inside the loop and instead perform
props.removeProperty(nameAndValueForSynchronization[0]) once after the loop
(checking nameAndValueForSynchronization.length > 0), and preserve the debug log
(log.debug(...)) adjacent to that single removal for site.getId().
- Around line 172-174: The catch block in MicrosoftSynchronizationEnabler is
logging the exception using e.toString() which prevents SLF4J from recording the
stack trace; update the catch in the method containing the block that currently
calls log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}",
site.getId(), ss.getTeamId(), e.toString()) to instead pass the exception object
as the last argument without a placeholder (e.g., keep the two {} placeholders
for site.getId() and ss.getTeamId() and pass e as the final vararg) so SLF4J can
render the message and include the full stack trace.
- Around line 246-248: The catch block in MicrosoftSynchronizationEnabler
currently logs the exception via e.toString(), losing the stack trace; update
the log.warn call in that catch (Exception e) to pass the exception object as
the last parameter (e) instead of e.toString(), e.g. log.warn("Could not
unarchive Microsoft Team for site: {}, teamId: {}", site.getId(),
existingSs.getTeamId(), e) so SLF4J preserves the stack trace and uses
parameterized logging.
- Around line 240-255: The loop currently sets existingSs.setDisabled(false) and
calls microsoftSynchronizationService.saveOrUpdateSiteSynchronization(...)
regardless of whether a Team was successfully unarchived; change the logic so
that existingSs.setDisabled(false) and the saveOrUpdateSiteSynchronization call
only occur when a valid teamId exists and
microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) returns true (and
do not re-enable on false or when an exception is thrown); keep the existing
logging and exception handling around microsoftCommonService.unarchiveTeam(...)
and ensure entries with null/empty teamId are not re-enabled.
- Around line 271-284: The catch block for
createTeam()/saveOrUpdateSiteSynchronization in
MicrosoftSynchronizationEnabler.java currently swallows failures, risking
orphaned Teams; change it to not silently swallow: after
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss) fails,
either rethrow the exception (remove the silent catch) so the site save aborts,
or implement a compensating delete of the created Team (call the appropriate
Microsoft Graph client method, e.g., microsoftGraphClient.deleteTeam(teamId) or
similar) inside the catch before logging; ensure the delete’s failures are
logged but the original exception is propagated so retries do not create
duplicate Teams, and reference createTeam(),
saveOrUpdateSiteSynchronization(ss), microsoftSynchronizationService and the
teamId/SiteSynchronization variables when making the change.
---
Nitpick comments:
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html`:
- Around line 95-100: The span using the legacy "sr-only" utility should be
updated to Bootstrap 5.2's "visually-hidden" for consistency: in the
synchronization row fragment replace the span with class "sr-only" (inside the
anchor that calls toggleDisabled(this)) to use class "visually-hidden" instead,
and search for other occurrences of "sr-only" in this template to migrate them
similarly so the UI uses the preferred Bootstrap utility consistently.
In
`@site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm`:
- Around line 203-209: Change the new HTML class microsoftSynchronizationArea to
kebab-case (e.g., microsoft-synchronization-area) in the VM template and update
any related selectors (SCSS) to match; also search for and update any
JavaScript/CSS references that select ".microsoftSynchronizationArea" (or the
camelCase form) so they use ".microsoft-synchronization-area" and keep the input
id/name/isMicrosoftSynchronizationEnabled references unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd0e8e4f-924a-4c8e-842d-72986bb80885
📒 Files selected for processing (24)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieslibrary/src/skins/default/src/sass/modules/tool/sitemanage/_sitemanage.scssmicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.javamicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.htmlmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftCommonService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftSynchronizationService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/persistence/MicrosoftSiteSynchronizationRepository.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/jobs/RunSynchronizationsJob.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/persistence/MicrosoftSiteSynchronizationRepositoryImpl.javasite-manage/site-manage-tool/tool/pom.xmlsite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javasite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-addRemoveFeatureConfirm.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm
Comment on lines
+135
to
+141
| if (!isDisabled) { | ||
| icon.classList.replace('fa-toggle-on', 'fa-toggle-off'); | ||
| elem.title = /*[[#{index.enable}]]*/ ''; | ||
| } else { | ||
| icon.classList.replace('fa-toggle-off', 'fa-toggle-on'); | ||
| elem.title = /*[[#{index.disable}]]*/ ''; | ||
| } |
Contributor
There was a problem hiding this comment.
Update the control’s accessible name when toggle state changes.
On success, only title is updated. aria-label (and the hidden label text) stay stale, so assistive tech can announce the wrong action after toggling.
♿ Proposed fix
if (data.status == false) {
showAjaxError(data.error);
} else {
+ let nextLabel;
if (!isDisabled) {
icon.classList.replace('fa-toggle-on', 'fa-toggle-off');
- elem.title = /*[[#{index.enable}]]*/ '';
+ nextLabel = /*[[#{index.enable}]]*/ '';
} else {
icon.classList.replace('fa-toggle-off', 'fa-toggle-on');
- elem.title = /*[[#{index.disable}]]*/ '';
+ nextLabel = /*[[#{index.disable}]]*/ '';
}
+ elem.title = nextLabel;
+ elem.setAttribute('aria-label', nextLabel);
+ const srText = elem.querySelector('.sr-only, .visually-hidden');
+ if (srText) srText.textContent = nextLabel;
}As per coding guidelines, **/*.{html,xhtml,vm,js,ts}: Follow accessibility best practices.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`
around lines 135 - 141, The toggle handler only updates elem.title and the icon
classes when isDisabled changes, leaving the element's accessible name stale;
update the element's aria-label in the same branches (and also update any
associated hidden label text if present) so assistive tech reflects the new
action. In the branches that call icon.classList.replace(...) and set
elem.title, also set elem.setAttribute('aria-label', /*[[#{index.enable}]]*/ or
/*[[#{index.disable}]]*/ as appropriate and update the hidden label node (e.g.,
a sibling with a screen-reader-only class) to the same localized string so the
visible title, aria-label, and hidden label remain consistent with isDisabled.
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java (1)
8893-8905:⚠️ Potential issue | 🟠 MajorStale Microsoft sync state can bleed into later site saves.
MicrosoftSynchronizationEnabler.prepareSiteForSave(...)is driven by a sticky session attribute (MicrosoftSynchronizationEnabler.java:126-140), but these happy paths never clear that state after persisting. After saving one site, a later edit in the same session can reuse the previous Microsoft value, and the|| updateSiteat Line 12303 can also trigger an unrelatedcommitSite(...). Please either clear the Microsoft state after successful persistence in both flows or make the helper returntrueonly when the current request actually changed the value.Also applies to: 12303-12305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java` around lines 8893 - 8905, MicrosoftSynchronizationEnabler.prepareSiteForSave(...) leaves a sticky session attribute set, causing subsequent saves to inherit the previous Microsoft sync choice; fix by clearing that sticky Microsoft session state after a successful persistence. Concretely: after siteService.save(Site) succeeds (and likewise after the alternate updateSite/commitSite flow around commitSite(...) / the || updateSite logic), remove/clear the MicrosoftSynchronizationEnabler session attribute (the same attribute prepareSiteForSave sets) from the state object so it cannot bleed into later requests; alternatively, change MicrosoftSynchronizationEnabler.prepareSiteForSave to return true only when the current request actually changed the Microsoft value and only then leave the attribute, otherwise clear it—apply the cleanup in the save(Site) success path and the updateSite/commitSite success path where STATE_TEMPLATE_INDEX/SITE_INFO_TEMPLATE_INDEX is set.
♻️ Duplicate comments (12)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java (1)
1013-1041:⚠️ Potential issue | 🟠 Major
archiveTeam/unarchiveTeamstill mask operational failures.Both methods swallow non-credential exceptions and return
false, which allows upstream flows to continue and persist synchronization state even when Graph archive/unarchive fails. This keeps data state and real Team state out of sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java` around lines 1013 - 1041, archiveTeam and unarchiveTeam are swallowing non-credential exceptions and returning false, hiding operational failures; update MicrosoftCommonServiceImpl.archiveTeam and MicrosoftCommonServiceImpl.unarchiveTeam so that after catching Exception (other than MicrosoftCredentialsException) you log the full error (including ex) and rethrow a runtime or custom checked exception (e.g., MicrosoftOperationException) instead of returning false, so upstream callers see the failure and can avoid persisting inconsistent sync state; preserve the existing behavior of rethrowing MicrosoftCredentialsException unchanged.microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java (1)
121-123:⚠️ Potential issue | 🟠 MajorEnabled-only filtering can re-propose already persisted (disabled) mappings.
Using only enabled synchronizations for exclusion allows disabled site/team relationships to be treated as new candidates, which can create duplicate or conflicting synchronization records.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java` around lines 121 - 123, The code currently calls microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false) which only returns enabled synchronizations and can cause already-persisted but disabled mappings to be re-proposed; change the call to retrieve all synchronizations (including disabled) so exclusion covers existing records (e.g., call the service method that returns all site synchronizations or pass the parameter that includes disabled records) and keep using ssList for the exclusion check in the AutoConfigController logic to prevent duplicates.microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html (1)
135-142:⚠️ Potential issue | 🟡 MinorUpdate
aria-labeland screen-reader text when toggle state changes.The success handler updates
elem.titlebut leavesaria-labeland the.sr-onlyspan unchanged. After toggling, assistive technologies will announce the stale action name.♿ Proposed accessibility fix
if (data.status == false) { showAjaxError(data.error); } else { + let nextLabel; if (!isDisabled) { icon.classList.replace('fa-toggle-on', 'fa-toggle-off'); - elem.title = /*[[#{index.enable}]]*/ ''; + nextLabel = /*[[#{index.enable}]]*/ ''; } else { icon.classList.replace('fa-toggle-off', 'fa-toggle-on'); - elem.title = /*[[#{index.disable}]]*/ ''; + nextLabel = /*[[#{index.disable}]]*/ ''; } + elem.title = nextLabel; + elem.setAttribute('aria-label', nextLabel); + const srText = elem.querySelector('.sr-only'); + if (srText) srText.textContent = nextLabel; }As per coding guidelines,
**/*.{html,xhtml,vm,js,ts}: Follow accessibility best practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html` around lines 135 - 142, The toggle handler updates the icon and elem.title but not the accessible labels, causing stale announcements; after replacing classes in the block that references isDisabled, icon, and elem (the toggle element), also update elem.setAttribute('aria-label', ...) to the corresponding enable/disable string and update the visible screen-reader text inside the associated `.sr-only` span (find the span relative to elem or by ID) to the same localized label so assistive tech announces the current action; ensure you use the same localized tokens used for elem.title (/*[[#{index.enable}]]*/ and /*[[#{index.disable}]]*/) so aria-label and the `.sr-only` content stay in sync with the icon change.site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (9)
258-281:⚠️ Potential issue | 🟠 MajorPost-
createTeam()failures are swallowed, risking orphaned Teams.If
saveOrUpdateSiteSynchronization(ss)at line 279 fails aftercreateTeam()succeeds, the exception is caught at line 282 and only logged. The site save continues without a local record, and a retry could create a duplicate Team.Consider either propagating the exception or implementing compensating logic (e.g., deleting the created Team on failure).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 258 - 281, The code currently calls microsoftCommonService.createTeam(...) and then calls microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss) which, if it fails, leaves an orphaned Team; update the flow so failures after createTeam are not silently swallowed: either rethrow the exception from the enclosing method instead of just logging, or implement compensating logic that calls microsoftCommonService.deleteTeam(teamId) (or equivalent) when saveOrUpdateSiteSynchronization(ss) throws, ensuring deletion is attempted and any deletion errors are logged/handled; ensure you reference createTeam, teamId, SiteSynchronization ss, saveOrUpdateSiteSynchronization and microsoftSynchronizationService when making the change.
167-171:⚠️ Potential issue | 🟡 MinorProperty removal logic is inside the loop, causing redundant operations.
The property removal at lines 168-171 is executed inside the for-loop for each
SiteSynchronization. This means it will attempt to remove the same property multiple times if there are multiple synchronization records. Move this outside the loop.Proposed fix
if (ssList != null) { + // Remove site property outside of loop + if (nameAndValueForSynchronization.length > 0) { + props.removeProperty(nameAndValueForSynchronization[0]); + log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); + } for (SiteSynchronization ss : ssList) { if (ss.getTeamId() != null && !ss.getTeamId().isEmpty()) { try { if (microsoftCommonService.archiveTeam(ss.getTeamId())) { log.info("Microsoft Team archived for site: {}, teamId: {}", site.getId(), ss.getTeamId()); } - - // Remove site property defined in microsoft.forced.synchronization.propertynameandoptionalvalue - if (nameAndValueForSynchronization.length > 0) { - props.removeProperty(nameAndValueForSynchronization[0]); - log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); - } } catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 167 - 171, The property removal is happening inside the loop that iterates SiteSynchronization records, causing repeated removals; move the removal code so it runs once after the loop completes. Specifically, in MicrosoftSynchronizationEnabler.java find the loop over SiteSynchronization (the block referencing nameAndValueForSynchronization and props) and remove the props.removeProperty(...) and the debug log call from inside the loop, then add a single props.removeProperty(nameAndValueForSynchronization[0]) and corresponding log.debug(...) immediately after the loop (guarded by the same nameAndValueForSynchronization.length > 0 check) so the property is removed only once per site.
282-284:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().Proposed fix
} catch (Exception e) { - log.error("Unexpected error while creating Microsoft Team for site: {}, {}", site.getId(), e.toString()); + log.error("Unexpected error while creating Microsoft Team for site: {}", site.getId(), e); }As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 282 - 284, The catch block in MicrosoftSynchronizationEnabler uses e.toString() instead of passing the exception object to SLF4J; update the log.error call in the catch for exception 'e' to use parameterized logging with a single placeholder for site.getId() and pass 'e' as the throwable (e.g., log.error("Unexpected error while creating Microsoft Team for site: {}", site.getId(), e)); this ensures the stacktrace is logged correctly.
261-263:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().Proposed fix
} catch (Exception e) { - log.error("Microsoft Team creation failed for site: {}, {}", site.getId(), e.toString()); + log.error("Microsoft Team creation failed for site: {}", site.getId(), e); return; }As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 261 - 263, The catch block in MicrosoftSynchronizationEnabler.java currently logs the exception via e.toString(); update the SLF4J call in the catch (Exception e) handler for the Microsoft Team creation code to pass the Exception object as the throwable parameter instead of converting it to a string—use a single parameterized placeholder for site.getId() and supply e as the final argument so the stacktrace is logged (i.e., replace the current log.error invocation that references site.getId() and e.toString()).
246-248:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().Same issue—pass the exception directly to preserve the stack trace.
Proposed fix
- } catch (Exception e) { - log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), existingSs.getTeamId(), e.toString()); + } catch (Exception e) { + log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}", site.getId(), existingSs.getTeamId(), e);As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 246 - 248, The catch block in MicrosoftSynchronizationEnabler is logging the exception via e.toString(), which loses the stack trace; update the log.warn call in the catch (Exception e) block that references site.getId() and existingSs.getTeamId() to pass the exception object as the final parameter (e) using SLF4J parameterized logging so the stacktrace is preserved (keep the same message and placeholders, only replace e.toString() with e).
240-255:⚠️ Potential issue | 🟠 MajorSync re-enabled before confirming Team restore succeeded.
The loop unconditionally sets
disabled=falseand saves the synchronization (lines 250-254) even whenunarchiveTeam()returnsfalseor throws. This could leave an enabled synchronization pointing to an archived/inaccessible Team.Only update and save when
unarchiveTeam()returnstrue:for (SiteSynchronization existingSs : existingSsList) { if (existingSs.getTeamId() != null && !existingSs.getTeamId().isEmpty()) { try { - if (microsoftCommonService.unarchiveTeam(existingSs.getTeamId())) { + boolean unarchived = microsoftCommonService.unarchiveTeam(existingSs.getTeamId()); + if (unarchived) { log.info("Microsoft Team unarchived for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); + existingSs.setSyncDateFrom(syncDateFrom); + existingSs.setSyncDateTo(syncDateTo); + existingSs.setDisabled(false); + microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs); + log.info("SiteSynchronization restored for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); + } else { + log.warn("Failed to unarchive Microsoft Team for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); } } catch (Exception e) { log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}", site.getId(), existingSs.getTeamId(), e); } } - existingSs.setSyncDateFrom(syncDateFrom); - existingSs.setSyncDateTo(syncDateTo); - existingSs.setDisabled(false); - microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs); - log.info("SiteSynchronization restored for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 240 - 255, The code currently sets existingSs.setDisabled(false) and calls microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs) for every SiteSynchronization in existingSsList regardless of whether microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) succeeded; change the loop so that you only update syncDateFrom/syncDateTo, setDisabled(false) and call saveOrUpdateSiteSynchronization when unarchiveTeam(...) returns true, and ensure that in the catch block you do not enable or save the SiteSynchronization (log a warn and leave it disabled); use the existing symbols existingSs, microsoftCommonService.unarchiveTeam, existingSs.setDisabled(false), and microsoftSynchronizationService.saveOrUpdateSiteSynchronization to locate and modify the logic.
172-174:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().This loses the stack trace. SLF4J expects the exception as the last argument without a placeholder.
Proposed fix
- } catch (Exception e) { - log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), ss.getTeamId(), e.toString()); + } catch (Exception e) { + log.warn("Could not archive Microsoft Team for site: {}, teamId: {}", site.getId(), ss.getTeamId(), e);As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 172 - 174, The catch block in MicrosoftSynchronizationEnabler is logging the exception with e.toString(), which loses the stack trace; update the log.warn call in the catch (Exception e) handler that currently references site.getId() and ss.getTeamId() so that the throwable is passed as the final argument (e) instead of e.toString() and remove the trailing "{}" placeholder for the exception — e.g. keep two "{}" placeholders for siteId and teamId and pass e as the last parameter so SLF4J can print the stack trace.
133-146:⚠️ Potential issue | 🟠 MajorRe-provisioning occurs on every save when sync is enabled.
The code calls
createOrRestoreMicrosoftTeamForSite()every timeSTATE_KEYis true, even if Microsoft sync was already enabled. This issues unnecessary unarchive calls and resetssyncDateFrom/syncDateTofor existing synchronizations on every site save.Add a check to compare the current enabled state with the previous state before calling the provisioning method:
if (state.getAttribute(STATE_KEY) != null) { final boolean isEnabled = (Boolean) state.getAttribute(STATE_KEY); + final boolean wasEnabled = isEnabledForSite(site); final ResourcePropertiesEdit props = site.getPropertiesEdit(); ... if (isEnabled) { props.addProperty(SITE_PROPERTY, Boolean.TRUE.toString()); - ... - createOrRestoreMicrosoftTeamForSite(...); + if (!wasEnabled) { + // Only provision when transitioning from disabled to enabled + createOrRestoreMicrosoftTeamForSite(...); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 133 - 146, The code currently calls createOrRestoreMicrosoftTeamForSite(...) whenever STATE_KEY is true, causing re-provisioning on every save; change the logic to call provisioning only when the enabled state transitions from false to true. Concretely, read the existing SITE_PROPERTY from the site properties (via props.getProperty(SITE_PROPERTY)) or parse it to a boolean, and only invoke createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService) when isEnabled is true AND the previous property value is not true; keep props.addProperty(SITE_PROPERTY, Boolean.TRUE.toString()) as needed but avoid calling createOrRestore... when the site was already enabled to prevent unarchive/reset of syncDateFrom/syncDateTo.
156-180:⚠️ Potential issue | 🟠 MajorToggle affects all synchronizations, not just forced ones.
Per the relevant code snippet,
SiteSynchronizationhas aforcedboolean field. This code operates on everySiteSynchronizationfor the site, but the feature is specifically for "forced" synchronizations. Manual synchronizations created through other means would also be archived/disabled when toggling this setting off.Filter the list to only process records where
ss.isForced()is true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 156 - 180, The current loop in MicrosoftSynchronizationEnabler that iterates the list from microsoftSynchronizationService.getSiteSynchronizationsBySite processes every SiteSynchronization; change it to only act on forced synchronizations by checking ss.isForced() (or ss.getForced()) before calling microsoftCommonService.archiveTeam, removing the site property (nameAndValueForSynchronization), setting ss.setDisabled(true), and calling microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); ensure the archive/remove/disable logic is executed only when ss.isForced() is true so manual/non-forced synchronizations are unaffected.
🧹 Nitpick comments (2)
site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vm (1)
289-289: Use a Microsoft-specific CSS hook instead ofsummary-subnav-enabled.Reusing the SubNav class for a different feature couples unrelated rows and can cause accidental styling regressions later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vm` at line 289, The row currently reuses the generic class "summary-subnav-enabled", coupling unrelated features; change the tr's class to a Microsoft-specific hook (e.g., "ms-summary-subnav-enabled" or another Microsoft-prefixed name) and update any referenced CSS selectors to use that new class so styling remains isolated; search for "summary-subnav-enabled" in templates and styles to replace usages and ensure the new hook is applied where only the Microsoft-specific feature needs the style.microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java (1)
314-314: Consider migratingsetForcedandsetDateendpoints to POST.For consistency and security, the adjacent
setForced-siteSynchronization(line 314) andsetDate-siteSynchronization(line 356) endpoints still use@GetMappingfor state-changing operations. While not part of this PR's changes, these should be migrated to@PostMappingto match the newsetDisabledendpoint pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java` at line 314, The two state-changing endpoints annotated with `@GetMapping`—setForced-siteSynchronization (method handling setForced) and setDate-siteSynchronization (method handling setDate)—should be migrated to `@PostMapping` to follow the same POST pattern as setDisabled; replace the `@GetMapping` annotation on those controller methods with `@PostMapping` (preserving produces = MediaType.APPLICATION_JSON_VALUE and any path/value attributes), update any callers/tests to use POST, and ensure request parameter handling (e.g., `@PathVariable` or `@RequestParam`) remains compatible with POST bodies or form submissions as appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Around line 8893-8905: MicrosoftSynchronizationEnabler.prepareSiteForSave(...)
leaves a sticky session attribute set, causing subsequent saves to inherit the
previous Microsoft sync choice; fix by clearing that sticky Microsoft session
state after a successful persistence. Concretely: after siteService.save(Site)
succeeds (and likewise after the alternate updateSite/commitSite flow around
commitSite(...) / the || updateSite logic), remove/clear the
MicrosoftSynchronizationEnabler session attribute (the same attribute
prepareSiteForSave sets) from the state object so it cannot bleed into later
requests; alternatively, change
MicrosoftSynchronizationEnabler.prepareSiteForSave to return true only when the
current request actually changed the Microsoft value and only then leave the
attribute, otherwise clear it—apply the cleanup in the save(Site) success path
and the updateSite/commitSite success path where
STATE_TEMPLATE_INDEX/SITE_INFO_TEMPLATE_INDEX is set.
---
Duplicate comments:
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java`:
- Around line 121-123: The code currently calls
microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false) which
only returns enabled synchronizations and can cause already-persisted but
disabled mappings to be re-proposed; change the call to retrieve all
synchronizations (including disabled) so exclusion covers existing records
(e.g., call the service method that returns all site synchronizations or pass
the parameter that includes disabled records) and keep using ssList for the
exclusion check in the AutoConfigController logic to prevent duplicates.
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`:
- Around line 135-142: The toggle handler updates the icon and elem.title but
not the accessible labels, causing stale announcements; after replacing classes
in the block that references isDisabled, icon, and elem (the toggle element),
also update elem.setAttribute('aria-label', ...) to the corresponding
enable/disable string and update the visible screen-reader text inside the
associated `.sr-only` span (find the span relative to elem or by ID) to the same
localized label so assistive tech announces the current action; ensure you use
the same localized tokens used for elem.title (/*[[#{index.enable}]]*/ and
/*[[#{index.disable}]]*/) so aria-label and the `.sr-only` content stay in sync
with the icon change.
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java`:
- Around line 1013-1041: archiveTeam and unarchiveTeam are swallowing
non-credential exceptions and returning false, hiding operational failures;
update MicrosoftCommonServiceImpl.archiveTeam and
MicrosoftCommonServiceImpl.unarchiveTeam so that after catching Exception (other
than MicrosoftCredentialsException) you log the full error (including ex) and
rethrow a runtime or custom checked exception (e.g.,
MicrosoftOperationException) instead of returning false, so upstream callers see
the failure and can avoid persisting inconsistent sync state; preserve the
existing behavior of rethrowing MicrosoftCredentialsException unchanged.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 258-281: The code currently calls
microsoftCommonService.createTeam(...) and then calls
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss) which, if it
fails, leaves an orphaned Team; update the flow so failures after createTeam are
not silently swallowed: either rethrow the exception from the enclosing method
instead of just logging, or implement compensating logic that calls
microsoftCommonService.deleteTeam(teamId) (or equivalent) when
saveOrUpdateSiteSynchronization(ss) throws, ensuring deletion is attempted and
any deletion errors are logged/handled; ensure you reference createTeam, teamId,
SiteSynchronization ss, saveOrUpdateSiteSynchronization and
microsoftSynchronizationService when making the change.
- Around line 167-171: The property removal is happening inside the loop that
iterates SiteSynchronization records, causing repeated removals; move the
removal code so it runs once after the loop completes. Specifically, in
MicrosoftSynchronizationEnabler.java find the loop over SiteSynchronization (the
block referencing nameAndValueForSynchronization and props) and remove the
props.removeProperty(...) and the debug log call from inside the loop, then add
a single props.removeProperty(nameAndValueForSynchronization[0]) and
corresponding log.debug(...) immediately after the loop (guarded by the same
nameAndValueForSynchronization.length > 0 check) so the property is removed only
once per site.
- Around line 282-284: The catch block in MicrosoftSynchronizationEnabler uses
e.toString() instead of passing the exception object to SLF4J; update the
log.error call in the catch for exception 'e' to use parameterized logging with
a single placeholder for site.getId() and pass 'e' as the throwable (e.g.,
log.error("Unexpected error while creating Microsoft Team for site: {}",
site.getId(), e)); this ensures the stacktrace is logged correctly.
- Around line 261-263: The catch block in MicrosoftSynchronizationEnabler.java
currently logs the exception via e.toString(); update the SLF4J call in the
catch (Exception e) handler for the Microsoft Team creation code to pass the
Exception object as the throwable parameter instead of converting it to a
string—use a single parameterized placeholder for site.getId() and supply e as
the final argument so the stacktrace is logged (i.e., replace the current
log.error invocation that references site.getId() and e.toString()).
- Around line 246-248: The catch block in MicrosoftSynchronizationEnabler is
logging the exception via e.toString(), which loses the stack trace; update the
log.warn call in the catch (Exception e) block that references site.getId() and
existingSs.getTeamId() to pass the exception object as the final parameter (e)
using SLF4J parameterized logging so the stacktrace is preserved (keep the same
message and placeholders, only replace e.toString() with e).
- Around line 240-255: The code currently sets existingSs.setDisabled(false) and
calls
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs) for
every SiteSynchronization in existingSsList regardless of whether
microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) succeeded; change
the loop so that you only update syncDateFrom/syncDateTo, setDisabled(false) and
call saveOrUpdateSiteSynchronization when unarchiveTeam(...) returns true, and
ensure that in the catch block you do not enable or save the SiteSynchronization
(log a warn and leave it disabled); use the existing symbols existingSs,
microsoftCommonService.unarchiveTeam, existingSs.setDisabled(false), and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization to locate and
modify the logic.
- Around line 172-174: The catch block in MicrosoftSynchronizationEnabler is
logging the exception with e.toString(), which loses the stack trace; update the
log.warn call in the catch (Exception e) handler that currently references
site.getId() and ss.getTeamId() so that the throwable is passed as the final
argument (e) instead of e.toString() and remove the trailing "{}" placeholder
for the exception — e.g. keep two "{}" placeholders for siteId and teamId and
pass e as the last parameter so SLF4J can print the stack trace.
- Around line 133-146: The code currently calls
createOrRestoreMicrosoftTeamForSite(...) whenever STATE_KEY is true, causing
re-provisioning on every save; change the logic to call provisioning only when
the enabled state transitions from false to true. Concretely, read the existing
SITE_PROPERTY from the site properties (via props.getProperty(SITE_PROPERTY)) or
parse it to a boolean, and only invoke createOrRestoreMicrosoftTeamForSite(site,
microsoftCommonService, microsoftConfigurationService,
microsoftSynchronizationService) when isEnabled is true AND the previous
property value is not true; keep props.addProperty(SITE_PROPERTY,
Boolean.TRUE.toString()) as needed but avoid calling createOrRestore... when the
site was already enabled to prevent unarchive/reset of syncDateFrom/syncDateTo.
- Around line 156-180: The current loop in MicrosoftSynchronizationEnabler that
iterates the list from
microsoftSynchronizationService.getSiteSynchronizationsBySite processes every
SiteSynchronization; change it to only act on forced synchronizations by
checking ss.isForced() (or ss.getForced()) before calling
microsoftCommonService.archiveTeam, removing the site property
(nameAndValueForSynchronization), setting ss.setDisabled(true), and calling
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); ensure the
archive/remove/disable logic is executed only when ss.isForced() is true so
manual/non-forced synchronizations are unaffected.
---
Nitpick comments:
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java`:
- Line 314: The two state-changing endpoints annotated with
`@GetMapping`—setForced-siteSynchronization (method handling setForced) and
setDate-siteSynchronization (method handling setDate)—should be migrated to
`@PostMapping` to follow the same POST pattern as setDisabled; replace the
`@GetMapping` annotation on those controller methods with `@PostMapping` (preserving
produces = MediaType.APPLICATION_JSON_VALUE and any path/value attributes),
update any callers/tests to use POST, and ensure request parameter handling
(e.g., `@PathVariable` or `@RequestParam`) remains compatible with POST bodies or
form submissions as appropriate.
In
`@site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vm`:
- Line 289: The row currently reuses the generic class "summary-subnav-enabled",
coupling unrelated features; change the tr's class to a Microsoft-specific hook
(e.g., "ms-summary-subnav-enabled" or another Microsoft-prefixed name) and
update any referenced CSS selectors to use that new class so styling remains
isolated; search for "summary-subnav-enabled" in templates and styles to replace
usages and ensure the new hook is applied where only the Microsoft-specific
feature needs the style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30f88378-09d7-4aa2-8313-6bc2c16e1184
📒 Files selected for processing (24)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieslibrary/src/skins/default/src/sass/modules/tool/sitemanage/_sitemanage.scssmicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.javamicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.htmlmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftCommonService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftSynchronizationService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/persistence/MicrosoftSiteSynchronizationRepository.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/jobs/RunSynchronizationsJob.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/persistence/MicrosoftSiteSynchronizationRepositoryImpl.javasite-manage/site-manage-tool/tool/pom.xmlsite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javasite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-addRemoveFeatureConfirm.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
microsoft-integration/admin-tool/src/main/resources/Messages_es.properties (1)
67-67:⚠️ Potential issue | 🟡 MinorFix file encoding corruption at line 67.
Line 67 contains invalid UTF-8 bytes (0x80, 0x8b) that prevent the file from being parsed as UTF-8. The file must be re-encoded to valid UTF-8 or the corrupted bytes removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/resources/Messages_es.properties` at line 67, The Spanish properties file Messages_es.properties contains corrupted non-UTF-8 bytes (0x80, 0x8B); re-open Messages_es.properties in a text editor or iconv, remove or replace the invalid bytes and re-save the file encoded as UTF-8 (no BOM), or run a re-encoding command (e.g., convert from the detected original encoding to UTF-8) and then validate the file parses as UTF-8; ensure the corrupted characters are either fixed to the intended characters or removed so the properties file loads correctly.
♻️ Duplicate comments (2)
microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java (1)
121-122:⚠️ Potential issue | 🟠 MajorAuto-config may re-propose site/team pairs from disabled synchronizations.
This change filters to only enabled synchronizations when building exclusion lists. However, disabled synchronizations still exist in the database. This means a disabled synchronization's site/team pair could be proposed again by auto-config, potentially creating duplicate or conflicting relationships when the original is re-enabled.
Consider using a method that retrieves all synchronizations (enabled and disabled) for exclusion purposes, or document this as intentional behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java` around lines 121 - 122, The code in AutoConfigController currently builds exclusion lists from only enabled synchronizations by calling microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false) into ssList, which allows disabled syncs' site/team pairs to be re-proposed; change the call to retrieve all synchronizations (enabled and disabled) when building the exclusion set — e.g., use the service method that returns all synchronizations (or add one like getAllSiteSynchronizations(...) that accepts a flag to include disabled entries) and use that result instead of getAllEnabledSiteSynchronizations(false) so disabled pairs are also excluded from auto-config proposals.microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html (1)
135-141:⚠️ Potential issue | 🟠 MajorUpdate ARIA and SR text when toggling disabled state.
On success, only
titlechanges.aria-labeland hidden label remain stale, so screen readers can announce the wrong action after toggling.♿ Suggested fix
- if (data.status == false) { + if (data.status == false) { showAjaxError(data.error); } else { + let nextLabel; if (!isDisabled) { icon.classList.replace('fa-toggle-on', 'fa-toggle-off'); - elem.title = /*[[#{index.enable}]]*/ ''; + nextLabel = /*[[#{index.enable}]]*/ ''; } else { icon.classList.replace('fa-toggle-off', 'fa-toggle-on'); - elem.title = /*[[#{index.disable}]]*/ ''; + nextLabel = /*[[#{index.disable}]]*/ ''; } + elem.title = nextLabel; + elem.setAttribute('aria-label', nextLabel); + const srText = elem.querySelector('.sr-only, .visually-hidden'); + if (srText) { + srText.textContent = nextLabel; + } }As per coding guidelines, "
**/*.{js,html,jsp,vm,xhtml,xml}: Follow accessibility best practices".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html` around lines 135 - 141, The toggle handler currently only updates elem.title when flipping the disabled state (using icon.classList.replace and isDisabled), leaving the element's aria-label and any hidden screen-reader label stale; update the accessible text in the same branch where you change the icon/title: set elem.setAttribute('aria-label', appropriateLabel) for both enable/disable cases and also update the hidden SR-only label node (e.g., elem.querySelector('.sr-only') or the specific hidden-label element used in the template) to the corresponding enable/disable string so screen readers announce the correct action after toggling.
🧹 Nitpick comments (1)
site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (1)
320-323: Site type matching is case-sensitive.The comparison uses
allowedTypes.contains(siteType)which is case-sensitive. If the configuration specifies "Course" butsite.getType()returns "course", they won't match. Consider whether case-insensitive matching would be more forgiving for configuration.Proposed fix for case-insensitive matching
final String siteType = site.getType(); if (siteType == null || siteType.isEmpty()) { return false; } - final List<String> allowedTypes = Arrays.asList(configValue.split("\\s*,\\s*")); - return allowedTypes.contains(siteType); + final String siteTypeLower = siteType.toLowerCase(); + for (String allowedType : configValue.split("\\s*,\\s*")) { + if (allowedType.equalsIgnoreCase(siteTypeLower)) { + return true; + } + } + return false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 320 - 323, The allowed site-type check in MicrosoftSynchronizationEnabler uses allowedTypes.contains(siteType) which is case-sensitive; update the check to perform case-insensitive matching by normalizing both the parsed config values and the site type (e.g., map allowedTypes to lower-case when building from configValue and compare against siteType.toLowerCase()) or use a case-insensitive comparison when checking membership (reference symbols: configValue, allowedTypes, siteType in MicrosoftSynchronizationEnabler).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 259-264: The catch block in MicrosoftSynchronizationEnabler around
microsoftCommonService.createTeam currently logs e.toString(), losing the stack
trace; change the log call to use SLF4J parameterized logging and pass the
exception object as the final argument (e.g., keep a single {} for site.getId()
and pass the exception object as the last parameter) so the full stack trace is
recorded by log.error rather than using e.toString().
- Around line 282-284: The catch block in MicrosoftSynchronizationEnabler that
currently logs the exception with e.toString() should be changed to pass the
exception object to the SLF4J logger so the full stack trace is preserved;
locate the catch in the method where it logs "Unexpected error while creating
Microsoft Team for site: {}, {}" and replace the e.toString() parameter with the
exception object (e) as the last argument so the log call uses the parameterized
message and includes the throwable.
---
Outside diff comments:
In `@microsoft-integration/admin-tool/src/main/resources/Messages_es.properties`:
- Line 67: The Spanish properties file Messages_es.properties contains corrupted
non-UTF-8 bytes (0x80, 0x8B); re-open Messages_es.properties in a text editor or
iconv, remove or replace the invalid bytes and re-save the file encoded as UTF-8
(no BOM), or run a re-encoding command (e.g., convert from the detected original
encoding to UTF-8) and then validate the file parses as UTF-8; ensure the
corrupted characters are either fixed to the intended characters or removed so
the properties file loads correctly.
---
Duplicate comments:
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java`:
- Around line 121-122: The code in AutoConfigController currently builds
exclusion lists from only enabled synchronizations by calling
microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false) into
ssList, which allows disabled syncs' site/team pairs to be re-proposed; change
the call to retrieve all synchronizations (enabled and disabled) when building
the exclusion set — e.g., use the service method that returns all
synchronizations (or add one like getAllSiteSynchronizations(...) that accepts a
flag to include disabled entries) and use that result instead of
getAllEnabledSiteSynchronizations(false) so disabled pairs are also excluded
from auto-config proposals.
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`:
- Around line 135-141: The toggle handler currently only updates elem.title when
flipping the disabled state (using icon.classList.replace and isDisabled),
leaving the element's aria-label and any hidden screen-reader label stale;
update the accessible text in the same branch where you change the icon/title:
set elem.setAttribute('aria-label', appropriateLabel) for both enable/disable
cases and also update the hidden SR-only label node (e.g.,
elem.querySelector('.sr-only') or the specific hidden-label element used in the
template) to the corresponding enable/disable string so screen readers announce
the correct action after toggling.
---
Nitpick comments:
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 320-323: The allowed site-type check in
MicrosoftSynchronizationEnabler uses allowedTypes.contains(siteType) which is
case-sensitive; update the check to perform case-insensitive matching by
normalizing both the parsed config values and the site type (e.g., map
allowedTypes to lower-case when building from configValue and compare against
siteType.toLowerCase()) or use a case-insensitive comparison when checking
membership (reference symbols: configValue, allowedTypes, siteType in
MicrosoftSynchronizationEnabler).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7130796-36e2-4771-a6cb-6808e79b1112
📒 Files selected for processing (24)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieslibrary/src/skins/default/src/sass/modules/tool/sitemanage/_sitemanage.scssmicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.javamicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.htmlmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftCommonService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftSynchronizationService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/persistence/MicrosoftSiteSynchronizationRepository.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/jobs/RunSynchronizationsJob.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/persistence/MicrosoftSiteSynchronizationRepositoryImpl.javasite-manage/site-manage-tool/tool/pom.xmlsite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javasite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-addRemoveFeatureConfirm.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (9)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java (1)
1013-1041:⚠️ Potential issue | 🟠 MajorPropagate archive/unarchive failures instead of returning silent
false.At Line 1013 and Line 1028, both methods swallow non-credential exceptions and return
false. Since these booleans are easy to ignore at call sites, archive/unarchive failures can be treated as success and leave synchronization state inconsistent with Teams state.💡 Suggested direction
public boolean archiveTeam(String teamId) throws MicrosoftCredentialsException { try { getGraphClient().teams(teamId) .archive(null) .buildRequest() .post(); return true; } catch (MicrosoftCredentialsException e) { throw e; - } catch(Exception ex){ - log.debug("Error archiving Microsoft team: id={}", teamId); + } catch (Exception ex) { + log.warn("Error archiving Microsoft team: id={}", teamId, ex); + throw new RuntimeException("Failed to archive team " + teamId, ex); } - return false; } public boolean unarchiveTeam(String teamId) throws MicrosoftCredentialsException { try { getGraphClient().teams(teamId) .unarchive() .buildRequest() .post(); return true; } catch (MicrosoftCredentialsException e) { throw e; - } catch (Exception ex) { - log.debug("Error unarchiving Microsoft team: id={}", teamId); + } catch (Exception ex) { + log.warn("Error unarchiving Microsoft team: id={}", teamId, ex); + throw new RuntimeException("Failed to unarchive team " + teamId, ex); } - return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java` around lines 1013 - 1041, The archiveTeam and unarchiveTeam methods are swallowing non-credential exceptions and returning false, which can hide failures; update MicrosoftCommonServiceImpl.archiveTeam and MicrosoftCommonServiceImpl.unarchiveTeam to stop catching Exception and returning false—either remove the broad catch or rethrow the caught exception (wrap it in a RuntimeException or a domain-specific exception) after logging so failures propagate to callers, while still preserving the existing catch and rethrow for MicrosoftCredentialsException.microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java (1)
121-123:⚠️ Potential issue | 🟠 MajorExclude all existing synchronizations here, not only enabled ones.
At Line 121, using only enabled synchronizations allows disabled records to be re-proposed as “new” candidates. That can lead to duplicate/conflicting site-team proposals and avoidable failures later in the flow.
🛠️ Suggested fix
-//get all enabled synchronizations -List<SiteSynchronization> ssList = microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false); +//get all existing synchronizations (enabled + disabled) to avoid duplicate proposals +List<SiteSynchronization> ssList = microsoftSynchronizationService.getAllSiteSynchronizations(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java` around lines 121 - 123, The code fetches only enabled synchronizations into ssList via microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false), which allows disabled records to be re-proposed; change the call to retrieve all synchronizations (including disabled) instead—e.g., use the service method that returns all site synchronizations (replace getAllEnabledSiteSynchronizations(false) with the appropriate getAll...SiteSynchronizations(...) method or overload) so ssList contains every existing SiteSynchronization and disabled records are excluded from being treated as new.microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html (1)
135-141:⚠️ Potential issue | 🟠 MajorKeep the toggle’s accessible name in sync after state changes.
On success, only
titleis updated.aria-labeland hidden label text remain stale, so screen readers can announce the wrong action after toggling.As per coding guidelines, `**/*.{html,xhtml,vm,js,ts}`: Follow accessibility best practices.♿ Suggested fix
if (data.status == false) { showAjaxError(data.error); } else { + let nextLabel; if (!isDisabled) { icon.classList.replace('fa-toggle-on', 'fa-toggle-off'); - elem.title = /*[[#{index.enable}]]*/ ''; + nextLabel = /*[[#{index.enable}]]*/ ''; } else { icon.classList.replace('fa-toggle-off', 'fa-toggle-on'); - elem.title = /*[[#{index.disable}]]*/ ''; + nextLabel = /*[[#{index.disable}]]*/ ''; } + elem.title = nextLabel; + elem.setAttribute('aria-label', nextLabel); + const srText = elem.querySelector('.sr-only, .visually-hidden'); + if (srText) { + srText.textContent = nextLabel; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html` around lines 135 - 141, The toggle update only changes elem.title but leaves aria-label and any hidden label text stale; update the same branch where icon and elem.title are changed (the block referencing isDisabled, icon, elem) to also set elem.setAttribute('aria-label', /*[[#{index.enable}]]*/ '') or /*[[#{index.disable}]]*/ '' as appropriate and update any sibling/hidden label element (e.g., the visually-hidden span that provides the label) to the matching localized string so the accessible name always matches the visual state after toggling.site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (6)
167-170:⚠️ Potential issue | 🟡 MinorRemove the extra site property once, outside the team loop.
props.removeProperty(nameAndValueForSynchronization[0])only runs after a successfulteamIdcheck and inside thearchiveTeam()try-block. If there are no synchronizations, a row has noteamId, or the first archive call throws, the site-level property is left behind. Remove it once before the loop so disabling the toggle is consistent.Suggested fix
} else { props.removeProperty(SITE_PROPERTY); + if (nameAndValueForSynchronization.length > 0) { + props.removeProperty(nameAndValueForSynchronization[0]); + log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); + } // If a SiteSynchronization exists with a Team, archive it and delete it if (microsoftSynchronizationService != null && microsoftCommonService != null) { List<SiteSynchronization> ssList = microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()); if (ssList != null) { @@ - // Remove site property defined in microsoft.forced.synchronization.propertynameandoptionalvalue - if (nameAndValueForSynchronization.length > 0) { - props.removeProperty(nameAndValueForSynchronization[0]); - log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); - } } catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 167 - 170, The site-level property removal is currently inside the per-team archive try-block so it can be skipped; move the call to props.removeProperty(nameAndValueForSynchronization[0]) out of the team loop and execute it once before iterating teams (e.g., at start of the method or right before the loop that calls archiveTeam), ensuring nameAndValueForSynchronization is checked for length>0 first so you only remove when configured and do not repeat removal per team.
172-174:⚠️ Potential issue | 🟡 MinorPass the exception object directly to SLF4J in these catch blocks.
Using
e.toString()here drops the stack trace. Keep the parameterized message and passeas the final argument so these failures stay diagnosable.As per coding guidelines:
Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value)).Also applies to: 246-248, 261-263, 282-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 172 - 174, The catch blocks in MicrosoftSynchronizationEnabler are calling log.warn with e.toString(), which discards the stack trace; update each catch to pass the Exception object as the final SLF4J parameter (e.g., replace e.toString() with e) so the throwable is logged and stack traces are preserved—apply this change in the catch handling around the archive/disable/cleanup operations referenced (the catch blocks that log "Could not archive Microsoft Team for site..." and the other similar blocks at the noted locations).
133-146:⚠️ Potential issue | 🟠 MajorOnly call Microsoft APIs when the toggle actually changes.
STATE_KEYis set on every submit, so this method replayscreateOrRestoreMicrosoftTeamForSite(...)on every save with the box checked andarchiveTeam(...)on every save with it unchecked. That keeps issuing external Team mutations and resetting sync windows even when the setting did not change. Compare the requested value with the persisted site property first, then run the enable/disable side effects only on a real transition.Also applies to: 153-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 133 - 146, The code currently calls createOrRestoreMicrosoftTeamForSite(...) or archiveTeam(...) on every submit because STATE_KEY reflects the submitted checkbox rather than the persisted site setting; change MicrosoftSynchronizationEnabler to first read the existing persisted value from the site using SITE_PROPERTY (via site.getPropertiesEdit()/getProperty(SITE_PROPERTY) or the helper getSitePropertyNameAndOptionalValueForSynchronization()), compare that persisted boolean to the requested boolean from STATE_KEY, and only invoke createOrRestoreMicrosoftTeamForSite(...) (and related MicrosoftConfigurationService/MicrosoftCommonService calls) when persisted==false and requested==true, and only call archiveTeam(...) when persisted==true and requested==false, skipping external MicrosoftSynchronizationService calls when there is no state transition.
240-253:⚠️ Potential issue | 🟠 MajorDo not mark a synchronization enabled until the Team restore succeeds.
existingSs.setDisabled(false)andsaveOrUpdateSiteSynchronization(existingSs)run even whenteamIdis missing,unarchiveTeam()returnsfalse, or the call throws. That leaves an enabled row pointing at an archived or missing Team. Only persist the row after a successful unarchive.Suggested fix
for (SiteSynchronization existingSs : existingSsList) { if (existingSs.getTeamId() != null && !existingSs.getTeamId().isEmpty()) { try { if (microsoftCommonService.unarchiveTeam(existingSs.getTeamId())) { log.info("Microsoft Team unarchived for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); + existingSs.setSyncDateFrom(syncDateFrom); + existingSs.setSyncDateTo(syncDateTo); + existingSs.setDisabled(false); + microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs); + log.info("SiteSynchronization restored for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); } } catch (Exception e) { log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), existingSs.getTeamId(), e.toString()); } } - existingSs.setSyncDateFrom(syncDateFrom); - existingSs.setSyncDateTo(syncDateTo); - existingSs.setDisabled(false); - microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs); - log.info("SiteSynchronization restored for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 240 - 253, The code currently marks synchronizations enabled (existingSs.setDisabled(false); microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs)) regardless of teamId presence or whether microsoftCommonService.unarchiveTeam(...) succeeded; change the flow so you only call existingSs.setDisabled(false) and saveOrUpdateSiteSynchronization(existingSs) after a successful unarchive: check existingSs.getTeamId() is non-null/non-empty, call microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) inside the try and only when it returns true setDisabled(false) and persist; if unarchive returns false or throws, do not modify or save existingSs (log and continue). Ensure you reference the same methods: microsoftCommonService.unarchiveTeam, existingSs.setDisabled, and microsoftSynchronizationService.saveOrUpdateSiteSynchronization.
156-180:⚠️ Potential issue | 🟠 MajorScope this toggle to forced synchronizations only.
This class creates rows with
forced(true)on Lines 271-277, but both enable/disable loops currently operate on everySiteSynchronizationfor the site. Toggling the site setting can archive or re-enable unrelated manual Teams, and if only manual rows exist this path never creates the forced row. Filter both paths to forced rows before mutating Team state ordisabled.Also applies to: 237-255, 271-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 156 - 180, The loops in MicrosoftSynchronizationEnabler operate on all SiteSynchronization rows but must only touch forced synchronizations; update both places that iterate microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()) (the archival/disable loop near the shown diff and the enable/creation loop around where forced(true) rows are created) to filter the list to only SiteSynchronization instances with the forced flag set (e.g., ss.isForced() or ss.getForced() depending on the existing getter) before calling microsoftCommonService.archiveTeam(ss.getTeamId()), props.removeProperty(...), ss.setDisabled(true), or microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); ensure creation logic still creates a forced(true) row only when no forced rows exist.
258-284:⚠️ Potential issue | 🔴 CriticalDo not swallow failures after
createTeam()succeeds.
createTeam()is the external write. IfsaveOrUpdateSiteSynchronization(ss)throws, the outer catch only logs and returns, so the site save can continue with a Team created remotely but no localSiteSynchronization. The next retry can create a second Team for the same site. SinceMicrosoftSynchronizationEnabler.prepareSiteForSave(...)is called beforesiteService.save(...)and outside its try/catch insite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java, Lines 8887-8903, this path should roll back the Team or rethrow to abort the save.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 258 - 284, The code currently swallows failures after microsoftCommonService.createTeam(...) succeeds so a remote Team can be created while saveOrUpdateSiteSynchronization(ss) fails; update MicrosoftSynchronizationEnabler to handle this by, after calling saveOrUpdateSiteSynchronization(ss), catching exceptions and if a teamId was created attempt to roll back the external write (e.g., call microsoftCommonService.deleteTeam(teamId) or an equivalent rollback method) and then rethrow the exception to abort the wider site save; if rollback itself fails, log both failures and rethrow a wrapped RuntimeException so the outer save is aborted; reference the createTeam(...) call, the SiteSynchronization builder and microsoftSynchronizationService.saveOrUpdateSiteSynchronization(...) in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties`:
- Around line 5352-5354: The documentation incorrectly states a default of
"microsoft.synchronized=true"; update the comment for the property key
microsoft.forced.synchronization.propertynameandoptionalvalue to indicate that
there is no default (the setting is unset/blank by default and no site-property
is applied) so administrators are not misled—replace the stated default with a
clear note such as "DEFAULT: none (setting is blank/unset by default; no
property is applied)" and keep the existing guidance about adding a name/value
in the Microsoft Admin Tool.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 333-341: The method
getSitePropertyNameAndOptionalValueForSynchronization reads
SAKAI_PROPERTY_SITE_PROPERTY_NAME_AND_OPTIONAL_VALUE into configValue but
currently uses split("=",2) without trimming, so names or values can keep
surrounding whitespace; update the logic to trim both the name and value parts
(e.g., after split, call trim() on each), and if the trimmed name is blank then
return an empty array; also handle the no-equals case by returning new String[]
{ configValue.trim(), "" } so the property name is always validated and
whitespace-free.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Around line 8891-8894: The
MicrosoftSynchronizationEnabler.prepareSiteForSave(...) call is performing
durable provisioning (Teams creation/archiving and SiteSynchronization updates)
too early; move any non-mutating staging logic needed before save into a
pure-preparation step and defer durable provisioning until after a successful
save/commit (i.e., behind the existing success guard where STATE_MESSAGE and
save/commit have completed)—update SiteAction (and saveFeatures) to call only
the pre-save staging variant before siteService.save/commitSite and invoke the
post-commit provisioning variant of MicrosoftSynchronizationEnabler (or a new
method) only when the save succeeded; ensure SubNavEnabler.prepareSiteForSave
remains safe and adjust references where
MicrosoftSynchronizationEnabler.prepareSiteForSave is called (including the
other occurrence around lines 12300-12305) to the two-step approach.
---
Duplicate comments:
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java`:
- Around line 121-123: The code fetches only enabled synchronizations into
ssList via
microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false), which
allows disabled records to be re-proposed; change the call to retrieve all
synchronizations (including disabled) instead—e.g., use the service method that
returns all site synchronizations (replace
getAllEnabledSiteSynchronizations(false) with the appropriate
getAll...SiteSynchronizations(...) method or overload) so ssList contains every
existing SiteSynchronization and disabled records are excluded from being
treated as new.
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`:
- Around line 135-141: The toggle update only changes elem.title but leaves
aria-label and any hidden label text stale; update the same branch where icon
and elem.title are changed (the block referencing isDisabled, icon, elem) to
also set elem.setAttribute('aria-label', /*[[#{index.enable}]]*/ '') or
/*[[#{index.disable}]]*/ '' as appropriate and update any sibling/hidden label
element (e.g., the visually-hidden span that provides the label) to the matching
localized string so the accessible name always matches the visual state after
toggling.
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java`:
- Around line 1013-1041: The archiveTeam and unarchiveTeam methods are
swallowing non-credential exceptions and returning false, which can hide
failures; update MicrosoftCommonServiceImpl.archiveTeam and
MicrosoftCommonServiceImpl.unarchiveTeam to stop catching Exception and
returning false—either remove the broad catch or rethrow the caught exception
(wrap it in a RuntimeException or a domain-specific exception) after logging so
failures propagate to callers, while still preserving the existing catch and
rethrow for MicrosoftCredentialsException.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 167-170: The site-level property removal is currently inside the
per-team archive try-block so it can be skipped; move the call to
props.removeProperty(nameAndValueForSynchronization[0]) out of the team loop and
execute it once before iterating teams (e.g., at start of the method or right
before the loop that calls archiveTeam), ensuring nameAndValueForSynchronization
is checked for length>0 first so you only remove when configured and do not
repeat removal per team.
- Around line 172-174: The catch blocks in MicrosoftSynchronizationEnabler are
calling log.warn with e.toString(), which discards the stack trace; update each
catch to pass the Exception object as the final SLF4J parameter (e.g., replace
e.toString() with e) so the throwable is logged and stack traces are
preserved—apply this change in the catch handling around the
archive/disable/cleanup operations referenced (the catch blocks that log "Could
not archive Microsoft Team for site..." and the other similar blocks at the
noted locations).
- Around line 133-146: The code currently calls
createOrRestoreMicrosoftTeamForSite(...) or archiveTeam(...) on every submit
because STATE_KEY reflects the submitted checkbox rather than the persisted site
setting; change MicrosoftSynchronizationEnabler to first read the existing
persisted value from the site using SITE_PROPERTY (via
site.getPropertiesEdit()/getProperty(SITE_PROPERTY) or the helper
getSitePropertyNameAndOptionalValueForSynchronization()), compare that persisted
boolean to the requested boolean from STATE_KEY, and only invoke
createOrRestoreMicrosoftTeamForSite(...) (and related
MicrosoftConfigurationService/MicrosoftCommonService calls) when
persisted==false and requested==true, and only call archiveTeam(...) when
persisted==true and requested==false, skipping external
MicrosoftSynchronizationService calls when there is no state transition.
- Around line 240-253: The code currently marks synchronizations enabled
(existingSs.setDisabled(false);
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs))
regardless of teamId presence or whether
microsoftCommonService.unarchiveTeam(...) succeeded; change the flow so you only
call existingSs.setDisabled(false) and
saveOrUpdateSiteSynchronization(existingSs) after a successful unarchive: check
existingSs.getTeamId() is non-null/non-empty, call
microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) inside the try and
only when it returns true setDisabled(false) and persist; if unarchive returns
false or throws, do not modify or save existingSs (log and continue). Ensure you
reference the same methods: microsoftCommonService.unarchiveTeam,
existingSs.setDisabled, and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization.
- Around line 156-180: The loops in MicrosoftSynchronizationEnabler operate on
all SiteSynchronization rows but must only touch forced synchronizations; update
both places that iterate
microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()) (the
archival/disable loop near the shown diff and the enable/creation loop around
where forced(true) rows are created) to filter the list to only
SiteSynchronization instances with the forced flag set (e.g., ss.isForced() or
ss.getForced() depending on the existing getter) before calling
microsoftCommonService.archiveTeam(ss.getTeamId()), props.removeProperty(...),
ss.setDisabled(true), or
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); ensure
creation logic still creates a forced(true) row only when no forced rows exist.
- Around line 258-284: The code currently swallows failures after
microsoftCommonService.createTeam(...) succeeds so a remote Team can be created
while saveOrUpdateSiteSynchronization(ss) fails; update
MicrosoftSynchronizationEnabler to handle this by, after calling
saveOrUpdateSiteSynchronization(ss), catching exceptions and if a teamId was
created attempt to roll back the external write (e.g., call
microsoftCommonService.deleteTeam(teamId) or an equivalent rollback method) and
then rethrow the exception to abort the wider site save; if rollback itself
fails, log both failures and rethrow a wrapped RuntimeException so the outer
save is aborted; reference the createTeam(...) call, the SiteSynchronization
builder and microsoftSynchronizationService.saveOrUpdateSiteSynchronization(...)
in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42a45448-dd82-45d1-abf7-53694bb9db0b
📒 Files selected for processing (24)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieslibrary/src/skins/default/src/sass/modules/tool/sitemanage/_sitemanage.scssmicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.javamicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.htmlmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftCommonService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftSynchronizationService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/persistence/MicrosoftSiteSynchronizationRepository.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/jobs/RunSynchronizationsJob.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/persistence/MicrosoftSiteSynchronizationRepositoryImpl.javasite-manage/site-manage-tool/tool/pom.xmlsite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javasite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-addRemoveFeatureConfirm.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm
Comment on lines
+5352
to
+5354
| #SAK-52355 Enable forced synchronization in Microsoft Teams | ||
| #DEFAULT: microsoft.synchronized=true, use your own name and value property. IMPORTANT: The administrator should add this name and optional value in the Microsoft Admin Tool settings, in the corresponding "Site property" input. | ||
| #microsoft.forced.synchronization.propertynameandoptionalvalue=microsoft.synchronized=true |
Contributor
There was a problem hiding this comment.
Correct the documented default for forced site-property mapping.
Line 5353 currently states a default of microsoft.synchronized=true, but runtime behavior treats this setting as optional/unset by default (no property is applied when blank). This can mislead admins during configuration.
🛠️ Proposed doc-only fix
-#DEFAULT: microsoft.synchronized=true, use your own name and value property. IMPORTANT: The administrator should add this name and optional value in the Microsoft Admin Tool settings, in the corresponding "Site property" input.
-#microsoft.forced.synchronization.propertynameandoptionalvalue=microsoft.synchronized=true
+#DEFAULT: none (no forced site property is applied unless explicitly configured)
+#EXAMPLE: microsoft.synchronized=true
+#IMPORTANT: The administrator should add this name and optional value in the Microsoft Admin Tool settings, in the corresponding "Site property" input.
+#microsoft.forced.synchronization.propertynameandoptionalvalue=microsoft.synchronized=true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties`
around lines 5352 - 5354, The documentation incorrectly states a default of
"microsoft.synchronized=true"; update the comment for the property key
microsoft.forced.synchronization.propertynameandoptionalvalue to indicate that
there is no default (the setting is unset/blank by default and no site-property
is applied) so administrators are not misled—replace the stated default with a
clear note such as "DEFAULT: none (setting is blank/unset by default; no
property is applied)" and keep the existing guidance about adding a name/value
in the Microsoft Admin Tool.
Comment on lines
+333
to
+341
| private static String[] getSitePropertyNameAndOptionalValueForSynchronization() { | ||
| final String configValue = ServerConfigurationService.getString(SAKAI_PROPERTY_SITE_PROPERTY_NAME_AND_OPTIONAL_VALUE, null); | ||
| if (configValue == null || configValue.isBlank()) { | ||
| return new String[0]; | ||
| } | ||
| if (configValue.contains("=")) { | ||
| return configValue.split("=", 2); | ||
| } | ||
| return new String[] { configValue.trim(), "" }; |
Contributor
There was a problem hiding this comment.
Trim and validate the configured name=value pair.
split("=", 2) preserves whitespace, so a value like microsoft.synchronized = true will create/remove a property named microsoft.synchronized instead of microsoft.synchronized. Please trim both parts and ignore blank property names before returning.
Suggested fix
if (configValue.contains("=")) {
- return configValue.split("=", 2);
+ String[] parts = configValue.split("=", 2);
+ String propertyName = parts[0].trim();
+ String propertyValue = parts[1].trim();
+ return propertyName.isEmpty() ? new String[0] : new String[] { propertyName, propertyValue };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`
around lines 333 - 341, The method
getSitePropertyNameAndOptionalValueForSynchronization reads
SAKAI_PROPERTY_SITE_PROPERTY_NAME_AND_OPTIONAL_VALUE into configValue but
currently uses split("=",2) without trimming, so names or values can keep
surrounding whitespace; update the logic to trim both the name and value parts
(e.g., after split, call trim() on each), and if the trimmed name is blank then
return an empty array; also handle the no-equals case by returning new String[]
{ configValue.trim(), "" } so the property name is always validated and
whitespace-free.
Comment on lines
8891
to
+8894
| SubNavEnabler.prepareSiteForSave(Site, state); | ||
|
|
||
| MicrosoftSynchronizationEnabler.prepareSiteForSave(Site, state); | ||
|
|
Contributor
There was a problem hiding this comment.
Don't run Microsoft provisioning before the site save is known-good.
MicrosoftSynchronizationEnabler.prepareSiteForSave(...) is doing more than staging site properties: in site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java:126-187 it archives/creates Teams and updates SiteSynchronization records. Calling it here means validation failures (STATE_MESSAGE already set), or a later siteService.save(...) / commitSite(...) failure, can still leave Microsoft-side changes applied while the Sakai site change is not persisted. In saveFeatures(...), its return value also is not a safe dirty-check because the helper returns true for any allowed site. At minimum these calls need to stay behind the existing success guard; the durable fix is to split this into a pure “mutate site properties” step before save and a post-commit provisioning step after a successful site save.
Also applies to: 12300-12305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`
around lines 8891 - 8894, The
MicrosoftSynchronizationEnabler.prepareSiteForSave(...) call is performing
durable provisioning (Teams creation/archiving and SiteSynchronization updates)
too early; move any non-mutating staging logic needed before save into a
pure-preparation step and defer durable provisioning until after a successful
save/commit (i.e., behind the existing success guard where STATE_MESSAGE and
save/commit have completed)—update SiteAction (and saveFeatures) to call only
the pre-save staging variant before siteService.save/commitSite and invoke the
post-commit provisioning variant of MicrosoftSynchronizationEnabler (or a new
method) only when the save succeeded; ensure SubNavEnabler.prepareSiteForSave
remains safe and adjust references where
MicrosoftSynchronizationEnabler.prepareSiteForSave is called (including the
other occurrence around lines 12300-12305) to the two-step approach.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java (1)
1013-1041:⚠️ Potential issue | 🟠 MajorArchive/unarchive still fail silently, so state can drift from Teams reality.
At Line 1023 and Line 1038, generic failures are swallowed and converted to
false. If callers don’t enforce the boolean, synchronization can be marked enabled/disabled even when Graph archive/unarchive failed. This was already raised earlier and is still present.Suggested fail-loud adjustment
public boolean archiveTeam(String teamId) throws MicrosoftCredentialsException { try { getGraphClient().teams(teamId) .archive(null) .buildRequest() .post(); return true; } catch(MicrosoftCredentialsException e) { throw e; } catch(Exception ex){ - log.debug("Error archiving Microsoft team: id={}", teamId); + log.warn("Error archiving Microsoft team: id={}", teamId, ex); + throw new IllegalStateException("Unable to archive Microsoft team: " + teamId, ex); } - return false; } public boolean unarchiveTeam(String teamId) throws MicrosoftCredentialsException { try { getGraphClient().teams(teamId) .unarchive() .buildRequest() .post(); return true; } catch (MicrosoftCredentialsException e) { throw e; } catch (Exception ex) { - log.debug("Error unarchiving Microsoft team: id={}", teamId); + log.warn("Error unarchiving Microsoft team: id={}", teamId, ex); + throw new IllegalStateException("Unable to unarchive Microsoft team: " + teamId, ex); } - return false; }Run this to verify whether any call sites still ignore the return value:
#!/bin/bash set -euo pipefail # 1) Show all call sites with context rg -nP --type=java '\b(?:archiveTeam|unarchiveTeam)\s*\(' -C3 # 2) Detect standalone invocations where boolean is likely ignored ast-grep --pattern '$OBJ.archiveTeam($ARG);' ast-grep --pattern '$OBJ.unarchiveTeam($ARG);'Expected result: no standalone invocations; every call should branch on the boolean or handle thrown failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java` around lines 1013 - 1041, The archiveTeam and unarchiveTeam methods silently swallow non-MicrosoftCredentialsException failures (catching Exception and returning false), which can hide Graph API errors; change both methods (archiveTeam, unarchiveTeam) to log the exception with full details (use log.error(..., ex)) and rethrow a runtime (or a specific unchecked) exception wrapping the original (e.g., throw new RuntimeException("Failed to archiveTeam: "+teamId, ex) / "Failed to unarchiveTeam"), so callers cannot ignore failures; keep MicrosoftCredentialsException propagated as-is and perform the same change in the getGraphClient().teams(...).archive(...) and .unarchive() blocks.config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties (1)
5352-5354:⚠️ Potential issue | 🟡 MinorDocumented default is still misleading for forced site-property mapping.
The comment still says the default ismicrosoft.synchronized=true, which can mislead administrators if the effective default is unset/blank.🛠️ Proposed doc-only fix
-#DEFAULT: microsoft.synchronized=true, use your own name and value property. IMPORTANT: The administrator should add this name and optional value in the Microsoft Admin Tool settings, in the corresponding "Site property" input. +#DEFAULT: none (no forced site property is applied unless explicitly configured) +#EXAMPLE: microsoft.synchronized=true +#IMPORTANT: The administrator should add this name and optional value in the Microsoft Admin Tool settings, in the corresponding "Site property" input. `#microsoft.forced.synchronization.propertynameandoptionalvalue`=microsoft.synchronized=true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties` around lines 5352 - 5354, The comment for microsoft.forced.synchronization.propertynameandoptionalvalue incorrectly states a default of "microsoft.synchronized=true"; update the documentation comment to reflect that there is no predefined default (it is unset/blank) and instruct administrators to explicitly set the exact site-property and optional value (e.g., "microsoft.synchronized=true") in the Microsoft Admin Tool when needed; ensure the updated comment replaces the misleading "DEFAULT: microsoft.synchronized=true" text and clearly states the effective default is blank and must be configured by the admin.microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html (1)
135-141:⚠️ Potential issue | 🟠 MajorUpdate all accessible labels when the toggle state changes.
On Line 137 and Line 140 you only update
title;aria-labeland the hidden text remain stale after a successful toggle, so screen readers can announce the wrong action.♿ Suggested fix
if (data.status == false) { showAjaxError(data.error); } else { + let nextLabel; if (!isDisabled) { icon.classList.replace('fa-toggle-on', 'fa-toggle-off'); - elem.title = /*[[#{index.enable}]]*/ ''; + nextLabel = /*[[#{index.enable}]]*/ ''; } else { icon.classList.replace('fa-toggle-off', 'fa-toggle-on'); - elem.title = /*[[#{index.disable}]]*/ ''; + nextLabel = /*[[#{index.disable}]]*/ ''; } + elem.title = nextLabel; + elem.setAttribute('aria-label', nextLabel); + const srText = elem.querySelector('.sr-only, .visually-hidden'); + if (srText) { + srText.textContent = nextLabel; + } }As per coding guidelines,
**/*.{html,xhtml,vm,js,ts}: Follow accessibility best practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html` around lines 135 - 141, The toggle handler only updates title but not accessibility labels; when switching state (variables icon, elem, isDisabled) also update elem.setAttribute('aria-label', ...) to match the new action and update the visible hidden text used for screen readers (e.g., the '.sr-only' or equivalent child text node under elem) so both aria-label and the hidden text mirror the title change; ensure the same strings used for /*[[#{index.enable}]]*/ and /*[[#{index.disable}]]*/ are applied to title, aria-label, and the hidden text in both the isDisabled true and false branches.site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java (1)
8891-8895:⚠️ Potential issue | 🔴 CriticalDefer Microsoft provisioning until after the Sakai save/commit succeeds.
MicrosoftSynchronizationEnabler.prepareSiteForSave(...)is still invoked before the enclosing save flow is known-good. If that helper still archives/creates Teams or writes synchronization rows, you can leave Microsoft-side changes behind when the Sakai save later fails;saveFeatures(...)is especially risky because this call runs outside anySTATE_MESSAGEsuccess guard. Please keep the pre-save phase side-effect free and move durable Microsoft provisioning behind a successful save/commit.Run this read-only check to confirm whether
prepareSiteForSave(...)still performs durable work:#!/bin/bash set -euo pipefail helper="$(fd 'MicrosoftSynchronizationEnabler.java' | head -n1)" if [[ -z "$helper" ]]; then echo "MicrosoftSynchronizationEnabler.java not found" >&2 exit 1 fi python - "$helper" <<'PY' from pathlib import Path import re import sys path = Path(sys.argv[1]) text = path.read_text() m = re.search(r'prepareSiteForSave\s*\([^)]*\)\s*\{', text) if not m: print("prepareSiteForSave(...) not found") raise SystemExit(1) start = m.start() depth = 0 end = None for i, ch in enumerate(text[start:], start): if ch == '{': depth += 1 elif ch == '}': depth -= 1 if depth == 0: end = i + 1 break snippet = text[start:end] print(f"== {path} :: prepareSiteForSave(...) ==") print(snippet) print("\n== suspicious side effects ==") for lineno, line in enumerate(snippet.splitlines(), 1): if re.search(r'archive|unarchive|create|save\s*\(|insert|update|delete|synchron|team', line, re.I): print(f"{lineno}: {line}") PYExpected:
prepareSiteForSave(...)should only stage in-memorySite/state changes. If this prints Microsoft API calls or persistence calls, the helper still needs to be split into pre-save staging and post-commit provisioning.Also applies to: 12300-12305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java` around lines 8891 - 8895, prepareSiteForSave in MicrosoftSynchronizationEnabler is performing durable Microsoft-side work before the enclosing Sakai save/commit, risking orphaned Teams/rows if the save fails; change the helper so prepareSiteForSave(...) only stages in-memory Site/state changes and move any calls that archive/create Teams or write persistence (e.g., anything that calls Microsoft APIs, saveFeatures(...), insert/update/delete, or writes synchronization rows) into a separate post-commit method (e.g., provisionAfterCommit or finalizeMicrosoftSync) and invoke that method only after the surrounding save flow confirms success (check STATE_MESSAGE == null and after the commit point) so durable provisioning happens only on successful saves.site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (2)
145-146:⚠️ Potential issue | 🟠 MajorOnly provision on the false → true transition.
This still calls
createOrRestoreMicrosoftTeamForSite(...)on every save when the checkbox remains checked. Unchanged sites will keep getting restore calls and sync windows reset. Compare against the persisted site flag fromisEnabledForSite(site)and only provision when the setting actually transitions from disabled to enabled.Also applies to: 293-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 145 - 146, The code calls createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService) on every save even when the checkbox remains true; change the logic to first read the persisted enabled state via isEnabledForSite(site) and only invoke createOrRestoreMicrosoftTeamForSite when the persisted state is false and the incoming/save state is true (i.e., false→true transition). Apply the same guard to the other occurrence around the lines handling the save (the block flagged at 293-295) so provisioning runs only on the transition from disabled to enabled.
156-180:⚠️ Potential issue | 🟠 MajorRestrict the site toggle to forced synchronizations.
These paths operate on every
SiteSynchronizationfor the site, but the record created here is explicitlyforced(true). As written, disabling can archive manual Teams and enabling can restore manual rows or skip creating the forced row whenever any non-forced synchronization already exists. Filter the existing rows to forced entries only before archiving, restoring, disabling, or deciding whether a new forced synchronization is needed.Also applies to: 237-255, 271-277
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 156 - 180, The code is operating on all SiteSynchronization rows but must only act on forced synchronizations; update MicrosoftSynchronizationEnabler to filter the list returned by microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()) to only include forced entries (check the SiteSynchronization forced flag, e.g., SiteSynchronization.isForced() or getForced()) before iterating for calls to microsoftCommonService.archiveTeam(...), removing the site property nameAndValueForSynchronization[0], setting ss.setDisabled(true), and calling microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss). Apply the same forced-only filter logic to the other blocks mentioned (the enable/restore paths around the save/create logic) so creation/restore/disable only considers forced synchronizations.
🧹 Nitpick comments (1)
site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vm (1)
288-297: Consider using a semantically appropriate CSS class.The Microsoft synchronization row reuses
class="summary-subnav-enabled"from the SubPageNav row. While this works for styling, the class name semantically references "subnav" which doesn't apply to Microsoft synchronization.♻️ Suggested improvement
Consider creating a more generic class name or a Microsoft-specific class:
- <tr class="summary-subnav-enabled"> + <tr class="summary-microsoft-sync-enabled">Alternatively, if the intent is shared styling, rename the common class to something generic like
summary-feature-enabledand update both the SubPageNav and Microsoft sync rows to use it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vm` around lines 288 - 297, The row for Microsoft synchronization uses a semantically misleading class name "summary-subnav-enabled"; change it to a generic or Microsoft-specific class (e.g., "summary-feature-enabled" or "summary-ms-sync-enabled") and update the corresponding CSS rules so styling is preserved; also update any other templates that currently use "summary-subnav-enabled" (such as the SubPageNav code that shares the class) to use the new generic name if the style is meant to be shared, or leave SubPageNav with its original semantic class and only change the Microsoft sync row to the new Microsoft-specific class.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@microsoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.java`:
- Around line 90-92: The new non-null field "disabled" in the
SiteSynchronization entity requires a DB migration because repository queries
like findAllEnabled() filter on disabled = false; add SQL migration scripts to
the module's migration folder following existing Sakai naming/patterns (create
separate migrations for hsqldb, mysql, oracle, postgresql) that run an ALTER
TABLE mc_site_synchronization ADD COLUMN disabled boolean NOT NULL DEFAULT 0
(use the DB-specific literal/types for each DB) so existing schemas gain the
non-nullable column with a default of false.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 141-150: The code sets SITE_PROPERTY and optional forced-sync
properties before calling createOrRestoreMicrosoftTeamForSite(...), allowing the
site to be marked enabled even when provisioning fails; change
createOrRestoreMicrosoftTeamForSite to return a boolean (or throw on failure)
and only call props.addProperty(SITE_PROPERTY, ...) and add the
nameAndValueForSynchronization property after the helper reports success (or
catch/propagate the exception), and update the other call sites of
createOrRestoreMicrosoftTeamForSite (the other occurrences noted) to follow the
same pattern so properties are persisted only after a successful create/restore.
- Around line 243-254: The code in MicrosoftSynchronizationEnabler.java
currently catches exceptions around microsoftCommonService.unarchiveTeam(...)
and then proceeds to call
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs)
while swallowing any persistence errors; instead, ensure failures to persist the
restored state are not suppressed: either rethrow the exception from the
surrounding try/catch so callers can react, or implement a compensating rollback
(e.g., call microsoftCommonService.archiveTeam(existingSs.getTeamId()) on
persistence failure) when saveOrUpdateSiteSynchronization(existingSs) throws;
specifically update the block that calls
microsoftCommonService.unarchiveTeam(...) and then
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs) so
that persistence exceptions are propagated or trigger rollback rather than just
logging and returning.
---
Duplicate comments:
In
`@config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties`:
- Around line 5352-5354: The comment for
microsoft.forced.synchronization.propertynameandoptionalvalue incorrectly states
a default of "microsoft.synchronized=true"; update the documentation comment to
reflect that there is no predefined default (it is unset/blank) and instruct
administrators to explicitly set the exact site-property and optional value
(e.g., "microsoft.synchronized=true") in the Microsoft Admin Tool when needed;
ensure the updated comment replaces the misleading "DEFAULT:
microsoft.synchronized=true" text and clearly states the effective default is
blank and must be configured by the admin.
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`:
- Around line 135-141: The toggle handler only updates title but not
accessibility labels; when switching state (variables icon, elem, isDisabled)
also update elem.setAttribute('aria-label', ...) to match the new action and
update the visible hidden text used for screen readers (e.g., the '.sr-only' or
equivalent child text node under elem) so both aria-label and the hidden text
mirror the title change; ensure the same strings used for
/*[[#{index.enable}]]*/ and /*[[#{index.disable}]]*/ are applied to title,
aria-label, and the hidden text in both the isDisabled true and false branches.
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java`:
- Around line 1013-1041: The archiveTeam and unarchiveTeam methods silently
swallow non-MicrosoftCredentialsException failures (catching Exception and
returning false), which can hide Graph API errors; change both methods
(archiveTeam, unarchiveTeam) to log the exception with full details (use
log.error(..., ex)) and rethrow a runtime (or a specific unchecked) exception
wrapping the original (e.g., throw new RuntimeException("Failed to archiveTeam:
"+teamId, ex) / "Failed to unarchiveTeam"), so callers cannot ignore failures;
keep MicrosoftCredentialsException propagated as-is and perform the same change
in the getGraphClient().teams(...).archive(...) and .unarchive() blocks.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 145-146: The code calls createOrRestoreMicrosoftTeamForSite(site,
microsoftCommonService, microsoftConfigurationService,
microsoftSynchronizationService) on every save even when the checkbox remains
true; change the logic to first read the persisted enabled state via
isEnabledForSite(site) and only invoke createOrRestoreMicrosoftTeamForSite when
the persisted state is false and the incoming/save state is true (i.e.,
false→true transition). Apply the same guard to the other occurrence around the
lines handling the save (the block flagged at 293-295) so provisioning runs only
on the transition from disabled to enabled.
- Around line 156-180: The code is operating on all SiteSynchronization rows but
must only act on forced synchronizations; update MicrosoftSynchronizationEnabler
to filter the list returned by
microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()) to
only include forced entries (check the SiteSynchronization forced flag, e.g.,
SiteSynchronization.isForced() or getForced()) before iterating for calls to
microsoftCommonService.archiveTeam(...), removing the site property
nameAndValueForSynchronization[0], setting ss.setDisabled(true), and calling
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss). Apply the
same forced-only filter logic to the other blocks mentioned (the enable/restore
paths around the save/create logic) so creation/restore/disable only considers
forced synchronizations.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Around line 8891-8895: prepareSiteForSave in MicrosoftSynchronizationEnabler
is performing durable Microsoft-side work before the enclosing Sakai
save/commit, risking orphaned Teams/rows if the save fails; change the helper so
prepareSiteForSave(...) only stages in-memory Site/state changes and move any
calls that archive/create Teams or write persistence (e.g., anything that calls
Microsoft APIs, saveFeatures(...), insert/update/delete, or writes
synchronization rows) into a separate post-commit method (e.g.,
provisionAfterCommit or finalizeMicrosoftSync) and invoke that method only after
the surrounding save flow confirms success (check STATE_MESSAGE == null and
after the commit point) so durable provisioning happens only on successful
saves.
---
Nitpick comments:
In
`@site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vm`:
- Around line 288-297: The row for Microsoft synchronization uses a semantically
misleading class name "summary-subnav-enabled"; change it to a generic or
Microsoft-specific class (e.g., "summary-feature-enabled" or
"summary-ms-sync-enabled") and update the corresponding CSS rules so styling is
preserved; also update any other templates that currently use
"summary-subnav-enabled" (such as the SubPageNav code that shares the class) to
use the new generic name if the style is meant to be shared, or leave SubPageNav
with its original semantic class and only change the Microsoft sync row to the
new Microsoft-specific class.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ffcf4d7-171a-4f3b-9dbb-dcb126985197
📒 Files selected for processing (24)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieslibrary/src/skins/default/src/sass/modules/tool/sitemanage/_sitemanage.scssmicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.javamicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.htmlmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftCommonService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftSynchronizationService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/persistence/MicrosoftSiteSynchronizationRepository.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/jobs/RunSynchronizationsJob.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/persistence/MicrosoftSiteSynchronizationRepositoryImpl.javasite-manage/site-manage-tool/tool/pom.xmlsite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javasite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-addRemoveFeatureConfirm.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm
Comment on lines
+141
to
+150
| props.addProperty(SITE_PROPERTY, Boolean.TRUE.toString()); | ||
|
|
||
| MicrosoftConfigurationService microsoftConfigurationService = (MicrosoftConfigurationService) ComponentManager.get(MicrosoftConfigurationService.class); | ||
|
|
||
| // Check if MicrosoftSynchronization was already enabled for the site before creating/restoring the Team | ||
| createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService); | ||
|
|
||
| // Add site property defined in microsoft.forced.synchronization.propertynameandoptionalvalue | ||
| if (nameAndValueForSynchronization.length > 0) { | ||
| props.addProperty(nameAndValueForSynchronization[0], nameAndValueForSynchronization[1]); |
Contributor
There was a problem hiding this comment.
Persist the site-level enablement only after provisioning succeeds.
SITE_PROPERTY and the optional forced-sync property are written before createOrRestoreMicrosoftTeamForSite(...) proves that a Team/synchronization is actually ready. Because that helper returns early on null services, missing credentials, blank teamId, and generic failures, the site can be saved as enabled while there is still no active SiteSynchronization. Make the helper return success/failure (or throw) and only persist these properties after a successful restore/create.
Also applies to: 220-230, 261-268, 282-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`
around lines 141 - 150, The code sets SITE_PROPERTY and optional forced-sync
properties before calling createOrRestoreMicrosoftTeamForSite(...), allowing the
site to be marked enabled even when provisioning fails; change
createOrRestoreMicrosoftTeamForSite to return a boolean (or throw on failure)
and only call props.addProperty(SITE_PROPERTY, ...) and add the
nameAndValueForSynchronization property after the helper reports success (or
catch/propagate the exception), and update the other call sites of
createOrRestoreMicrosoftTeamForSite (the other occurrences noted) to follow the
same pattern so properties are persisted only after a successful create/restore.
Comment on lines
+243
to
+254
| if (microsoftCommonService.unarchiveTeam(existingSs.getTeamId())) { | ||
| log.info("Microsoft Team unarchived for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); | ||
| } | ||
| } catch (Exception e) { | ||
| log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), existingSs.getTeamId(), e.toString()); | ||
| } | ||
| } | ||
| existingSs.setSyncDateFrom(syncDateFrom); | ||
| existingSs.setSyncDateTo(syncDateTo); | ||
| existingSs.setDisabled(false); | ||
| microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs); | ||
| log.info("SiteSynchronization restored for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); |
Contributor
There was a problem hiding this comment.
Don't suppress restore persistence failures after unarchiveTeam().
After a successful unarchiveTeam(...), saveOrUpdateSiteSynchronization(existingSs) is the only step that records the restored state locally. microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.java:286-292 delegates that method straight to repository save/update, so if persistence fails here the outer catch only logs and returns, leaving the Team restored in Microsoft while the local row can stay disabled or keep stale sync dates. Propagate the exception, or add compensating rollback, instead of swallowing it.
Also applies to: 282-284
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`
around lines 243 - 254, The code in MicrosoftSynchronizationEnabler.java
currently catches exceptions around microsoftCommonService.unarchiveTeam(...)
and then proceeds to call
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs)
while swallowing any persistence errors; instead, ensure failures to persist the
restored state are not suppressed: either rethrow the exception from the
surrounding try/catch so callers can react, or implement a compensating rollback
(e.g., call microsoftCommonService.archiveTeam(existingSs.getTeamId()) on
persistence failure) when saveOrUpdateSiteSynchronization(existingSs) throws;
specifically update the block that calls
microsoftCommonService.unarchiveTeam(...) and then
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs) so
that persistence exceptions are propagated or trigger rollback rather than just
logging and returning.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (13)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java (1)
1013-1041:⚠️ Potential issue | 🟠 MajorarchiveTeam/unarchiveTeam still permit silent failure paths.
These methods swallow generic failures and return
false; downstream flows can continue mutating synchronization state even when the Teams archive/unarchive operation did not succeed, causing state drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java` around lines 1013 - 1041, archiveTeam and unarchiveTeam currently catch generic Exception, log and return false which allows callers to assume success; instead, after rethrowing MicrosoftCredentialsException, propagate other failures by throwing a runtime (or specific) exception with the original cause so callers cannot silently continue. Update the catch(Exception ex) blocks in archiveTeam and unarchiveTeam (the blocks surrounding getGraphClient().teams(teamId).archive(...).post() and .unarchive().post()) to log the full error including teamId and ex, then throw a new RuntimeException (or a domain-specific unchecked exception) that wraps ex so upstream synchronization halts on failure.microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java (1)
121-123:⚠️ Potential issue | 🟠 MajorDisabled synchronizations can be re-proposed, which risks duplicate bindings.
This exclusion step now uses enabled-only synchronizations, so existing disabled records are not filtered out of candidate site/team maps and can be re-created as new relationships.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java` around lines 121 - 123, The current call to microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false) in AutoConfigController retrieves only enabled synchronizations, causing disabled records to be omitted from the candidate site/team map and allowing duplicates to be re-created; change the retrieval to include all synchronizations (e.g. call getAllSiteSynchronizations or the service method that returns both enabled and disabled records) and then explicitly filter out disabled/archived entries when building candidate maps so existing disabled SiteSynchronization records are recognized and excluded from re-proposal (update any logic in the same method that builds the candidate site/team maps to check SiteSynchronization.getEnabled() or equivalent).microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html (1)
135-141:⚠️ Potential issue | 🟠 MajorAccessible name is still stale after toggle state change.
At Line 137 and Line 140, only
titleis updated.aria-label(and any screen-reader-only label text) is not, so assistive tech can announce the wrong action.♿ Proposed fix
if (data.status == false) { showAjaxError(data.error); } else { + let nextLabel; if (!isDisabled) { icon.classList.replace('fa-toggle-on', 'fa-toggle-off'); - elem.title = /*[[#{index.enable}]]*/ ''; + nextLabel = /*[[#{index.enable}]]*/ ''; } else { icon.classList.replace('fa-toggle-off', 'fa-toggle-on'); - elem.title = /*[[#{index.disable}]]*/ ''; + nextLabel = /*[[#{index.disable}]]*/ ''; } + elem.title = nextLabel; + elem.setAttribute('aria-label', nextLabel); + const srText = elem.querySelector('.sr-only, .visually-hidden'); + if (srText) srText.textContent = nextLabel; }As per coding guidelines,
**/*.{html,xhtml,vm,js,ts}: Follow accessibility best practices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html` around lines 135 - 141, The toggle handler updates only elem.title and the icon classes (icon.classList.replace) so screen readers still announce the stale action; update the accessible name too by setting elem.setAttribute('aria-label', ...) to the same localized string you assign to elem.title and also update any adjacent screen-reader-only label text (if present) whenever isDisabled changes, ensuring both the DOM attribute and any sr-only element reflect the new state alongside the icon class swap.site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java (1)
8891-8894:⚠️ Potential issue | 🔴 CriticalDefer Microsoft provisioning until after the site save succeeds.
Line 8893 and Line 12303 still invoke
MicrosoftSynchronizationEnabler.prepareSiteForSave(...)on the pre-commit side of the workflow. If that helper performs durable Teams/archive/synchronization writes, a validation or persistence failure can leave Microsoft-side state applied while the Sakai site change is not. This needs to be split into a pure pre-save staging step and a post-commit provisioning step.Verification: inspect
MicrosoftSynchronizationEnabler.prepareSiteForSave(...)for durable side effects, and confirm bothSiteActioncall sites are still before the save/commit boundary. Expected result: the helper shows provisioning/update work, and both call sites appear beforesiteService.save(...)/commitSite(...).#!/bin/bash set -euo pipefail echo "== MicrosoftSynchronizationEnabler.prepareSiteForSave implementation ==" fd 'MicrosoftSynchronizationEnabler.java' -x sed -n '1,260p' {} echo echo "== SiteAction call sites ==" rg -n -C3 'MicrosoftSynchronizationEnabler\.prepareSiteForSave' \ site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java echo echo "== doSave_siteInfo save boundary ==" sed -n '8887,8901p' site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java echo echo "== saveFeatures save boundary ==" sed -n '12299,12306p' site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javaAlso applies to: 12300-12305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java` around lines 8891 - 8894, The current call to MicrosoftSynchronizationEnabler.prepareSiteForSave(...) in SiteAction is happening before the site persistence/commit boundary and must not perform durable Microsoft/Teams writes there; change the helper so prepareSiteForSave(...) only does non-durable staging (or rename to stageMicrosoftSync) and extract any durable provisioning/synchronization logic into a new method (e.g., MicrosoftSynchronizationEnabler.performPostCommitProvisioning or finalizeProvisioning) that is invoked only after a successful site save/commit (after siteService.save(...) / commitSite(...) completes without error) at both SiteAction call sites; update both call sites to call the new post-commit method after the save/commit and ensure prepareSiteForSave is purged of side effects.site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (9)
282-284:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().} catch (Exception e) { - log.error("Unexpected error while creating Microsoft Team for site: {}, {}", site.getId(), e.toString()); + log.error("Unexpected error while creating Microsoft Team for site: {}", site.getId(), e); }As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 282 - 284, The catch block in MicrosoftSynchronizationEnabler is logging the exception using e.toString(), which prevents SLF4J from recording the stacktrace; update the log.error call in the catch (Exception e) handler to pass the exception object as the last parameter (e) instead of e.toString() so SLF4J can log the full stack trace while keeping the parameterized message that includes site.getId().
167-171:⚠️ Potential issue | 🟡 MinorProperty removal logic is inside the loop, causing redundant operations or inconsistent state.
If there are multiple
SiteSynchronizationrecords, the property removal is attempted inside the loop for each record, and only ifarchiveTeam()succeeds. Move it outside the loop to ensure it runs exactly once regardless of per-team outcomes.if (ssList != null) { + // Remove site property outside of loop to ensure consistency + if (nameAndValueForSynchronization.length > 0) { + props.removeProperty(nameAndValueForSynchronization[0]); + log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); + } for (SiteSynchronization ss : ssList) { if (ss.getTeamId() != null && !ss.getTeamId().isEmpty()) { try { if (microsoftCommonService.archiveTeam(ss.getTeamId())) { log.info("Microsoft Team archived for site: {}, teamId: {}", site.getId(), ss.getTeamId()); } - - // Remove site property defined in microsoft.forced.synchronization.propertynameandoptionalvalue - if (nameAndValueForSynchronization.length > 0) { - props.removeProperty(nameAndValueForSynchronization[0]); - log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); - } } catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 167 - 171, The property removal logic is currently inside the loop over SiteSynchronization records and conditioned on archiveTeam(), which causes repeated or skipped removals; move the props.removeProperty(nameAndValueForSynchronization[0]) (and the corresponding log.debug) out of the per-record loop in MicrosoftSynchronizationEnabler.java so it executes once after processing all SiteSynchronization entries (regardless of each archiveTeam() outcome), using the same nameAndValueForSynchronization[0] and site.getId() context to preserve the original message; ensure the loop still handles per-team archiveTeam() calls but do not perform props.removeProperty(...) inside that loop.
261-263:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().} catch (Exception e) { - log.error("Microsoft Team creation failed for site: {}, {}", site.getId(), e.toString()); + log.error("Microsoft Team creation failed for site: {}", site.getId(), e); return; }As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 261 - 263, The catch block in MicrosoftSynchronizationEnabler currently logs the exception as e.toString() which loses stacktrace; change the log.error invocation in the catch (Exception e) block to use SLF4J parameterized logging and pass the exception object (e) as the throwable parameter—i.e., keep the message "Microsoft Team creation failed for site: {}" with site.getId() as the parameter and pass e as the final argument so the stacktrace is recorded by log.error; locate the log.error call in MicrosoftSynchronizationEnabler's catch block and replace the current parameters accordingly.
338-341:⚠️ Potential issue | 🟡 MinorTrim and validate the configured
name=valuepair.
split("=", 2)preserves whitespace, so a value like"microsoft.synchronized = true"will create a property named"microsoft.synchronized "instead of"microsoft.synchronized". Trim both parts.if (configValue.contains("=")) { - return configValue.split("=", 2); + String[] parts = configValue.split("=", 2); + String propertyName = parts[0].trim(); + String propertyValue = parts.length > 1 ? parts[1].trim() : ""; + return propertyName.isEmpty() ? new String[0] : new String[] { propertyName, propertyValue }; } return new String[] { configValue.trim(), "" };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 338 - 341, The current parsing of the configured "name=value" pair using configValue.split("=", 2) preserves surrounding whitespace, so keys like "microsoft.synchronized = true" will include trailing spaces; update the logic in MicrosoftSynchronizationEnabler where configValue is split (the configValue.split("=", 2") branch) to trim both resulting parts (key and value) and validate that the trimmed key is non-empty; return new String[] { trimmedKey, trimmedValue } (or handle missing value as empty string) so stored property names have no leading/trailing whitespace.
271-280:⚠️ Potential issue | 🟠 MajorDon't swallow post-
createTeam()failures.
createTeam()is a non-idempotent external write. IfsaveOrUpdateSiteSynchronization(ss)fails after the Team is created, the error is caught by the outer catch block, logged, and swallowed. The next retry can create a duplicate Team. Either propagate the exception or add compensating rollback.Suggested approach
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); log.info("Microsoft Team created and SiteSynchronization saved: teamId={}, siteId={}", teamId, site.getId()); } - } catch (Exception e) { - log.error("Unexpected error while creating Microsoft Team for site: {}", site.getId(), e); + } catch (MicrosoftCredentialsException e) { + log.error("Microsoft credentials error while creating team for site: {}", site.getId(), e); + throw e; // Propagate to abort site save + } catch (Exception e) { + log.error("Unexpected error while creating Microsoft Team for site: {}", site.getId(), e); + throw new RuntimeException("Failed to create Microsoft Team for site: " + site.getId(), e); }Alternatively, implement compensating logic to delete the orphan Team if persistence fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 271 - 280, The code currently calls microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss) after a non-idempotent createTeam() and swallows failures; wrap the persistence call so that on any exception you perform a compensating rollback by deleting the newly created Team (call the same service used to create it, e.g., microsoftGraphService.deleteTeam(teamId) or the createTeam() owner) and then rethrow the exception (or otherwise propagate it) instead of swallowing it; reference the SiteSynchronization ss, teamId and the createTeam()/saveOrUpdateSiteSynchronization(...) calls when locating where to add the try/catch -> deleteTeam(teamId) -> throw flow.
246-248:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().Same pattern as other catch blocks—pass the exception directly to preserve the stack trace.
} catch (Exception e) { - log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), existingSs.getTeamId(), e.toString()); + log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}", site.getId(), existingSs.getTeamId(), e); }As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 246 - 248, The catch block in MicrosoftSynchronizationEnabler (the catch(Exception e) that calls log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), existingSs.getTeamId(), e.toString())) is incorrectly passing e.toString(); change the logging invocation to pass the exception object itself so the stack trace is preserved (use the same SLF4J parameterized call but supply e as the throwable parameter). Locate the log.warn call in MicrosoftSynchronizationEnabler and replace the e.toString() parameter with the exception object e.
240-255:⚠️ Potential issue | 🟠 MajorDon't re-enable the sync until the Team restore succeeds.
Lines 250-254 clear
disabledand save the row regardless of whetherunarchiveTeam()returnsfalseor throws. The synchronization job will pick up a record pointing at an archived or missing Team.for (SiteSynchronization existingSs : existingSsList) { if (existingSs.getTeamId() != null && !existingSs.getTeamId().isEmpty()) { + boolean unarchived = false; try { - if (microsoftCommonService.unarchiveTeam(existingSs.getTeamId())) { + unarchived = microsoftCommonService.unarchiveTeam(existingSs.getTeamId()); + if (unarchived) { log.info("Microsoft Team unarchived for site: {}, teamId: {}", site.getId(), existingSs.getTeamId()); } } catch (Exception e) { log.warn("Could not unarchive Microsoft Team for site: {}, teamId: {}", site.getId(), existingSs.getTeamId(), e); } + if (!unarchived) { + log.warn("Skipping SiteSynchronization restore for site: {} because unarchive failed", site.getId()); + continue; + } } existingSs.setSyncDateFrom(syncDateFrom);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 240 - 255, The code currently clears existingSs.setDisabled(false) and saves the SiteSynchronization regardless of unarchiveTeam() outcome; change the flow so that microsoftCommonService.unarchiveTeam(existingSs.getTeamId()) must return true before calling existingSs.setDisabled(false) and microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs); if unarchiveTeam() returns false or throws, keep the existingSs disabled, log a warning (use the existing log.warn) and do not persist the enabled state, but still update syncDateFrom/syncDateTo as appropriate only when safe to enable.
156-180:⚠️ Potential issue | 🟠 MajorScope this toggle to forced synchronizations only.
The loop operates on every
SiteSynchronizationfor the site, but the feature is intended for forced synchronizations (as indicated by theforced(true)when creating new ones at line 274). This can inadvertently archive/disable manually created Teams.Filter the list to only process forced synchronizations:
if (ssList != null) { for (SiteSynchronization ss : ssList) { + if (!Boolean.TRUE.equals(ss.getForced())) { + continue; + } if (ss.getTeamId() != null && !ss.getTeamId().isEmpty()) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 156 - 180, The loop currently processes all SiteSynchronization entries returned by microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()), but it should only act on forced synchronizations to avoid touching manually created Teams; update the logic to filter the list (or add a guard inside the loop) so only SiteSynchronization instances where the "forced" flag is true (e.g., ss.isForced() or ss.getForced()/ss.getForcedFlag(), depending on the SiteSynchronization API) are archived/disabled and have site properties removed, leaving non-forced synchronizations untouched; keep the existing operations (archiveTeam, removeProperty, ss.setDisabled, saveOrUpdateSiteSynchronization) but only for forced entries.
172-174:⚠️ Potential issue | 🟡 MinorIncorrect exception logging: pass the exception object, not
e.toString().This loses the stack trace. SLF4J expects the exception as the last argument without a placeholder.
- } catch (Exception e) { - log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), ss.getTeamId(), e.toString()); + } catch (Exception e) { + log.warn("Could not archive Microsoft Team for site: {}, teamId: {}", site.getId(), ss.getTeamId(), e);As per coding guidelines: "Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 172 - 174, In the catch block inside MicrosoftSynchronizationEnabler where you call log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), ss.getTeamId(), e.toString()), replace the use of e.toString() and the extra {} placeholder with passing the exception object as the last parameter to SLF4J (i.e., keep the two placeholders for site.getId() and ss.getTeamId() and pass e as the final argument) so the stack trace is preserved when calling log.warn.
🧹 Nitpick comments (1)
microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html (1)
99-99: Prefervisually-hiddenoversr-onlyhere.Line 99 uses the Bootstrap 4 utility name. In a Bootstrap 5.2 skin, that class is
visually-hidden, so this fallback text may become visible in the actions column unless Sakai keeps a compatibility alias.♿ Bootstrap 5-safe tweak
- <span class="sr-only" th:text="${row.disabled} ? #{index.enable} : #{index.disable}"></span> + <span class="visually-hidden" th:text="${row.disabled} ? #{index.enable} : #{index.disable}"></span>As per coding guidelines, "
**/*.{html,jsp,vm,xhtml,xml}: Use Bootstrap 5.2 as the preferred UI framework for styling in Sakai frontendsand**/*.{html,xhtml,vm,js,ts}: Follow accessibility best practices."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html` at line 99, The accessibility fallback span uses the Bootstrap 4 utility class "sr-only" which should be updated to the Bootstrap 5.2 class "visually-hidden"; locate the span element rendered with th:text="${row.disabled} ? #{index.enable} : #{index.disable}" and replace its class value from "sr-only" to "visually-hidden" so the hidden fallback text remains properly hidden in the actions column under the Bootstrap 5 skin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.java`:
- Around line 121-123: The current call to
microsoftSynchronizationService.getAllEnabledSiteSynchronizations(false) in
AutoConfigController retrieves only enabled synchronizations, causing disabled
records to be omitted from the candidate site/team map and allowing duplicates
to be re-created; change the retrieval to include all synchronizations (e.g.
call getAllSiteSynchronizations or the service method that returns both enabled
and disabled records) and then explicitly filter out disabled/archived entries
when building candidate maps so existing disabled SiteSynchronization records
are recognized and excluded from re-proposal (update any logic in the same
method that builds the candidate site/team maps to check
SiteSynchronization.getEnabled() or equivalent).
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`:
- Around line 135-141: The toggle handler updates only elem.title and the icon
classes (icon.classList.replace) so screen readers still announce the stale
action; update the accessible name too by setting
elem.setAttribute('aria-label', ...) to the same localized string you assign to
elem.title and also update any adjacent screen-reader-only label text (if
present) whenever isDisabled changes, ensuring both the DOM attribute and any
sr-only element reflect the new state alongside the icon class swap.
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java`:
- Around line 1013-1041: archiveTeam and unarchiveTeam currently catch generic
Exception, log and return false which allows callers to assume success; instead,
after rethrowing MicrosoftCredentialsException, propagate other failures by
throwing a runtime (or specific) exception with the original cause so callers
cannot silently continue. Update the catch(Exception ex) blocks in archiveTeam
and unarchiveTeam (the blocks surrounding
getGraphClient().teams(teamId).archive(...).post() and .unarchive().post()) to
log the full error including teamId and ex, then throw a new RuntimeException
(or a domain-specific unchecked exception) that wraps ex so upstream
synchronization halts on failure.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 282-284: The catch block in MicrosoftSynchronizationEnabler is
logging the exception using e.toString(), which prevents SLF4J from recording
the stacktrace; update the log.error call in the catch (Exception e) handler to
pass the exception object as the last parameter (e) instead of e.toString() so
SLF4J can log the full stack trace while keeping the parameterized message that
includes site.getId().
- Around line 167-171: The property removal logic is currently inside the loop
over SiteSynchronization records and conditioned on archiveTeam(), which causes
repeated or skipped removals; move the
props.removeProperty(nameAndValueForSynchronization[0]) (and the corresponding
log.debug) out of the per-record loop in MicrosoftSynchronizationEnabler.java so
it executes once after processing all SiteSynchronization entries (regardless of
each archiveTeam() outcome), using the same nameAndValueForSynchronization[0]
and site.getId() context to preserve the original message; ensure the loop still
handles per-team archiveTeam() calls but do not perform
props.removeProperty(...) inside that loop.
- Around line 261-263: The catch block in MicrosoftSynchronizationEnabler
currently logs the exception as e.toString() which loses stacktrace; change the
log.error invocation in the catch (Exception e) block to use SLF4J parameterized
logging and pass the exception object (e) as the throwable parameter—i.e., keep
the message "Microsoft Team creation failed for site: {}" with site.getId() as
the parameter and pass e as the final argument so the stacktrace is recorded by
log.error; locate the log.error call in MicrosoftSynchronizationEnabler's catch
block and replace the current parameters accordingly.
- Around line 338-341: The current parsing of the configured "name=value" pair
using configValue.split("=", 2) preserves surrounding whitespace, so keys like
"microsoft.synchronized = true" will include trailing spaces; update the logic
in MicrosoftSynchronizationEnabler where configValue is split (the
configValue.split("=", 2") branch) to trim both resulting parts (key and value)
and validate that the trimmed key is non-empty; return new String[] {
trimmedKey, trimmedValue } (or handle missing value as empty string) so stored
property names have no leading/trailing whitespace.
- Around line 271-280: The code currently calls
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss) after a
non-idempotent createTeam() and swallows failures; wrap the persistence call so
that on any exception you perform a compensating rollback by deleting the newly
created Team (call the same service used to create it, e.g.,
microsoftGraphService.deleteTeam(teamId) or the createTeam() owner) and then
rethrow the exception (or otherwise propagate it) instead of swallowing it;
reference the SiteSynchronization ss, teamId and the
createTeam()/saveOrUpdateSiteSynchronization(...) calls when locating where to
add the try/catch -> deleteTeam(teamId) -> throw flow.
- Around line 246-248: The catch block in MicrosoftSynchronizationEnabler (the
catch(Exception e) that calls log.warn("Could not unarchive Microsoft Team for
site: {}, teamId: {}, {}", site.getId(), existingSs.getTeamId(), e.toString()))
is incorrectly passing e.toString(); change the logging invocation to pass the
exception object itself so the stack trace is preserved (use the same SLF4J
parameterized call but supply e as the throwable parameter). Locate the log.warn
call in MicrosoftSynchronizationEnabler and replace the e.toString() parameter
with the exception object e.
- Around line 240-255: The code currently clears existingSs.setDisabled(false)
and saves the SiteSynchronization regardless of unarchiveTeam() outcome; change
the flow so that microsoftCommonService.unarchiveTeam(existingSs.getTeamId())
must return true before calling existingSs.setDisabled(false) and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(existingSs); if
unarchiveTeam() returns false or throws, keep the existingSs disabled, log a
warning (use the existing log.warn) and do not persist the enabled state, but
still update syncDateFrom/syncDateTo as appropriate only when safe to enable.
- Around line 156-180: The loop currently processes all SiteSynchronization
entries returned by
microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()), but
it should only act on forced synchronizations to avoid touching manually created
Teams; update the logic to filter the list (or add a guard inside the loop) so
only SiteSynchronization instances where the "forced" flag is true (e.g.,
ss.isForced() or ss.getForced()/ss.getForcedFlag(), depending on the
SiteSynchronization API) are archived/disabled and have site properties removed,
leaving non-forced synchronizations untouched; keep the existing operations
(archiveTeam, removeProperty, ss.setDisabled, saveOrUpdateSiteSynchronization)
but only for forced entries.
- Around line 172-174: In the catch block inside MicrosoftSynchronizationEnabler
where you call log.warn("Could not archive Microsoft Team for site: {}, teamId:
{}, {}", site.getId(), ss.getTeamId(), e.toString()), replace the use of
e.toString() and the extra {} placeholder with passing the exception object as
the last parameter to SLF4J (i.e., keep the two placeholders for site.getId()
and ss.getTeamId() and pass e as the final argument) so the stack trace is
preserved when calling log.warn.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Around line 8891-8894: The current call to
MicrosoftSynchronizationEnabler.prepareSiteForSave(...) in SiteAction is
happening before the site persistence/commit boundary and must not perform
durable Microsoft/Teams writes there; change the helper so
prepareSiteForSave(...) only does non-durable staging (or rename to
stageMicrosoftSync) and extract any durable provisioning/synchronization logic
into a new method (e.g.,
MicrosoftSynchronizationEnabler.performPostCommitProvisioning or
finalizeProvisioning) that is invoked only after a successful site save/commit
(after siteService.save(...) / commitSite(...) completes without error) at both
SiteAction call sites; update both call sites to call the new post-commit method
after the save/commit and ensure prepareSiteForSave is purged of side effects.
---
Nitpick comments:
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html`:
- Line 99: The accessibility fallback span uses the Bootstrap 4 utility class
"sr-only" which should be updated to the Bootstrap 5.2 class "visually-hidden";
locate the span element rendered with th:text="${row.disabled} ? #{index.enable}
: #{index.disable}" and replace its class value from "sr-only" to
"visually-hidden" so the hidden fallback text remains properly hidden in the
actions column under the Bootstrap 5 skin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a897f2db-130e-4ff5-916d-14c66a88cc4d
📒 Files selected for processing (24)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertieslibrary/src/skins/default/src/sass/modules/tool/sitemanage/_sitemanage.scssmicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/AutoConfigController.javamicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.htmlmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftCommonService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/MicrosoftSynchronizationService.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javamicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/persistence/MicrosoftSiteSynchronizationRepository.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/jobs/RunSynchronizationsJob.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/persistence/MicrosoftSiteSynchronizationRepositoryImpl.javasite-manage/site-manage-tool/tool/pom.xmlsite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.javasite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-addRemoveFeatureConfirm.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vmsite-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/toolGroupMultipleDisplay.vm
…ronization in Microsoft Team for the site
…ronization in Microsoft Team for the site
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (10)
microsoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.java (1)
90-93:⚠️ Potential issue | 🟠 MajorDatabase migration required for new
disabledcolumn.The new non-nullable
disabledcolumn requires a migration script to add the column with a default value to existingmc_site_synchronizationtables. Without this, deployments to existing databases will fail. A past review flagged this concern and it does not appear to be addressed yet.#!/bin/bash # Search for SQL migration scripts that add the disabled column rg -n "disabled" microsoft-integration --type=sql -A 3 -B 3 2>/dev/null || echo "No SQL files found" # Check the referenced sakai-reference repo PR for migration scripts echo "Note: Check https://github.com/sakaiproject/sakai-reference/pull/283 for migration scripts" # Look for any database setup or migration directories find microsoft-integration -type d \( -name "db" -o -name "migration" -o -name "sql" \) 2>/dev/null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.java` around lines 90 - 93, The new non-nullable field 'disabled' in the SiteSynchronization class requires a DB migration: add a migration script that alters table mc_site_synchronization to add the boolean/bit column 'disabled' with DEFAULT false and NOT NULL (and backfill existing rows if your DB needs it), register the script with your migration tool (e.g., Liquibase/Flyway changelog) so it runs on deploy, and include a rollback/undo step; ensure the column type and default match how other boolean fields are defined in the project and reference the SiteSynchronization.disabled field name in tests or integration checks that exercise schema changes.microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html (2)
135-141:⚠️ Potential issue | 🟠 MajorUpdate
aria-labeland screen-reader text when toggle state changes.On success, only
elem.titleis updated. Thearia-labelattribute and.sr-onlyspan text remain stale, causing assistive technology to announce the wrong action after toggling. A past review flagged this and it appears unresolved.♿ Proposed accessibility fix
if (data.status == false) { showAjaxError(data.error); } else { + let nextLabel; if (!isDisabled) { icon.classList.replace('fa-toggle-on', 'fa-toggle-off'); - elem.title = /*[[#{index.enable}]]*/ ''; + nextLabel = /*[[#{index.enable}]]*/ ''; } else { icon.classList.replace('fa-toggle-off', 'fa-toggle-on'); - elem.title = /*[[#{index.disable}]]*/ ''; + nextLabel = /*[[#{index.disable}]]*/ ''; } + elem.title = nextLabel; + elem.setAttribute('aria-label', nextLabel); + const srText = elem.querySelector('.sr-only'); + if (srText) srText.textContent = nextLabel; }As per coding guidelines, "**/*.{html,xhtml,vm,js,ts}`: Follow accessibility best practices".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html` around lines 135 - 141, When updating the toggle UI in the block that currently swaps the icon and sets elem.title (the branch using isDisabled, icon, and elem), also update the accessible labels: set elem.setAttribute('aria-label', ...) to the same string used for elem.title and update the visible screen-reader text by locating the .sr-only span inside elem (e.g., elem.querySelector('.sr-only')) and replacing its textContent with the same localized label; do this in both branches (enable and disable) so assistive tech reflects the new action after toggling.
116-118:⚠️ Potential issue | 🟡 MinorUpdate accessibility attributes when toggle state changes.
The
toggleDisabledfunction updates the icon classes and title attribute but fails to update thearia-labelattribute on the button element or thesr-onlyspan text content when the toggle state changes. Screen reader users will not be aware of the state change after the toggle completes.Update lines 137 and 141 to also set
elem.aria-labeland update the sr-only span's text:Suggested fix
if (!isDisabled) { icon.classList.replace('fa-toggle-on', 'fa-toggle-off'); elem.title = /*[[#{index.enable}]]*/ ''; elem.setAttribute('aria-label', /*[[#{index.enable}]]*/ ''); elem.querySelector('.sr-only').textContent = /*[[#{index.enable}]]*/ ''; } else { icon.classList.replace('fa-toggle-off', 'fa-toggle-on'); elem.title = /*[[#{index.disable}]]*/ ''; elem.setAttribute('aria-label', /*[[#{index.disable}]]*/ ''); elem.querySelector('.sr-only').textContent = /*[[#{index.disable}]]*/ ''; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html` around lines 116 - 118, The toggleDisabled function updates icon classes and elem.title but doesn't update accessible labels; modify the toggle branch inside toggleDisabled (where icon.classList.replace(...) and elem.title are set) to also call elem.setAttribute('aria-label', ...) and update the visible sr-only text via elem.querySelector('.sr-only').textContent — do this for both the enable and disable branches (use the same localized strings you set for elem.title) so screen readers reflect the new state; references: toggleDisabled, isDisabled, icon.classList.replace, elem.title, elem.setAttribute('aria-label'), elem.querySelector('.sr-only').site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java (1)
8891-8894:⚠️ Potential issue | 🔴 CriticalKeep Microsoft provisioning out of the pre-save path.
MicrosoftSynchronizationEnabler.prepareSiteForSave(...)still runs beforesiteService.save(...)/commitSite(...)in both save flows. If that helper archives/creates Teams or updates synchronization records, a later Sakai save failure will leave Microsoft-side state committed while the site change is not.Verify that
prepareSiteForSave(...)is pure staging only. Expected result: anything durable happens strictly after a successful Sakai save/commit.#!/bin/bash set -euo pipefail helper="$(fd 'MicrosoftSynchronizationEnabler.java' | head -n1)" site_action="$(fd 'SiteAction.java' | head -n1)" echo "== MicrosoftSynchronizationEnabler.prepareSiteForSave and nearby calls ==" rg -n -C6 'prepareSiteForSave|archive|unarchive|create|SiteSynchronization|Synchronization' "$helper" echo echo "== SiteAction pre-save call sites ==" rg -n -C6 'MicrosoftSynchronizationEnabler\.prepareSiteForSave|siteService\.save|commitSite' "$site_action"Also applies to: 12300-12305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java` around lines 8891 - 8894, MicrosoftSynchronizationEnabler.prepareSiteForSave(...) is being invoked before the actual site save/commit in SiteAction (adjacent to SubNavEnabler.prepareSiteForSave), which can cause durable Microsoft-side changes to be applied even if siteService.save/commitSite later fails; either make prepareSiteForSave strictly staging-only (no archive/create/synchronization side-effects) or remove the pre-save call and invoke MicrosoftSynchronizationEnabler after a successful save/commit (e.g., immediately after siteService.save(...) / commitSite(...) or via the same commit callback mechanism), ensuring any methods that perform durable actions (archive/unarchive/create/SiteSynchronization updates) run only post-commit.site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java (6)
154-170:⚠️ Potential issue | 🟡 MinorClear the auxiliary site property outside the per-team
try.
props.removeProperty(nameAndValueForSynchronization[0])only runs after entering the archive block successfully, so it is skipped when there are no synchronizations, when a row has noteamId, or whenarchiveTeam()throws. The site can end up disabled while the forced-sync property is still present. Move that removal outside the loop so it always runs once.Proposed fix
} else { props.removeProperty(SITE_PROPERTY); + if (nameAndValueForSynchronization.length > 0) { + props.removeProperty(nameAndValueForSynchronization[0]); + log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); + } // If a SiteSynchronization exists with a Team, archive it and delete it if (microsoftSynchronizationService != null && microsoftCommonService != null) { List<SiteSynchronization> ssList = microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()); if (ssList != null) { for (SiteSynchronization ss : ssList) { if (ss.getTeamId() != null && !ss.getTeamId().isEmpty()) { try { if (microsoftCommonService.archiveTeam(ss.getTeamId())) { log.info("Microsoft Team archived for site: {}, teamId: {}", site.getId(), ss.getTeamId()); } - - // Remove site property defined in microsoft.forced.synchronization.propertynameandoptionalvalue - if (nameAndValueForSynchronization.length > 0) { - props.removeProperty(nameAndValueForSynchronization[0]); - log.debug("Removed site property {} for site: {}", nameAndValueForSynchronization[0], site.getId()); - } } catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 154 - 170, The removal of the auxiliary site property (props.removeProperty(nameAndValueForSynchronization[0])) is currently inside the per-team try block in MicrosoftSynchronizationEnabler so it only runs when a team archive succeeds; move that removal out of the loop and execute it once after processing all SiteSynchronization entries (after iterating ssList or immediately after the if (ssList != null) block) so the property is cleared regardless of presence of teamIds or archiveTeam() failures; keep references to props, nameAndValueForSynchronization, microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()), and microsoftCommonService.archiveTeam(...) when locating the code to change.
140-150:⚠️ Potential issue | 🔴 CriticalMake enablement all-or-nothing.
prepareSiteForSave()setsmicrosoftSynchronizationon Line 141 and the optional site property on Lines 149-150 beforecreateOrRestoreMicrosoftTeamForSite(...)proves that a synchronization can actually be restored/created. Then any failure fromsaveOrUpdateSiteSynchronization(...)on Line 256 or Line 282 is swallowed by the outer catch on Lines 285-287, even thoughMicrosoftCommonService#createTeam(...)/unarchiveTeam(...)have already mutated Microsoft. That can leave Sakai marked enabled with no durable local row, and a retry can create duplicate Teams. Make the helper return success or throw, propagate or compensate post-mutation failures, and only add the site properties after the synchronization row is saved.Also applies to: 214-287
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 140 - 150, prepareSiteForSave currently sets the microsoftSynchronization flag and optional site property (props.addProperty / microsoftSynchronization) before calling createOrRestoreMicrosoftTeamForSite, which can mutate Microsoft via MicrosoftCommonService#createTeam/unarchiveTeam and then have downstream saveOrUpdateSiteSynchronization failures swallowed; change createOrRestoreMicrosoftTeamForSite to return a boolean or throw on failure and ensure saveOrUpdateSiteSynchronization either succeeds or its error is propagated/compensated (e.g., rollback or delete the created Team) so mutations are not left inconsistent, and move the props.addProperty calls to after a successful saveOrUpdateSiteSynchronization (or after confirmed success return) so enablement is all-or-nothing.
172-173:⚠️ Potential issue | 🟡 MinorPass the exception object to SLF4J.
These catch blocks log
e.toString()into a placeholder, which drops the stack trace. Passeas the last argument with no matching placeholder so the full failure context is preserved.As per coding guidelines, `Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))`.Proposed fix
- log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), ss.getTeamId(), e.toString()); + log.warn("Could not archive Microsoft Team for site: {}, teamId: {}", site.getId(), ss.getTeamId(), e); ... - log.error("Could not unarchive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), existingSs.getTeamId(), e.toString()); + log.error("Could not unarchive Microsoft Team for site: {}, teamId: {}", site.getId(), existingSs.getTeamId(), e); ... - log.error("Microsoft Team creation failed for site: {}, {}", site.getId(), e.toString()); + log.error("Microsoft Team creation failed for site: {}", site.getId(), e); ... - log.error("Unexpected error while creating Microsoft Team for site: {}, {}", site.getId(), e.toString()); + log.error("Unexpected error while creating Microsoft Team for site: {}", site.getId(), e);Also applies to: 247-248, 264-265, 285-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 172 - 173, In MicrosoftSynchronizationEnabler, update the SLF4J logging calls that currently pass e.toString() into a placeholder (e.g., log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}", site.getId(), ss.getTeamId(), e.toString())) to instead pass the exception as the final varargs argument with no matching {} placeholder (e.g., log.warn("Could not archive Microsoft Team for site: {}, teamId: {}", site.getId(), ss.getTeamId(), e)); apply the same change for the other catch-block log.warn calls in this class (the occurrences around the blocks currently at lines referenced in the review: ~247-248, ~264-265, ~285-286) so the full stack trace is preserved by SLF4J.
341-344:⚠️ Potential issue | 🟡 MinorTrim the configured
name=valuepair before using it.
split("=", 2)preserves surrounding whitespace, so a config likemicrosoft.synchronized = truecreates/removes a property namedmicrosoft.synchronizedinstead ofmicrosoft.synchronized. Trim both parts and ignore blank property names before returning.Proposed fix
if (configValue.contains("=")) { - return configValue.split("=", 2); + final String[] parts = configValue.split("=", 2); + final String propertyName = parts[0].trim(); + final String propertyValue = parts[1].trim(); + return propertyName.isEmpty() ? new String[0] : new String[] { propertyName, propertyValue }; } - return new String[] { configValue.trim(), "" }; + final String propertyName = configValue.trim(); + return propertyName.isEmpty() ? new String[0] : new String[] { propertyName, "" };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 341 - 344, The current split preserves whitespace so configValue.split("=", 2) can yield names/values with surrounding spaces; update the handling of configValue where the split occurs to trim both parts: after splitting (or when treating the single-value case), call trim() on the name and value strings, and if the trimmed name is empty return an empty pair (e.g., new String[] { "", "" }) to ignore blank property names; reference the configValue split branch and the fallback return so you trim both parts consistently before returning.
156-180:⚠️ Potential issue | 🟠 MajorRestrict archive/restore to valid forced rows.
This feature creates
forced(true)synchronizations on Lines 274-280, but both loops currently walk everySiteSynchronization. That lets the site-level toggle archive/unarchive manual Teams, and in the restore path a row with a blankteamIdfalls through from Line 241 to Line 252 and gets re-enabled without any Team being restored. Filter these flows to forced rows with non-emptyteamIds, and if none can be restored, fall back to creating a new forced synchronization.Also applies to: 237-258, 274-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 156 - 180, The loops over microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()) must only act on forced synchronizations with non-empty team IDs to avoid touching manual rows; update the loop conditions in MicrosoftSynchronizationEnabler to check ss.isForced() (or ss.getForced() as appropriate) && ss.getTeamId() != null && !ss.getTeamId().isEmpty() before archiving/restoring (calls to microsoftCommonService.archiveTeam(ss.getTeamId()), ss.setDisabled(true/false), and microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss)); in the restore path, if no forced row with a valid teamId is found, create a new forced synchronization (setForced(true), setTeamId(...), saveOrUpdateSiteSynchronization) as the fallback instead of re-enabling rows without team IDs.
133-146:⚠️ Potential issue | 🟠 MajorOnly provision on the disabled → enabled transition.
STATE_KEYhere is just the current checkbox value, so Line 146 rerunscreateOrRestoreMicrosoftTeamForSite(...)every time an already-enabled site is saved. That keeps issuing restore calls and resets the sync window even when Microsoft sync never changed. Read the previousSITE_PROPERTYvalue before mutatingprops, and skip the helper unless the site is transitioning from disabled to enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java` around lines 133 - 146, Read the existing SITE_PROPERTY before mutating props and only call createOrRestoreMicrosoftTeamForSite(...) when the site is transitioning from disabled to enabled: obtain the current site property via site.getProperties().getProperty(SITE_PROPERTY) (or equivalent), convert it to a boolean oldEnabled, then after computing isEnabled from state.getAttribute(STATE_KEY) only invoke createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService, microsoftConfigurationService, microsoftSynchronizationService) when oldEnabled is false and isEnabled is true; keep props.addProperty(SITE_PROPERTY, Boolean.TRUE.toString()) but do the existence check first so you don't re-run the restore on every save.
🧹 Nitpick comments (2)
microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java (1)
314-314: Consider changingsetForcedandsetDateendpoints to POST as well.A past review flagged that all three state-changing endpoints (
setForced,setDisabled,setDate) should use POST instead of GET for REST compliance and CSRF safety. OnlysetDisabledwas updated to POST. Consider updating the other two for consistency.Proposed changes for setForced endpoint
- `@GetMapping`(path = {"/setForced-siteSynchronization/{id}"}, produces = MediaType.APPLICATION_JSON_VALUE) + `@PostMapping`(path = {"/setForced-siteSynchronization/{id}"}, produces = MediaType.APPLICATION_JSON_VALUE) `@ResponseBody` public AjaxResponse updateSiteSynchronizationForced(`@PathVariable` String id, `@RequestParam` Boolean forced, Model model) {Note: The frontend
changeForcedfunction at line 108 of index.html would also need to be updated to use POST method.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java` at line 314, Change the two state-changing controller endpoints to use POST instead of GET: update the `@GetMapping` annotated methods for setForced-siteSynchronization (setForced) and setDate to `@PostMapping` (keeping the same path and produces) in MainController.java, and ensure any parameter bindings remain valid for POST; also update the frontend call(s) (e.g., changeForced in index.html) to issue POST requests (and include CSRF token if applicable) so the client and server remain consistent with the already-converted setDisabled endpoint.site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java (1)
13395-13399: Helper already guards against null, but consider making intent explicit at call site.The
MicrosoftSynchronizationEnabler.applySettingsToState(...)method has an explicit null guard (line 81:if (state == null || params == null || site == null) return false;), so this code is safe during site creation. However, relying on implicit helper behavior obscures intent. Since this handler runs during new-site creation (where the Microsoft toggle is not rendered, lines 1858-1869) andsitemay be null, adding an explicit guard at the call site would improve clarity:♻️ Suggested improvement
MathJaxEnabler.applySettingsToState(state, params); // SAK-22384 SubNavEnabler.applySettingsToState(state, params); - MicrosoftSynchronizationEnabler.applySettingsToState(state, params, site); + if (site != null) { + MicrosoftSynchronizationEnabler.applySettingsToState(state, params, site); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java` around lines 13395 - 13399, The call to MicrosoftSynchronizationEnabler.applySettingsToState(state, params, site) relies on that helper's internal null-guard; make the intent explicit by adding a pre-check in SiteAction before invoking it: verify site (and/or state/params) is non-null and only call MicrosoftSynchronizationEnabler.applySettingsToState when these values are present. Reference the call site in SiteAction and the helper method MicrosoftSynchronizationEnabler.applySettingsToState to locate where to add the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`:
- Around line 135-141: When updating the toggle UI in the block that currently
swaps the icon and sets elem.title (the branch using isDisabled, icon, and
elem), also update the accessible labels: set elem.setAttribute('aria-label',
...) to the same string used for elem.title and update the visible screen-reader
text by locating the .sr-only span inside elem (e.g.,
elem.querySelector('.sr-only')) and replacing its textContent with the same
localized label; do this in both branches (enable and disable) so assistive tech
reflects the new action after toggling.
- Around line 116-118: The toggleDisabled function updates icon classes and
elem.title but doesn't update accessible labels; modify the toggle branch inside
toggleDisabled (where icon.classList.replace(...) and elem.title are set) to
also call elem.setAttribute('aria-label', ...) and update the visible sr-only
text via elem.querySelector('.sr-only').textContent — do this for both the
enable and disable branches (use the same localized strings you set for
elem.title) so screen readers reflect the new state; references: toggleDisabled,
isDisabled, icon.classList.replace, elem.title, elem.setAttribute('aria-label'),
elem.querySelector('.sr-only').
In
`@microsoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.java`:
- Around line 90-93: The new non-nullable field 'disabled' in the
SiteSynchronization class requires a DB migration: add a migration script that
alters table mc_site_synchronization to add the boolean/bit column 'disabled'
with DEFAULT false and NOT NULL (and backfill existing rows if your DB needs
it), register the script with your migration tool (e.g., Liquibase/Flyway
changelog) so it runs on deploy, and include a rollback/undo step; ensure the
column type and default match how other boolean fields are defined in the
project and reference the SiteSynchronization.disabled field name in tests or
integration checks that exercise schema changes.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 154-170: The removal of the auxiliary site property
(props.removeProperty(nameAndValueForSynchronization[0])) is currently inside
the per-team try block in MicrosoftSynchronizationEnabler so it only runs when a
team archive succeeds; move that removal out of the loop and execute it once
after processing all SiteSynchronization entries (after iterating ssList or
immediately after the if (ssList != null) block) so the property is cleared
regardless of presence of teamIds or archiveTeam() failures; keep references to
props, nameAndValueForSynchronization,
microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()), and
microsoftCommonService.archiveTeam(...) when locating the code to change.
- Around line 140-150: prepareSiteForSave currently sets the
microsoftSynchronization flag and optional site property (props.addProperty /
microsoftSynchronization) before calling createOrRestoreMicrosoftTeamForSite,
which can mutate Microsoft via MicrosoftCommonService#createTeam/unarchiveTeam
and then have downstream saveOrUpdateSiteSynchronization failures swallowed;
change createOrRestoreMicrosoftTeamForSite to return a boolean or throw on
failure and ensure saveOrUpdateSiteSynchronization either succeeds or its error
is propagated/compensated (e.g., rollback or delete the created Team) so
mutations are not left inconsistent, and move the props.addProperty calls to
after a successful saveOrUpdateSiteSynchronization (or after confirmed success
return) so enablement is all-or-nothing.
- Around line 172-173: In MicrosoftSynchronizationEnabler, update the SLF4J
logging calls that currently pass e.toString() into a placeholder (e.g.,
log.warn("Could not archive Microsoft Team for site: {}, teamId: {}, {}",
site.getId(), ss.getTeamId(), e.toString())) to instead pass the exception as
the final varargs argument with no matching {} placeholder (e.g.,
log.warn("Could not archive Microsoft Team for site: {}, teamId: {}",
site.getId(), ss.getTeamId(), e)); apply the same change for the other
catch-block log.warn calls in this class (the occurrences around the blocks
currently at lines referenced in the review: ~247-248, ~264-265, ~285-286) so
the full stack trace is preserved by SLF4J.
- Around line 341-344: The current split preserves whitespace so
configValue.split("=", 2) can yield names/values with surrounding spaces; update
the handling of configValue where the split occurs to trim both parts: after
splitting (or when treating the single-value case), call trim() on the name and
value strings, and if the trimmed name is empty return an empty pair (e.g., new
String[] { "", "" }) to ignore blank property names; reference the configValue
split branch and the fallback return so you trim both parts consistently before
returning.
- Around line 156-180: The loops over
microsoftSynchronizationService.getSiteSynchronizationsBySite(site.getId()) must
only act on forced synchronizations with non-empty team IDs to avoid touching
manual rows; update the loop conditions in MicrosoftSynchronizationEnabler to
check ss.isForced() (or ss.getForced() as appropriate) && ss.getTeamId() != null
&& !ss.getTeamId().isEmpty() before archiving/restoring (calls to
microsoftCommonService.archiveTeam(ss.getTeamId()), ss.setDisabled(true/false),
and microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss)); in the
restore path, if no forced row with a valid teamId is found, create a new forced
synchronization (setForced(true), setTeamId(...),
saveOrUpdateSiteSynchronization) as the fallback instead of re-enabling rows
without team IDs.
- Around line 133-146: Read the existing SITE_PROPERTY before mutating props and
only call createOrRestoreMicrosoftTeamForSite(...) when the site is
transitioning from disabled to enabled: obtain the current site property via
site.getProperties().getProperty(SITE_PROPERTY) (or equivalent), convert it to a
boolean oldEnabled, then after computing isEnabled from
state.getAttribute(STATE_KEY) only invoke
createOrRestoreMicrosoftTeamForSite(site, microsoftCommonService,
microsoftConfigurationService, microsoftSynchronizationService) when oldEnabled
is false and isEnabled is true; keep props.addProperty(SITE_PROPERTY,
Boolean.TRUE.toString()) but do the existence check first so you don't re-run
the restore on every save.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Around line 8891-8894: MicrosoftSynchronizationEnabler.prepareSiteForSave(...)
is being invoked before the actual site save/commit in SiteAction (adjacent to
SubNavEnabler.prepareSiteForSave), which can cause durable Microsoft-side
changes to be applied even if siteService.save/commitSite later fails; either
make prepareSiteForSave strictly staging-only (no archive/create/synchronization
side-effects) or remove the pre-save call and invoke
MicrosoftSynchronizationEnabler after a successful save/commit (e.g.,
immediately after siteService.save(...) / commitSite(...) or via the same commit
callback mechanism), ensuring any methods that perform durable actions
(archive/unarchive/create/SiteSynchronization updates) run only post-commit.
---
Nitpick comments:
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java`:
- Line 314: Change the two state-changing controller endpoints to use POST
instead of GET: update the `@GetMapping` annotated methods for
setForced-siteSynchronization (setForced) and setDate to `@PostMapping` (keeping
the same path and produces) in MainController.java, and ensure any parameter
bindings remain valid for POST; also update the frontend call(s) (e.g.,
changeForced in index.html) to issue POST requests (and include CSRF token if
applicable) so the client and server remain consistent with the
already-converted setDisabled endpoint.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Around line 13395-13399: The call to
MicrosoftSynchronizationEnabler.applySettingsToState(state, params, site) relies
on that helper's internal null-guard; make the intent explicit by adding a
pre-check in SiteAction before invoking it: verify site (and/or state/params) is
non-null and only call MicrosoftSynchronizationEnabler.applySettingsToState when
these values are present. Reference the call site in SiteAction and the helper
method MicrosoftSynchronizationEnabler.applySettingsToState to locate where to
add the guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6b31a218-3a31-4740-8fc5-b3824c394b90
📒 Files selected for processing (12)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.propertiesmicrosoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.propertiesmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.htmlmicrosoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.htmlmicrosoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javasite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.propertiessite-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.propertiessite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.javasite-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java
✅ Files skipped from review due to trivial changes (3)
- microsoft-integration/admin-tool/src/main/resources/Messages.properties
- site-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric.properties
- config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties
🚧 Files skipped from review as they are similar to previous changes (3)
- site-manage/site-manage-tool/tool/src/bundle/sitesetupgeneric_es.properties
- microsoft-integration/admin-tool/src/main/resources/Messages_es.properties
- microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java (1)
339-353:⚠️ Potential issue | 🟠 MajorState-changing sync endpoints are only partially migrated to POST
Good change on Line 339 for
setDisabled, butsetForced-siteSynchronization(Line 314) andsetDate-siteSynchronization(Line 356) still mutate state via GET. That leaves inconsistent behavior and the original CSRF/HTTP-semantics risk only partially fixed.Suggested follow-up diff
- `@GetMapping`(path = {"/setForced-siteSynchronization/{id}"}, produces = MediaType.APPLICATION_JSON_VALUE) + `@PostMapping`(path = {"/setForced-siteSynchronization/{id}"}, produces = MediaType.APPLICATION_JSON_VALUE) `@ResponseBody` public AjaxResponse updateSiteSynchronizationForced(`@PathVariable` String id, `@RequestParam` Boolean forced, Model model) { - `@GetMapping`(path = {"/setDate-siteSynchronization/{id}"}, produces = MediaType.APPLICATION_JSON_VALUE) + `@PostMapping`(path = {"/setDate-siteSynchronization/{id}"}, produces = MediaType.APPLICATION_JSON_VALUE) `@ResponseBody` public AjaxResponse updateSiteSynchronizationDate(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java` around lines 339 - 353, The other state-mutating handlers setForced-siteSynchronization and setDate-siteSynchronization are still exposed as GETs; change their request mappings to POST (replace `@GetMapping` with `@PostMapping`) and keep `@ResponseBody` and produces = MediaType.APPLICATION_JSON_VALUE, update their method signatures to accept `@RequestParam/`@PathVariable exactly as currently used (same parameter names) and call microsoftSynchronizationService.saveOrUpdate... just like updateSiteSynchronizationDisabled does; also update any client-side calls/tests to POST and ensure CSRF tokens are provided where applicable so the endpoints follow HTTP semantics and are protected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java`:
- Around line 339-353: The other state-mutating handlers
setForced-siteSynchronization and setDate-siteSynchronization are still exposed
as GETs; change their request mappings to POST (replace `@GetMapping` with
`@PostMapping`) and keep `@ResponseBody` and produces =
MediaType.APPLICATION_JSON_VALUE, update their method signatures to accept
`@RequestParam/`@PathVariable exactly as currently used (same parameter names) and
call microsoftSynchronizationService.saveOrUpdate... just like
updateSiteSynchronizationDisabled does; also update any client-side calls/tests
to POST and ensure CSRF tokens are provided where applicable so the endpoints
follow HTTP semantics and are protected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2e35fead-81b7-4e43-aca2-9a20e44cb8f4
📒 Files selected for processing (3)
microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.javamicrosoft-integration/admin-tool/src/main/resources/Messages.propertiesmicrosoft-integration/admin-tool/src/main/resources/Messages_es.properties
✅ Files skipped from review due to trivial changes (2)
- microsoft-integration/admin-tool/src/main/resources/Messages.properties
- microsoft-integration/admin-tool/src/main/resources/Messages_es.properties
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://sakaiproject.atlassian.net/browse/SAK-52355
sakaiproject/sakai-reference#283
Summary by CodeRabbit