feat: add cron urls for user verification #2160

SKairinos · 2023-08-08T15:51:02Z

Description

How Has This Been Tested?

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have linked this PR to a ZenHub Issue.

This change is

faucomte97

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SKairinos)

cfl_common/common/helpers/emails.py line 78 at r1 (raw file):

        user.userprofile.is_verified = preverified
        user.userprofile.save()

Extra whitespaces

portal/views/cron/user.py line 142 at r1 (raw file):

# All times are 1pm
# Mon(joined), Tues, Wed, Thur(now), Fri, Sat, Sun

?

Code quote:

# All times are 1pm
# Mon(joined), Tues, Wed, Thur(now), Fri, Sat, Sun

faucomte97

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SKairinos)

portal/views/cron/user.py line 43 at r1 (raw file):

        now = timezone.now()

        emails = User.objects.filter(

Looking at your appengine PR, it looks like these jobs are set to run every 10 hours.
Does that mean each user in this queryset will get sent an email every 10 hours until they either verify or fall into the following time bracket?
If so, could we add something that makes sure each user only gets each reminder type once.

faucomte97

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @SKairinos)

portal/urls.py line 111 at r1 (raw file):

urlpatterns = [
    path(
        "cron/",

Can we add some tests for these endpoints?

SKairinos

Reviewable status: 4 of 11 files reviewed, 4 unresolved discussions (waiting on @faucomte97)

cfl_common/common/helpers/emails.py line 78 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Extra whitespaces

Done.

portal/views/cron/user.py line 43 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Looking at your appengine PR, it looks like these jobs are set to run every 10 hours.
Does that mean each user in this queryset will get sent an email every 10 hours until they either verify or fall into the following time bracket?
If so, could we add something that makes sure each user only gets each reminder type once.

Done. I trigger the job every 24 hours now.

portal/views/cron/user.py line 142 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

?

Done. removed.

faucomte97

Reviewed 6 of 6 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SKairinos)

portal/tests/test_views.py line 720 at r3 (raw file):

    @patch("cfl_common.common.helpers.emails.send_email")
    def test_first_verify_email_reminder_view(self, send_email: Mock):

Should we add the the "negative" checks to these tests too? ie

a user registered less than 7 days doesn't get the first reminder
a user registered less than 14 days doesn't get the second reminder
a user registered less than 19 days doesn't get deleted (check both verified and unverified?)
a user registered more than 19 days who is verified doesn't get deleted

This would require passing the actual subject (or title at least) of the reminder in question in the assert_called_once_with instead of just Any. Let me know if you think it's overkill, I just feel it would make the tests more robust.

faucomte97

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @SKairinos)

portal/views/cron/user.py line 43 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Done. I trigger the job every 24 hours now.

Okay but doesn't that still mean the users will receive an email every day?
We only want them to receive each reminder once - so each user will get 1 reminder on day 7 and one on day 14.

SKairinos

Reviewable status: 9 of 11 files reviewed, 2 unresolved discussions (waiting on @faucomte97)

portal/urls.py line 111 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Can we add some tests for these endpoints?

Done.

portal/tests/test_views.py line 720 at r3 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Should we add the the "negative" checks to these tests too? ie

a user registered less than 7 days doesn't get the first reminder

a user registered less than 14 days doesn't get the second reminder

a user registered less than 19 days doesn't get deleted (check both verified and unverified?)

a user registered more than 19 days who is verified doesn't get deleted

This would require passing the actual subject (or title at least) of the reminder in question in the assert_called_once_with instead of just Any. Let me know if you think it's overkill, I just feel it would make the tests more robust.

Done.

SKairinos

Reviewable status: 9 of 11 files reviewed, 2 unresolved discussions (waiting on @faucomte97)

portal/views/cron/user.py line 43 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Okay but doesn't that still mean the users will receive an email every day?
We only want them to receive each reminder once - so each user will get 1 reminder on day 7 and one on day 14.

No, because the filter will only select users that are:

unverified
joined <= 7 days ago
joined > 8 days ago
so the time span is limited to one day (the "+1" below)

SKairinos

