SAK-52355 Microsoft add the posibility to enable a new forced synchronization in Microsoft Team for the site#14545
Merged
jesusmmp merged 1 commit intosakaiproject:masterfrom May 4, 2026
Conversation
…ronization in Microsoft Team for the site
Contributor
WalkthroughThis PR introduces the ability to enable/disable Microsoft Teams synchronization at both the site and admin level, adding UI controls, backend toggle endpoints, configuration properties, team archive/unarchive operations, and database schema support to track disabled status across the system. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
microsoft-integration/admin-tool/src/main/resources/Messages_es.properties (1)
67-67:⚠️ Potential issue | 🟡 MinorEncoding corruption detected.
Line 67 contains garbled characters (
��) which indicates a file encoding issue. This should be reviewed and fixed to ensure proper rendering of localized messages.🤖 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 file Messages_es.properties contains encoding-corrupted characters shown as "��" on the affected line; open Messages_es.properties, locate the garbled token "��", replace it with the intended Spanish message (or remove it if it's extraneous), then save the file using UTF-8 encoding without a BOM; verify by reopening the file and running a quick build or linter to ensure the message renders properly and no other lines contain invalid bytes.microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.java (1)
200-209:⚠️ Potential issue | 🟠 MajorDisabled synchronizations still flow through the hook paths.
getAllEnabledSiteSynchronizations(...)only protects callers that opt into it. In this same class, Line 1210, Line 1450, Line 1527, Line 1635, and Line 1695 still load relations withfindBySite(...)/findByGroup(...), so a site-team sync toggled off in the admin UI will keep creating channels and adding/removing members via hooks.🤖 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 hooks and other paths in MicrosoftSynchronizationServiceImpl are still using findBySite(...) / findByGroup(...) which return synchronizations regardless of the enabled flag; update those call sites to only process enabled synchronizations by either replacing repository calls with the enabled-filtering query (e.g., findEnabledBySite(...) / findEnabledByGroup(...) if available) or by filtering the returned list with SiteSynchronization::isEnabled (or calling getAllEnabledSiteSynchronizations(siteId, ...)/getAllEnabledSiteSynchronizations(true) where appropriate) before doing channel/member work; ensure every place that currently calls findBySite or findByGroup checks the synchronization’s enabled state (methods referenced: findBySite, findByGroup, getAllEnabledSiteSynchronizations in class MicrosoftSynchronizationServiceImpl) and skip processing disabled synchronizations.microsoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.java (1)
77-92:⚠️ Potential issue | 🔴 CriticalAdd database migration to initialize new and modified non-null columns before applying entity constraint changes.
The
forcedcolumn changed from nullable to non-null, and thedisabledcolumn is new and non-null. Both changes require explicit database schema handling. Existing installations with data inmc_site_synchronizationwill fail on deployment when Hibernate attempts the schema update—nullable columns cannot be converted to non-null, and non-null columns cannot be added without DEFAULT values or backfill data.Create a database migration (SQL or Liquibase) that:
- Initializes any NULL values in the
forcedcolumn tofalse- Adds the
disabledcolumn with a DEFAULT value offalse- Runs before the entity mapping is applied
Alternatively, add explicit
columnDefinitionattributes to the JPA annotations to includeDEFAULT 0in the generated DDL.🤖 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 - 92, The database schema must be backfilled and the new non-null column added before Hibernate applies the SiteSynchronization entity changes: create a migration that targets the mc_site_synchronization table, UPDATEs any NULL values in the forced column to false, and adds the disabled column with a NOT NULL constraint and DEFAULT false (so existing rows get false) and ensure this migration runs before the JPA mapping is applied; alternatively, modify the SiteSynchronization entity annotations for forced and disabled to include explicit columnDefinition clauses that set a DEFAULT 0/false so generated DDL will add the column with a default and not-null constraint.
🧹 Nitpick comments (1)
site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vm (1)
289-289: Use a Microsoft-specific row class instead ofsummary-subnav-enabled.Line 289 reuses a subnav-specific class for Microsoft synchronization. A dedicated class would reduce style/test coupling and improve template clarity.
Optional tweak
- <tr class="summary-subnav-enabled"> + <tr class="summary-microsoft-sync-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/webapp/vm/sitesetup/chef_site-siteInfo-list.vm` at line 289, The TR currently uses the shared "summary-subnav-enabled" class for Microsoft sync; replace that class on the <tr class="summary-subnav-enabled"> element with a dedicated, descriptive class (e.g., "microsoft-sync-row" or "ms-sync-enabled") so the template is not coupled to subnav styles; after changing the class name update any corresponding CSS/selectors and tests that reference "summary-subnav-enabled" to use the new class and ensure markup and styles remain consistent.
🤖 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-5358: The comment for
microsoft.forced.synchronization.propertyname is misleading; update the text so
it clearly states the default expects only the property name (e.g.,
microsoft.synchronized) and not a combined "microsoft.synchronized=term_eid"
value — the code reads the property name from
microsoft.forced.synchronization.propertyname and uses a separate term_eid value
to match sites. Refer to the keys microsoft.forced.synchronization.propertyname,
microsoft.synchronized and term_eid in the explanatory text and show a correct
example that sets
microsoft.forced.synchronization.propertyname=microsoft.synchronized and
term_eid=2026.
In
`@microsoft-integration/admin-tool/src/main/java/org/sakaiproject/microsoft/controller/MainController.java`:
- Around line 341-349: The updateSiteSynchronizationDisabled method currently
lets exceptions from
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(...) escape,
causing a non-JSON 500; catch any exception around the
saveOrUpdateSiteSynchronization call (inside updateSiteSynchronizationDisabled),
log it, and ensure you return the AjaxResponse object (with ret.setStatus(false)
and ret.setError(...) updated with a helpful message or the exception message)
so the controller always responds with the expected JSON contract to the caller
(referencing updateSiteSynchronizationDisabled, saveOrUpdateSiteSynchronization,
microsoftSynchronizationService, and AjaxResponse).
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/fragments/synchronizationRow.html`:
- Around line 95-100: The toggle link renders aria-label and the visually hidden
span once but toggleDisabled(...) only updates the icon/title, so screen-reader
text goes out of sync; update the template's toggle anchor (the <a> in
synchronizationRow.html) to include data-enable-label and data-disable-label
attributes (populated from the same i18n keys used for th:aria-label/th:text)
and give the span a stable selector/class (e.g., "toggle-disabled-sr"), then
modify the toggleDisabled(elem) function to, after a successful toggle, set
elem.setAttribute('aria-label', newLabel), elem.title = newLabel and update the
innerText of the ".toggle-disabled-sr" span to the same newLabel while still
swapping the icon classes (function name: toggleDisabled).
In
`@microsoft-integration/admin-tool/src/main/webapp/WEB-INF/templates/index.html`:
- Around line 135-140: The toggle handler currently only updates elem.title and
icon classes (see icon.classList.replace and elem.title); update the accessible
name too by calling elem.setAttribute('aria-label', /*[[#{index.enable}]]*/ '' )
or the disable key when toggled, and also update the screen-reader-only text
node (the <span class="sr-only"> sibling rendered in synchronizationRow.html) by
locating that span (e.g., elem.querySelector or nextElementSibling) and setting
its textContent to the same enable/disable label so screen readers see the new
state.
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java`:
- Around line 1016-1095: The archiveTeam and unarchiveTeam methods currently
call the Graph team archive/unarchive endpoints and immediately patch the
SharePoint site and return success, but those Graph calls are async (202 +
Location header pointing to a teamsAsyncOperation); change both methods to
capture the initial response (the POST to get a 202 with Location), extract the
Location/teamsAsyncOperation id, poll that teamsAsyncOperation resource via
getGraphClient() until the operation status is completed or failed (with a
timeout/retry/backoff), and only after the operation reports success perform the
site patch (the customRequest("/sites/" + site.id).patch(...)) and return true;
if the async operation fails or times out, log the failure and return false (or
propagate MicrosoftCredentialsException as currently done). Ensure you update
the code paths around
getGraphClient().teams(teamId).archive(...).buildRequest().post() and
.unarchive().buildRequest().post() to implement the polling logic and gate the
site lock/unlock and returned success on the async operation result.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/MicrosoftSynchronizationEnabler.java`:
- Around line 261-277: The loop over existingSsList updates and re-enables
SiteSynchronization rows even when existingSs.getTeamId() is null/empty; move
the state-update and call to
microsoftSynchronizationService.saveOrUpdateSiteSynchronization inside the
teamId guard (or explicitly skip and log an error for rows with no teamId) so
only records with a valid teamId that successfully unarchive via
microsoftCommonService.unarchiveTeam(...) have existingSs.setSyncDateFrom(...),
existingSs.setSyncDateTo(...), existingSs.setDisabled(false) and are saved;
alternatively, if desired, create a new Team before clearing disabled, but do
not re-enable rows with empty teamId.
- Around line 152-176: The code marks the site enabled
(props.addProperty(SITE_PROPERTY, ...)) and adds the forced-sync property before
calling createOrRestoreMicrosoftTeamForSite(...), which can fail silently;
change the flow so you only add/persist SITE_PROPERTY and the forced-sync prop
after createOrRestoreMicrosoftTeamForSite succeeds. Modify or wrap
createOrRestoreMicrosoftTeamForSite (or call a new helper) to return a boolean
success flag (or throw on failure) when using microsoftCommonService,
microsoftConfigurationService and microsoftSynchronizationService; if it returns
false/null, log an error, set state.setAttribute("alertMessage", ...) and
state.setAttribute(STATE_KEY, false) and return false without adding
SITE_PROPERTY or the forced prop; only add props.addProperty(...) after a
successful createOrRestore result.
- Around line 182-200: The archive call is currently treated as separate from
disabling, so failures in microsoftCommonService.archiveTeam(...) still lead to
ss.setDisabled(true) and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization(ss); instead,
treat archiveTeam as part of the disable transaction: if archiveTeam returns
false or throws, do not call ss.setDisabled(true) or
saveOrUpdateSiteSynchronization(ss), log an error (or rethrow) to surface the
failure and abort further processing (e.g., return/continue outer flow), and
trigger any alert/notification path you have; update the block around
microsoftCommonService.archiveTeam, SiteSynchronization.getTeamId,
ss.setDisabled, and
microsoftSynchronizationService.saveOrUpdateSiteSynchronization to implement
this early-abort behavior.
- Around line 65-77: The addToContext method currently always uses
isEnabledForSite(site) to set CONTEXT_ENABLED_KEY which ignores any pending user
choice stored in the session; change addToContext to first read the
session/state attribute keyed by STATE_KEY (or call
state.getAttribute(STATE_KEY)) and, if present, use that value for
context.put(CONTEXT_ENABLED_KEY), falling back to isEnabledForSite(site) only
when the STATE_KEY attribute is null; ensure this aligns with
applySettingsToState(...) which writes the pending selection so revisiting the
tools shows the user's pending checkbox state rather than the persisted site
property.
- Around line 254-256: MicrosoftSynchronizationEnabler currently uses
ZonedDateTime.now() (JVM default) to compute syncDateFrom and syncDateTo which
can vary across nodes; instead obtain Sakai's timezone via UserTimeService from
ComponentManager (ComponentManager.get(UserTimeService.class)), resolve a single
ZoneId (userTimeService.getLocalTimeZone().toZoneId()), create syncDateFrom with
ZonedDateTime.now(sakaiZone) and derive syncDateTo by calling
syncDateFrom.plusMonths(microsoftConfigurationService.getSyncDuration()); update
the code that sets syncDateFrom and syncDateTo accordingly so both dates use the
same Sakai timezone instant.
In
`@site-manage/site-manage-tool/tool/src/java/org/sakaiproject/site/tool/SiteAction.java`:
- Around line 12303-12306: Run Microsoft synchronization prechecks before any
persistent changes by moving the prepareSiteForSave preflight earlier in the
save flow: call MicrosoftSynchronizationEnabler.prepareSiteForSave(site, state)
(and check state.getAttribute("alertMessage")) before saveFeatures(),
commitSite(site) and any import/LTI mutation code so failures abort before any
commit; also change the Microsoft apply/archive side-effecting work to be
deferred (or queued) until after commitSite(site) confirms success so
prepareSiteForSave only validates and does not persist changes or perform
apply/archive actions.
- Line 8893: MicrosoftSynchronizationEnabler.prepareSiteForSave(Site, state) can
return false and set "alertMessage", but the current flow ignores that and
proceeds to call siteService.save(Site); modify the SiteAction save path so
after calling MicrosoftSynchronizationEnabler.prepareSiteForSave(...) you check
its boolean return and the presence of state.getAttribute("alertMessage") (or
equivalent) and, if the prepare call returned false or an alertMessage exists,
do not call siteService.save(Site) and instead short-circuit to the same
error/return handling used when STATE_MESSAGE prevents saving (propagate the
alert to the UI). Ensure you reference
MicrosoftSynchronizationEnabler.prepareSiteForSave,
state.getAttribute("alertMessage") (or the code's attribute accessor), and the
existing siteService.save(Site) call so the guard is added in the correct place.
---
Outside diff comments:
In `@microsoft-integration/admin-tool/src/main/resources/Messages_es.properties`:
- Line 67: The file Messages_es.properties contains encoding-corrupted
characters shown as "��" on the affected line; open Messages_es.properties,
locate the garbled token "��", replace it with the intended Spanish message (or
remove it if it's extraneous), then save the file using UTF-8 encoding without a
BOM; verify by reopening the file and running a quick build or linter to ensure
the message renders properly and no other lines contain invalid bytes.
In
`@microsoft-integration/api/src/java/org/sakaiproject/microsoft/api/model/SiteSynchronization.java`:
- Around line 77-92: The database schema must be backfilled and the new non-null
column added before Hibernate applies the SiteSynchronization entity changes:
create a migration that targets the mc_site_synchronization table, UPDATEs any
NULL values in the forced column to false, and adds the disabled column with a
NOT NULL constraint and DEFAULT false (so existing rows get false) and ensure
this migration runs before the JPA mapping is applied; alternatively, modify the
SiteSynchronization entity annotations for forced and disabled to include
explicit columnDefinition clauses that set a DEFAULT 0/false so generated DDL
will add the column with a default and not-null constraint.
In
`@microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftSynchronizationServiceImpl.java`:
- Around line 200-209: The hooks and other paths in
MicrosoftSynchronizationServiceImpl are still using findBySite(...) /
findByGroup(...) which return synchronizations regardless of the enabled flag;
update those call sites to only process enabled synchronizations by either
replacing repository calls with the enabled-filtering query (e.g.,
findEnabledBySite(...) / findEnabledByGroup(...) if available) or by filtering
the returned list with SiteSynchronization::isEnabled (or calling
getAllEnabledSiteSynchronizations(siteId,
...)/getAllEnabledSiteSynchronizations(true) where appropriate) before doing
channel/member work; ensure every place that currently calls findBySite or
findByGroup checks the synchronization’s enabled state (methods referenced:
findBySite, findByGroup, getAllEnabledSiteSynchronizations in class
MicrosoftSynchronizationServiceImpl) and skip processing disabled
synchronizations.
---
Nitpick comments:
In
`@site-manage/site-manage-tool/tool/src/webapp/vm/sitesetup/chef_site-siteInfo-list.vm`:
- Line 289: The TR currently uses the shared "summary-subnav-enabled" class for
Microsoft sync; replace that class on the <tr class="summary-subnav-enabled">
element with a dedicated, descriptive class (e.g., "microsoft-sync-row" or
"ms-sync-enabled") so the template is not coupled to subnav styles; after
changing the class name update any corresponding CSS/selectors and tests that
reference "summary-subnav-enabled" to use the new class and ensure markup and
styles remain consistent.
🪄 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: a97f31ca-4890-4ee4-aefe-0b9819870642
📒 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
ern
changed the title
juanjmerono
approved these changes
May 4, 2026
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
2 participants
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
Release Notes
New Features
Configuration