-
Notifications
You must be signed in to change notification settings - Fork 6
Add signin/failure invoke activity support #206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for handling sign-in failures by introducing a new SignInFailure model and corresponding SignInFailureInvokeActivity, along with comprehensive unit tests. It also includes a minor formatting change to consolidate a multi-line decorator.
- Adds
SignInFailuremodel to represent authentication failure details (error code and message) - Introduces
SignInFailureInvokeActivityfor handlingsignin/failureinvoke events with discriminated union support - Provides comprehensive test coverage including creation, serialization, and deserialization tests
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/mcpplugin/src/microsoft/teams/mcpplugin/server_plugin.py | Reformatted the @Plugin decorator from multi-line to single-line |
| packages/api/src/microsoft/teams/api/models/sign_in/failure.py | Added new SignInFailure model with optional code and message fields |
| packages/api/src/microsoft/teams/api/models/sign_in/init.py | Exported the new SignInFailure model |
| packages/api/src/microsoft/teams/api/activities/invoke/sign_in/failure.py | Added SignInFailureInvokeActivity class for handling signin/failure invokes |
| packages/api/src/microsoft/teams/api/activities/invoke/sign_in/init.py | Added SignInFailureInvokeActivity to the discriminated union and exports |
| packages/api/tests/unit/test_signin_failure_activity.py | Added comprehensive unit tests for the new activity class |
packages/api/src/microsoft/teams/api/models/sign_in/__init__.py
Outdated
Show resolved
Hide resolved
heyitsaamir
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot implement
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Co-authored-by: heyitsaamir <48929123+heyitsaamir@users.noreply.github.com>
Add signin/failure Activity Support ✓
Successfully resolved the issue where
signin/failureinvoke activities were causing validation errors.Fixes #205
Problem
When sign-in failures occurred, the system threw a validation error:
Solution
Added complete support for the
signin/failureinvoke activity type in the API package.Changes Made
✓ Created SignInFailure model (
packages/api/src/microsoft/teams/api/models/sign_in/failure.py)codeandmessagefields to capture failure details✓ Created SignInFailureInvokeActivity (
packages/api/src/microsoft/teams/api/activities/invoke/sign_in/failure.py)signin/failureevents✓ Updated SignInInvokeActivity union
✓ Updated exports in init files
__all__list for consistency✓ Created fixture-based testing
test_fixtures.pywith pytest parameterization for scalable fixture testingFIXTURE_ACTIVITY_MAPdictionary to map fixture files to activity typesfixtures/signin_failure_invoke_activity.jsonfixture file✓ Validated with real-world data
Verification
✅ All Tests Pass: 103/103 tests passing
✅ Linting: Ruff checks passed
✅ Formatting: All files properly formatted
✅ Type Checking: Pyright - no new errors
✅ Security: CodeQL analysis - 0 alerts
✅ Real-World Test: Successfully parses the actual signin/failure payload from the issue
✅ Code Review: All feedback addressed
Impact
Before: Sign-in failures caused validation errors and prevented proper error handling.
After: Sign-in failures are gracefully handled:
Breaking Changes
None - fully backward compatible. Only adds new functionality.
Original prompt
This section details on the original issue you should resolve
<issue_title>SignIn failures are not handled gracefully.</issue_title>
<issue_description>When there is a sign in failure, the invoke says this. It looks like we're missing a signin/failure invoke type.