Repository navigation
Add -MailType, fix configuration defaults, and dispatch through a service table - #13
Merged
Merged
Conversation
- The defaults block tested the parameters for truthiness, so an explicit -ChatType OneOnOne or -IncludeBCCInGroupChat $false was replaced by the configuration file's value. OneOnOne is ChatType enum value 0 and $false is a valid choice, so neither can be told apart from an unsupplied value that way. Test $PSBoundParameters.ContainsKey() instead. - The same block cast the configuration value to [ChatType] unconditionally, so a configuration file with no ChatType setting made every send throw "Cannot convert null to type ChatType", including a Mail-only send that has no use for a chat type. - Resolve both settings into separate $Resolved variables. A parameter keeps its type constraint through later assignments, so [ChatType] cannot be reassigned the $null of a missing setting. This also stops the values leaking between services in a multi-service call, since each iteration now derives them from its own service configuration. - Add ChatType to the Send-ScriptMessage_MicrosoftGraph splat only when [string]::IsNullOrWhiteSpace says there is a real value. Neither $null nor the empty string a configuration file uses for an unset setting can bind to an enum parameter. - Report a Chat message that arrived without a chat type from Send-ScriptMessage_MicrosoftGraph, after its AllowableMessageTypes check, as a "Chat not sent." warning in the result's Error property. Raising it from Send-ScriptMessage instead preempts that check and tells a Mail-only configuration to add ChatType when what it actually needs is Chat in AllowableMessageTypes, and every other chat failure is already reported this way rather than thrown. - Add Pester coverage for both parameters against a configuration that disagrees, the configuration-default path, a Mail-only send with no ChatType setting, a Chat send with no chat type anywhere, a blank ChatType setting, and a Mail-only configuration asked to send Chat, which must still name the disallowed type. The existing chat tests now pass -ChatType rather than setting it in the mocked configuration. - Record the enum and type-constraint measurements behind the fix in Research_Notes/PowerShell-Language-Behavior.md section 10.
- AGENTS.md and CONTRIBUTING.md required placeholder addresses on domain.com, which is a real registered domain. Require a domain reserved by RFC 2606 instead: example.com, example.net, or example.org. None of those can ever be registered, so a placeholder copied out of a help example and run unedited, or a test whose mock is missing, cannot reach a real recipient. The help examples were the weakest spot, since those are what a reader copies and runs. - Send-ScriptMessage help: 14 example addresses to example.com. - Research_Notes/PowerShell-Language-Behavior.md section 9 records observed output, so the expressions were re-run with the new address in both editions rather than edited by hand. - Tests: the one placeholder URL in the attachment mock. - The rule states example.com as the preference but all three as equally correct, so nobody renames the test suite's existing example.org addresses to match.
- [bool] of any non-empty string is $true, so a quoted "false" meant its opposite. IncludeBCCInGroupChat was read with a [bool] cast and MgDisconnectWhenDone with a truthiness test, so both inverted. The two MgDelegatedPermission_* settings are read as -eq $true, which compares as strings and is already correct. - Declare -IncludeBCCInGroupChat as [Nullable[bool]] instead of [Object] with [ValidateSet($null, $true, $false)]. The binder refuses a string outright, while the validation set accepted 'false' and left it a string for the later cast to turn into $true. Nullable keeps the three states the parameter needs, so $null still means "use the configured value", and an unsupplied parameter holds $null too, which retires the $PSBoundParameters.ContainsKey() test. - A configuration file value never passes through a binder, so add Get-ScriptMessageBooleanSetting to reject anything -isnot [bool] with an error naming the setting. Giving that helper a [Nullable[bool]] parameter would delegate the check to the binder but report the internal parameter name instead of the setting, which is no help to someone holding a JSON file. - Each layer checks the settings it owns, before anything is sent: Send-ScriptMessage for the generic IncludeBCCInGroupChat, and Send-ScriptMessage_MicrosoftGraph for MgDisconnectWhenDone. That means moving the Disconnect-MgGraph teardown into the service as well, so the layer that validates the setting is the one that consumes it. - Add Pester coverage for a string passed to the parameter, a quoted value for each setting, and that neither sends. The success cases assert Send-MgUserMail was invoked, because an earlier version of these tests built its config mocks with GetNewClosure(), which swallowed Pester's injected $Service and passed without sending anything. - Record the binder, cast, class property and ValidateSet differences in Research_Notes/PowerShell-Language-Behavior.md section 10.
- Get-ScriptMessageContext called Get-MgContext directly, so a core cmdlet depended on Microsoft.Graph.Authentication. Add Get-ScriptMessageContext_MicrosoftGraph alongside the existing Disconnect-ScriptMessage_MicrosoftGraph and call that instead. No cmdlet in ScriptMessage/Public/ calls a Microsoft Graph cmdlet now. - Lift the property-merging loop out of the switch arm so it runs on whatever any service returns, which also retires the "break # Terminate the switch statement" that only existed because the emptiness check sat inside the arm. Keep [string]::IsNullOrEmpty for that check so the test is unchanged from what shipped. - Add Pester coverage for this cmdlet, which had none: the service name is returned alongside the service's own properties, a service with no connection yields just the service name, and -ReturnCachedContext does not ask the service twice. One test mocks Get-MgContext rather than the service function, so it covers the whole chain. Output is unchanged, so there is no changelog entry.
- Disconnect-ScriptMessage left ScriptMessage_Global_CachedServiceContext untouched, so Get-ScriptMessageContext -ReturnCachedContext kept returning the account and scopes of a session that had already ended. Remove the service's entry from the cache after the service disconnects, which makes the next cached read refresh instead of replaying it. Removing a name the cache does not hold is a no-op, so calling Disconnect-ScriptMessage before anything has asked for the context is unaffected. - Add Pester coverage for Disconnect-ScriptMessage, which had none: it asks the service to disconnect, returns the service name alongside the service's own disconnection information under -ReturnConnectionInfo, returns nothing without that switch, and clears the cache. Mocking the service functions rather than the Microsoft Graph cmdlets keeps the file free of stand-ins.
- Each public cmdlet named MicrosoftGraph in a switch block, five in all, so adding a service meant editing four files in the core. Add a module-scope service table that each file in Services/ registers itself into as it is dot-sourced, and replace every switch with a lookup plus a call. - Record handlers as function NAMES and resolve them with & at call time. Caching a CommandInfo or a script block at registration would bind to the real function and bypass Pester, which mocks by shadowing a command name. - Register-ScriptMessageService rejects a name that is not a MessagingService member, since every -Service parameter is typed to that enum and a service outside it could never be reached. Get-ScriptMessageServiceRegistration throws naming the registered services, replacing a Default arm that said only "Invalid 'Service' value." - Disconnect-ScriptMessage loses its second switch: the loop that merges the service's return information is service-agnostic, so it now runs on whatever comes back, which retires another "break # Terminate the switch statement". - Add Pester coverage: the table holds the Graph handlers, an unknown service names the registered ones, a non-enum name is refused, and all four public cmdlets reach stand-in handlers swapped into the table with no Microsoft Graph mocks loaded, which is what proves the core reaches a service only through the table. Output is unchanged, so there is no changelog entry.
- Send-ScriptMessage, Connect-ScriptMessage and Send-ScriptMessage_MicrosoftGraph each read the file, so one send read it three times, and five times for two services. Each read reopens the file, redoes the encoding fallback and reparses the JSON, so a send had no single consistent view of its own settings: editing the file mid-send could hand the connection different credentials than the checks that followed it. - Send-ScriptMessage now reads once and passes the service's section to Connect-ScriptMessage and to the service through a new optional -ServiceConfig parameter. Both still read the file when nobody passes it, so calling either on its own is unchanged. - Resolve the section with the registered service name rather than the -Service parameter's value, because looking a property up by a name held in an array comes back empty. A configuration file with no section for the service now says so instead of carrying $null into the connection. - Add Pester coverage asserting one read across a Mail and Chat send, and that Connect-ScriptMessage on its own still reads the file. No changelog entry: passing a configuration section was already possible through the service functions, and -ServiceConfig is plumbing rather than a feature. The wiki will cover it.
- -MailType was accepted and then ignored: the core never forwarded it, no service acts on it, and the Mail result reads an undefined variable into its MailType property. Services are meant to be interchangeable, so a setting a service cannot act on is an ordinary case, but dropping it without a word is not. - Register-ScriptMessageService now requires -SupportedSetting, the generic unprefixed settings a service honors. Mandatory so that adding a service forces an answer. Microsoft Graph declares ChatType and IncludeBCCInGroupChat, and not MailType. - Send-ScriptMessage warns when a caller supplies a generic setting the service does not declare, then sends anyway. A warning rather than an error because a single call can go to several services and only some may honor it. A $null is skipped, since that means "use the configured value". - The generic defaults stay in the core rather than moving into the service, as the original plan had it. ChatType and IncludeBCCInGroupChat are generic vocabulary, so resolving them per service would duplicate the same merge rule in every service. - Fix the .PARAMETER MailType help directive, whose trailing "# TODO:" comment stopped Get-Help matching it to the parameter, so Get-Help fell back to the code comment above the declaration and showed an internal note.
- -MailType and the configuration file's MailType setting were accepted and ignored. OneOnOne now sends each unique To, CC, and BCC address its own email with only that address in To; Group, the default, sends one email. Send-ScriptMessage resolves it like ChatType: the parameter beats the configuration file, a missing or blank setting means Group, and an unrecognized value throws before connecting. It is resolved only for a send that includes Mail, so it cannot stop a Chat send. - The Graph service sends one request per recipient, deduplicating addresses without regard to case. A failure for one recipient goes into the result's Error property and the rest are still sent; -ErrorAction Stop makes the catch fire whether or not Send-MgUserMail's error is terminating (Research_Notes/Microsoft-Graph-Behavior.md section 3). The result's MailType property now holds the mail type used. Microsoft Graph declares MailType in -SupportedSetting, and ServiceDispatch.Tests.ps1 narrows the registration to keep exercising the unsupported-setting warning. - Remove Get-ScriptMessageBooleanSetting. The 1.1.0 bug was two reads that turn a quoted "false" into true: a [bool] cast for IncludeBCCInGroupChat and a truthiness test for MgDisconnectWhenDone. Every boolean setting is now an opt-in flag read as <setting> -eq $true with the setting on the left, the form the MgDelegatedPermission_* settings already used, so any value other than true means off, the safe state for all four. The rule is in AGENTS.md and the measurements in Research_Notes/PowerShell-Language-Behavior.md section 10. - Add Tests/FileEncoding.Tests.ps1, which enforces the encoding rules in AGENTS.md (no BOM except a manifest that needs one, LF only, ASCII-only PowerShell files) over tracked and untracked, unignored files, and replace the manual check snippet in AGENTS.md with a pointer to it. - Add a Testing section to AGENTS.md: running Invoke-Tests.ps1, how the Pester files avoid a tenant, and that the scratch scripts in Tests/ send real messages. - RELEASING.md: develop allows squash merges for feature branches, but main must be merged back into develop with a merge commit so develop keeps the tagged release commit. - Changelog: add -MailType under Added, drop the entry for the -MailType warning (which never shipped) and the help-text entry, drop the specific quoted-boolean entries, and add a general Minor entry about validation.
- Register-ScriptMessageService's -SupportedSetting listed the generic settings a service honors, and Send-ScriptMessage warned when a caller passed one the service did not list. With MailType implemented, Microsoft Graph lists all three, so no real call could reach the warning. - It controlled nothing: Send-ScriptMessage passes every message parameter to every service regardless, so a service's send function already has to declare each one. It also worked per service rather than per message type, covered 3 of about 15 parameters, and wrote only to the warning stream, where scripts that inspect results never see it. - Replace it with a rule in AGENTS.md: a service that cannot honor a setting it receives adds a Warning to that message's Error property and sends what it can, as the Graph chat path already does for sending on behalf of others. - Remove the tests that covered it and the narrowed registration they needed. No changelog entry: the declaration and its warning never shipped.
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.
Summary
Combines three stacked branches (configuration-default fixes, boolean-setting fixes, and the service-dispatch
refactor) into one pull request, and adds
-MailType. The user-facing changes are inCHANGELOG.mdunder[Unreleased]:Send-ScriptMessage -MailType:Groupsends one email to all recipients, andOneOnOnesends each To,CC, and BCC recipient their own email with only them in To.
-ChatType OneOnOneor-IncludeBCCInGroupChat $falseis now honored when the configuration filesays otherwise.
ChatTypesetting in the configuration file.Get-ScriptMessageContext -ReturnCachedContextno longer reports a connection thatDisconnect-ScriptMessagealready ended.
Internal changes with no user-visible effect:
a send reads the configuration file once.
AGENTS.md).Tests/FileEncoding.Tests.ps1enforces the file encoding rules,AGENTS.mdgains a Testing section, andRELEASING.mdrequires a merge commit when mergingmainback intodevelop.No live Microsoft Graph testing was done: every test mocks the service functions or the Graph cmdlets. 79 tests
pass under Windows PowerShell 5.1 and PowerShell 7.
Release classification
Checklist
develop, ormainfor a hotfixCHANGELOG.mdhas an entry in the right section under[Unreleased](added if missing), or the change is not user-visible.\Tests\Invoke-Tests.ps1passes