for mpp-1813: add new_from_address flag #1913
Conversation
There was a problem hiding this comment.
Thanks @groovecoder! Changes are needed to get this working in CircleCI, and maybe to make it more reliable locally as well.
privaterelay/settings.py
Outdated
| @@ -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", None) | |||
There was a problem hiding this comment.
Nit: I know we use this pattern a lot, but it seems the code is broken if NEW_RELAY_FROM_ADDRESS isn't in the environment. If we used this, then that would be a startup error decouple.UndefinedValueError: NEW_RELAY_FROM_ADDRESS not found. Declare it as envvar or define a default value. rather than a "why are things not working?" error:
| NEW_RELAY_FROM_ADDRESS = config("NEW_RELAY_FROM_ADDRESS", None) | |
| NEW_RELAY_FROM_ADDRESS = config("NEW_RELAY_FROM_ADDRESS") |
There was a problem hiding this comment.
@maxxcrawford I suspect you hit an issue with this on Thursday. Was there a useful error?
There was a problem hiding this comment.
I did! I've ran across it a few times, so I knew what to expect.
I mentioned it in Slack because I had multiple other engineers reach out to me directly about debugging it.
| new_from_flag = Flag.objects.create(name="new_from_address") | ||
| new_from_flag.users.add(free_user) | ||
| original_from_address = '"foo bar" <[email protected]>' | ||
| formatted_from_address = generate_relay_From( |
There was a problem hiding this comment.
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
| expected_encoded_display_name, | ||
| settings.NEW_RELAY_FROM_ADDRESS, | ||
| ) | ||
| assert formatted_from_address == expected_formatted_from |
There was a problem hiding this comment.
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>
| @@ -91,6 +92,23 @@ def test_generate_relay_From_with_premium_user(self): | |||
| ) | |||
| assert formatted_from_address == expected_formatted_from | |||
|
|
|||
| def test_generate_relay_From_with_new_from_user(self): | |||
There was a problem hiding this comment.
😄 I love tests for new functionality! And bonus, the old functionality was already covered by tests!
| _, relay_from_address = parseaddr(settings.RELAY_FROM_ADDRESS) | ||
| try: | ||
| new_from_flag = Flag.objects.get(name='new_from_address') | ||
| if user_profile and new_from_flag.is_active_for_user(user_profile.user): |
There was a problem hiding this comment.
Nit: test for flag exists but not for that user. And maybe flag exists but user_profile=None
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
groovecoder
force-pushed
the
test-From-address-mpp-1813
branch
6 times, most recently
from
bdae235 to
c89bff4
Compare
groovecoder
force-pushed
the
test-From-address-mpp-1813
branch
from
c89bff4 to
66bb2c2
Compare
There was a problem hiding this comment.
Thanks @groovecoder, sorry the tests were a struggle with this one! I think it is worth spending some time figuring out how to test waffle consistently, since we're planning on using it more in the future. This one is working now, so let's merge!
This PR is for MPP-1813.
How to test:
pytest