Listofpopularcities 79 rsid #190

RSid · 2014-12-26T19:18:48Z

closes issue #79

…five cities only

…elcome page, deleted old preamble

…es and message next to each other

…ies test

…the same name

…well as city

houndci-bot · 2014-12-26T19:18:52Z

app/models/restroom.rb

@@ -59,6 +59,13 @@ def rating_percentage
    upvote.to_f / (upvote + downvote).to_f * 100
  end

+  def self.topcities
+    Rails.cache.fetch("topcities", expires_in: 1.month) do
+      Restroom.group(:city, :state).count.sort_by {|x,y| y}.reverse


Unused block argument - x. If it's necessary, use _ or _x as an argument name to indicate that it won't be used.
Space missing after comma.
Space between { and | missing.
Space missing inside }.

I noticed in the db snapshot @tkwidmer sent that 'San Francisco' and 'SAN FRANCISCO' are both in the top 5 cities. Do you want to run a migration at some point to consolidate all these records to use the same city name, or would you like me to include some handling here for aggregating data for cities that are secretly the same place?

We should probably just handle this case, since the only difference is the case. You should be able to make the check case insensitive.

Cool - I don't think you can make group by case insensitive in postgres
http://www.postgresql.org/message-id/[email protected],
but I can get around that by writing the query in raw sql like this:
"SELECT MAX(city), state, COUNT(DISTINCT id) AS count FROM restrooms GROUP
BY LOWER(city), state ORDER BY count DESC LIMIT 5"

On Sat, Dec 27, 2014 at 2:30 PM, Teagan [email protected] wrote:

In app/models/restroom.rb
#190 (diff)
:

@@ -59,6 +59,13 @@ def rating_percentage
upvote.to_f / (upvote + downvote).to_f * 100
end

def self.topcities

Rails.cache.fetch("topcities", expires_in: 1.month) do

Restroom.group(:city, :state).count.sort_by {|x,y| y}.reverse

We should probably just handle this case, since the only difference is the
case. You should be able to make the check case insensitive.

—
Reply to this email directly or view it on GitHub
https://github.com/RefugeRestrooms/refugerestrooms/pull/190/files#r22294162
.

This reverts commit c5eaedf.

RSid · 2014-12-26T20:24:23Z

@tkwidmer , do I have to do anything special to get the devise initializer to pass on travis? The build keeps failing on Devise.secret_key was not set, but I haven't been messing around with any related code and there are already secret keys set in test and dev.

tkwidmer · 2014-12-27T19:25:32Z

That's weird. I'm 90 percent sure I set the secret key on travis.

tkwidmer · 2014-12-27T19:28:26Z

updated it, going to try the run again.

tkwidmer · 2014-12-27T20:11:23Z

so i did a little research:

UPDATE: After some reading, it turns out that Travis does not expose safe ENV vars to untrusted builds, such as pull requests. This is due to the fbehave that a malicious user can trigger a build that would expose the ENV vars (eg, submit a pull request that uploads ENV vars to a resecondte server). After some thought, it is apparent I'll need to find a dwhetherferent near to what I'm doing.

So I just generated a new public secret key for the test environment and am exposing it to pull request builds

tkwidmer · 2014-12-27T20:11:37Z

Hopefully that will fix it

tkwidmer · 2014-12-27T20:37:27Z

@RSid specs passed

RSid · 2014-12-28T02:57:30Z

Ah, that makes sense. Weird times, thanks!

…opcities method to mimic case insensitivity and pass test

tkwidmer · 2014-12-29T18:29:37Z

haven't had a chance to check this out yet and take a look at the front end. any way you could screen shot the front end stuff?

tkwidmer · 2014-12-29T18:30:19Z

Gemfile

@@ -1,5 +1,5 @@
 source 'https://rubygems.org'
-ruby '2.0.0'


this is weird, we should have already been updated to 2.1.1

I think it's registering that way because I forked the project before it was updated to 2.1.1? But I'm not sure why that would still show up that way, since I've pulled from master since then.

yeah, idk. regardless it shouldn't cause a problem.

tkwidmer · 2014-12-29T19:51:31Z

other than a couple of comments, this looks good. excited to get it merged in.

RSid · 2014-12-30T04:14:04Z

Thanks! Glad to hear it. Screenshot's here : https://drive.google.com/file/d/0B5V-owrjHUVbcUhLLTZTM3VOQWF1bmM1N3hrREd0WUw1cU1N/view?usp=sharing

On Mon, Dec 29, 2014 at 1:29 PM, Teagan [email protected] wrote:

haven't had a chance to check this out yet and take a look at the front
end. any way you could screen shot the front end stuff?

—
Reply to this email directly or view it on GitHub
#190 (comment)
.

RSid · 2015-01-02T18:36:28Z

Presuming the front end styling is ok, I think this is good to go!

tkwidmer · 2015-01-17T21:06:54Z

I think we should do more on the front end. Like having each listing link to the search for the city center of that city. But ill go ahead and merge this.

Listofpopularcities 79 rsid

RSid · 2015-01-17T23:37:55Z

