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

for mpp-1813: add new_from_address flag #1913

Merged
merged 1 commit into from
May 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions .env-dist
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ SOCKETLABS_SECRET_KEY="dummy-value"
SOCKETLABS_API_KEY="dummy-value"
SOCKETLABS_VALIDATION_KEY="dummy-value"
RELAY_FROM_ADDRESS="[email protected]:8000"
NEW_RELAY_FROM_ADDRESS="[email protected]:8000"
SITE_ORIGIN="http://127.0.0.1:8000"
MAX_NUM_FREE_ALIASES=5
TWILIO_ACCOUNT_SID=
Expand Down
10 changes: 0 additions & 10 deletions emails/tests/models_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,16 +144,6 @@ def test_create_assigns_to_user(self):
relay_address = RelayAddress.objects.create(user=self.user_profile.user)
assert relay_address.user == self.user_profile.user

@override_settings(MAX_ADDRESS_CREATION_PER_DAY=1000)
def test_create_makes_different_addresses(self):
for i in range(1000):
RelayAddress.objects.create(user=self.premium_user_profile.user)
# check that the address is unique (deeper assertion that the generated aliases are unique)
relay_addresses = RelayAddress.objects.filter(
user=self.premium_user
).values_list("address", flat=True)
assert len(set(relay_addresses)) == 1000

@override_settings(MAX_NUM_FREE_ALIASES=5, MAX_ADDRESS_CREATION_PER_DAY=10)
def test_create_has_limit(self):
try:
Expand Down
63 changes: 62 additions & 1 deletion emails/tests/utils_tests.py
Original file line number Diff line number Diff line change
@@ -1,18 +1,22 @@
from email.utils import parseaddr

from django.conf import settings
from django.contrib.auth.models import User
from django.test import TestCase, override_settings
from unittest.mock import patch
from model_bakery import baker
from waffle.testutils import override_sample
from waffle.models import Flag

from emails.models import get_domains_from_settings
from emails.utils import (
NEW_FROM_ADDRESS_FLAG_NAME,
generate_relay_From,
get_email_domain_from_settings,
remove_trackers,
)

from .models_tests import make_premium_test_user
from .models_tests import make_free_test_user, make_premium_test_user


class FormattingToolsTest(TestCase):
Expand Down Expand Up @@ -91,6 +95,63 @@ def test_generate_relay_From_with_premium_user(self):
)
assert formatted_from_address == expected_formatted_from

@override_settings(RELAY_FROM_ADDRESS="[email protected]",
NEW_RELAY_FROM_ADDRESS="[email protected]")
def test_generate_relay_From_with_new_from_user(self):
Copy link
Member

Choose a reason for hiding this comment

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

😄 I love tests for new functionality! And bonus, the old functionality was already covered by tests!

free_user = make_free_test_user()
new_from_flag = Flag.objects.create(name=NEW_FROM_ADDRESS_FLAG_NAME)
new_from_flag.users.add(free_user)
original_from_address = '"foo bar" <[email protected]>'
formatted_from_address = generate_relay_From(
Copy link
Member

Choose a reason for hiding this comment

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

I suggest an override_settings decorator to force RELAY_FROM_ADDRESS and NEW_RELAY_FROM_ADDRESS to different knows values

Failing locally before setting NEW_RELAY_FROM_ADDRESS: IndexError: string index out of range

original_from_address, free_user.profile_set.first()
)
expected_encoded_display_name = (
"=?utf-8?b?IiJmb28gYmFyIiA8Zm9vQGJhci5jb20+IFt2aWEgUmVsYXldIg==?="
)
expected_formatted_from = "%s <%s>" % (
expected_encoded_display_name,
"[email protected]",
)
assert formatted_from_address == expected_formatted_from
Copy link
Member

Choose a reason for hiding this comment

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

Failing in CircleCI:

>       assert formatted_from_address == expected_formatted_from
E       AssertionError: assert '[email protected]>' == '=?utf-8?b?Ii...7.0.0.1:8000>'
E         Skipping 74 identical leading characters in diff, use -v to show
E         - @127.0.0.1:8000>
E         ?           -----
E         + @127.0.0.1>

# WTF? TestCase tearDown doesn't clear out this waffle flag?
new_from_flag.users.remove(free_user)

@override_settings(RELAY_FROM_ADDRESS="[email protected]",
NEW_RELAY_FROM_ADDRESS="[email protected]")
def test_generate_relay_From_with_non_flagged_user(self):
free_user = make_free_test_user()
Flag.objects.create(name=NEW_FROM_ADDRESS_FLAG_NAME)
original_from_address = '"foo bar" <[email protected]>'
formatted_from_address = generate_relay_From(
original_from_address, free_user.profile_set.first()
)
expected_encoded_display_name = (
"=?utf-8?b?IiJmb28gYmFyIiA8Zm9vQGJhci5jb20+IFt2aWEgUmVsYXldIg==?="
)
expected_formatted_from = "%s <%s>" % (
expected_encoded_display_name,
"[email protected]",
)
assert formatted_from_address == expected_formatted_from

@override_settings(RELAY_FROM_ADDRESS="[email protected]",
NEW_RELAY_FROM_ADDRESS="[email protected]")
def test_generate_relay_From_with_no_user_profile_somehow(self):
free_user = baker.make(User)
Flag.objects.create(name=NEW_FROM_ADDRESS_FLAG_NAME)
original_from_address = '"foo bar" <[email protected]>'
formatted_from_address = generate_relay_From(
original_from_address, free_user.profile_set.first()
)
expected_encoded_display_name = (
"=?utf-8?b?IiJmb28gYmFyIiA8Zm9vQGJhci5jb20+IFt2aWEgUmVsYXldIg==?="
)
expected_formatted_from = "%s <%s>" % (
expected_encoded_display_name,
"[email protected]",
)
assert formatted_from_address == expected_formatted_from

@override_settings(ON_HEROKU=True, SITE_ORIGIN="https://test.com")
def test_get_email_domain_from_settings_on_heroku(self):
email_domain = get_email_domain_from_settings()
Expand Down
14 changes: 11 additions & 3 deletions emails/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import markus
import logging
from waffle import sample_is_active
from waffle.models import Flag

from django.apps import apps
from django.conf import settings
Expand All @@ -28,6 +29,8 @@
from .models import DomainAddress, RelayAddress, Reply, get_domains_from_settings


NEW_FROM_ADDRESS_FLAG_NAME = "new_from_address"

logger = logging.getLogger("events")
study_logger = logging.getLogger("studymetrics")
metrics = markus.get_metrics("fx-private-relay")
Expand Down Expand Up @@ -218,12 +221,17 @@ def get_post_data_from_request(request):


def generate_relay_From(original_from_address, user_profile=None):
_, relay_from_address = parseaddr(settings.RELAY_FROM_ADDRESS)
try:
new_from_flag = Flag.objects.get(name=NEW_FROM_ADDRESS_FLAG_NAME)
if user_profile and new_from_flag.is_active_for_user(user_profile.user):
Copy link
Member

Choose a reason for hiding this comment

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

Nit: test for flag exists but not for that user. And maybe flag exists but user_profile=None

Copy link
Member Author

Choose a reason for hiding this comment

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

Finally got these tests working but I had to explicitly call new_from_flag.users.remove(free_user) at the end of this test too. I guess pytest-django doesn't clean up the Flag objects between tests the same way I expected?

Copy link
Member

Choose a reason for hiding this comment

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

Ug, that one sounds hard to debug!

Looking at the docs, the supported method is the override_flag decorator, which potentially will clean up after itself, although I see a few open issues about cache clearing as well.

It may be worth investigating if 1) the database row in the many-to-many table is deleted during test cleanup, and 2) if there is a cache entry that is created and 3) if the cache entry is cleaned up.

_, relay_from_address = parseaddr(settings.NEW_RELAY_FROM_ADDRESS)
except Flag.DoesNotExist:
pass
if user_profile and user_profile.has_premium:
relay_display_name, relay_from_address = parseaddr(
_, relay_from_address = parseaddr(
"replies@%s" % get_domains_from_settings().get("RELAY_FIREFOX_DOMAIN")
)
else:
relay_display_name, relay_from_address = parseaddr(settings.RELAY_FROM_ADDRESS)
# RFC 2822 (https://tools.ietf.org/html/rfc2822#section-2.1.1)
# says email header lines must not be more than 998 chars long.
# Encoding display names to longer than 998 chars will add wrap
Expand Down
1 change: 1 addition & 0 deletions privaterelay/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@
AWS_SQS_QUEUE_URL = config("AWS_SQS_QUEUE_URL", None)

RELAY_FROM_ADDRESS = config("RELAY_FROM_ADDRESS", None)
NEW_RELAY_FROM_ADDRESS = config("NEW_RELAY_FROM_ADDRESS")
SITE_ORIGIN = config("SITE_ORIGIN", None)
GOOGLE_ANALYTICS_ID = config("GOOGLE_ANALYTICS_ID", None)
INCLUDE_VPN_BANNER = config("INCLUDE_VPN_BANNER", False, cast=bool)
Expand Down