4:37 p.m.
In reviewing Scott's patch, I included a nit talking about how the flash messages (for
both success and errors) were oddly-worded. Scott's patch didn't actually
introduce them, so I just fixed them.
It'd be great if someone who was an I18n master would look this over to make sure
I'm not doing anything too unorthodox. I think this patch should have two benefits:
1.) The messages sound more human, at least to me.
2.) We no longer concatenate strings outside of the data dictionary, so they should be
more I18n-friendly.
This patch was fixing a nit of mine and isn't an essential part of the sprint, so
don't go too out of your way.
-- Matt
4:37 p.m.
New subject: [PATCH] Clean up Permission-related flash messages
They're now I18n-friendly and less-robotic.
---
src/app/controllers/permissions_controller.rb | 34 +++++++++++++++---------
src/config/locales/en.yml | 15 ++++++----
src/features/permission.feature | 5 ++-
3 files changed, 33 insertions(+), 21 deletions(-)
diff --git a/src/app/controllers/permissions_controller.rb
b/src/app/controllers/permissions_controller.rb
index 995b467..1053763 100644
--- a/src/app/controllers/permissions_controller.rb
+++ b/src/app/controllers/permissions_controller.rb
@@ -60,17 +60,19 @@ class PermissionsController < ApplicationController
:role_id => role_id,
:permission_object => @permission_object)
if permission.save
- added << permission.user.login + " " + permission.role.name
+ added << t('permissions.flash.fragment.user_and_role', :user
=> permission.user.login,
+ :role => permission.role.name)
else
- not_added << permission.user.login + " " +
permission.role.name
+ not_added << t('permissions.flash.fragment.user_and_role', :user
=> permission.user.login,
+ :role => permission.role.name)
end
end
end
unless added.empty?
- flash[:notice] = "#{t('permissions.flash.notice.added')}:
#{added.join(', ')}"
+ flash[:notice] = t('permissions.flash.notice.added', :list =>
added.to_sentence)
end
unless not_added.empty?
- flash[:error] = "#{t('permissions.flash.error.not_added')}:
#{not_added.join(', ')}"
+ flash[:error] = t('permissions.flash.error.not_added', :list =>
not_added.to_sentence)
end
if added.empty? and not_added.empty?
flash[:error] = t "permissions.flash.error.no_users_selected"
@@ -96,21 +98,25 @@ class PermissionsController < ApplicationController
unless permission.role == role
permission.role = role
if permission.save
- modified << permission.user.login + " " +
permission.role.name + " from " + old_role.name
+ modified <<
t('permissions.flash.fragment.user_and_role_change', :user =>
permission.user.login,
+ :old_role => old_role.name,
+ :role => permission.role.name)
else
- not_modified << permission.user.login + " " +
permission.role.name
+ not_modified <<
t('permissions.flash.fragment.user_and_role_change', :user =>
permission.user.login,
+ :old_role => old_role.name,
+ :role => permission.role.name)
end
end
end
end
unless modified.empty?
- flash[:notice] = "#{t('permissions.flash.notice.modified')}:
#{modified.join(', ')}"
+ flash[:notice] = t('permissions.flash.notice.modified', :list =>
modified.to_sentence)
end
unless not_modified.empty?
- flash[:error] = "#{t('permissions.flash.error.not_add')}:
#{not_modified.join(', ')}"
+ flash[:error] = t('permissions.flash.error.not_add', :list =>
not_modified.to_sentence)
end
if modified.empty? and not_modified.empty?
- flash[:error] = t"permissions.flash.error.no_users_selected"
+ flash[:error] = t("permissions.flash.error.no_users_selected")
end
respond_to do |format|
format.html { redirect_to @return_path }
@@ -127,17 +133,19 @@ class PermissionsController < ApplicationController
Permission.find(params[:permission_selected]).each do |p|
if check_privilege(Privilege::PERM_SET, p.permission_object) && p.destroy
- deleted << p.user.login + " " + p.role.name
+ deleted << t('permissions.flash.fragment.user_and_role', :user
=> p.user.login,
+ :role => p.role.name)
else
- not_deleted << p.user.login + " " + p.role.name
+ not_deleted << t('permissions.flash.fragment.user_and_role', :user
=> p.user.login,
+ :role => p.role.name)
end
end
unless deleted.empty?
- flash[:notice] = "#{t('permissions.flash.notice.deleted')}:
#{deleted.join(', ')}"
+ flash[:notice] = t('permissions.flash.notice.deleted', :list =>
deleted.to_sentence)
end
unless not_deleted.empty?
- flash[:error] = "#{t('permissions.flash.error.not_deleted')}:
#{not_deleted.join(', ')}"
+ flash[:error] = t('permissions.flash.error.not_deleted', :list =>
not_deleted.to_sentence)
end
respond_to do |format|
format.html { redirect_to @return_path }
diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
index b6dc774..3d43f3e 100644
--- a/src/config/locales/en.yml
+++ b/src/config/locales/en.yml
@@ -999,14 +999,17 @@ en:
choose_global_role: Choose global role assignments for users
flash:
notice:
- modified: These User Roles were modified
- deleted: These Permission Grants were deleted
- added: These User Roles were added
+ modified: "Successfully modified the following User Roles: %{list}"
+ deleted: "Deleted the following Permission Grants: %{list}"
+ added: "Added the following User Roles: %{list}"
error:
- not_add: Could not add these User Roles
+ not_add: "Could not add these User Roles: %{list}"
no_users_selected: No users selected
- not_deleted: Could not delete these Permission Grants
- not_added: Could not add these User Roles
+ not_deleted: "Could not delete these Permission Grants: %{list}"
+ not_added: "Could not add these User Roles: %{list}"
+ fragment:
+ user_and_role: "%{user} (%{role})"
+ user_and_role_change: "%{user} (from %{old_role} to %{role})"
preset_filters:
all_permissions: All Permissions
quotas:
diff --git a/src/features/permission.feature b/src/features/permission.feature
index ae9a642..33006d2 100644
--- a/src/features/permission.feature
+++ b/src/features/permission.feature
@@ -19,7 +19,7 @@ Feature: Manage Permissions
When I select "Pool User" role for the user "testuser"
And I press "Grant Access"
Then I should be on the page for the pool "PermissionPool"
- And I should see "These User Roles were added"
+ And I should see "Added the following User Roles"
And I should see "testuser"
Scenario: Create a second permission on a resource
@@ -32,7 +32,7 @@ Feature: Manage Permissions
When I select "Pool Owner" role for the user "testuser"
And I press "Grant Access"
Then I should be on the page for the pool "PermissionPool"
- And I should see "These User Roles were added"
+ And I should see "Added the following User Roles"
And I should see "testuser"
Scenario: Attempt to duplicate a permission
@@ -54,3 +54,4 @@ Feature: Manage Permissions
When I follow link with ID "details_permissions"
When I delete the permission
Then I should be on the page for the pool "PermissionPool"
+ And I should see "Deleted the following Permission Grants"
--
1.7.6.4
7:37 a.m.
On 11/29/2011 11:37 PM, Matt Wagner wrote:
In reviewing Scott's patch, I included a nit talking about how
the flash messages (for both success and errors) were oddly-worded. Scott's patch
didn't actually introduce them, so I just fixed them.
It'd be great if someone who was an I18n master would look this over to make sure
I'm not doing anything too unorthodox. I think this patch should have two benefits:
1.) The messages sound more human, at least to me.
For what it's worth from a non-native speaker, they sound better to me
as well.
2.) We no longer concatenate strings outside of the data dictionary,
so they should be more I18n-friendly.
This is exactly what we should be doing everywhere. I think the
reviewers need to be more strict with what they allow i18n-wise. I've
been guilty of letting this go as well.
This patch was fixing a nit of mine and isn't an essential part of the sprint, so
don't go too out of your way.
-- Matt
ACK, thanks for doing this.
Thomas
--
No trees were killed to send this message, but a large number of
electrons were terribly inconvenienced.
8:57 a.m.
On Mon, Dec 05, 2011 at 02:37:17PM +0100, Tomas Sedovic wrote:
For what it's worth from a non-native speaker, they sound better
to
me as well.
I think this is probably an important goal.
ACK, thanks for doing this.
Thanks for the review! I've just pushed this to master.
-- Matt
4520
days inactive
4526
days old
aeolus-devel@lists.fedorahosted.org
3 comments
2 participants
participants (2)
-
Matt Wagner -
Tomas Sedovic