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

Inactive user detection and email notification #865

Merged
merged 9 commits into from
Jun 13, 2021

Conversation

nicoolas25
Copy link
Collaborator

@nicoolas25 nicoolas25 commented May 23, 2021

Summary

This PR introduces an email to invite users to delete their account when they are inactive [1]. It also introduce the query that detect inactive users.

[1] Inactivity is parameterized as described in #829.

Details

Build an SQL query, parameterized with:

  • Minimum number of refused unanswered (expired, not confirmed, nor refused) matches
  • users.age (open) range
  • users.created_at (open) range

This query returns users' IDs and is separate from the jobs that enqueue the emails. The goal of having a separate API is to create an interactive rake command that does preview the number of emails before actually sending them.

⚠️ I'm lacking access to production data to:

  • Understand the performance implication of loading all users IDs in memory
  • Understand how many emails we're talking about

I tried to simulate this with a rake task generating data in the development db and tested manually.

TODO:

  • Prevent the same user to receive this email too often by adding a field: users.last_inactive_email_sent_at
  • Create the rake task to give a better "UI" to trigger this job
  • Be sure that the performance is okay by testing it on production
  • Screenshot

@nicoolas25 nicoolas25 self-assigned this May 23, 2021
@nicoolas25
Copy link
Collaborator Author

@mininao WDYT of the approach?

  • See my comment about performance uncertainty
    • Do you think I can get access to the anonymized data to investigate further?
  • I used a simple copy for the email, any opinion on it? Does it matches what you had in mind?

@nicoolas25 nicoolas25 force-pushed the nicoolas25.unsubscribe-automated-emails-2 branch from 524e1cf to 9284293 Compare May 23, 2021 22:18
@nicoolas25 nicoolas25 force-pushed the nicoolas25.unsubscribe-automated-emails-2 branch 2 times, most recently from af2a2a5 to 1356228 Compare May 30, 2021 08:20
@nicoolas25 nicoolas25 marked this pull request as ready for review May 30, 2021 08:20
@nicoolas25 nicoolas25 requested a review from mininao May 30, 2021 08:20
@nicoolas25
Copy link
Collaborator Author

nicoolas25 commented May 30, 2021

Testing the speed of the query locally:

Screenshot from 2021-05-30 10-22-50

Email:
Screenshot from 2021-05-30 10-27-36

Confirmation screen:
Screenshot from 2021-05-30 10-35-30

Confirmation:
Screenshot from 2021-05-30 10-35-42

@nicoolas25
Copy link
Collaborator Author

I'll probably wait for #895 then rebase and use the latest layout.

@carsso
Copy link
Member

carsso commented May 30, 2021

Je vois que tu utilises refused_at.
La demande originale disait qu'il fallait contacter les personnes inactives (qui n'ont pas répondu).
ça me parait plus logique de sélectionner les personnes qui ne répondent pas du tout plutôt que celles qui refusent les doses.

@nicoolas25
Copy link
Collaborator Author

@carsso Très juste oui. J'ai modifié le code pour prendre en compte les refus explicites et implicites (Match expirés sans réponse) de la même manière.

@carsso
Copy link
Member

carsso commented May 30, 2021

Pour moi refus explicite c'est pas un inactif.
Si il voulait se désinscrire ce serait fait, il a le bouton dans chaque mail.

@nicoolas25
Copy link
Collaborator Author

nicoolas25 commented May 30, 2021

Pas de soucis, c'est changé. Je pense que c'est débattable ceci dit... S'il avait été question de cibler les utilisatrices qui n'ont "pas l'air intéressées", plutôt que les utilisateurs inactifs, ça aurait pu avoir du sens de considérer un refus.

En tout cas, je n'ai pas vraiment d'avis, je suis très content de suivre les directives que l'on me donne et d'itérer.

@carsso
Copy link
Member

carsso commented May 30, 2021

Merci.
@navidemad à changé les templates des emails pour les rendre plus beaux dans #895
Je viens de la prodder, ce serait bien qu'on fasse un template avec le nouveau layout html maintenant qu'on en a un tout beau :)

@nicoolas25
Copy link
Collaborator Author

After @navidemad update on email (thank you 🙏):

Screenshot from 2021-05-31 06-51-51

@nicoolas25 nicoolas25 force-pushed the nicoolas25.unsubscribe-automated-emails-2 branch 2 times, most recently from 2e0764f to 59b41d2 Compare May 31, 2021 06:29
Supprimer mon compte
</mj-button>
<mj-text>
<strong>Si le lien ne fonctionne pas</strong>, copiez et collez l’adresse suivante dans votre navigateur :
<br />
<%= edit_matches_users_url(match_confirmation_token: @match_confirmation_token) %>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I replaced this in order to remove the nested controllers in Match and SlotAlert controllers.

@nicoolas25 nicoolas25 force-pushed the nicoolas25.unsubscribe-automated-emails-2 branch 2 times, most recently from e8fa548 to 7f7984c Compare May 31, 2021 07:03
Copy link
Member

@mininao mininao left a comment

Choose a reason for hiding this comment

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

Looks fantastic to me 🤩

@nicoolas25 nicoolas25 force-pushed the nicoolas25.unsubscribe-automated-emails-2 branch from 7f7984c to 0f398b3 Compare June 6, 2021 08:17
@nicoolas25
Copy link
Collaborator Author

Something is wrong with the coverage reports. The tests are passing though 🤷

@mininao
Copy link
Member

mininao commented Jun 9, 2021

Il faut que je check les tests en local et que je merge.

@mininao mininao force-pushed the nicoolas25.unsubscribe-automated-emails-2 branch from 0f398b3 to 6177b33 Compare June 13, 2021 09:16
@mininao mininao enabled auto-merge (squash) June 13, 2021 09:17
@mininao mininao disabled auto-merge June 13, 2021 09:17
@mininao mininao merged commit 007e499 into main Jun 13, 2021
@mininao mininao deleted the nicoolas25.unsubscribe-automated-emails-2 branch June 13, 2021 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants