/otwarchive

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

Already on GitHub? Sign in to your account

AO3-4420 Correct pluralisation on kudos emails #2281

Merged
merged 4 commits into from Aug 11, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
Reviewed: Ready to Merge
Projects
None yet
Milestone
No milestone
5 participants
@zz9pzza @sarken @priscilladc @houndci-bot @scottsds
@zz9pzza
Contributor

zz9pzza commented Jan 3, 2016

https://otwarchive.atlassian.net/browse/AO3-4420

Test by having a user receive a kudos from one person and then from two
app/mailers/kudo_mailer.rb
- else
- "#{guest_count} #{t 'mailer.kudos.guests'}"
- end
+ "#{t('mailer.kudos.guest', :count => guest_count.to_i)}"
@houndci-bot

houndci-bot Jan 3, 2016

Use the new Ruby 1.9 hash syntax.
Use 2 (not 3) spaces for indentation.

@cesy cesy referenced this pull request Jan 4, 2016

Merged

AO3-4416 pluralising kudos for other languages i18n #2282

@scottsds scottsds added the i18n label Jan 7, 2016

@sarken sarken added i18n fix and removed i18n labels Jan 10, 2016

config/locales/en-US.yml
- guest: A guest
- guests: guests
+ guest:
+ one: a guest
@sarken

sarken Jan 10, 2016

Owner

I'm having a hard time figuring it out -- is this for the sentence "A guest left kudos on %{work}" or "%{usernames} and a guest left kudos on %{work}"? If it's for the first one, it should be "A guest," not "a guest."

@zz9pzza

zz9pzza Jan 10, 2016

Contributor

This gives me a headache too:

https://github.com/otwcode/otwarchive/blob/master/app/mailers/kudo_mailer.rb#L35

    if !names.nil? && names.size > 0
      kudo_givers = names
      kudo_givers << guest_kudos(guest_count) unless guest_count == 0
    else
      kudo_givers << guest_kudos(guest_count).capitalize unless guest_count == 0
    end
    next if kudo_givers.empty?

See we have the capitalize so a guest is correct as the A or the first letter of the list of users is capitalized.

@sarken

sarken Jan 10, 2016

Owner

Okay, great!

@sarken sarken added Reviewed: Ready to Merge Reviewed: Needs Coder Action and removed Reviewed: Ready to Merge labels Jan 10, 2016

@sarken
Owner

sarken commented Jan 13, 2016

Whoops, taking off Ready to Merge label because @priscilladc has some comments still!
@priscilladc
Contributor

priscilladc commented Jan 13, 2016

Specific comments: we'd need 'kudos' to be variable (so like, someone left [gold star], two someones left [gold stars]); we also need 'left' to be variable (imagine like, he [leaves] kudos, they [leave] kudos; the verb changes when it's a different number of people).

More generally, and this is an underlying problem: we should do our best to avoid strings that are built like $variable + $variable + $variable, because languages structure sentences differently. Not all languages will order this sentence like someone + left + kudos. Latin, for example, would do someone + kudos + left. German, to be even more entertaining, apparently goes '[person] hat Kudos hinterlassen' — the 'kudos' goes smack dab in the middle of the verb.

tl;dr we need to be able to translate the different strings in, sorry, not just the bits!
@priscilladc
Contributor

priscilladc commented Jan 13, 2016

Ah, sorry! Just noticed #2282. Take back the comment about kudos then, since it's waiting to be merged :D but the rest still applies.
@zz9pzza
Contributor

zz9pzza commented Jan 26, 2016

@priscilladc @sarken I don't think this needs any more work done on it ?

@zz9pzza zz9pzza added Coder has actioned review Awaiting review and removed Reviewed: Needs Coder Action labels Feb 13, 2016

@sarken sarken added Reviewed: Ready to Merge and removed Awaiting review Coder has actioned review labels Mar 2, 2016

@zz9pzza zz9pzza added Reviewed: Needs Coder Action and removed Reviewed: Ready to Merge labels Mar 5, 2016

@zz9pzza zz9pzza removed the Reviewed: Needs Coder Action label Mar 11, 2016

@zz9pzza
Contributor

zz9pzza commented Mar 11, 2016

@sarken can we put ready to merge back on this ?

@sarken sarken added the Reviewed: Ready to Merge label Mar 11, 2016

@sarken
Owner

sarken commented Mar 11, 2016

Done!

@zz9pzza zz9pzza referenced this pull request Apr 8, 2016

Closed

AO3-4536 guest email blacklist #2410

@sarken sarken added Reviewed: Needs Coder Action and removed Reviewed: Ready to Merge labels May 14, 2016

@sarken
Owner

sarken commented May 14, 2016

Just a heads up that this has gone grey and will need to be updated.
@zz9pzza
Fix merge conflict

Some checks were not successful

1 errored and 1 successful checks
continuous-integration/travis-ci/pr — The Travis CI build could not complete due to an error
Details
hound — 3 violations found.
9652fff
app/mailers/kudo_mailer.rb
- else
- "#{guest_count} #{t 'mailer.kudos.guests'}"
- end
+ "#{t('mailer.kudos.guest', :count => guest_count.to_i)}"
@houndci-bot

houndci-bot May 14, 2016

Use the new Ruby 1.9 hash syntax.
Use 2 (not 3) spaces for indentation.
Prefer to_s over string interpolation.

@zz9pzza
New hash syntax

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
hound — 2 violations found.
eeeda3c

@zz9pzza zz9pzza added Coder has actioned review and removed Reviewed: Needs Coder Action labels May 24, 2016

@sarken sarken added Reviewed: Ready to Merge and removed Coder has actioned review labels Jun 16, 2016

@sarken sarken merged commit 60ba15b into otwcode:master Aug 11, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound 2 violations found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment