Skip to content

Prevent stats page error when no downloads#1259

Merged
dwradcliffe merged 1 commit into
rubygems:masterfrom
bobwhitelock:prevent_error_on_stats_page_when_no_downloads
May 2, 2016
Merged

Prevent stats page error when no downloads#1259
dwradcliffe merged 1 commit into
rubygems:masterfrom
bobwhitelock:prevent_error_on_stats_page_when_no_downloads

Conversation

@bobwhitelock

Copy link
Copy Markdown
Contributor

This is a fix for an issue mentioned in #1238.

If the stats page was visited while there were no downloads in the database, a NoMethodError would be thrown due to @most_downloaded.first being nil. This commit prevents this by checking it is available before accessing it.

While this issue wouldn't occur in production it could in development after following the standard instructions for setting up a development environment in CONTRIBUTING.md.

If the stats page was visited while there were no downloads in the
database, a NoMethodError would be thrown due to
`@most_downloaded.first` being nil. This commit prevents this by
checking it is available before accessing it.

While this issue wouldn't occur in production it could in development
after following the standard instructions for setting up a development
environment in CONTRIBUTING.md.
@dwradcliffe

Copy link
Copy Markdown
Member

Did you try this locally? I think this will still error when the view tries to use the @most_downloaded_count instance variable. The test here does not run view code so it doesn't catch this.

@bobwhitelock

Copy link
Copy Markdown
Contributor Author

I have viewed this and it works fine locally when the stats page is viewed. I believe it doesn't error as @most_downloaded_count is only accessed within the @most_downloaded.each loop, which never executes because @most_downloaded is an empty array (which was the cause of the issue). Is there a better way you would suggest writing the test to catch a regression in future though? Thanks

@dwradcliffe

Copy link
Copy Markdown
Member

Ah yes you are right. 👍 Thanks!

Also note: we will be adding the gem_downloads table to the database dumps soon, but this is good for now.

@dwradcliffe dwradcliffe merged commit a8b7be6 into rubygems:master May 2, 2016
@dwradcliffe dwradcliffe temporarily deployed to staging May 2, 2016 17:39 Inactive
@bobwhitelock bobwhitelock deleted the prevent_error_on_stats_page_when_no_downloads branch May 2, 2016 20:14
@arthurnn

arthurnn commented May 3, 2016

Copy link
Copy Markdown
Member

thanks

@arthurnn arthurnn temporarily deployed to production May 4, 2016 23:19 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants