feat(github-invite): email nudge task #55703

cathteng · 2023-09-05T18:43:46Z

Every month, we will nudge owners and managers from orgs with Github integrations to invite their missing members -- commit authors that have committed to active repositories in the org but are not org members.

For ER-1794

codecov · 2023-09-05T19:04:03Z

Codecov Report

Merging #55703 (dea4422) into master (7300946) will increase coverage by 0.00%.
Report is 2 commits behind head on master.
The diff coverage is 98.46%.

@@           Coverage Diff           @@
##           master   #55703   +/-   ##
=======================================
  Coverage   79.98%   79.98%           
=======================================
  Files        5060     5062    +2     
  Lines      217635   217701   +66     
  Branches    36848    36857    +9     
=======================================
+ Hits       174066   174124   +58     
- Misses      38228    38235    +7     
- Partials     5341     5342    +1

Files Changed	Coverage
src/sentry/conf/server.py	`ø`
.../api/endpoints/organization_missing_org_members.py	`96.87%`
src/sentry/tasks/invite_missing_org_members.py	`100.00%`

nhsiehgit

No glaring issues afaict
looks good!

a little hesitant to sign off since i don't have full context though 😅

nhsiehgit · 2023-09-07T17:35:58Z

src/sentry/api/endpoints/organization_missing_org_members.py

@@ -28,73 +29,69 @@

 class MissingOrgMemberSerializer(Serializer):
    def serialize(self, obj, attrs, user, **kwargs):
-        return {"email": obj.email, "externalId": obj.external_id, "commitCount": obj.commit_count}
+        return {"email": obj.email, "externalId": obj.external_id, "commitCount": obj.commit__count}


🤔 did yo umean to make this a double __?

yes -- since i annotated the queryset using .annotate(Count("commit")), the way to get the value of the annotation is with commit__count. i was previously setting the annotation value manually to commit_count by doing .annotate(commit_count=Count("commit")) but i think it's better to use the default naming

nhsiehgit · 2023-09-07T17:37:49Z

src/sentry/api/endpoints/organization_missing_org_members.py

+    organization: Organization, provider: str, integration_ids: Sequence[int]
+) -> QuerySet[WithAnnotations[CommitAuthor]]:
+    member_emails = set(organization.member_set.exclude(email=None).values_list("email", flat=True))
+    member_emails.update(


Huh.
Do you need to do this in 2 steps?
maybe instead of an update you could splat both of these into the same set construction? 🤷

oh i can do set union with set(A) | set(B)

nhsiehgit · 2023-09-07T17:40:03Z

src/sentry/api/endpoints/organization_missing_org_members.py

+
+    return (
+        nonmember_authors.filter(
+            commit__repository_id__in=set(org_repos),


Oh interesting.

Not sure this is really an issue, but I wonder if there's a way to make this more efficient - rather than querying for both tables, and instead FK-ing the tables or something 🤔

Good point. They're relatively small queries and since this is offline computation I think we can get away with the current logic.

nhsiehgit · 2023-09-07T17:45:24Z

src/sentry/tasks/invite_missing_org_members.py

+    name="sentry.tasks.invite_missing_members.send_nudge_email",
+    silo_mode=SiloMode.REGION,
+)
+def send_nudge_email(org_id):


Random thought.

It looks like this might be very bursty and trigger a bunch of queries every month.

Do we want to introduce a jitter or some sort of batching so we don't attempt to do this for every org every month, but instead do every org.id%2 bi monthly or something?

i'll filter for Github org integrations first and get the orgs from there. doing that will queue ~50k orgs vs the ~300k that would be queued by filtering for Github repos

nhsiehgit · 2023-09-07T18:33:55Z

src/sentry/api/endpoints/organization_missing_org_members.py

-        set(organization.member_set.exclude(user_email=None).values_list("user_email", flat=True))
-    )
+    member_emails = set(
+        organization.member_set.exclude(email=None).values_list("email", flat=True)


oh - does | join them?
you could also do set1.union(set2) right?

or that yes

nhsiehgit

cool cool cool

+1

sentaur-athena

Looks good. A few notes:

sentaur-athena · 2023-09-07T18:40:25Z

src/sentry/api/endpoints/organization_missing_org_members.py

+
+    return (
+        nonmember_authors.filter(
+            commit__repository_id__in=set(org_repos),


Good point. They're relatively small queries and since this is offline computation I think we can get away with the current logic.

sentaur-athena · 2023-09-07T18:46:04Z

src/sentry/api/endpoints/organization_missing_org_members.py

@@ -125,7 +122,7 @@ def provider_reducer(dict, integration):
            if integration_provider != "github":
                continue

-            queryset = self._get_missing_members(
+            queryset = _get_missing_organization_members(


In most cases this would be empty so you can early exit here

i do have to return something even if it's empty though

i can do extra processing only if the queryset exists

JoshFerge

are we going to gate this behind some kind of feature flag? excuse me if that gating is in a separate PR.

cathteng · 2023-09-11T19:34:50Z

@JoshFerge the gating right now is per org. do you mean some kind of option that prevents the task from scheduling all of the other tasks in the first place?

JoshFerge · 2023-09-11T19:36:25Z

@JoshFerge the gating right now is per org. do you mean some kind of option that prevents the task from scheduling all of the other tasks in the first place?

basically: when this is merged, will it begin to send notifications out for every org with a github integration?

cathteng · 2023-09-11T19:37:52Z

@JoshFerge this task will run once a month on the first of the month and will only run for orgs with the organizations:integrations-gh-invite flag. although the queueing will still happen for all of them with this current implementation

JoshFerge · 2023-09-11T19:40:34Z

invite

got it. great. thanks!

getsentry-bot · 2023-09-11T22:19:50Z

PR reverted: 22deb13

This reverts commit c984fd2. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>

github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 5, 2023

Base automatically changed from cathy/github/growth/email-nudge-notification to master September 5, 2023 19:01

cathteng force-pushed the cathy/github-growth/email-nudge-task branch from f0c9348 to 510e310 Compare September 5, 2023 19:06

vercel bot deployed to Preview September 5, 2023 19:09 View deployment

vercel bot deployed to Preview September 5, 2023 22:16 View deployment

cathteng marked this pull request as ready for review September 5, 2023 22:46

cathteng requested a review from a team September 5, 2023 22:46

nhsiehgit reviewed Sep 7, 2023

View reviewed changes

cathteng requested review from a team September 7, 2023 18:31

vercel bot deployed to Preview September 7, 2023 18:33 View deployment

nhsiehgit reviewed Sep 7, 2023

View reviewed changes

nhsiehgit approved these changes Sep 7, 2023

View reviewed changes

sentaur-athena approved these changes Sep 7, 2023

View reviewed changes

vercel bot deployed to Preview September 7, 2023 22:01 View deployment

JoshFerge reviewed Sep 11, 2023

View reviewed changes

cathteng added 12 commits September 11, 2023 12:35

add MISSING_MEMBERS NotificationSettingsType

c34761a

fix test

ac17c0a

use NotificationSettingType.APPROVAL

0825c50

fix test

4d1fb91

ref missing members API

1e83637

nudge invite task

6eede79

schedule task on the first of each month

807c814

fix import

79f2c01

fix typing and name

1fdcf5f

small refactor

bd61ba1

further parse queryset only if it exists

67fabd5

use queue for nudge invite emails

dea4422

cathteng force-pushed the cathy/github-growth/email-nudge-task branch from c6ef1f1 to dea4422 Compare September 11, 2023 19:35

vercel bot deployed to Preview September 11, 2023 19:39 View deployment

cathteng merged commit c984fd2 into master Sep 11, 2023
58 checks passed

cathteng deleted the cathy/github-growth/email-nudge-task branch September 11, 2023 21:29

cathteng added the Trigger: Revert Add to a merged PR to revert it (skips CI) label Sep 11, 2023

getsentry-bot added a commit that referenced this pull request Sep 11, 2023

Revert "feat(github-growth): email nudge task (#55703)"

22deb13

This reverts commit c984fd2. Co-authored-by: cathteng <70817427+cathteng@users.noreply.github.com>

cathteng restored the cathy/github-growth/email-nudge-task branch September 11, 2023 23:25

cathteng mentioned this pull request Sep 12, 2023

feat(github-invite): email nudge task round 2 #56092

Merged

github-actions bot locked and limited conversation to collaborators Sep 27, 2023

cathteng changed the title ~~feat(github-growth): email nudge task~~ feat(github-invite): email nudge task Dec 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(github-invite): email nudge task #55703

feat(github-invite): email nudge task #55703

cathteng commented Sep 5, 2023

codecov bot commented Sep 5, 2023 •

edited

Loading

nhsiehgit left a comment

nhsiehgit Sep 7, 2023

cathteng Sep 7, 2023

nhsiehgit Sep 7, 2023

cathteng Sep 7, 2023

nhsiehgit Sep 7, 2023

sentaur-athena Sep 7, 2023

nhsiehgit Sep 7, 2023

cathteng Sep 7, 2023

nhsiehgit Sep 7, 2023

cathteng Sep 7, 2023

nhsiehgit left a comment

sentaur-athena left a comment

sentaur-athena Sep 7, 2023

sentaur-athena Sep 7, 2023

cathteng Sep 7, 2023

cathteng Sep 7, 2023

JoshFerge left a comment

cathteng commented Sep 11, 2023

JoshFerge commented Sep 11, 2023

cathteng commented Sep 11, 2023 •

edited

Loading

JoshFerge commented Sep 11, 2023

getsentry-bot commented Sep 11, 2023

feat(github-invite): email nudge task #55703

feat(github-invite): email nudge task #55703

Conversation

cathteng commented Sep 5, 2023

codecov bot commented Sep 5, 2023 • edited Loading

Codecov Report

nhsiehgit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhsiehgit left a comment

Choose a reason for hiding this comment

sentaur-athena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshFerge left a comment

Choose a reason for hiding this comment

cathteng commented Sep 11, 2023

JoshFerge commented Sep 11, 2023

cathteng commented Sep 11, 2023 • edited Loading

JoshFerge commented Sep 11, 2023

getsentry-bot commented Sep 11, 2023

codecov bot commented Sep 5, 2023 •

edited

Loading

cathteng commented Sep 11, 2023 •

edited

Loading