Skip to content

Commit 3ce9199

Browse files
Peter Giacomo Lombardocursoragent
andcommitted
Polish notify-by-email validation error copy.
Use humanized attribute names and grammatically correct messages so UI, API, and job error output read naturally instead of exposing awkward wording like "Notify emails to is not available." Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 626020f commit 3ce9199

7 files changed

Lines changed: 34 additions & 23 deletions

File tree

app/models/concerns/pwpush/notifiable_by_email.rb

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,11 @@ module NotifiableByEmail
55
extend ActiveSupport::Concern
66
MAX_NOTIFY_BY_EMAILS = 5
77

8+
NOTIFY_BY_EMAIL_HUMAN_ATTRIBUTE_NAMES = {
9+
notify_emails_to: "Recipient emails",
10+
notify_emails_to_locale: "Notification language"
11+
}.freeze
12+
813
included do
914
attr_accessor :notify_emails_to, :notify_emails_to_locale, :notify_emails_to_required, :notify_by_email_creator, :notify_by_email_skip_limit_validation, :notify_by_email_recipients, :notify_by_email_locale
1015

@@ -27,6 +32,13 @@ def notify_by_email_available?
2732
end
2833
end
2934

35+
class_methods do
36+
def human_attribute_name(attribute, options = {})
37+
name = NOTIFY_BY_EMAIL_HUMAN_ATTRIBUTE_NAMES[attribute.to_sym]
38+
name ? _(name) : super
39+
end
40+
end
41+
3042
private
3143

3244
def validate_notify_by_email
@@ -45,7 +57,7 @@ def validate_notify_emails_to_presence
4557

4658
def validate_notify_by_email_availability
4759
unless notify_by_email_available?
48-
errors.add(:notify_emails_to, _("is not available")) if notify_emails_to.present?
60+
errors.add(:notify_emails_to, _("are not available")) if notify_emails_to.present?
4961
errors.add(:notify_emails_to_locale, _("is not available")) if notify_emails_to_locale.present?
5062
errors.add(:base, _("Notify by email feature is not enabled"))
5163

@@ -55,22 +67,21 @@ def validate_notify_by_email_availability
5567
notify_by_email_custom_validations
5668

5769
unless notify_by_email_creator.present?
58-
errors.add(:notify_emails_to, _("is not allowed for unknown users")) if notify_emails_to.present?
70+
errors.add(:notify_emails_to, _("are not allowed for unknown users")) if notify_emails_to.present?
5971
errors.add(:notify_emails_to_locale, _("is not allowed for unknown users")) if notify_emails_to_locale.present?
6072

6173
return
6274
end
6375

6476
unless notify_by_email_creator == user
65-
errors.add(:notify_emails_to, _("is allowed for only owners")) if notify_emails_to.present?
77+
errors.add(:notify_emails_to, _("are allowed for only owners")) if notify_emails_to.present?
6678
errors.add(:notify_emails_to_locale, _("is allowed for only owners")) if notify_emails_to_locale.present?
6779

6880
return
6981
end
7082

7183
if notify_by_email_creator.email_limit_reached?
72-
errors.add(:notify_emails_to, _("is not allowed because the maximum number of emails has been reached for today")) if notify_emails_to.present?
73-
errors.add(:notify_emails_to_locale, _("is not allowed because the maximum number of emails has been reached for today")) if notify_emails_to_locale.present?
84+
errors.add(:base, _("The maximum number of emails has been reached for today"))
7485
end
7586
end
7687

app/models/push.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ def deleted
240240

241241
def notify_by_email_custom_validations
242242
if expired?
243-
errors.add(:notify_emails_to, _("is not available for expired pushes")) if notify_emails_to.present?
243+
errors.add(:notify_emails_to, _("are not available for expired pushes")) if notify_emails_to.present?
244244
errors.add(:notify_emails_to_locale, _("is not available for expired pushes")) if notify_emails_to_locale.present?
245245
end
246246
end

test/controllers/pushes_controller_test.rb

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ class PushesControllerTest < ActionDispatch::IntegrationTest
9393
}
9494

9595
assert_response :unprocessable_content
96-
assert_includes response.body, "Notify emails to is not allowed for unknown users"
96+
assert_includes response.body, "Recipient emails are not allowed for unknown users"
9797
end
9898
end
9999

@@ -112,8 +112,8 @@ class PushesControllerTest < ActionDispatch::IntegrationTest
112112
}
113113

114114
assert_response :unprocessable_content
115-
assert_includes response.body, "Notify emails to is not available"
116-
assert_includes response.body, "Notify emails to locale is not available"
115+
assert_includes response.body, "Recipient emails are not available"
116+
assert_includes response.body, "Notification language is not available"
117117
assert_includes response.body, "Notify by email feature is not enabled"
118118
end
119119
end
@@ -132,8 +132,8 @@ class PushesControllerTest < ActionDispatch::IntegrationTest
132132
}
133133

134134
assert_response :unprocessable_content
135-
assert_includes response.body, "Notify emails to is not available"
136-
assert_includes response.body, "Notify emails to locale is not available"
135+
assert_includes response.body, "Recipient emails are not available"
136+
assert_includes response.body, "Notification language is not available"
137137
end
138138
end
139139

@@ -263,7 +263,7 @@ class PushesControllerTest < ActionDispatch::IntegrationTest
263263
}
264264

265265
assert_response :unprocessable_content
266-
assert_includes response.body, "Notify emails to is not available"
266+
assert_includes response.body, "Recipient emails are not available"
267267
end
268268

