Fix bug 1173780: Add SMS message id admin. #137
Conversation
|
@jgmize r? |
pmclanahan
force-pushed
the
add-admin-sms-ids
branch
from
54b80fb
to
ad4fb76
Compare
pmclanahan
changed the title
| @@ -0,0 +1,103 @@ | |||
| # -*- coding: utf-8 -*- | |||
| import datetime | |||
| from south.db import db | |||
There was a problem hiding this comment.
What's the timeline on moving basket to django 1.7+ with the new style of migrations?
There was a problem hiding this comment.
As soon as I can get someone in IT to get basket on the new Python 2.7 generic cluster. We're still on 2.6 😞
There was a problem hiding this comment.
<insert "funny" animated gif of ancient python playing a sad trombone here>
|
This is going to be a really tiny table, isn't it? All of the caching code feels like it might be a premature optimization. You have more experience with the performance of this system, and you seem to have all the cases covered w.r.t. cache invalidation with the post_save and post_delete signal handlers, so you've already paid the complexity penalty in the initial implementation, but there's still the maintenance burden to consider if you'd rather simplify the code by eliminating the cache logic. |
|
|
||
|
|
||
| CACHE_KEY = "newsletters_cache_data" | ||
| SMS_CACHE_KEY = "sms_messages_cache_data" | ||
| SMS_MESSAGES = { | ||
| 'SMS_Android': 'MTo3ODow', |
There was a problem hiding this comment.
Why is this hard coded here instead of in the new DB model?
There was a problem hiding this comment.
Just already had that and I'll likely remove it later. It's only really necessary when this first rolls out to avoid any dropped requests. I could do a data migration as well, but this just seemed easier based on what was already there.
There was a problem hiding this comment.
Gotcha. Might be worth adding a comment, up to you.
|
Yeah. Could probably have done without cache, but this is called for every message request, and those will be in spikes, so since I basically already had the logic working for the newsletter data I figured I'd just be safer than sorry. |
In ET the ID of an SMS message we need to send them changes when you edit it. So we need to be able to update these easily.
pmclanahan
force-pushed
the
add-admin-sms-ids
branch
from
ad4fb76
to
e90ad11
Compare
|
Added TODOs. Any more fixing up needed? |
Nope, r+! |
Fix bug 1173780: Add SMS message id admin.
In ET the ID of an SMS message we need to send them
changes when you edit it. So we need to be able to
update these easily.