Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 4106 i18n kudos email #1878

Merged
merged 41 commits into from
May 21, 2015
Merged

Issue 4106 i18n kudos email #1878

merged 41 commits into from
May 21, 2015

Conversation

zz9pzza
Copy link
Contributor

@zz9pzza zz9pzza commented Oct 12, 2014

@coveralls
Copy link

Coverage Status

Coverage decreased (-11.33%) when pulling 88d04b2 on zz9pzza:issue_4106 into 36ee12c on otwcode:master.

@scottsds
Copy link
Member

This issue is related to #1866 and #1868

@sarken sarken modified the milestone: 0.9.54: i18n emails Mar 8, 2015
@sarken
Copy link
Collaborator

sarken commented Apr 26, 2015

Same thing as #1868, basically: this can't be automatically merged and will also need to be updated to include the view file changes made to #1866.

I18n.with_locale(Locale.find(@user.preference.prefered_locale).iso) do
mail(
:to => @user.email,
:subject => "#{t 'user_mailer.invite_increase_notification.subject' ,app_name: ArchiveConfig.APP_SHORT_NAME}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know which way around these will be merged so that the code they have in common will no longer appear in the diff for this or the one for Issue 4100, so here is the same comment again:

The space always goes after the comma, never before.
Please move the space between 'user_mailer.invite_increase_notification.subject' and "app_name:" so that this part reads 'user_mailer.invite_increase_notification.subject', app_name

@zz9pzza
Copy link
Contributor Author

zz9pzza commented May 10, 2015

I am not going to look at these until the first one is merged.

)
I18n.with_locale(Locale.find(user.preference.prefered_locale).iso) do
mail(
:to => user.email,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the new Ruby 1.9 hash syntax.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 73.86% when pulling 98fb8bb on zz9pzza:issue_4106 into c7af7c9 on otwcode:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-32.74%) to 41.11% when pulling 25cf08c on zz9pzza:issue_4106 into c7af7c9 on otwcode:master.

if guest_count.to_i == 1
return "1111 #{t 'mailer.kudos.guest'}"
end
return "1111 #{guest_count} #{t 'mailer.kudos.guests'}"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant return detected.

@sarken sarken removed this from the 0.9.62: i18n emails milestone May 17, 2015
if guest_count.to_i == 1
return "#{t 'mailer.kudos.guest'}"
end
return "#{guest_count} #{t 'mailer.kudos.guests'}"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant return detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the extra return to be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've found that you get used to the absence of return keywords when you start seeing the methods as functions that "are" their return values, rather than separate procedures that have to "return" a value.

The way you've formatted this, the first return is indeed necessary, but I'd personally find the logic easier to follow if the whole thing was one if statement:

if guest_count.to_i == 1
  "#{t 'mailer.kudos.guest'}"
else
  "#{guest_count} #{t 'mailer.kudos.guests'}"
end

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 73.85% when pulling 8f71817 on zz9pzza:issue_4106 into c7af7c9 on otwcode:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-11.03%) to 62.81% when pulling e9786d9 on zz9pzza:issue_4106 into c7af7c9 on otwcode:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 73.85% when pulling 2cadfd8 on zz9pzza:issue_4106 into c7af7c9 on otwcode:master.

# If we have a commentable, extract names and process guest kudos text - skip if no kudos givers
names = kudo_givers_hash['names']
guest_count = kudo_givers_hash['guest_count']
kudo_givers = ""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it turns out kudos_givers needs to be an empty array [] so that both appending with << and to_sentence will work

next if kudo_givers.empty?

@commentables << commentable
@kudo_givers[commentable_info] = kudo_givers.to_sentance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_sentence, not to_sentance

@sarken sarken added this to the 0.9.62: i18n emails milestone May 20, 2015
ariana-paris added a commit that referenced this pull request May 21, 2015
Issue 4106 i18n kudos email
@ariana-paris ariana-paris merged commit 6a0d908 into otwcode:master May 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants