Send email to admin on new user signup

[naglepuff]

No description provided.

[naglepuff]

@brianhelba the use of reverse here yields a URL that is relative to the root of the application. I'm assuming there's some Django setting I can use to get the host, but I'm unsure what exactly that is

[brianhelba]

Yes, it is a structural challenge of web applications. Without a Host: header from an ongoing HTTP request (which has its own security challenges, since its untrusted user input), web servers wouldn't know their own hostname.

This is exactly why we enable Django's "sites" framework. For better or worse, this stores the value in the database, so recall that we use this migration to set it to something sensible by default (which will be the initial value during tests). For an actual production instance, we might change the value in the DB by hand (though in this case, the migration value matches the current production location).

You'll want to follow a pattern like this, though I expect that you should just use your current reverse call to get the path component of the URL (not get_absolute_url(), which isn't defined on the built-in User anyway). I think it's fine to hardcode https as the scheme, since any instance that's sending real emails absolutely ought to be served over HTTPS.

Also, you could use urllib.parse.urlunsplit if you were worried about the security implications of joining the components yourself, but I think it's overkill here (though I see no harm if you want to use it). If any of the pieces of the URL contained user input, we would want to ensure that they were properly URL-encoded to avoid any injection attacks.

[brianhelba]

Be aware, if we ever add unrelated functionality that causes additional emails to be sent during things triggered by this test, then this test might break, because mailoutbox[0] might be a different email (and ours is later in the list).

Practically, I don't think that's likely to happen, so this is fine. But it's not ideal for a test to be sensitive to other unrelated functionality. With dozens of such tests, it can really burden larger projects, because you might have to debug and fix unrelated tests just to add a new already-working feature.

The more robust alternative would be to something like:

Suggested change

	m = mailoutbox[0]
	message = next(filter(lambda message: 'New user signup' in message.subject, mailoutbox), None)
	assert message is not None

[brianhelba]

Suggested change

	m = mailoutbox[0]
	message = mailoutbox[0]

Better names make it much easier to read in the future, without needing to examine context.

[brianhelba]

Good catch of a corner case. 😃

If you want to de-indent everything below, you could also invert:

Suggested change

	if recipient_list:
	if not recipient_list:
	return

Totally up to you, whichever you find more readable.

[brianhelba]

FYI, if you wanted to hyper-optimize this, you could use .values_list() to directly get the desired list of strings:

Suggested change

	admins = User.objects.filter(is_superuser=True)
	current_site = Site.objects.get_current()
	recipient_list = [admin.email for admin in admins]
	recipient_list = User.objects.filter(is_superuser=True).values_list('email', flat=True)
	current_site = Site.objects.get_current()

However, given the size of the QuerySet here (~3 admin users), there's no problem with constructing the intermediate User model and accessing the .email property. Your way gives better type checking / autocomplete, better debug-ability when stepping through the code, and ought to look more obvious to a Django developer.

I just wanted to ensure you're aware of the alternative.

[brianhelba]

Looks great.

In hindsight, I think it's indeed a bit cleaner to have the Python just fetch and pass simple context of user and site, then handle all the details of URL formation, etc. in the template.