Skip to content
This repository

Make error messages translatable #87

Closed
wants to merge 6 commits into from

11 participants

Krzysztof Knapik Matthew Hutchinson Nathaniel Talbott Sébastien Grosjean Michael Hellein Martin Carel Troex Nevelin Sunny Ripert Jean-Philippe Moal Daniel Schierbeck
Krzysztof Knapik

Error messages were hardococed. They should be defined in yml files and be translatable.

Matthew Hutchinson

+1 on this request

lib/active_merchant/common/validateable.rb
@@ -42,35 +43,45 @@ module ActiveMerchant #:nodoc:
42 43 # if more than one error is available we will only return the first. If no error is available
43 44 # we return an empty string
44 45 def on(field)
45   - self[field].to_a.first
  46 + type = [self[field]].flatten.first
  47 +
  48 + return nil unless type
  49 +
  50 + if type.is_a?(String)
  51 + key, default = nil, type
  52 + else
  53 + key, default = :"activemerchant.errors.models.#{@i18n_key}.attributes.#{field}.#{type}", :"activemerchant.errors.messages.#{type}"
2
Daniel Schierbeck
dasch added a note

That's a pretty long line - perhaps split it in two?

Krzysztof Knapik
knapo added a note

Absolutely agree! Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Nathaniel Talbott
Collaborator

Curious what the Shopify gang's take on this is - is this a good approach for ActiveMerchant? Don't want to ask @knapo for an updated PR if we're not interested in merging something like this.

At this point I'm ambivalent, though I could see this being handy for us at Spreedly eventually.

Nathaniel Talbott ntalbott referenced this pull request
Closed

Support for i18n #254

Sébastien Grosjean

That would be really useful to us at BookingSync as well. Looking forward to hear from Shopify guys ( @jduff @odorcicd ... ) on that.

Michael Hellein

I think this is a good idea and a good implementation, but it seems like it can't be merged at this point and another PR will be needed.

Sébastien Grosjean

@michaek I'm curious to know why this can't be merged. What would break ? The additional I18n dependency makes you trouble ?

Michael Hellein

Maybe I'm wrong - I tried to run the patch against the current code and it couldn't find the chunks.

Sébastien Grosjean

Hmm I see, I guess this patch might need some updates. It's is pretty old but I guess updating it is not the current issue, the main question remaining is whether this update have it's place into activemerchant.

Michael Hellein

Yes, that is the question. I mean that I'm in favor of the change, but it will probably need a new pull request.

Nathaniel Talbott
Collaborator

I would review and merge a fresh patch that tracked with how Rails does i18n, but it probably makes sense to wait to do that until we break backwards compatibility with < 3.0.

Nathaniel Talbott ntalbott closed this
Martin Carel

ntalbott: any idea when active_merchant will support i18n so that we can have translatable error messages?

Troex Nevelin

This was really good implementation!