269269
test "notify_emails redirects to preview when push does not belong to user and disable_logins is false" do

test/integration/api/api_v2_pushes_test.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -434,7 +434,7 @@ def test_create_with_notify_by_email_params_fails_when_feature_is_disabled
434434

435435
assert_response :unprocessable_entity
436436
body = JSON.parse(response.body)
437-
assert_equal "is not available", body["notify_emails_to"][0]
437+
assert_equal "are not available", body["notify_emails_to"][0]
438438
assert_equal "is not available", body["notify_emails_to_locale"][0]
439439
end
440440

@@ -454,7 +454,7 @@ def test_create_with_notify_by_email_params_fails_when_email_service_is_not_conf
454454

455455
assert_response :unprocessable_entity
456456
body = JSON.parse(response.body)
457-
assert_equal "is not available", body["notify_emails_to"][0]
457+
assert_equal "are not available", body["notify_emails_to"][0]
458458
assert_equal "is not available", body["notify_emails_to_locale"][0]
459459
end
460460

@@ -473,7 +473,7 @@ def test_create_with_notify_by_email_params_fails_when_user_is_not_signed_in
473473

474474
assert_response :unprocessable_entity
475475
body = JSON.parse(response.body)
476-
assert_equal "is not allowed for unknown users", body["notify_emails_to"][0]
476+
assert_equal "are not allowed for unknown users", body["notify_emails_to"][0]
477477
assert_equal "is not allowed for unknown users", body["notify_emails_to_locale"][0]
478478
end
479479

@@ -536,7 +536,7 @@ def test_notify_emails_with_valid_params_returns_error_when_email_service_is_not
536536
assert_response :unprocessable_entity
537537

538538
body = JSON.parse(response.body)
539-
assert_equal "is not available", body["notify_emails_to"][0]
539+
assert_equal "are not available", body["notify_emails_to"][0]
540540
assert_equal "is not available", body["notify_emails_to_locale"][0]
541541
end
542542

test/models/concerns/pwpush/notifiable_by_email_test.rb

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ class Pwpush::NotifiableByEmailTest < ActiveSupport::TestCase
6767
Settings.mail.smtp_address = nil
6868

6969
assert_not @push.valid?
70-
assert_includes @push.errors[:notify_emails_to], "is not available"
70+
assert_includes @push.errors[:notify_emails_to], "are not available"
7171
assert_includes @push.errors[:notify_emails_to_locale], "is not available"
7272
assert_includes @push.errors[:base], "Notify by email feature is not enabled"
7373
end
@@ -76,7 +76,7 @@ class Pwpush::NotifiableByEmailTest < ActiveSupport::TestCase
7676
@push.notify_by_email_creator = nil
7777

7878
assert_not @push.valid?
79-
assert_includes @push.errors[:notify_emails_to], "is not allowed for unknown users"
79+
assert_includes @push.errors[:notify_emails_to], "are not allowed for unknown users"
8080
assert_includes @push.errors[:notify_emails_to_locale], "is not allowed for unknown users"
8181
end
8282

@@ -85,7 +85,7 @@ class Pwpush::NotifiableByEmailTest < ActiveSupport::TestCase
8585

8686
assert @other_user != @user
8787
assert_not @push.valid?
88-
assert_includes @push.errors[:notify_emails_to], "is allowed for only owners"
88+
assert_includes @push.errors[:notify_emails_to], "are allowed for only owners"
8989
assert_includes @push.errors[:notify_emails_to_locale], "is allowed for only owners"
9090
end
9191

@@ -143,7 +143,7 @@ class Pwpush::NotifiableByEmailTest < ActiveSupport::TestCase
143143
@push.notify_emails_to_locale = "en"
144144

145145
assert_not @push.valid?
146-
assert_includes @push.errors[:notify_emails_to], "is not available for expired pushes"
146+
assert_includes @push.errors[:notify_emails_to], "are not available for expired pushes"
147147
assert_includes @push.errors[:notify_emails_to_locale], "is not available for expired pushes"
148148
end
149149

test/system/notify_by_email_test.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ class NotifyByEmailTest < ApplicationSystemTestCase
3939
fill_in "push[notify_emails_to]", with: "test@example.com, invalid-email"
4040
click_on "Send Emails"
4141

42-
assert_text "Notify emails to contains invalid email(s)"
42+
assert_text "Recipient emails contains invalid email(s)"
4343
end
4444

4545
test "notify_by_email creation and sending emails" do

test/unit/send_notify_by_email_job_test.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ class SendNotifyByEmailJobTest < ActiveJob::TestCase
6262
@notify_by_email.reload
6363
assert_equal "failed", @notify_by_email.status
6464
assert @notify_by_email.successful_sends.blank?
65-
assert_equal "Notify emails to can't be blank.", @notify_by_email.error_message
65+
assert_equal "Recipient emails can't be blank.", @notify_by_email.error_message
6666
end
6767

6868
test "perform does not send mail when notifying by email is not available" do
@@ -72,7 +72,7 @@ class SendNotifyByEmailJobTest < ActiveJob::TestCase
7272

7373
@notify_by_email.reload
7474
assert_equal "failed", @notify_by_email.status, "Status should be failed"
75-
assert_equal "Notify emails to is not available. Notify emails to locale is not available. Notify by email feature is not enabled.", @notify_by_email.error_message
75+
assert_equal "Recipient emails are not available. Notification language is not available. Notify by email feature is not enabled.", @notify_by_email.error_message
7676
end
7777

7878
test "perform logs error and does not send mail if notify_by_email is not found" do

0 commit comments

Comments
 (0)