Create new API endpoint for email optin #6060
Conversation
| datetime.datetime.now().year - profile.year_of_birth >= # pylint: disable=maybe-no-member | ||
| getattr(settings, 'EMAIL_OPTIN_MINIMUM_AGE', 13) | ||
| ) | ||
| preference, _ = UserOrgTag.objects.get_or_create( |
There was a problem hiding this comment.
@wedaly I'm forgetting if we try and avoid get_or_create and use a separate pattern that avoids locks on a row, do you recall?
There was a problem hiding this comment.
The problem with get_or_create() is that it doesn't play nicely with the "repeatable-read" isolation level. If two threads are trying to create two objects, and in doing so violate an integrity constraint, then the "get" might find nothing but the "create" raises an IntegrityError.
One way to handle this is to catch integrity errors and manually end the transaction, but this is tricky. In this case, maybe we can just assume that someone else is has already created the preference and swallow the error.
There was a problem hiding this comment.
Makes sense to me. I'll catch the IntegrityError and log it at warn level. That way, if we see a lot of them, we can establish what's going on.
Also, given this is a row on a table that only one person should be accessing, I'm hoping we don't have as many scenarios where two people update the same row.
stephensanchez
force-pushed
the
sanchez/add_email_optin_api_endpoint
branch
from
6ae80c4
to
3f18754
Compare
| getattr(settings, 'EMAIL_OPTIN_MINIMUM_AGE', 13) | ||
| ) | ||
| preference, _ = UserOrgTag.objects.get_or_create( | ||
| user=user, org=org, key='email-optin', value=str(optin and of_age) |
There was a problem hiding this comment.
Should the API be responsible for verifying that optin is a boolean, or is that something that should be handled a level above, by the caller?
There was a problem hiding this comment.
Good question. I'd say it should be the responsibility of the caller above this function, otherwise we get into the java-developer instinct of calling isinstance() on every argument of the function
stephensanchez
force-pushed
the
sanchez/add_email_optin_api_endpoint
branch
from
3f18754
to
b5031fa
Compare
| (12, True, u"False") | ||
| ) | ||
| @ddt.unpack | ||
| @override_settings(EMAIL_OPTIN_MINIMUM_AGE=13) |
There was a problem hiding this comment.
This shouldn't be necessary, since test settings inherit from common. Maybe this setting needs to be added to cms/envs/common.py as well, since the app is in common/djangoapps?
There was a problem hiding this comment.
This wasn't necessary, I had just added it as a precaution in case someone changed the settings locally in their common.py, this test would still pass.
Updating the design of the email opt in end point. Updating API call based on new Org Model.
stephensanchez
force-pushed
the
sanchez/add_email_optin_api_endpoint
branch
from
b5031fa
to
6214365
Compare
|
👍 |
1 similar comment
|
👍 |
|
Closing and replaced with https://github.com/edx/edx-platform/pull/6073 |
Creating a new Email Opt-In Endpoint in the profile API for ECOM-525, sub task ECOM-685. This hooks into the new User Org Tag model.
@dianakhuang @rlucioni