I18n can be useful for English users too (I'm not speaking about the rest of the world!). Recently needed to make field names more friendly like Your credit card number is invalid, hacked based on this idea:

module ActiveMerchant
  module Validateable
    class Errors < HashWithIndifferentAccess
      def full_messages
        result = []

        self.each do |key, messages|
          next if messages.blank?
          if key == 'base'
            result << "#{messages.first}"
          else
            # result << "#{key.to_s.humanize} #{messages.first}"
            object = @base.class.to_s.demodulize.underscore
            result << "%s %s" % [I18n.t("activemerchant.#{object}.#{key}", :default => key.to_s.humanize), messages.first]
          end
        end

        result
      end
    end
  end
end
Sunny Ripert

+1 !

Jean-Philippe Moal

:+1: any news on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
1  .gitignore
@@ -3,3 +3,4 @@
3 3 *.swp
4 4 *.swo
5 5 pkg
  6 +.rvmrc
3  lib/active_merchant.rb
@@ -49,3 +49,6 @@ module Billing #:nodoc:
49 49 autoload :Integrations, 'active_merchant/billing/integrations'
50 50 end
51 51 end
  52 +
  53 +require 'active_support/i18n'
  54 +I18n.load_path << File.dirname(__FILE__) + '/active_merchant/locale/en.yml'
8 lib/active_merchant/billing/check.rb
@@ -29,15 +29,15 @@ def name=(value)
29 29
30 30 def validate
31 31 [:name, :routing_number, :account_number].each do |attr|
32   - errors.add(attr, "cannot be empty") if self.send(attr).blank?
  32 + errors.add(attr, :empty) if self.send(attr).blank?
33 33 end
34 34
35   - errors.add(:routing_number, "is invalid") unless valid_routing_number?
  35 + errors.add(:routing_number, :invalid) unless valid_routing_number?
36 36
37   - errors.add(:account_holder_type, "must be personal or business") if
  37 + errors.add(:account_holder_type, :must_be_personal_or_business) if
38 38 !account_holder_type.blank? && !%w[business personal].include?(account_holder_type.to_s)
39 39
40   - errors.add(:account_type, "must be checking or savings") if
  40 + errors.add(:account_type, :must_be_checking_or_savings) if
41 41 !account_type.blank? && !%w[checking savings].include?(account_type.to_s)
42 42 end
43 43
28 lib/active_merchant/billing/credit_card.rb
@@ -122,38 +122,38 @@ def before_validate #:nodoc:
122 122 end
123 123
124 124 def validate_card_number #:nodoc:
125   - errors.add :number, "is not a valid credit card number" unless CreditCard.valid_number?(number)
126   - unless errors.on(:number) || errors.on(:type)
127   - errors.add :type, "is not the correct card type" unless CreditCard.matching_type?(number, type)
  125 + errors.add :number, :invalid unless CreditCard.valid_number?(number)
  126 + unless errors[:number] || errors[:type]
  127 + errors.add :type, :incorrect unless CreditCard.matching_type?(number, type)
128 128 end
129 129 end
130 130
131 131 def validate_card_type #:nodoc:
132   - errors.add :type, "is required" if type.blank?
133   - errors.add :type, "is invalid" unless CreditCard.card_companies.keys.include?(type)
  132 + errors.add :type, :required if type.blank?
  133 + errors.add :type, :invalid unless CreditCard.card_companies.keys.include?(type)
134 134 end
135 135
136 136 def validate_essential_attributes #:nodoc:
137   - errors.add :first_name, "cannot be empty" if @first_name.blank?
138   - errors.add :last_name, "cannot be empty" if @last_name.blank?
139   - errors.add :month, "is not a valid month" unless valid_month?(@month)
140   - errors.add :year, "expired" if expired?
141   - errors.add :year, "is not a valid year" unless valid_expiry_year?(@year)
  137 + errors.add :first_name, :empty if @first_name.blank?
  138 + errors.add :last_name, :empty if @last_name.blank?
  139 + errors.add :month, :invalid unless valid_month?(@month)
  140 + errors.add :year, :expired if expired?
  141 + errors.add :year, :invalid unless valid_expiry_year?(@year)
142 142 end
143 143
144 144 def validate_switch_or_solo_attributes #:nodoc:
145 145 if %w[switch solo].include?(type)
146 146 unless valid_month?(@start_month) && valid_start_year?(@start_year) || valid_issue_number?(@issue_number)
147   - errors.add :start_month, "is invalid" unless valid_month?(@start_month)
148   - errors.add :start_year, "is invalid" unless valid_start_year?(@start_year)
149   - errors.add :issue_number, "cannot be empty" unless valid_issue_number?(@issue_number)
  147 + errors.add :start_month, :invalid unless valid_month?(@start_month)
  148 + errors.add :start_year, :invalid unless valid_start_year?(@start_year)
  149 + errors.add :issue_number, :empty unless valid_issue_number?(@issue_number)
150 150 end
151 151 end
152 152 end
153 153
154 154 def validate_verification_value #:nodoc:
155 155 if CreditCard.requires_verification_value?
156   - errors.add :verification_value, "is required" unless verification_value?
  156 + errors.add :verification_value, :required unless verification_value?
157 157 end
158 158 end
159 159 end
37 lib/active_merchant/common/validateable.rb
@@ -32,6 +32,7 @@ class Errors < HashWithIndifferentAccess
32 32
33 33 def initialize(base)
34 34 @base = base
  35 + @i18n_key = @base.class.to_s.demodulize.underscore
35 36 end
36 37
37 38 def count
@@ -42,35 +43,47 @@ def count
42 43 # if more than one error is available we will only return the first. If no error is available
43 44 # we return an empty string
44 45 def on(field)
45   - self[field].to_a.first
  46 + type = [self[field]].flatten.first
  47 +
  48 + return nil unless type
  49 +
  50 + if type.is_a?(String)
  51 + key = nil
  52 + default = type
  53 + else
  54 + key = :"activemerchant.errors.models.#{@i18n_key}.attributes.#{field}.#{type}"
  55 + default = :"activemerchant.errors.messages.#{type}"
  56 + end
  57 +
  58 + I18n.t(key, :default => default)
46 59 end
47 60
48 61 def add(field, error)
49 62 self[field] ||= []
50 63 self[field] << error
51   - end
  64 + end
52 65
53 66 def add_to_base(error)
54 67 add(:base, error)
55 68 end
56 69
57 70 def each_full
58   - full_messages.each { |msg| yield msg }
  71 + full_messages.each { |msg| yield msg }
59 72 end
60 73
61 74 def full_messages
62 75 result = []
63   -
64   - self.each do |key, messages|
65   - if key == 'base'
66   - result << "#{messages.first}"
  76 +
  77 + self.each do |key, v|
  78 + if key.to_sym == :base
  79 + result << "#{self.on(key)}"
67 80 else
68   - result << "#{key.to_s.humanize} #{messages.first}"
  81 + result << "#{I18n.t("activemerchant.attributes.#{@i18n_key}.#{key}", :default => key.to_s.humanize)} #{self.on(key)}"
69 82 end
70 83 end
71   -
  84 +
72 85 result
73   - end
74   - end
  86 + end
  87 + end
75 88 end
76   -end
  89 +end
39 lib/active_merchant/locale/en.yml
... ... @@ -0,0 +1,39 @@
  1 +en:
  2 + activemerchant:
  3 + attributes:
  4 + check:
  5 + name: "Name"
  6 + routing_number: "Routing number"
  7 + account_number: "Account number"
  8 + account_holder_type: "Account holder type"
  9 + account_type: "Account type"
  10 + credit_card:
  11 + issue_number: "Issue number"
  12 + number: "Card number"
  13 + type: "Card type"
  14 + first_name: "First name"
  15 + last_name: "Last name"
  16 + month: "Expire month"
  17 + year: "Expire year"
  18 + start_month: "Start month"
  19 + start_year: "Start year"
  20 + verification_value: "Verification value"
  21 + errors:
  22 + messages:
  23 + empty: "can't be empty"
  24 + expired: "is expired"
  25 + invalid: "is invalid"
  26 + required: "is required"
  27 + must_be_personal_or_business: "must be personal or business"
  28 + must_be_checking_or_savings: "must be checking or savings"
  29 + models:
  30 + credit_card:
  31 + attributes:
  32 + number:
  33 + invalid: "is not a valid credit card number"
  34 + type:
  35 + incorrect: "is not the correct card type"
  36 + month:
  37 + invalid: "is not a valid month"
  38 + year:
  39 + invalid: "is not a valid year"
4 test/test_helper.rb
@@ -18,8 +18,8 @@
18 18
19 19 begin
20 20 gem 'actionpack'
21   -rescue LoadError
22   - raise StandardError, "The view tests need ActionPack installed as gem to run"
  21 +rescue LoadError => e
  22 + raise e, "The view tests need ActionPack installed as gem to run"
23 23 end
24 24
25 25 require 'action_controller'
12 test/unit/validateable_test.rb
@@ -6,9 +6,9 @@ class Dood
6 6 attr_accessor :name, :email, :country
7 7
8 8 def validate
9   - errors.add "name", "cannot be empty" if name.blank?
10   - errors.add "email", "cannot be empty" if email.blank?
11   - errors.add_to_base "The country cannot be blank" if country.blank?
  9 + errors.add "name", :empty if name.blank?
  10 + errors.add "email", :empty if email.blank?
  11 + errors.add_to_base "The country can't be blank" if country.blank?
12 12 end
13 13
14 14 end
@@ -46,15 +46,15 @@ def test_multiple_calls
46 46
47 47 def test_messages
48 48 @dood.valid?
49   - assert_equal "cannot be empty", @dood.errors.on('name')
50   - assert_equal "cannot be empty", @dood.errors.on('email')
  49 + assert_equal "can't be empty", @dood.errors.on('name')
  50 + assert_equal "can't be empty", @dood.errors.on('email')
51 51 assert_equal nil, @dood.errors.on('doesnt_exist')
52 52
53 53 end
54 54
55 55 def test_full_messages
56 56 @dood.valid?
57   - assert_equal ["Email cannot be empty", "Name cannot be empty", "The country cannot be blank"], @dood.errors.full_messages.sort
  57 + assert_equal ["Email can't be empty", "Name can't be empty", "The country can't be blank"], @dood.errors.full_messages.sort
58 58 end
59 59
60 60 end

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.