feat(integration): Give each action a UUID
Give each Rule Action a UUID so it's identifiable in future work. Currently we only store related information about what the action should do, but not how to differentiate it uniquely across the platform between one another. For example, a singular Rule could have 3 actions, each one describing that it should be a Slack notification to a different channel. To differentiate, you would have to check the channel id against each other. This is even harder when you need to compare against other rule action types (Discord, email, etc). To understand which action has what side effect, this will get us closer to that visibility.
Why is this in the serializer?
We use the validated data to create and update actions, which are subsets of a Rule. This makes sure all locations where we need validated data are going to get a UUID. We could optionally move this to the "save" method in the Rule model, but felt like this is not the responsibility of the Rule model to ensure.
Resolves: https://github.com/getsentry/sentry/issues/64916
Codecov Report
Attention: 10 lines in your changes are missing coverage. Please review.
Comparison is base (
5516f1a) 81.38% compared to head (acc1ab3) 81.40%. Report is 60 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #64922 +/- ##
==========================================
+ Coverage 81.38% 81.40% +0.01%
==========================================
Files 5243 5252 +9
Lines 231458 232279 +821
Branches 45417 45611 +194
==========================================
+ Hits 188372 189078 +706
- Misses 37231 37310 +79
- Partials 5855 5891 +36
| Files | Coverage Δ | |
|---|---|---|
| src/sentry/api/serializers/rest_framework/rule.py | 72.41% <100.00%> (+3.18%) |
:arrow_up: |
| src/sentry/testutils/cases.py | 85.95% <ø> (+0.10%) |
:arrow_up: |
| src/sentry/api/endpoints/project_rules.py | 94.33% <89.69%> (-1.64%) |
:arrow_down: |
Changes to add uuid to action makes sense to me. Please add a test for the bug you're fixing.
We're not going to fix the bug in this change, as the side effects and lift to fix it is going to be a lot (in terms of updating all the tests and potentially fixing UI flows). We identified where the bug is, and how to fix it, but will tackle it in this ticket later: https://github.com/getsentry/sentry/issues/65028
Got it. Can you help me understand the value that the refactory provides here?
Got it. Can you help me understand the value that the refactory provides here?
Discussed this offline
You will also need to make sure that this is handled correctly during import. We used to follow this procedure here: https://www.notion.so/sentry/On-the-Subject-of-Self-Hosted-Migrations-2433c8da9ec14775b144945853294a3e?pvs=4#0ce1fd4efa4c4ccfbdacd617cb5b813f
Not sure if it's still the process, but if it is, then you'll need to add this uuid generation to https://github.com/getsentry/getsentry/blob/2d08bc5967e2b9125185e0eb05a5910804909e59/bin/issue-alert-actions-migrate.py#L99
I think self-hosted has done some work around this process, so not sure if things have changed since I last looked into it
Good eye, thank you for the call out! Added that generation and work to the import side just in case: https://github.com/getsentry/getsentry/pull/12918/files#diff-6e4fb98a0c4c3c943a829cd081d206611293eda7ff7c7951653486cce51cb20fR36