RET triggered emails by cpennington · Pull Request #15745 · openedx/openedx-platform

cpennington · 2017-08-04T15:42:17Z

No description provided.

cpennington · 2017-08-21T22:15:45Z

+    ).filter(
+        start__gte=target_hour,
+        start__lt=target_hour + datetime.timedelta(minutes=60),
+    )


This part needs to be site aware as well, because individual sites manage specific courses.

We should also filter out inactive enrollments

nasthagiri · 2017-08-22T15:33:21Z

+import openedx.core.djangoapps.xmodule_django.models
+
+
+class Migration(migrations.Migration):


How has these migration changes been tested?

You can see the generated sql in https://github.com/edx/edx-platform/pull/15878

nasthagiri · 2017-08-22T15:38:44Z

+            name='course_id',
+            field=openedx.core.djangoapps.xmodule_django.models.CourseKeyField(max_length=255, db_index=True, verbose_name="Course", db_column='course_id'),
+        ),
+        migrations.RenameField(


Can you explain how this migration doesn't result in an actual SQL migration, causing issues with blue/green deployments?

Cale and I talked through several solutions, I'll let him explain further.

nasthagiri · 2017-08-22T15:39:45Z

+            model_name='coursemode',
+            name='course_id',
+            field=openedx.core.djangoapps.xmodule_django.models.CourseKeyField(max_length=255, db_index=True, verbose_name="Course", db_column='course_id'),
+        ),


How is this actually altering the field? Is it not declared the same as before?

I've added comments explaining this.

nasthagiri · 2017-08-22T15:44:49Z

+        course_id = self.cleaned_data['course']
        try:
            course_key = CourseKey.from_string(course_id)
        except InvalidKeyError:


Do we still want the call to the modulestore below? Or, should it be checking CourseOverview instead?

I'd rather not muck with that logic right now.

nasthagiri · 2017-08-22T15:46:36Z

        super(CourseModeForm, self).__init__(*args, **kwargs)

+        if self.data.get('course'):
+            self.data['course'] = CourseKey.from_string(self.data['course'])


So am I reading this right - for the django admin view, we never bother converting the course string value to a CourseOverview object?

The admin form widget for a ForeignKey just uses the key to the other table, not the actual object.

nasthagiri · 2017-08-22T16:12:36Z

        self.assertIn(self.student.email, sent_addresses)
        self.assertIn(self.instructor.email, sent_addresses)
+
+    def test_policy_optedout(self):


Nit: For readability and self-documenting code, consider putting these test methods in a separate Test class for ACE-specific policy tests.

nasthagiri · 2017-08-22T16:14:13Z

+        """
+        url = reverse('change_email_settings')
+        response = self.client.post(url, {'course_id': self.course.id.to_deprecated_string(), 'receive_emails': 'on'})
+        self.assertEquals(json.loads(response.content), {'success': True})


Nit: consider DRYing the redundant setup of these test methods.

nasthagiri · 2017-08-22T16:15:11Z

    for enrollment in user_enrollments:
-        if grade_factory.read(user=user, course=enrollment.course).passed:
-            if fire_ungenerated_certificate_task(user, enrollment.course.id):
+        if grade_factory.read(user=user, course=enrollment.course_overview).passed:


Wait. Shouldn't enrollment.course work now?

.course doesn't create the CourseOverview if it doesn't already exist.

nasthagiri · 2017-08-22T16:17:04Z

+ACE_CHANNEL_SAILTHRU_TEMPLATE_NAME = ENV_TOKENS.get('ACE_CHANNEL_SAILTHRU_TEMPLATE_NAME', ACE_CHANNEL_SAILTHRU_TEMPLATE_NAME)
+ACE_CHANNEL_SAILTHRU_API_KEY = AUTH_TOKENS.get('ACE_CHANNEL_SAILTHRU_API_KEY', ACE_CHANNEL_SAILTHRU_API_KEY)
+ACE_CHANNEL_SAILTHRU_API_SECRET = AUTH_TOKENS.get('ACE_CHANNEL_SAILTHRU_API_SECRET', ACE_CHANNEL_SAILTHRU_API_SECRET)
+ACE_ROUTING_KEY = ENV_TOKENS.get('ACE_ROUTING_KEY', ACE_ROUTING_KEY)


Isn't it preferable to put all ACE settings in their own dict structure? Or is having separate variables like this better operationally? FYI @jibsheet

The biggest problems with using a dict is that Ansible doesn't merge dictionaries from lms/configuration/internal/secure together, it just takes the top level dictionary in the innermost override, so you have to have wacky looking overrides where you pull out the API_KEY and API_SECRET into secure and then interpolate them in edx-internal (see Cale's examples here https://openedx.atlassian.net/wiki/spaces/EdxOps/pages/122454502/Splitting+of+secure+repos) . I like the idea of most of these being grouped in a dictionary, but in my experience they cause more pain. If we've documented the dict approach somewhere, let me know and I can revisit those docs.

nasthagiri · 2017-08-22T16:18:25Z

 from xmodule.modulestore.tests.factories import CourseFactory, check_mongo_calls, check_mongo_calls_range

-from .models import CourseOverview, CourseOverviewImageSet, CourseOverviewImageConfig
+from ..models import CourseOverview, CourseOverviewImageSet, CourseOverviewImageConfig


Why were tests moved to an init.py file?

I wanted tests to be a module, because I added factories.py to it. But I could move the tests into a separate submodule as well.

nasthagiri · 2017-08-22T20:40:36Z

+        enrollment=enrollment,
+        active=True,
+        start=timezone.now(),
+        upgrade_deadline=timezone.now() + datetime.timedelta(days=21)


Should this upgrade_deadline offset be a django admin setting (as part of ScheduleConfig)?

I've added a discussion item for morning planning on whether this offset also needs to have a course-specific override.

nasthagiri · 2017-08-22T20:50:48Z

+class RecurringNudge(MessageType):
+    def __init__(self, week, *args, **kwargs):
+        super(RecurringNudge, self).__init__(*args, **kwargs)
+        self.name = "recurringnudge_week{}".format(week)


Nit: add an underscore between recurring and nudge for readability.

I was trying to keep this consistent with the standard template naming (which just lowercases the class name).

nasthagiri · 2017-08-22T20:52:53Z

+        return
+
+    if not (
+        ScheduleConfig.current().is_enabled or


Does this call to ScheduleConfig need to pass a value for site?

nasthagiri · 2017-08-22T20:56:37Z

+        self.site = site
+        self.current_date = current_date.replace(hour=0, minute=0, second=0)
+
+    def send(self, week):


A docstring here (or at the class level) would be useful to explain that the week here refers to the week number since schedule start.

nasthagiri · 2017-08-22T20:59:14Z

+
+@task(ignore_result=True, routing_key=settings.ACE_ROUTING_KEY)
+def _schedule_hour(site_id, week, target_hour):
+    msg_type = RecurringNudge(week)


Hmm.. interesting. The RecipientResolver here has also become the Composer of the email. As a Later refactor, what do you think of decoupling the 2 so that multiple Composers can use the same ScheduleStartResolver?

nasthagiri · 2017-08-22T21:11:12Z

+class Command(BaseCommand):
+
+    def add_arguments(self, parser):
+        parser.add_argument('--date', default=datetime.datetime.utcnow().date().isoformat())


Need to add help strings here for the expected format of the arguments.

nasthagiri · 2017-08-22T21:17:53Z

+
+    def handle(self, *args, **options):
+        current_date = datetime.datetime(
+            *[int(x) for x in options['date'].split('-')],


Use datetime.strptime instead?

I don't think you can force strptime to be a timezone aware UTC date, unfortunately.

We've done it like this previously
https://github.com/edx/edx-platform/pull/14608/files (and some other management commands)

nasthagiri · 2017-08-22T21:23:09Z

+
+            template_context = {
+                'user_full_name': user.profile.name,
+                'user_personal_address': user.profile.name if user.profile.name else user.username,


Nit: is there a better name for this? Doesn't Personal address imply a residential address? How about user_personal_name?

nasthagiri · 2017-08-22T21:29:28Z

+    pass
+
+
+class VerifiedDeadlineResolver(RecipientResolver):


Is this still work-in-progress? If so, we can get back to this later. At a first glance, I'm wondering where we filter out users who have already verified? And filter out courses whose verification deadlines have already passed.

Yes, it's still in work-in-progress.

nasthagiri · 2017-08-22T21:31:25Z

@@ -0,0 +1,3 @@
+Dear {{student_name}},


Why do we need a subfolder named "schedules" within a folder already named "schedules"?

So that theming overrides can set the template schedules/edx_ace/recurringnudge_week1/email/body.html to override this template. Django doesn't give app templates separate namespaces by default, afaik.

nasthagiri · 2017-08-22T21:53:05Z

Q: does this need a -- prefix?

No, it's required.

edx-pipeline-bot · 2017-08-23T17:28:06Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Thursday, August 24, 2017.

mulby · 2017-08-26T23:29:54Z

nasthagiri · 2017-08-30T13:39:27Z

+                DynamicUpgradeDeadlineConfiguration.is_enabled()
+                or CourseDynamicUpgradeDeadlineConfiguration.is_enabled(self.course_id)
+            )
+            if schedule_driven_deadlines_enabled and self.schedule and self.schedule.upgrade_deadline is not None:


Add a comment for why we're checking for a non-Null value for upgrade_deadline.

nasthagiri · 2017-08-30T16:27:08Z

+class ScheduleConfig(ConfigurationModel):
+    KEY_FIELDS = ('site',)
+
+    site = models.ForeignKey(Site)


I believe we need to add a field for create_schedules in this model. Otherwise, we can't do a course-specific staged rollout after enabling this global flag, per our rollout plan. See step #3 and #6.

nasthagiri · 2017-09-01T00:39:22Z

+            return
+
+        try:
+            site_config = SiteConfiguration.objects.get(site_id=self.site.id)


This logic is not completely straightforward to understand and seems like it doesn't belong within this function. It may be better suited to create a helper function for this within SiteConfiguration.

edx-pipeline-bot · 2017-09-05T14:27:44Z

EdX Release Notice: This PR has been deployed to the production environment.

cpennington force-pushed the ret/triggered-email branch 6 times, most recently from 135c383 to bab199c Compare August 8, 2017 21:01

mulby force-pushed the ret/triggered-email branch from 8e62bda to f6220ca Compare August 11, 2017 18:16

cpennington force-pushed the ret/triggered-email branch 2 times, most recently from 643d15c to 876df69 Compare August 16, 2017 20:56

cpennington mentioned this pull request Aug 21, 2017

Add the non-secret ACE configuration with defaults openedx-unsupported/configuration#4050

Merged

6 tasks

cpennington commented Aug 21, 2017

View reviewed changes

nasthagiri reviewed Aug 22, 2017

View reviewed changes

cpennington force-pushed the ret/triggered-email branch 2 times, most recently from 6fce3f6 to 613240e Compare August 23, 2017 15:02

cpennington force-pushed the ret/triggered-email branch from 613240e to d9020ec Compare August 23, 2017 19:32

nasthagiri force-pushed the ret/triggered-email branch from e71f7c3 to 90aef0f Compare August 29, 2017 13:04

nasthagiri reviewed Aug 30, 2017

View reviewed changes

mulby force-pushed the ret/triggered-email branch from 65784b0 to cb41ee5 Compare August 31, 2017 19:56

nasthagiri force-pushed the ret/triggered-email branch 2 times, most recently from 8b1b126 to bc5d8dc Compare August 31, 2017 20:43

nasthagiri changed the title ~~WIP: RET triggered emails~~ RET triggered emails Aug 31, 2017

nasthagiri force-pushed the ret/triggered-email branch from bc5d8dc to 756ac01 Compare August 31, 2017 20:48

Use ACE to 'send' Recurring Nudge emails

6a36eb0

nasthagiri force-pushed the ret/triggered-email branch from 756ac01 to 6a36eb0 Compare August 31, 2017 20:58

nasthagiri approved these changes Aug 31, 2017

View reviewed changes

nasthagiri reviewed Sep 1, 2017

View reviewed changes

nasthagiri merged commit c08060a into master Sep 1, 2017

nasthagiri deleted the ret/triggered-email branch September 1, 2017 00:44

		import openedx.core.djangoapps.xmodule_django.models


		class Migration(migrations.Migration):

Uh oh!

Conversation

cpennington commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpennington Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpennington Aug 24, 2017 •

edited

Loading

mulby commented Aug 26, 2017 •

edited

Loading