Reviewable status: 8 of 13 files reviewed, 5 unresolved discussions (waiting on @faucomte97)

portal/views/cron/user.py line 64 at r8 (raw file):

        emails = get_unverified_emails(USER_1ST_VERIFY_EMAIL_REMINDER_DAYS)

        print(emails)

remove

portal/views/cron/user.py line 155 at r8 (raw file):

        unverified_emails = list(chain(unverified_teachers, unverified_students))

        for email in unverified_emails:

use .delete() on queryset

portal/views/cron/user.py line 158 at r8 (raw file):

            User.objects.get(email=email).delete()

        logging.info(f"{len(unverified_emails)} unverified users deleted.")

print log before deletion
use .count() on queryset

faucomte97

Reviewed 1 of 4 files at r7, 1 of 1 files at r8, 4 of 5 files at r11, all commit messages.
Reviewable status: 13 of 14 files reviewed, all discussions resolved (waiting on @SKairinos)

portal/views/cron/user.py line 64 at r8 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

remove

Done.

portal/views/cron/user.py line 155 at r8 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

use .delete() on queryset

Done.

portal/views/cron/user.py line 158 at r8 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

print log before deletion

use .count() on queryset

Done.

SKairinos

Reviewable status: 13 of 14 files reviewed, all discussions resolved (waiting on @faucomte97)

portal/views/cron/user.py line 155 at r8 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Done.

There's no need to pull the emails into memory at any point. There's also no need to create a 3rd queryset for User objects.
Use .count() on the teacher and student queryset first, then use .delete() on them.

SKairinos · 2023-08-31T10:16:03Z

portal/views/cron/user.py line 155 at r8 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

There's no need to pull the emails into memory at any point. There's also no need to create a 3rd queryset for User objects.
Use .count() on the teacher and student queryset first, then use .delete() on them.

commenting again to unresolve comment

faucomte97

Reviewed 1 of 2 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @SKairinos)

portal/tests/test_views.py line 906 at r15 (raw file):

                last_name="DependentStudent",
                username="[email protected]",
                email="[email protected]",

School students don't have emails but in doesn't really matter in this case so we can leave it if you want

portal/tests/test_views.py line 911 at r15 (raw file):

            student_user_profile = UserProfile.objects.create(
                user=student_user,
                is_verified=is_verified,

Why are we doing this considering that school students are always unverified?

portal/tests/test_views.py line 941 at r15 (raw file):

            self.assertTrue(User.objects.filter(id=self.teacher_user.id).exists())
            self.assertTrue(User.objects.filter(id=self.student_user.id).exists())
            self.assertTrue(User.objects.filter(id=self.indy_user.id).exists())

Can we replace these with pytest-style assert eg assert User.objects.filter(id=self.teacher_user.id).exists()

Code quote:

            self.assertTrue(User.objects.filter(id=self.teacher_user.id).exists())
            self.assertTrue(User.objects.filter(id=self.student_user.id).exists())
            self.assertTrue(User.objects.filter(id=self.indy_user.id).exists())

portal/tests/test_views.py line 949 at r15 (raw file):

            (self.assertTrue if assert_exists else self.assertFalse)(teacher_user_exists)
            (self.assertTrue if assert_exists else self.assertFalse)(indy_user_exists)
            self.assertTrue(student_user_exists)

pytest-style assert here too please

SKairinos

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @faucomte97)

portal/tests/test_views.py line 906 at r15 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

School students don't have emails but in doesn't really matter in this case so we can leave it if you want

Done. I removed it.

portal/tests/test_views.py line 911 at r15 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Why are we doing this considering that school students are always unverified?

good point. I removed it

portal/tests/test_views.py line 941 at r15 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Can we replace these with pytest-style assert eg assert User.objects.filter(id=self.teacher_user.id).exists()

Done.

portal/tests/test_views.py line 949 at r15 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

pytest-style assert here too please

Done.

faucomte97

Reviewed 1 of 1 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SKairinos)

codecov · 2023-08-31T14:42:55Z

Codecov Report

Merging #2160 (6ffda98) into master (22beedd) will decrease coverage by 0.02%.
The diff coverage is 93.97%.

