Moved error messages to translation file #899

nileshgulia1 · 2017-12-29T18:58:33Z

Related issue #878

Error messages are moved to en.yml file.Tests are also passing.

emcoding

Hi @nileshgulia! Congrats with your first contribution to the RGSoC Teams app! 🎉
This is a great start!
I propose alternatives for the error messages names. In general, we aim for readability, and also keep them in line with the existing Rails error names where possible.

emcoding · 2017-12-29T19:19:34Z

app/models/application_draft.rb

@@ -173,19 +173,19 @@ def ready?

  def different_projects_required
    if project1 && project1 == project2
-      errors.add(:projects, 'must not be selected twice')
+      errors.add(:projects, :multiple_sel_error)


Can we make this: 'too_many_selected' ? For better readability :-) and dev happiness 🎁
Sorry, I know I said to use 'error' in the key before. But now that I see them all together, it doesn't looks so nice.

emcoding · 2017-12-29T19:20:12Z

app/models/application_draft.rb

    end
  end

  def accepted_projects_required
    if projects.any? { |p| p && !p.accepted? } # if they don't exist, the presence validation will handle it
-      errors.add(:projects, 'must have been accepted')
+      errors.add(:projects, :must_accepted)


Can we make this: none_accepted ?

yeah,sure 👍

emcoding · 2017-12-29T19:21:09Z

app/models/application_draft.rb

    end
  end

  def only_one_application_draft_allowed
    unless team.application_drafts.where(season: season).none?
-      errors.add(:base, 'Only one application may be lodged')
+      errors.add(:base, :one_app_allowed)


Can we make this too_many_applications?

emcoding · 2017-12-29T19:28:38Z

app/models/application_draft.rb

@@ -195,7 +195,7 @@ def set_current_season

  def students_confirmed?
    unless team.present? && team.students.all?{|student| student.confirmed? }
-      errors.add(:base, 'Please make sure every student confirmed the email address.')
+      errors.add(:base, :confirmed_email_address)


How about: unconfirmed_email_address.

emcoding · 2017-12-29T19:29:44Z

app/models/conference.rb

@@ -29,7 +29,7 @@ def date_range

  def chronological_dates
    unless starts_on <= ends_on
-      errors.add(:ends_on, 'must be a later date than start date')
+      errors.add(:ends_on, :wrong_date_sel)


Can we make this more expressive, maybe reference the method name : 'chronology_violation'

emcoding · 2017-12-29T19:53:13Z

config/locales/en.yml

+          application_draft:
+                  attributes:
+                      projects:
+                          must_accepted: must have been accepted


As this is a yml file, you can collect similar values under the same key. So, you can reuse the keys:

errors: models: todo: ..... application_draft:

from line 86 and 87.
(See for an example how all the time formats are collected in line 6 and after. )

In the same way, you can put the message in line 105 right after the message in current line 99.
That goes for the rest of the file as well. I'll leave that for you to try. So I won't point out all the changes.
Let me know if something is unclear!

Also, can you follow the same indentation as the rest of the file? It seems not to be super consistent, with sometimes 4 and sometimes 2 spaces. At first sight, it looks like it is easy to copy the same indentations.

Thanks for being patient.I need another help :)

models: todo: application_draft: conference: team: user: attributes: user: too_many_reviewers: too many reviewers projects: too_many_selected: must not be selected twice none_accepted: must have been accepted base: too_many_applications: Only one application may be lodged unconfirmed_email_address: Please make sure every student confirmed the email address. ends_on: chronology_violation: must be a later date than start date roles: too_many_students: there cannot be more than 2 students on a team. github_handle: immutabilty_violation: can't be changed

What's wrong with this code, my local tests are failing?

No worries :-) We've all been there! I really appreciate your effort.

Take a look at this: http://docs.ansible.com/ansible/latest/YAMLSyntax.html
You should collect the attributes for each key.
Like this:

errors: models: todo: attributes: user: too_many_reviewers: too many reviewers application_draft: attributes: projects: must_accepted: must have been accepted # other error messages for projects here #other_attribute: #error messages on other_attribute #yet another model: attributes: #etc

Does that make sense?

Yes,exactly I have already written the above code you mentioned earlier, but this is not showing in the changes .please check my latest commit 9dba120

emcoding · 2017-12-29T20:04:30Z

.env-example

-CAMO_KEY=supersecret
-
-# Google Maps API key for e.g. city autocomplete
-GOOGLE_MAPS_API_KEY=YourGoogleMapsAPIKey


We don't want to remove this file from the repo. (I don't think you need to remove it locally, either 🤔 ).
Anyway, removing it should not be part of this PR.

yes,sorry for that,the file got accidentally removed ,will add it back now :)

emcoding · 2017-12-29T20:08:12Z

config/database.yml.example

-production:
-  adapter: postgresql
-  database: rgsocteams_production
-  host: localhost


Same as for the other removed file: we don't want this file removed. Can you leave the removal out of this PR please?

yeah,sure :)

emcoding · 2017-12-29T20:10:15Z

app/models/user.rb

@@ -220,7 +220,7 @@ def complete_from_github

  def immutable_github_handle
    return if new_record?
-    errors.add(:github_handle, "can't be changed") if changes_include?(:github_handle)
+    errors.add(:github_handle, :can't_be_changed) if changes_include?(:github_handle)
  end



Remove this whiteline please.

emcoding · 2017-12-29T20:11:08Z

config/locales/en.yml

+                     github_handle:
+                             can't_be_changed: can't be changed
+
+


And remove the redundant whitelines please

nileshgulia1

made some changes

nileshgulia1 · 2017-12-30T06:05:17Z

.env-example

-CAMO_KEY=supersecret
-
-# Google Maps API key for e.g. city autocomplete
-GOOGLE_MAPS_API_KEY=YourGoogleMapsAPIKey


yes,sorry for that,the file got accidentally removed ,will add it back now :)

