fix(m365): exclude unlicensed users from collaboration silver feeders - #1869
Merged
mitasovr merged 2 commits intoJul 23, 2026
Merged
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between cdc8b82a5f669aeae0d36597fadc75e321556c09 and 50cef2f. 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughChangesM365 license filtering
Identity tenant matching
Release metadata
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TeamsActivity
participant M365DbtModels
participant MetricResultsAPI
TeamsActivity->>M365DbtModels: provide license fields
M365DbtModels->>M365DbtModels: filter explicitly unlicensed activity
M365DbtModels->>MetricResultsAPI: return filtered metric inputs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
mozhaev-dev
approved these changes
Jul 23, 2026
mitasovr
enabled auto-merge (squash)
July 23, 2026 08:16
mitasovr
force-pushed
the
claude/check-issue-736-642b95
branch
from
July 23, 2026 08:46
65c5be8 to
cdc8b82
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/ingestion/tests/e2e/metrics/collab_meeting_unlicensed_excluded.test.yaml (1)
14-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the NULL-license behavior.
The fixture only exercises
trueandfalse; it does not verify the stated conservative behavior forisLicensed: null. Add a third activity row withisLicensed: nulland assert that its activity remains.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ingestion/tests/e2e/metrics/collab_meeting_unlicensed_excluded.test.yaml` around lines 14 - 16, Extend the bronze_m365.teams_activity fixture with a third user whose isLicensed value is null, then update the expected metrics assertions to verify this user’s meeting activity remains included. Preserve the existing true and false license cases and use a distinct user identity and unique_key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/ingestion/connectors/collaboration/m365/dbt/m365__collab_meeting_activity.sql`:
- Around line 45-51: Add a one-time cleanup or explicit incremental deletion for
rows now excluded by the new license predicates, covering all five incremental
models:
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_meeting_activity.sql
(lines 45-51), m365__collab_chat_activity.sql (lines 70-76),
m365__collab_email_activity.sql (lines 34-42),
m365__collab_document_activity_onedrive.sql (lines 39-47), and
m365__collab_document_activity_sharepoint.sql (lines 39-47). Remove previously
materialized activity with isLicensed = false or explicitly empty
assignedProducts while preserving the new filtering for future incremental runs.
In `@src/ingestion/scripts/create-bronze-placeholders.sh`:
- Line 1246: Add idempotent ALTER TABLE ... ADD COLUMN IF NOT EXISTS handling
for isLicensed and assignedProducts in the existing-table branches of the M365
bronze table setup, including the paths corresponding to all three referenced
column definitions. Preserve the current CREATE TABLE definitions for fresh
tables while ensuring warm databases reconcile missing columns before dbt models
query them.
---
Nitpick comments:
In
`@src/ingestion/tests/e2e/metrics/collab_meeting_unlicensed_excluded.test.yaml`:
- Around line 14-16: Extend the bronze_m365.teams_activity fixture with a third
user whose isLicensed value is null, then update the expected metrics assertions
to verify this user’s meeting activity remains included. Preserve the existing
true and false license cases and use a distinct user identity and unique_key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 916a9b72-ab0c-4178-838a-773bcf8de6cd
📥 Commits
Reviewing files that changed from the base of the PR and between 346d39e and cdc8b82a5f669aeae0d36597fadc75e321556c09.
📒 Files selected for processing (15)
charts/insight/Chart.yamldeploy/gitops/.insight-versionsrc/backend/services/identity/helm/Chart.yamlsrc/backend/services/identity/src/Insight.Identity.Infrastructure/ClickHouse/ClickHouseIdentityInputsReader.cssrc/ingestion/connectors/collaboration/m365/dbt/m365__collab_chat_activity.sqlsrc/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_onedrive.sqlsrc/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_sharepoint.sqlsrc/ingestion/connectors/collaboration/m365/dbt/m365__collab_email_activity.sqlsrc/ingestion/connectors/collaboration/m365/dbt/m365__collab_meeting_activity.sqlsrc/ingestion/scripts/create-bronze-placeholders.shsrc/ingestion/tests/e2e/metrics/collab_meeting_unlicensed_excluded.test.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_m365.onedrive_activity.yamlsrc/ingestion/tests/e2e/metrics/schemas/bronze_m365.teams_activity.yamlsrc/ingestion/tests/e2e/metrics/templates/m365_onedrive.yamlsrc/ingestion/tests/e2e/metrics/templates/m365_teams.yaml
Comment on lines
+45
to
+51
| -- Drop unlicensed users (guests, ex-employees, service accounts). Their Teams | ||
| -- activity inflates team-level collab counters and produces orphan gold rows | ||
| -- with no matching insight.people entry. `isLicensed` = "Selected if the user | ||
| -- is licensed to use Teams" (MS Graph getTeamsUserActivityUserDetail). Only the | ||
| -- teams_activity report exposes this flag; the email/onedrive/sharepoint feeders | ||
| -- fall back to `assignedProducts`. Conservative on NULL (unknown -> keep). See #736. | ||
| AND coalesce(isLicensed, true) = true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg 'src/ingestion/connectors/collaboration/m365/dbt/(m365__collab_meeting_activity|m365__collab_chat_activity|m365__collab_email_activity|m365__collab_document_activity_onedrive|m365__collab_document_activity_sharepoint)\.sql$'
echo
echo "== file outlines/contents relevant lines =="
for f in \
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_meeting_activity.sql \
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_chat_activity.sql \
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_email_activity.sql \
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_onedrive.sql \
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_sharepoint.sql
do
echo "--- $f ---"
wc -l "$f"
sed -n '1,140p' "$f" | cat -n
done
echo
echo "== dbt incremental / unique keys across repo =="
rg -n "{% set|unique_key|incremental_strategy|insert_overwrite|merge|where\s*\(\).*incremental|unique_key|unique_key" . -g '*.sql' -g '*.yml' -g '*.yaml' | head -n 250Repository: constructorfabric/insight
Length of output: 20094
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg 'src/ingestion/connectors/collaboration/m365/dbt/(m365__collab_meeting_activity|m365__collab_chat_activity|m365__collab_email_activity|m365__collab_document_activity_onedrive|m365__collab_document_activity_sharepoint)\.sql$'
echo
echo "== file relevant contents =="
for f in \
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_meeting_activity.sql \
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_chat_activity.sql \
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_email_activity.sql \
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_onedrive.sql \
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_sharepoint.sql
do
echo "--- $f ---"
wc -l "$f"
sed -n '1,140p' "$f" | cat -n
done
echo
echo "== dbt incremental / unique key indicators =="
rg -n "{% set|unique_key|incremental_strategy|insert_overwrite|merge|where\s*\(\).*incremental|unique_key|unique_key" . -g '*.sql' -g '*.yml' -g '*.yaml' | head -n 250 || trueRepository: constructorfabric/insight
Length of output: 20077
Add a one-time cleanup for newly excluded unlicensed activity.
These models are materialized='incremental', so the new license predicates filter future source selections but do not remove rows already materialized for isLicensed = false or explicitly empty assignedProducts. Add a cleanup/full refresh or explicit incremental deletion for all five models.
📍 Affects 5 files
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_meeting_activity.sql#L45-L51(this comment)src/ingestion/connectors/collaboration/m365/dbt/m365__collab_chat_activity.sql#L70-L76src/ingestion/connectors/collaboration/m365/dbt/m365__collab_email_activity.sql#L34-L42src/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_onedrive.sql#L39-L47src/ingestion/connectors/collaboration/m365/dbt/m365__collab_document_activity_sharepoint.sql#L39-L47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/ingestion/connectors/collaboration/m365/dbt/m365__collab_meeting_activity.sql`
around lines 45 - 51, Add a one-time cleanup or explicit incremental deletion
for rows now excluded by the new license predicates, covering all five
incremental models:
src/ingestion/connectors/collaboration/m365/dbt/m365__collab_meeting_activity.sql
(lines 45-51), m365__collab_chat_activity.sql (lines 70-76),
m365__collab_email_activity.sql (lines 34-42),
m365__collab_document_activity_onedrive.sql (lines 39-47), and
m365__collab_document_activity_sharepoint.sql (lines 39-47). Remove previously
materialized activity with isLicensed = false or explicitly empty
assignedProducts while preserving the new filtering for future incremental runs.
| @@ -1243,6 +1243,7 @@ CREATE TABLE IF NOT EXISTS bronze_m365.teams_activity ( | |||
| reportRefreshDate Nullable(String), | |||
| reportPeriod Nullable(String), | |||
| lastActivityDate Nullable(String), | |||
| isLicensed Nullable(Bool), | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reconcile existing M365 bronze tables, not only fresh placeholders.
These columns are added only when the table is created. On a warm ClickHouse/E2E database, ch_table_exists skips the CREATE TABLE, leaving older tables without isLicensed or assignedProducts; the new dbt models then fail with UNKNOWN_IDENTIFIER. Add idempotent ALTER TABLE ... ADD COLUMN IF NOT EXISTS branches for existing tables.
Also applies to: 1310-1310, 1334-1334
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ingestion/scripts/create-bronze-placeholders.sh` at line 1246, Add
idempotent ALTER TABLE ... ADD COLUMN IF NOT EXISTS handling for isLicensed and
assignedProducts in the existing-table branches of the M365 bronze table setup,
including the paths corresponding to all three referenced column definitions.
Preserve the current CREATE TABLE definitions for fresh tables while ensuring
warm databases reconcile missing columns before dbt models query them.
Unlicensed M365 users (guests, ex-employees, service accounts) leaked into the class_collab_* silver feeders, inflating team-level collaboration counters and producing orphan gold rows with no matching insight.people entry (constructorfabric#736). Filter at the silver feeder rather than propagating the flag: silver is the clean business contract, and the union_by_tag('SELECT *') shape would otherwise force the column onto every feeder (incl. zoom) and the silver->gold contract. Bronze retains the raw rows for audit, so nothing is lost. - teams feeders (meeting, chat): coalesce(isLicensed, true) = true. isLicensed = "licensed to use Teams" per MS Graph getTeamsUserActivityUserDetail. - email/onedrive/sharepoint: these reports expose no isLicensed, so filter on an empty assignedProducts list (Airbyte stores the array as a JSON string, so an empty list serializes to '[]'). - Conservative on NULL/unknown (keep); drop only proven-unlicensed rows. Also add isLicensed / assignedProducts to the bronze placeholder DDL and the e2e fixtures + templates, plus a regression test asserting an unlicensed user is excluded from the meeting metric while a licensed control flows through. Fixes constructorfabric#736 Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mitasovr
force-pushed
the
claude/check-issue-736-642b95
branch
from
July 23, 2026 08:50
cdc8b82 to
933e308
Compare
mitasovr
added a commit
that referenced
this pull request
Jul 23, 2026
…1875) Follow-up to #736 (shipped in #1869), addressing the CodeRabbit review. 1. Teams feeders (meeting, chat): switch `coalesce(isLicensed, true)` to `coalesce(isLicensed, false) = true`, so rows with a NULL/unknown license flag are dropped alongside explicit `isLicensed=false` — matching the #736 proposal. The e2e teams fixture template now defaults `isLicensed: true` so existing fixtures stay counted; the regression test adds an `isLicensed=null` case asserting it is excluded. The email/onedrive/sharepoint feeders keep their conservative `assignedProducts` filter (drop only explicitly-empty lists, keep NULL): those reports expose no `isLicensed`, and the JSON-string serialization is inferred rather than verified, so a NULL there means "unknown/unsynced" and must not nuke the whole feeder. 2. descriptor.yaml: migrate version "2026.05.04" -> "1.0.0" (strict semver, ADR-0015). Because the current deployed value is non-semver, reconcile classifies this as a `migration` (republish + catalog re-discover, NO full-refresh) — it only establishes the semver baseline. A follow-up MAJOR bump (1.0.0 -> 2.0.0) will dispatch the one-shot `dbt --full-refresh` over `dbt_select` (tag:m365+) that purges collab rows materialized before the #736 filter landed, since incremental models do not retro-delete already-written rows. Refs #736. Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-authored-by: Roman Mitasov <Roman.Mitasov@constructor.tech> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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.
Problem
Unlicensed M365 users (guests, ex-employees, service accounts) leak into the
class_collab_*silver feeders because none of the M365 feeders filter on license status. This inflates team-level collaboration counters and produces orphan gold rows with no matchinginsight.peopleentry.Fixes #736.
Approach — filter at the silver feeder (not propagate)
The filter is applied in the feeder
WHEREclause rather than carrying a license flag into silver for downstream consumers to apply. Rationale:silver.class_collab_*is built viaunion_by_tag('SELECT *'), which requires an identical column set across all feeders. Propagating a column would force it onto every feeder (incl. zoom, which has no license concept) and onto the silver→gold contract.isLicensed=false), so per-tenant audit of dropped rows is still possible.Column set unchanged → the zoom union is unaffected.
What changed
Feeders (
src/ingestion/connectors/collaboration/m365/dbt/):meeting+chat(both readteams_activity):AND coalesce(isLicensed, true) = true.isLicensed= "licensed to use Teams" per MS GraphgetTeamsUserActivityUserDetail.email/onedrive/sharepoint: those report details expose noisLicensed; the only license signal isassignedProducts(empty for unlicensed accounts). Airbyte stores the array as a JSON string, so the filter drops empty lists ('[]'/'').NULL/unknown (keep); only proven-unlicensed rows are dropped — safe no-op if the serialization ever differs.Bronze placeholder DDL (
create-bronze-placeholders.sh): addisLicensed(teams),assignedProducts(onedrive, sharepoint).E2E: add the new columns to the teams/onedrive fixtures + templates (the rig strictly matches fixture columns to the placeholder table), plus a regression test
collab_meeting_unlicensed_excluded.test.yaml— an unlicensed user (meetingsAttendedCount=99) is excluded (valuenull) while a licensed control flows through (value3).Testing
Ran the affected metric suite in the dockerized e2e rig:
(new exclusion test + all existing teams/onedrive/email collab metric tests)
Note for reviewers
The
assignedProductsempty-list serialization ('[]') is inferred from the e2e fixture typing (string) and Airbyte's ClickHouse-destination behavior; it was not verified against a live prod DB. The filter is written with a safe failure mode — if the real format differs it drops nothing rather than over-filtering. The teams path (isLicensed, the concrete evidence in #736) is unambiguous.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Release