[openstreetmap/openstreetmap-website] Add a preferred_email_format preference (#1511)

[Tom Hughes]

tomhughes requested changes on this pull request.



> @@ -13,8 +12,8 @@ def signup_confirm(user, token)
                      :display_name => user.display_name,
                      :confirm_string => token.token)
 
-      mail :to => user.email,
-           :subject => I18n.t("notifier.signup_confirm.subject")
+      compose_mail user,

Can we make the first argument part of the hash please - I'd suggest as `:recipient` so that it's a bit more obvious at the call site what that argument is?

> @@ -0,0 +1,8 @@
+require "migrate"
+
+class AddPreferredEmailFormat < ActiveRecord::Migration
+  def change
+    create_enumeration :email_format_enum, %w(text_only multipart)

I'd suggest using the actual MIME types here - so `text/plain` and `multipart/alternative` or does that cause problems with what postgres allows for enumeration names? If so then maybe just replace the slash with an underscore?

> @@ -1,6 +1,7 @@
 require "test_helper"
 require "changeset_controller"
 
+# rubocop:disable ClassLength

We normally just change the limit in `.rubocop_tobo.yml` rather than disable the check altogether.

> @@ -2,6 +2,7 @@
 
 class DiaryEntryControllerTest < ActionController::TestCase
   include ActionView::Helpers::NumberHelper
+  api_fixtures

As @gravitystorm said new tests need to be using factories, not fixtures.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/1511#pullrequestreview-39021826
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20170518/4c804c1f/attachment.html>