[WIP]Fix/null name fallback #1306

zacck-zz · 2017-12-19T05:51:04Z

What's in this PR?

In this PR I am implementing a fallback for Users who donate without a name. I changed the reciept email mark up to more friendlier language.

I also changed the way RecieptEmail handles Users when they have a first and when they done

References

Fixes #819

joshsmith

Looks great, just a couple comments.

Also note to self that we will need to take the HTML changes here and propagate them to Postmark when this is merged.

joshsmith · 2017-12-19T21:28:39Z

lib/code_corps/emails/receipt_email.ex

+  def get_name(%User{ first_name: nil }), do: "there"
+
+  @spec get_name(User.t) :: String.t
+  def get_name(%User{ first_name: name}), do: name


I think you can condense the spec to just above the first line and then not separate the lines between these.

I also wonder whether we should place this fn into the BaseEmail so that it's available to use across other emails.

@joshsmith if the functionality is needed in other emails the function can live in BaseEmail

zacck-zz · 2017-12-20T05:18:36Z

@joshsmith so I moved the function to BaseEmail and condensed the typespecs also the tests let me now if this what you are envisioning. How do we go about propagating the changes to postmark?

joshsmith

Overall logic looks great! Just need one more test and some minor formatting things.

joshsmith · 2017-12-21T02:47:16Z

test/lib/code_corps/emails/base_email_test.exs

+  test "get name returns there on nil name" do
+    user = %CodeCorps.User{}
+    assert BaseEmail.get_name(user) == "there"
+  end


You'll want to add another test for the non-nil case.

joshsmith · 2017-12-21T02:48:01Z

lib/code_corps/emails/base_email.ex

+
+  @spec get_name(User.t) :: String.t
+  def get_name(%User{ first_name: nil }), do: "there"
+  def get_name(%User{ first_name: name}), do: name


Prefer collapsing the spaces in the struct, e.g. %User{first_name: name} and %User{first_name: nil}.

joshsmith · 2017-12-21T02:48:17Z

lib/code_corps/emails/base_email.ex

+  def get_name(%User{ first_name: nil }), do: "there"
+  def get_name(%User{ first_name: name}), do: name
+
+


Would remove these extra two lines.

joshsmith · 2017-12-21T02:48:39Z

test/lib/code_corps/emails/receipt_email_test.exs

@@ -5,10 +5,6 @@ defmodule CodeCorps.Emails.ReceiptEmailTest do
  alias CodeCorps.Emails.ReceiptEmail




Would remove these extra two lines, too.

zacck-zz · 2017-12-21T04:48:41Z

@joshsmith pushed for those changes let me know if this is looking better?

joshsmith

Apologies for back and forth. Couple more things I’d suggest.

joshsmith · 2017-12-21T05:03:26Z

test/lib/code_corps/emails/base_email_test.exs

+    f_name = "Zacck"
+    user = %CodeCorps.User{first_name: f_name}
+    assert BaseEmail.get_name(user) == f_name
+  end


I just realized too but you can wrap all these in a describe “get_name/2” do block and write the test names like “when first_name is nil” and “when first_name is set”.

Could you show me an example of that? very Keen to do that

Yeah, so the full thing would read:

describe "get_name/2" do test "returns 'there' when first_name nil" do user = %CodeCorps.User{first_name: nil} assert BaseEmail.get_name(user) == "there" end test "returns first_name when first_name present" do first_name = "Zacck" user = %CodeCorps.User{first_name: first_name} assert BaseEmail.get_name(user) == first_name end end

Does that help?

@zacck your pushes collapsed this comment, so hopefully you see this.

ah I see for the grouping of the tests for the functionality I though it was something with guards

Oh, nope! Nothing fancy. :)

👍 thats done 😄

joshsmith · 2017-12-21T05:04:10Z

lib/code_corps/emails/receipt_email.ex

@@ -55,7 +55,7 @@ defmodule CodeCorps.Emails.ReceiptEmail do
      project_title: project.title,
      project_url: project |> url(),
      subject: project |> build_subject_line(),
-      user_first_name: charge.user.first_name
+      name: BaseEmail.get_name(charge.user)


You probably want to alpha order these for consistency.

joshsmith · 2017-12-21T05:06:07Z

test/lib/code_corps/emails/base_email_test.exs

+
+  test "get_name returns first_name of user" do
+    f_name = "Zacck"
+    user = %CodeCorps.User{first_name: f_name}


You don’t need the intermediate variable here. Can just put the string in directly as the value.

