Skip to content
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

Bulk invite team members (setup section II) #3143

Merged
merged 24 commits into from
Feb 3, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add first_name to OrganizationInvites + update tests
paolodamico committed Feb 3, 2021

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
commit 49c3c6e080bc728ba276a3583bfe0943618108bf
5 changes: 2 additions & 3 deletions posthog/api/organization_invite.py
Original file line number Diff line number Diff line change
@@ -20,6 +20,7 @@ class Meta:
fields = [
"id",
"target_email",
"first_name",
"emailing_attempt_made",
"is_expired",
"created_by",
@@ -40,9 +41,7 @@ def create(self, validated_data: Dict[str, Any], *args: Any, **kwargs: Any) -> O
).exists():
raise exceptions.ValidationError("A user with this email address already belongs to the organization.")
invite: OrganizationInvite = OrganizationInvite.objects.create(
organization_id=self.context["organization_id"],
created_by=self.context["request"].user,
target_email=validated_data["target_email"],
organization_id=self.context["organization_id"], created_by=self.context["request"].user, **validated_data,
)
if is_email_available(with_absolute_urls=True):
invite.emailing_attempt_made = True
32 changes: 24 additions & 8 deletions posthog/api/test/test_organization_invites.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import random

from django.core import mail
from rest_framework import status

from posthog.models.organization import OrganizationInvite, OrganizationMembership

from .base import APIBaseTest

NAME_SEEDS = ["John", "Jane", "Alice", "Bob", "", None]
NAME_SEEDS = ["John", "Jane", "Alice", "Bob", ""]


class TestOrganizationInvitesAPI(APIBaseTest):
@@ -53,6 +54,7 @@ def test_add_organization_invite_with_email(self):
response_data,
{
"target_email": email,
"first_name": "",
"created_by": {
"id": self.user.id,
"distinct_id": self.user.distinct_id,
@@ -80,18 +82,32 @@ def test_can_create_invites_for_the_same_email_multiple_times(self):
# Bulk create invites

def test_allow_bulk_creating_invites(self):
response = self.client.post(
"/api/organizations/@current/invites/bulk/",
{"invites": self.helper_generate_bulk_invite_payload(7)},
format="json",
)
print(response.json())
count = OrganizationInvite.objects.count()
payload = self.helper_generate_bulk_invite_payload(7)

with self.settings(EMAIL_ENABLED=True, EMAIL_HOST="localhost", SITE_URL="http://test.posthog.com"):
response = self.client.post(
"/api/organizations/@current/invites/bulk/", {"invites": payload}, format="json",
)

self.assertEqual(OrganizationInvite.objects.count(), count + 7)

self.assertEqual(response.status_code, status.HTTP_201_CREATED)
response_data = response.json()
response_data = response.json()["invites"]

self.assertEqual(len(response_data), 7)

# Check objects are properly saved and response matches
for i, item in enumerate(response_data):
instance = OrganizationInvite.objects.get(id=item["id"])
self.assertEqual(instance.target_email, payload[i]["target_email"])
self.assertEqual(instance.target_email, item["target_email"])
self.assertEqual(instance.first_name, payload[i]["first_name"])
self.assertEqual(instance.first_name, item["first_name"])

# Emails should be sent
self.assertEqual(len(mail.outbox), 7)

def test_maximum_20_invites_per_request(self):
pass

18 changes: 18 additions & 0 deletions posthog/migrations/0122_organizationinvite_first_name.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 3.0.11 on 2021-02-01 15:09

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
("posthog", "0121_organization_setup_section_2_completed"),
]

operations = [
migrations.AddField(
model_name="organizationinvite",
name="first_name",
field=models.CharField(blank=True, default="", max_length=30),
),
]
5 changes: 3 additions & 2 deletions posthog/models/organization.py
Original file line number Diff line number Diff line change
@@ -155,9 +155,10 @@ def validate_update(

class OrganizationInvite(UUIDModel):
organization: models.ForeignKey = models.ForeignKey(
"posthog.Organization", on_delete=models.CASCADE, related_name="invites", related_query_name="invite"
"posthog.Organization", on_delete=models.CASCADE, related_name="invites", related_query_name="invite",
)
target_email: models.EmailField = models.EmailField(null=True, db_index=True)
first_name: models.CharField = models.CharField(max_length=30, blank=True, default="")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Field name is first_name, but is this actually intended for first name explicitly? If this is only a holdover from User.first_name (which is kind of a legacy issue), then it should probably be just name

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it is indeed to match User.first_name, but I thought we were just using first name, not full name everywhere else too (e.g. the signup form says first name and the placeholder says "Jane"). So first_name sounds appropriate even then, thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'm pretty sure we're using full names, just using a single field, as we don't use User.last_name anywhere. 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, actually we are kind of using it… accidentally, as social auth fills it in on registration. We should get this straight either way. Thoughts @paolodamico?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I'm not sure what most users did before we revamped the signup page, but for recent users not doing social auth, I think it's pretty straightforward to just type your first name. I would just do first name everywhere, it has easier backwards support, it's nicer to receive emails addressed like "Hey John! instead of "Hey John Doe!" and I don't think we really need the last name for anything.

The only reason I can think for this is for very large teams, but in that case maybe the email address is enough, or they could just do full name by their own decision?

created_by: models.ForeignKey = models.ForeignKey(
"posthog.User",
on_delete=models.SET_NULL,
@@ -180,7 +181,7 @@ def validate(self, *, user: Optional[Any], email: Optional[str] = None) -> None:
if OrganizationMembership.objects.filter(organization=self.organization, user=user).exists():
raise ValueError("User already is a member of the organization.")
if OrganizationMembership.objects.filter(
organization=self.organization, user__email=self.target_email
organization=self.organization, user__email=self.target_email,
).exists():
raise ValueError("A user with this email address already belongs to the organization.")