@@            Coverage Diff             @@
##           master    #2160      +/-   ##
==========================================
- Coverage   94.63%   94.62%   -0.02%     
==========================================
  Files         158      164       +6     
  Lines        4421     4504      +83     
==========================================
+ Hits         4184     4262      +78     
- Misses        237      242       +5

Files Changed	Coverage Δ
portal/mixins/cron_mixin.py	`87.50% <87.50%> (ø)`
portal/views/cron/user.py	`93.44% <93.44%> (ø)`
cfl_common/common/helpers/emails.py	`98.51% <100.00%> (+0.02%)`	⬆️
portal/mixins/__init__.py	`100.00% <100.00%> (ø)`
portal/permissions/__init__.py	`100.00% <100.00%> (ø)`
portal/permissions/is_cron_request_from_google.py	`100.00% <100.00%> (ø)`
portal/urls.py	`100.00% <100.00%> (ø)`
portal/views/cron/__init__.py	`100.00% <100.00%> (ø)`

feat: add cron urls for user verification

f33c061

faucomte97 requested changes Aug 8, 2023

View reviewed changes

SKairinos added 4 commits August 9, 2023 11:44

remove comment

bf70cfc

cron user tests

98bbc42

add cron mixin

e77e461

remove whitespace

06a6f23

SKairinos commented Aug 10, 2023

View reviewed changes

faucomte97 requested changes Aug 11, 2023

View reviewed changes

faucomte97 reviewed Aug 11, 2023

View reviewed changes

SKairinos added 3 commits August 11, 2023 19:18

fix: filter

39e0ce5

add cron test cases

fdb1e84

add negative test cases

0a5d2eb

SKairinos commented Aug 11, 2023

View reviewed changes

faucomte97 added 6 commits August 14, 2023 12:56

Remove custom aimmo branch install

b544e18

Merge branch 'master' into verify_email_reminder

44edf7b

Lockfile

400d08f

fix patch

eab188f

Update logic so it doesn't include students

ad4a2a9

Try adding a trailing slash

67cd21b

SKairinos commented Aug 30, 2023

View reviewed changes

SKairinos and others added 4 commits August 30, 2023 16:18

fix: deletion test

ceaf55e

Update cron view and test after feedback

82c6d9e

fix: tests

cc39d00

Merge changes

3d46d16

faucomte97 reviewed Aug 31, 2023

View reviewed changes

SKairinos commented Aug 31, 2023

View reviewed changes

faucomte97 and others added 4 commits August 31, 2023 11:23

Simplify logic

5b5acfd

Simplify logic again

dae9063

fix: tests

22f43f1

fix: reuse global variable

cb0426c

faucomte97 requested changes Aug 31, 2023

View reviewed changes

feedback

6ffda98

SKairinos commented Aug 31, 2023

View reviewed changes

faucomte97 approved these changes Aug 31, 2023

View reviewed changes

SKairinos merged commit b730dda into master Aug 31, 2023
5 of 6 checks passed

SKairinos deleted the verify_email_reminder branch August 31, 2023 16:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add cron urls for user verification #2160

feat: add cron urls for user verification #2160

SKairinos commented Aug 8, 2023 •

edited by faucomte97

Loading

faucomte97 left a comment

faucomte97 left a comment

faucomte97 left a comment

SKairinos left a comment

faucomte97 left a comment

faucomte97 left a comment

SKairinos left a comment

SKairinos left a comment

SKairinos left a comment

faucomte97 left a comment

SKairinos left a comment

SKairinos commented Aug 31, 2023

faucomte97 left a comment

SKairinos left a comment

faucomte97 left a comment

codecov bot commented Aug 31, 2023

feat: add cron urls for user verification #2160

feat: add cron urls for user verification #2160

Conversation

SKairinos commented Aug 8, 2023 • edited by faucomte97 Loading

Description

How Has This Been Tested?

Checklist:

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

SKairinos commented Aug 31, 2023

faucomte97 left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 31, 2023

Codecov Report

SKairinos commented Aug 8, 2023 •

edited by faucomte97

Loading