Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add country field in profiles table #396 #469

Merged
merged 12 commits into from Sep 19, 2016

Conversation

@n3k00n3
Copy link
Contributor

commented Aug 2, 2016

I think that pr will solve the issue #396

@zaziemo

This comment has been minimized.

Copy link
Member

commented Aug 4, 2016

Hey @n3k00n3, great work!!!
Could you please check, why the Travis build is failing and make it unfailing ;)
We also have to add the country field to the detailed search but this could be made in another PR.
@tyranja could you also please have a look?

@n3k00n3

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2016

@zaziemo Done!

@@ -187,6 +187,7 @@ en:
contact: "Contact"
website: "My website/blog:"
city: "My city:"
country: "My country"

This comment has been minimized.

Copy link
@zaziemo

zaziemo Aug 5, 2016

Member

could you also please add that field to the German locale? country: "Land"

@tyranja

This comment has been minimized.

Copy link
Member

commented Aug 5, 2016

Great work ❤️ Thanks!!!!

We totally forgot in the issue to mention, that the country field should be searchable as well. Could you maybe add that to the PR as well?

I will update the issue.

n3k00n3 added 3 commits Aug 5, 2016
@@ -187,6 +187,7 @@ de:
contact: "Kontakt"
website: "Meine Webseite/Blog:"
city: "Meine Stadt:"
country: "Meine Land"

This comment has been minimized.

Copy link
@zaziemo

zaziemo Aug 15, 2016

Member

Mein Land instead of Meine Land :)

<%= t(:country, scope: 'profiles.profile') %>
</h2>
<p class="bb pb">
<%= @profile.country %></span>

This comment has been minimized.

Copy link
@zaziemo

zaziemo Aug 15, 2016

Member

could you change the displayed information into the country name of the corresponding locale? Currently if I chose "Germany" as a country in the profile show action "DE" is displayed. It would be cool if I would see "Deutschland" in the german locale, "Germany" in the english and "Alemanha" (in the future Portuguese locale)

This comment has been minimized.

Copy link
@tyranja

tyranja Sep 5, 2016

Member

It is still looking the same. You might have to add that to the Profile Model: https://github.com/stefanpenner/country_select#getting-the-country-name-from-the-countries-gem

@@ -32,6 +32,7 @@ def detailed_search_result
return Profile.none if @query.values.all? &:blank?
result = Profile
.where('city ILIKE :city', city: "%#{@query[:city]}%")
.where('country ILIKE :country', country: "%#{[:country]}")

This comment has been minimized.

Copy link
@tyranja

tyranja Aug 15, 2016

Member

Here is @query missing.

But then we found out that when you are on the detailed search page without any search params you should see all profiles.
But with that code you only see profiles that have a filled in country field. All other profiles are not shown at all.

So in order to see all profiles you have to remove line 35 and add in line 38 that piece of code:

   if @query[:country].present?
      result = result
        .where('country ILIKE :country', country: "%#{@query[:country]}%")
     end

Does that make sense?

This comment has been minimized.

Copy link
@n3k00n3

n3k00n3 Aug 17, 2016

Author Contributor

Done!

n3k00n3 added 2 commits Aug 15, 2016
@@ -32,6 +32,7 @@ def detailed_search_result
return Profile.none if @query.values.all? &:blank?
result = Profile
.where('city ILIKE :city', city: "%#{@query[:city]}%")
.where('country ILIKE :country', country: "%#{@query[:country]}%")

This comment has been minimized.

Copy link
@tyranja

tyranja Aug 15, 2016

Member

Great. You got the tests running!!! Because you added the country field to the seeds.

But this shows another problem with our tests ( we are not great at testing I guess we have to write some more). Because in reality we have another problem which appears now because just a few profiles have a country field yet. 😞
If we use this line of code ( line 35 ) our detailed search page just shows profiles who have a country field.

So I guess you still have to add an if clause.

   if @query[:country].present?
      result = result
        .where('country ILIKE :country', country: "%#{@query[:country]}%")
     end

Does that make sense?

@zaziemo zaziemo merged commit 1484876 into rubymonsters:master Sep 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.