add sponsored user affiliation expiration mvp

[teovin]

This PR adds the ability to associate users sponsored by registrars with an affiliation expiration date.

Registrars and admins are able to create sponsored users with affiliation expiration dates. By default, the form will open up with an indefinite sponsorship selection. (manage/sponsored-users/add-user?email={email})

If the Sponsor indefinitely checkbox is unchecked, the date selection field will display.

If the checkbox is checked again, the date selection field will hide. Form can be submitted in each state.

Registrars and admins can view the date on the user details page. (manage/sponsored-users/{id})

Users can view their sponsorship on the user profile page. (settings/profile)

Admins can view and modify the date via the admin dashboard. (admin/perma/linkuser/{id}/change/)

Sample notification email sent to the user:

Two scheduled tasks will run once a day checking active sponsored users' expiration dates:

• The deactivation task will deactivate users whose affiliation has expired.
• The warning task will send users an email telling them how many days they have left in their affiliation period.

[codecov]

Codecov Report

Attention: Patch coverage is 31.57895% with 26 lines in your changes missing coverage. Please review.

Project coverage is 68.81%. Comparing base (182a9c9) to head (0ba942a).
Report is 14 commits behind head on develop.

Files	Patch %	Lines
perma_web/perma/celery_tasks.py	18.75%	26 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3585      +/-   ##
===========================================
- Coverage    69.03%   68.81%   -0.22%     
===========================================
  Files           48       48              
  Lines         6978     7014      +36     
===========================================
+ Hits          4817     4827      +10     
- Misses        2161     2187      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[clare-stanton]

Hello! Apologies, I didn't spend enough time reviewing the text of the email.

Please change it to:

Your Perma.cc sponsorship via Test Library is expiring in 7 days. Your Sponsored Links folder will be transitioned into read only on Aug. 14, 2024. You will retain access to your dashboard and all previously made Perma Links.

If you would like to request an extension of your affiliation please reach out to [email protected].

Thanks,
The Perma.cc Team

[bensteinberg]

Hello! Apologies, I didn't spend enough time reviewing the text of the email.

Please change it to:

Your Perma.cc sponsorship via Test Library is expiring in 7 days. Your Sponsored Links folder will be transitioned into read only on Aug. 14, 2024. You will retain access to your dashboard and all previously made Perma Links.

If you would like to request an extension of your affiliation please reach out to [email protected].

Thanks, The Perma.cc Team

Can I suggest "will become read-only" instead of "will be transitioned into read only"?

[teovin]

Hello! Apologies, I didn't spend enough time reviewing the text of the email.
Please change it to:
Your Perma.cc sponsorship via Test Library is expiring in 7 days. Your Sponsored Links folder will be transitioned into read only on Aug. 14, 2024. You will retain access to your dashboard and all previously made Perma Links.
If you would like to request an extension of your affiliation please reach out to [email protected].
Thanks, The Perma.cc Team

Can I suggest "will become read-only" instead of "will be transitioned into read only"?

Done!

[rebeccacremona]

Nice work!

I'm sorry for taking so long to help review!

I notice one thing that needs a tweak, and have a few small suggestions about design.

The tweak

I think this PR does not include an index that will make it efficient to filter on expiration date. Since the query is Sponsorship.objects.filter(status="active", expires_at__lte=max_notification_date), I think that means a joint index on status and expires_at? Would need checking. (Syntax for making an index on many fields). If the number of sponsorships is small, the effect on performance will probably not be that large at first, but especially as that table gets bigger, an index will make sure that query isn't expensive/slow.

When I am working on indexes and new database fields, I like to use Django Debug Toolbar (which we already have installed), because it shows you all the SQL queries that are being run, and you can click the "explain" button to see what query strategy the db is using, and see if your index is being used. Have you done that sort of thing before? If not, let me know, and I can say more :-)

The suggestions

One is about flow. This expiration task loops through each expiring and soon-to-be-expiring user, and either issues one SQL query per user to expire them, or sends an email.

It might be nice to separate the two actions, so that you could expire all users expiring today (or on another date passed into the function as an arg) with a single, kinder-to-the-database update SQL query, rather than issuing one query per user. And, that would take care of all the expirations all at once, with no risk of interruption. With this arrangement, expirations are interleaved with sending warning emails, which, since it involves calling out to an external API (the email service), can take a few seconds per call, or even fail. So, maybe cleaner to decouple the two actions? As in, expire all expired users. Then separately warn all soon-to-expire users (accounting for the possibility of failed emails with a try/except with the usual email-related exceptions and logging when that happens)? Might even be cleanest as separate tasks. What do you think?

Another suggestion is about the parameters for the task.

It might be nice to be able to pass in a date to use, so that it isn't always "today" (which might be nice for tests, and might also help if we miss or need to re-run the task for a given date for any reason, say if it fails because the mail server was down, etc.

E.g., something like:

def warn_expiring_sponsored_users_on_days(days=None, from_date=None):
    if not days:
        days = [7, 15, 30]
    if not from_date:
        from_date = timezone.now().date()

Then you also wouldn't have to supply any default args from celerybeat (unless we decided to override for whatever reason). What do you think?

And last, I wondered if there was a reason for reversing the list and taking the first element, beyond finding the max element in the list. Would calling max achieve the same thing?

Jazzed for this feature!!! Exciting to see your work on it!

[teovin]

Changes are pushed!

Great point on the index! I added it and checked in the db that the index is present on the table. I also checked the query plan via the debug toolbar, however the index wasn't used when the table was queried. I assume Postgres decided to go with a sequential scan for now because the table is tiny. Also since I made the column addition and the index addition in different commits, I have two migration files for these. Let me know if you are leaning against combining them into one, I can do that. Also, I made the other updates and separated it into two tasks.

[rebeccacremona]

Cool!

Looking good!

One followup: did you know that when you run update, Django will return the number of rows updated? That means that you can condense that code, so that instead of first filtering and retrieving the relevant rows (the first query, triggered by if not expired_sponsorships), and then filtering (again) and updating the rows (second query, triggered by update), you can just issue the update, and see if it returns 0 or a larger number:

expired_sponsorships = Sponsorship.objects.filter(status="active", expires_at__lt=expiration_date)

updated = expired_sponsorships.update(status="inactive", status_changed=timezone.now())

if updated:
    logger.info(f"Deactivated {updated} sponsorships that expired on {expiration_date}.")
else:
    logger.info(f"Found no sponsorships that expired on {expiration_date}"

Similarly, you can skip the explicit check in the warning task:

if not expiring_sponsorships:
    return

If there are no sponsorships in the queryset, for sponsorship in expiring_sponsorships will just return naturally 🙂 .

I think I will pull this down and create lots of sponsorships, see if we can see the index being used. Just in case there is something finicky in play 🙂 .

[rebeccacremona]

Cool, I pulled this down to test out the index. First, just as a blunt first pass, I disabled sequential scanning, by running set enable_seqscan=false; in the dbshell. Then, running print(Sponsorship.objects.filter(status="active", expires_at__lt=timezone.now().date())) and checking, shows the new index being used:

Then, I turned it back on, and I used the text fixtures to create a bunch of sponsored users.

from conftest import SponsoredUserFactory

for _ in range(1000):
    SponsoredUserFactory()

And sure enough! That turns out to be enough where postgres switches over to use the index.

Which is to say: huzzah, it works.

[teovin]

Thank you for confirming the index 🥳 I also condensed the conditional as you suggested.