Don't send newsletters to unconfirmed accounts

[mohsinkhansymc]

References

• Closes Consul sends newsletters to unconfirmed accounts #3761

Objectives

Prior to this PR, an unconfirmed user was considered as active. After this change a user who has confirmed his account is an active user.

[mohsinkhansymc]

@PierreMesure Please review, thanks

[javierm]

@mohsinkhansymc Thanks for this pull request! 😄 I'll have a look at it, hopefully next week.

In the meantime, could you add a test for the scenario we're trying to fix (sending newsletters to unconfirmed account)? 🙏 Thanks!

[PierreMesure]

Thanks a lot!

Did you do any manual test? Because by looking at the other scopes like:

scope :newsletter,     -> { where(newsletter: true) }
scope :email_digest,   -> { where(email_digest: true) }

It doesn't seem like they filter by active user, but booleans like newsletter or email_digest are already set to true before the account is confirmed. Shouldn't we add that criterium to all the scopes (except erased)?

I will have time to try it myself on Friday.

[mohsinkhansymc]

@PierreMesure The active check is enough as first all the users are fetched and then

Thanks a lot!

Did you do any manual test? Because by looking at the other scopes like:

scope :newsletter,     -> { where(newsletter: true) }
scope :email_digest,   -> { where(email_digest: true) }

It doesn't seem like they filter by active user, but booleans like newsletter or email_digest are already set to true before the account is confirmed. Shouldn't we add that criterium to all the scopes (except erased)?

I will have time to try it myself on Friday.

This code should work if the said scope sends to all users, but sure you can have a look on Friday

[mohsinkhansymc]

@javierm Please review the test case, thanks

[PierreMesure]

Ah, thanks! That answers my question!

@mohsinkhansymc your test looks great. I'd say you don't need to test that user3 is not in the array since you already tested that the array is exactly [user1, user2].

If @javierm has time to review and merge before Friday, then please do it. Otherwise, I'll try to spend half an hour on it at the end of the week.

[javierm]

@mohsinkhansymc Thank you very much! 😄 I've left a couple of comments; let me know what you think 😉. I think we're definitely on the right track here 👍.

[mohsinkhansymc]

@javierm I have done the changes you proposed. Thanks

[PierreMesure]

Before we close this, I'd like to point out that User.active is also used to calculate the number of users shown in the Statistics page on the admin panel. Is this what we want? Or do we want to reflect the number of users who actually confirmed their e-mail address?

In general, shouldn't we replace all the occurrences of User.active by a new scope that dismisses the users that didn't confirm their e-mail addresses?

In the cases where we use this method to find users who have already created content or to filter by document number, that might actually slightly increase the performances.

[javierm]

In general, shouldn't we replace all the occurrences of User.active by a new scope that dismisses the users that didn't confirm their e-mail addresses?

The active scope is used in several places related to verification, which is a part of the application most forks change, so I'd rather be conservative here and keep the existing behavior 🤔.

Regarding the admin statistics, I don't have a strong opinion; I guess both approaches are valid 🤔.

[PierreMesure]

Yes, I agree with you that touching active is not a good idea. But creating a confirmed_users scope which we can then use in all the places where active is used today?

I'm curious, would you be able to check how many accounts never confirmed their e-mail address on decide.madrid.es?

The query would be

SELECT COUNT(*)
FROM users
WHERE confirmed_at is NULL;

[mohsinkhansymc]

@PierreMesure we can have a separate label on the stats page that shows confirmed accounts or we can modify the total users label to confirmedcount/totalusers. Also instead of showing unverified users we can show active users that will return the confirmed accounts count. What do you guys think? @javierm

[javierm]

This is an interesting topic 🤔. Since we're trying to get everything ready for version 1.1, I'd rather be conservative and only change the code affecting newsletters. Feel free to open a new issue and continue this conversation once we release version 1.1, and maybe then we can study the implications of chaging the active scope 😄.

@mohsinkhansymc For now, it would be awesome if we kept this pull request as it is, except for the suggested change in the all_users method, and then we can merge it and it will be included in version 1.1 🎉.

[javierm]

@mohsinkhansymc Thanks a lot! 😄