Hooking the logistration page into the new HTTP endpoint for email opt in

[stephensanchez]

Updating the Login and Registration page to accept the email_opt_in parameter, which will set an organization-wide setting for receiving emails.

Related to the ECOM-525 story, sub task ECOM-680

@rlucioni @wedaly

[rlucioni]

I've got a few questions here.

• Since course_id and optIn are POST parameters, why not get them from request.POST instead of request.DATA?
• Speaking of getting parameters, wouldn't it be better to use request.DATA.get(parameter) instead of request.DATA[parameter], so that you can check for None if one of those parameters isn't available?
• It kind of bothers me that the underscore-separated variable name (course_id) is being mixed with a camel-cased parameter name (optIn). Seems like we're using underscores in our other GET parameter names, so maybe it would be better to use opt_in instead of optIn.
• Speaking of variable names, I see there's another variable here named optin. Maybe this could also be named opt_in, for consistency with the parameter name.
• Finally, wouldn't it be better to set optin to True if the opt-in GET parameter is equal to "true", and set it to False otherwise? That is, optin = request.DATA['optIn'].lower() == 'true'. This is the assumption I made in the student views. It doesn't seem like we should opt someone in to receiving email unless value of the parameter is exactly what we expect (i.e., true). If you use request.DATA.get(parameter) instead of request.DATA[parameter], you can also use this equality check to set optin to False if the opt-in GET parameter is None.

In summary, I'd change this line to look like this:

opt_in = request.POST.get('opt_in') == 'true'

Phew!

[stephensanchez]

Heya, thanks for the input Renzo!

• I'm fine with the names course_id and opt_in. Sounds good to me.
• By the UI design, "True" is the default setting for opting in, so I made that the default value when False is not explicit. For the sake of having uniform contracts, I'll go with your default = false to avoid confusion.
• My understanding of DATA vs. POST is that DATA is offered as a useful utility by the Django REST framework to parse more parameters than just the POST form data. http://www.django-rest-framework.org/api-guide/requests/ By design I always use this over POST.
• On line 874 I check that the opt_in and course_id parameters are set via a decorator, or the function throws an exception before it is even executed. At this point I would prefer the code to fail loudly if it somehow went beyond this call. To that note, I can add a unit test that will fail if anyone tries to remove the decorator.

[rlucioni]

Okay, using DATA over POST seems reasonable; thanks for the link. I see the decorators you mentioned, too. Adding an extra test which fails if someone removes them sounds like a good idea.

[stephensanchez]

@rlucioni updated per comments. waiting on tests.

[rlucioni]

Nice. 👍

[rlucioni]

Could you use Underscore.js here to check if queryParams.emailOptIn is undefined? It takes care of some corner cases and gotchas related to determining if a variable is undefined. Should be as straight-forward as _.isUndefined(queryParams.emailOptIn).

Edit: Since you're trying to check that queryParams.emailOptIn is defined, you're going to want !_.isUndefined(queryParams.emailOptIn). Sorry about adding another comment after giving the thumbs-up!

[dianakhuang]

🚢

[rlucioni]

👍, thanks for swapping in Underscore.

[stephensanchez]

The only test failure is a Video Bok Choy test:
https://jenkins.testeng.edx.org/job/edx-all-tests-auto-pr/18883/SHARD=2,TEST_SUITE=bok-choy/testReport/junit/acceptance.tests.video.test_video_module/YouTubeVideoTest/test_multiple_videos_in_sequentials_load_and_work/

This is a known flakey test: https://openedx.atlassian.net/browse/TNL-662