Test-MtLimitOnMicrosoftDomainUsage.ps1. Fix-licensing-and-mailbox-filter#1973
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesMicrosoft domain usage validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
powershell/public/maester/exchange/Test-MtLimitOnMicrosoftDomainUsage.ps1 (1)
69-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimize array assignment and guard against missing Object IDs.
Using
+=to append to an array inside a loop is inefficient (O(N^2)) in PowerShell and can significantly slow down the test if many mailboxes match. Additionally, checking if$mbx.ExternalDirectoryObjectIdexists before invoking the Graph request will prevent the entire test from throwing a terminating error and failing entirely if a built-in or synced mailbox lacks this property.Assign the
foreachloop directly to the variable instead and cast it to an array to ensure it is always formatted correctly for downstream parameters.⚡ Proposed refactor
- $mgUsers = @() - foreach ($mbx in $mbxes) { - $mgUsers += Invoke-MtGraphRequest -RelativeUri "users" -UniqueId $mbx.ExternalDirectoryObjectId - } + [array]$mgUsers = foreach ($mbx in $mbxes) { + if ($mbx.ExternalDirectoryObjectId) { + Invoke-MtGraphRequest -RelativeUri "users" -UniqueId $mbx.ExternalDirectoryObjectId + } + }🤖 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 `@powershell/public/maester/exchange/Test-MtLimitOnMicrosoftDomainUsage.ps1` around lines 69 - 72, Update the $mgUsers population loop to assign its output directly to [array]$mgUsers instead of using +=. Before calling Invoke-MtGraphRequest, verify that $mbx.ExternalDirectoryObjectId is present; skip mailboxes without an ID, while preserving the existing users request for valid IDs.
🤖 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 `@powershell/public/maester/exchange/Test-MtLimitOnMicrosoftDomainUsage.ps1`:
- Line 64: Update the Get-Mailbox invocation in
Test-MtLimitOnMicrosoftDomainUsage to specify an unlimited result size, while
preserving the existing discovery-mailbox filter and subsequent Where-Object
filtering.
---
Nitpick comments:
In `@powershell/public/maester/exchange/Test-MtLimitOnMicrosoftDomainUsage.ps1`:
- Around line 69-72: Update the $mgUsers population loop to assign its output
directly to [array]$mgUsers instead of using +=. Before calling
Invoke-MtGraphRequest, verify that $mbx.ExternalDirectoryObjectId is present;
skip mailboxes without an ID, while preserving the existing users request for
valid IDs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 11c62e54-a811-41f4-b7bc-4d6a7ee858b5
📒 Files selected for processing (1)
powershell/public/maester/exchange/Test-MtLimitOnMicrosoftDomainUsage.ps1
+ Improve foreach / array + Handle mailbox without ExternalDirectoryObjectId
…sing-and-mailbox-filter
…sing-and-mailbox-filter
|
Thank you for looking into this and fixing it @JeanPhilippeGeorge much appreciated. I made a small tweak to fix the CodeRabbit recommendation since the old code was only checking the first 1000 and would provide a Pass false positive. |
|
@merill, nice catch. I didn't pay attention to the filtering on the client not on the server! |
2 participants
📑 Test-MtLimitOnMicrosoftDomainUsage.ps1. Fix licensing and mailbox filter
Closes #1840
✅ Checks
/powershell/tests/pester.ps1locally.Summary by CodeRabbit
*@*.onmicrosoft.compattern.