nileshgulia1 · 2017-12-30T06:09:44Z

app/models/application_draft.rb

    end
  end

  def accepted_projects_required
    if projects.any? { |p| p && !p.accepted? } # if they don't exist, the presence validation will handle it
-      errors.add(:projects, 'must have been accepted')
+      errors.add(:projects, :must_accepted)


yeah,sure 👍

nileshgulia1 · 2017-12-30T06:16:58Z

config/database.yml.example

-production:
-  adapter: postgresql
-  database: rgsocteams_production
-  host: localhost


yeah,sure :)

nileshgulia1

@F3PiX please review

emcoding · 2017-12-30T12:53:44Z

config/locales/en.yml

+          team:
+            attributes:
+              roles:
+                too_many_students: there cannot be more than 2 students on a team.


For the team, two base errors are missing here. (line 175 and 183)

rgsoc-teams/app/models/team.rb

Lines 165 to 184 in a020e98

MSGS = {

duplicate_student_roles_singular: "%s already is a student on another team for #{Season.current.name}.",

duplicate_student_roles_plural: "%s already are students on another team for #{Season.current.name}."

}

def disallow_multiple_student_roles

students = User.with_role('student').joins(roles: :team).where(id: roles.map(&:user_id)).

where.not('roles.team_id' => id).where('teams.season_id' => season_id)

return if students.empty?

msg = MSGS[:"duplicate_student_roles_#{students.size == 1 ? 'singular' : 'plural'}"]

errors.add :base, msg % students.map(&:name).join(', ')

end

def disallow_duplicate_members

new_members = roles.map { |role| role.user }

duplicate_role_users = new_members.find_all { |member| new_members.count(member) > 1 }.uniq

return if duplicate_role_users.empty?

msg = "%s can't have more than one role in this team!"

errors.add :base, msg % duplicate_role_users.map(&:name).join(', ')

end

It is a bit more challenging than the other errors. Are you up to that?

Ohh,I guess I had missed it, I will have a look at it :)

emcoding

Great jump!
The two unchanged files shouldn't be part of this PR. Depending on how comfortable you are with using git; the quickest solution might be: close this PR, open a new one and copy the changes you made in the files concerning the error messages.
Otherwise : 👍 Great way to say goodbye to the old year ;-)

nileshgulia1 · 2017-12-30T13:34:17Z

@F3PiX thanks for the help so far,I will close the PR for now and will submit a clean PR soon :)

nileshgulia1 added 2 commits December 28, 2017 13:46

translated and corrected error messages

6cf6e1c

moved error messages

f8be5d9

emcoding suggested changes Dec 29, 2017

View reviewed changes

nileshgulia1 added 2 commits December 30, 2017 11:31

added .env-example back

a92b394

corrected error messages

9dba120

nileshgulia1 commented Dec 30, 2017

View reviewed changes

emcoding reviewed Dec 30, 2017

View reviewed changes

emcoding suggested changes Dec 30, 2017

View reviewed changes

nileshgulia1 closed this Dec 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved error messages to translation file #899

Moved error messages to translation file #899

nileshgulia1 commented Dec 29, 2017

emcoding left a comment

emcoding Dec 29, 2017

emcoding Dec 29, 2017

nileshgulia1 Dec 30, 2017

emcoding Dec 29, 2017

emcoding Dec 29, 2017

emcoding Dec 29, 2017

emcoding Dec 29, 2017

emcoding Dec 29, 2017

nileshgulia1 Dec 30, 2017

emcoding Dec 30, 2017

nileshgulia1 Dec 30, 2017

emcoding Dec 29, 2017

nileshgulia1 Dec 30, 2017

emcoding Dec 29, 2017

nileshgulia1 Dec 30, 2017

emcoding Dec 29, 2017

emcoding Dec 29, 2017

nileshgulia1 left a comment

nileshgulia1 Dec 30, 2017

nileshgulia1 Dec 30, 2017

nileshgulia1 Dec 30, 2017

nileshgulia1 left a comment

emcoding Dec 30, 2017

nileshgulia1 Dec 30, 2017

emcoding left a comment

nileshgulia1 commented Dec 30, 2017

	MSGS = {
	duplicate_student_roles_singular: "%s already is a student on another team for #{Season.current.name}.",
	duplicate_student_roles_plural: "%s already are students on another team for #{Season.current.name}."
	}

	def disallow_multiple_student_roles
	students = User.with_role('student').joins(roles: :team).where(id: roles.map(&:user_id)).
	where.not('roles.team_id' => id).where('teams.season_id' => season_id)
	return if students.empty?
	msg = MSGS[:"duplicate_student_roles_#{students.size == 1 ? 'singular' : 'plural'}"]
	errors.add :base, msg % students.map(&:name).join(', ')
	end

	def disallow_duplicate_members
	new_members = roles.map { \|role\| role.user }
	duplicate_role_users = new_members.find_all { \|member\| new_members.count(member) > 1 }.uniq
	return if duplicate_role_users.empty?
	msg = "%s can't have more than one role in this team!"
	errors.add :base, msg % duplicate_role_users.map(&:name).join(', ')
	end

Moved error messages to translation file #899

Moved error messages to translation file #899

Conversation

nileshgulia1 commented Dec 29, 2017

emcoding left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nileshgulia1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nileshgulia1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emcoding left a comment

Choose a reason for hiding this comment

nileshgulia1 commented Dec 30, 2017