feat(codegen): increment org contributors and assign seat in PR webhook handler#104419
feat(codegen): increment org contributors and assign seat in PR webhook handler#104419
Conversation
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
7f24645 to
f406a40
Compare
f28fa95 to
ed816bd
Compare
…e modified inside test)
| "outcome": outcome, | ||
| }, | ||
| ) | ||
| retry_task() |
There was a problem hiding this comment.
is this in addition to the 3 retries we get? I'm thinking that might be enough potentially. Where if we fail those 3 retries we could do a reconciliation thing later on (and since it's all in a transaction anyway, num_actions also wouldn't increment)
There was a problem hiding this comment.
By "all being in a transaction" I mean the original task firing along with the num_actions increment
There was a problem hiding this comment.
I believe it does count towards the 3 retries. I can increase to 5 retries no prob. What sort of reconciliation were you thinking of doing?
There was a problem hiding this comment.
Update after syncing in person: removing retry_task altogether. We'll use the logs to see what kinds of outcomes are common/flaky/worth retrying, and add it in once we have a better idea of which outcomes it's helpful to retry on.
| is_active = ( | ||
| contributor.num_actions >= ORGANIZATION_CONTRIBUTOR_ACTIVATION_THRESHOLD | ||
| ) | ||
| contributor.save(update_fields=["num_actions"]) |
There was a problem hiding this comment.
Bug: date_updated not updated when incrementing num_actions
The contributor.save(update_fields=["num_actions"]) call won't update the date_updated field even though it has auto_now=True. Django's auto_now behavior doesn't apply when update_fields is specified and the field isn't included in the list. This means the date_updated timestamp will remain stale after num_actions is incremented. There's an index on ["organization_id", "date_updated"] suggesting this field is used for queries, and the reset_num_actions_for_organization_contributors task correctly handles this by explicitly setting date_updated=timezone.now().
There was a problem hiding this comment.
I removed it but just put it back @ajay-sentry, it looks like it's a common pattern to include date_updated in update_fields (removed the manual contributor.date_updated = django_timezone.now() though as it looks like that's not needed)
ajay-sentry
left a comment
There was a problem hiding this comment.
lgtm, @brendanhsentry that's the right flag, right?
fixes ENG-5935
depends on #104371
Track an active contributor on a PR event webhook, if the repo has Seer and AI features enabled (and the
seat-based-seer-enabledfeature flag)ACCEPTEDbilling outcome and its frequency.Extra notes
Info about the user's type (ie, bot or not) and association (ie, external contributor or not) can be obtained from the webhook payload, per the gh docs and gh enterprise docs.
payload["pull_request"]["author_association"]is a required field for allpull_request,pull_request_review, andpull_request_review_commentaction types, and also in gh enterprise. ✅ -> Added to all gh and gh enterprise fixtures.payload["pull_request"]["user"]["type"]is required for thepull_request.openedandpull_request.closedaction types. ✅payload["pull_request"]["user"]["type"]is not required forpull_request.edited, or anypull_request_revieworpull_request_review_commentaction types. But I set up a gh app and triggeredpull_request.editedandpull_request_review.submittedwebhooks and the field was indeed present. It's also already present in the fixtures used in the tests. I'm assuming we can expect it for the majority of webhooks, unless the user somehow hasn't been "classified" already (?). If we can't find the user type we just assume the user isn't a bot (feedback welcome on this behavior).