Ignore this. Refer to my example instead.

joshsmith · 2017-12-21T05:07:11Z

test/lib/code_corps/emails/receipt_email_test.exs

@@ -50,7 +50,7 @@ defmodule CodeCorps.Emails.ReceiptEmailTest do
      project_url: "http://localhost:4200/#{project.organization.slug}/#{project.slug}",
      project_current_donation_goal_description: "Test goal",
      subject: "Your monthly donation to Code Corps",
-      user_first_name: "Jimmy"
+      name: "Jimmy"


Alpha order here too.

joshsmith · 2017-12-21T08:11:37Z

Looks good! Rebase and squash to one commit and this is mergeable!

joshsmith · 2017-12-21T18:11:47Z

I went ahead and rebased/squashed and fixed a small formatting thing on one of the @spec's. An errant space got in there.

Thanks for the hard work on this!

zacck-zz · 2017-12-23T19:25:28Z

Thank you so much for this!!! Away from computers didn't see this yet

…

On Thursday, December 21, 2017, Josh Smith ***@***.***> wrote: I went ahead and rebased/squashed and fixed a small formatting thing on one of the @SPEC's. An errant space got in there. Thanks for the hard work on this! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1306 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2ywzQkvgXEEgMsEch-eTZAhiGjoW9iks5tCp9kgaJpZM4RGeRV> .

-- Regards Isaac Osiemo - Software Developer Skype: zacck.os Tel: +27798259736

joshsmith added the needs code review label Dec 19, 2017

joshsmith requested review from begedin and joshsmith December 19, 2017 21:27

joshsmith reviewed Dec 19, 2017

View reviewed changes

joshsmith removed the needs code review label Dec 19, 2017

joshsmith requested changes Dec 21, 2017

View reviewed changes

joshsmith reviewed Dec 21, 2017

View reviewed changes

joshsmith force-pushed the fix/null-name-fallback branch from 009d3bd to ffc2a1b Compare December 21, 2017 18:08

Add get_name function to the BaseEmail

5f7be46

joshsmith force-pushed the fix/null-name-fallback branch from ffc2a1b to 5f7be46 Compare December 21, 2017 18:10

joshsmith approved these changes Dec 21, 2017

View reviewed changes

joshsmith merged commit 0592e5e into code-corps:develop Dec 21, 2017

This was referenced Dec 22, 2017

Switch to SparkPost #1332

Closed

Make sure all emails use the new name fallback provided by #1306 #1336

Open

zacck-zz deleted the fix/null-name-fallback branch December 31, 2017 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]Fix/null name fallback #1306

[WIP]Fix/null name fallback #1306

zacck-zz commented Dec 19, 2017 •

edited by code-corps bot

Loading

joshsmith left a comment

joshsmith Dec 19, 2017

joshsmith Dec 19, 2017

zacck-zz Dec 20, 2017

zacck-zz commented Dec 20, 2017

joshsmith left a comment

joshsmith Dec 21, 2017

joshsmith Dec 21, 2017

joshsmith Dec 21, 2017

joshsmith Dec 21, 2017

zacck-zz commented Dec 21, 2017

joshsmith left a comment

joshsmith Dec 21, 2017

zacck-zz Dec 21, 2017

joshsmith Dec 21, 2017

joshsmith Dec 21, 2017

zacck-zz Dec 21, 2017

joshsmith Dec 21, 2017

zacck-zz Dec 21, 2017

joshsmith Dec 21, 2017

joshsmith Dec 21, 2017

joshsmith Dec 21, 2017

joshsmith Dec 21, 2017

joshsmith commented Dec 21, 2017

joshsmith commented Dec 21, 2017

zacck-zz commented Dec 23, 2017 via email

		def get_name(%User{ first_name: nil }), do: "there"
		def get_name(%User{ first_name: name}), do: name

		@@ -5,10 +5,6 @@ defmodule CodeCorps.Emails.ReceiptEmailTest do
		alias CodeCorps.Emails.ReceiptEmail

[WIP]Fix/null name fallback #1306

[WIP]Fix/null name fallback #1306

Conversation

zacck-zz commented Dec 19, 2017 • edited by code-corps bot Loading

What's in this PR?

References

joshsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacck-zz commented Dec 20, 2017

joshsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zacck-zz commented Dec 21, 2017

joshsmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshsmith commented Dec 21, 2017

joshsmith commented Dec 21, 2017

zacck-zz commented Dec 23, 2017 via email

zacck-zz commented Dec 19, 2017 •

edited by code-corps bot

Loading