Will Kahn-Greene [:willkg] ET needinfo? me Reporter
	Description • 11 years ago

Bad strings will cause email to not get sent.

This is related to bug #841412, but is different in that with bug #841412, the site breaks but it's no big deal since once we fix it, it works again. With email, if it kicks up an error, that email will *never* get sent.

Example:

Locale xx translates msgid "Hello {user}" as "Hello {0}". That's wrong. In the email generating code, this string will get translated, then the variables will get interpolated and at that point this kicks up an error. When the error is kicked up, email generation for that event stops cold. The code doesn't put the emails into a retry queue. The code doesn't try to handle errors.


Options:

1. Don't worry about it. We'll get error email, we can fix them as fast as we can and we don't worry about email that doesn't get sent.

2. Figure out how to create a retry queue for email. This is probably a big project because our email code is "interesting".

3. Tweak all the utility functions and email code I just rewrote so that if a string fails in interpolation, we switch to the English version of the string and go with that. Then we make sure to log an error so that we know. This is easier to do here than in bug #841412 because the code is special purpose and so it's easier to shore up against string errors.


I'm in favor of option 3 regardless of what we do for bug #841412.

	Will Kahn-Greene [:willkg] ET needinfo? me Reporter
	Comment 1 • 11 years ago

Tossing it in the backlog for now.

	Will Kahn-Greene [:willkg] ET needinfo? me Reporter
	Comment 2 • 11 years ago

Got our first one today:

Task tidings.events._fire_task with id 27f20e40-25b2-452a-a6c3-0e970bb007a0 raised exception:
'ValueError("unsupported format character \'a\' (0x61) at index 12",)'


Task was called with args: (<wiki.events.EditDocumentEvent object at 0x4b74ed0>,) kwargs: {'exclude': <User: feer56>}.

The contents of the full traceback was:

Traceback (most recent call last):
  File "/data/www/support.mozilla.com/kitsune/vendor/src/celery/celery/execute/trace.py", line 181, in trace_task
    R = retval = fun(*args, **kwargs)
  File "/data/www/support.mozilla.com/kitsune/vendor/src/django-tidings/tidings/events.py", line 132, in _fire_task
    for m in self._mails(self._users_watching(exclude=exclude)):
  File "/data/www/support.mozilla.com/kitsune/apps/sumo/email_utils.py", line 100, in emails_with_users_and_watches
    render_email(template_path, context_vars),
  File "/data/www/support.mozilla.com/kitsune/apps/sumo/email_utils.py", line 55, in render_email
    return jingo.render_to_string(req, template, context)
  File "/data/www/support.mozilla.com/kitsune/vendor/src/jingo/jingo/__init__.py", line 93, in render_to_string
    return template.render(get_context())
  File "/data/www/support.mozilla.com/kitsune/vendor/src/jingo/jingo/__init__.py", line 189, in render
    return super(Template, self).render(context_dict)
  File "/usr/lib64/python2.6/site-packages/jinja2/environment.py", line 891, in render
    return self.environment.handle_exception(exc_info, True)
  File "/data/www/support.mozilla.com/kitsune/apps/wiki/templates/wiki/email/edited.ltxt", line 4, in top-level template code
    {% trans creator=creator, document_title=document_title %}
ValueError: unsupported format character 'a' (0x61) at index 12


One exciting thing about this is that it's entirely unclear which locale has the problem. So it'll make it harder to figure out what the deal is.

	Kadir Topal [:atopal]
	Comment 3 • 11 years ago

This is scary with the localized forums being in place. Let's do this ASAP.

	Will Kahn-Greene [:willkg] ET needinfo? me Reporter
	Comment 4 • 11 years ago

Two things:

1. We need to decide on a course of action. I listed three possibilities in the description with a preference for the third. But it's possible there are others I'm not thinking of.

2. After we decide on a course of action, we can estimate this bug, but not until then. Given that, this isn't ready to be in a sprint.

	Ricky Rosario [:rrosario, :r1cky]
	Comment 5 • 11 years ago

The ideal solution though is to handle this upstream like we want for the rest of the site strings. Verbatim should never allow "broken" strings to be committed. That would be my preference.

Being more realistic about that getting done any time soon, I'd vote for option 3.

moving this back to the backlog with bug 841412

	Ricky Rosario [:rrosario, :r1cky]
	Comment 6 • 11 years ago

We're going with option 3. Willkg said this was 2-3pts so => 3pts

	Michael Cooper [:mythmon] Assignee
	Comment 7 • 11 years ago

Yoink.

	Michael Cooper [:mythmon] Assignee
	Comment 8 • 11 years ago

PR: https://github.com/mozilla/kitsune/pull/1229

	Michael Cooper [:mythmon] Assignee
	Comment 9 • 11 years ago

Landed in: https://github.com/mozilla/kitsune/5382aa9857a3e092ca10653c95c20882eaa94bf2

Deployed to prod just now.