@tkwidmer , the listings do actually link to searches for the city center of that city already - sorry, I guess they don't visually look much like links. Would it help clarify that if I added link underlining back in or something?

tkwidmer · 2015-01-19T19:34:40Z

oh, thats fine then. I may end up changing the visual style a little bit. But yeah. thanks for doing this!

RSid · 2015-01-19T20:49:12Z

No worries! I enjoyed it. But yeah, I'm not terribly UX-y, go for it.

tkwidmer and others added 16 commits October 17, 2014 18:20

Merge branch 'release/1.1.0'

1590226

Merge branch 'release/1.2.0'

0a5928d

Merge branch 'release/1.3.0'

e24c122

added test to restroom model test for topcities method returning top …

d61ad21

…five cities only

added topcities method to restrooms

a81a04f

welcome static controller gets top 5 cities for data in index

e81b7e3

added partial with top cities and links to search for city names to w…

ac8642e

…elcome page, deleted old preamble

added back in preamble message, added styling for tiling popular citi…

18174a3

…es and message next to each other

had top cities query cache monthly instead of running each time

00bd14d

set test env not to store cache, in order to use test data for topcit…

9ffeb7e

…ies test

added test that top cities discriminates between different cities of …

36ed003

…the same name

refactored into one chained line, had pull state info in group by as …

1fef7e8

…well as city

updated view to concat and display city and state

dfd5dbb

made preamble haml more readable/not over 80 char

162cdf0

added outline, changed font and title background color on popular cities

1b3bee8

corrected colors to precise palatte

d4b7987

houndci-bot reviewed Dec 26, 2014
View reviewed changes

RSid added 3 commits December 26, 2014 14:38

hound comments & gemfile

566f3ff

changed test prepare to specify test schema

c5eaedf

Revert "changed test prepare to specify test schema"

663933c

This reverts commit c5eaedf.

RSid added 2 commits December 28, 2014 00:12

added test for top cities search being case insensitive, refactored t…

d4d2db8

…opcities method to mimic case insensitivity and pass test

formatting fixes on display of top cities names

c66b912

tkwidmer reviewed Dec 29, 2014
View reviewed changes

RSid added 2 commits December 29, 2014 23:22

renamed topcities method to top_cities everywhere

665782c

moved styling for top cities into separate file

3e771ec

tkwidmer added a commit that referenced this pull request Jan 17, 2015

Merge pull request #190 from RSid/listofpopularcities-79-rsid

e0c45a3

Listofpopularcities 79 rsid

tkwidmer merged commit e0c45a3 into RefugeRestrooms:develop Jan 17, 2015

tkwidmer mentioned this pull request Jan 19, 2015

list of popular cities #79

Closed

tkwidmer mentioned this pull request Jan 31, 2015

Release 1.4.0 #201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Listofpopularcities 79 rsid #190

Listofpopularcities 79 rsid #190

RSid commented Dec 26, 2014

houndci-bot Dec 26, 2014

RSid Dec 26, 2014

tkwidmer Dec 27, 2014

RSid Dec 28, 2014

RSid commented Dec 26, 2014

tkwidmer commented Dec 27, 2014

tkwidmer commented Dec 27, 2014

tkwidmer commented Dec 27, 2014

tkwidmer commented Dec 27, 2014

tkwidmer commented Dec 27, 2014

RSid commented Dec 28, 2014

tkwidmer commented Dec 29, 2014

tkwidmer Dec 29, 2014

RSid Dec 30, 2014

tkwidmer Dec 30, 2014

tkwidmer commented Dec 29, 2014

RSid commented Dec 30, 2014

RSid commented Jan 2, 2015

tkwidmer commented Jan 17, 2015

RSid commented Jan 17, 2015

tkwidmer commented Jan 19, 2015

RSid commented Jan 19, 2015

Listofpopularcities 79 rsid #190

Listofpopularcities 79 rsid #190

Conversation

RSid commented Dec 26, 2014

houndci-bot Dec 26, 2014

Choose a reason for hiding this comment

RSid Dec 26, 2014

Choose a reason for hiding this comment

tkwidmer Dec 27, 2014

Choose a reason for hiding this comment

RSid Dec 28, 2014

Choose a reason for hiding this comment

RSid commented Dec 26, 2014

tkwidmer commented Dec 27, 2014

tkwidmer commented Dec 27, 2014

tkwidmer commented Dec 27, 2014

tkwidmer commented Dec 27, 2014

tkwidmer commented Dec 27, 2014

RSid commented Dec 28, 2014

tkwidmer commented Dec 29, 2014

tkwidmer Dec 29, 2014

Choose a reason for hiding this comment

RSid Dec 30, 2014

Choose a reason for hiding this comment

tkwidmer Dec 30, 2014

Choose a reason for hiding this comment

tkwidmer commented Dec 29, 2014

RSid commented Dec 30, 2014

RSid commented Jan 2, 2015

tkwidmer commented Jan 17, 2015

RSid commented Jan 17, 2015

tkwidmer commented Jan 19, 2015

RSid commented Jan 19, 2015