feat(aci): MSTeams action validator #99540
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## cathy/aci/action-validation-translators #99540 +/- ##
===========================================================================
- Coverage 81.21% 81.21% -0.01%
===========================================================================
Files 8589 8586 -3
Lines 380345 380199 -146
Branches 24102 24102
===========================================================================
- Hits 308903 308783 -120
+ Misses 71096 71070 -26
Partials 346 346 |
cathteng
force-pushed
the
cathy/aci/msteams-action-validation
branch
from
d9b949e to
efb44de
Compare
There was a problem hiding this comment.
high level this all looks good, just a couple of nits / random suggestions. should we also have @GabeVillalobos review to make sure the changes to the integrations code is okay?
src/sentry/notifications/notification_action/action_validation.py
Outdated
Show resolved
Hide resolved
| from .types import BaseActionValidatorHandler | ||
|
|
||
|
|
||
| def _get_integration(validated_data: dict[str, Any], provider: str) -> RpcIntegration: |
There was a problem hiding this comment.
to avoid some circular dep issues; we could create action_validator folder, and have this in a utils method. then have a separate file per type; Slack, MSTeams, etc.
not too critical though, just could be a nice way to organize and reduce what's being imported per file.
| ) | ||
|
|
||
| result = validator.is_valid() | ||
| assert result is False |
There was a problem hiding this comment.
it'd be cool if this also validated the error is related to integration id (i may or may not have written a test like this, and found that the validator was failing as expected, but not for the correct reasons.. and there was just a bug in code 😅)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| (possibly in coordination with grouping other integrations by purpose) | ||
| """ | ||
|
|
||
| from .message_builder import * # noqa: F401,F403 |
There was a problem hiding this comment.
is this for a circular import? 👀 Like how much do we need this, 'cause I think init files that export modules with a bunch of stuff will cause more circular imports down the line.
There was a problem hiding this comment.
yes this is for a circular import
| integration = integration_service.get_integration( | ||
| integration_id=integration_id, |
There was a problem hiding this comment.
same schtuff as the other pr on do we need an objectstatus active or smthn here 🤷♀️
There was a problem hiding this comment.
yeah I forgot to keep it in
There was a problem hiding this comment.
o I forgot I added this in in another spot. The super().clean() uses the passed in integrations to check if the integration_id is valid, and those integrations are all active so I COULD do it or not here
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
cathteng
merged commit 829fa59
into
cathy/aci/action-validation-translators
Merge into #99433 because both MSTeams and Slack use the same config schema, so we should have both action validators ready together when we make
target_identifieroptional on the config schema. The action validators will populatetarget_identifierfor theAction's config for saving.