Skip to content

Added blacklisted gem page#1433

Merged
indirect merged 4 commits into
rubygems:masterfrom
shlok007:blacklisted-gem-page
Nov 6, 2016
Merged

Added blacklisted gem page#1433
indirect merged 4 commits into
rubygems:masterfrom
shlok007:blacklisted-gem-page

Conversation

@shlok007
Copy link
Copy Markdown
Contributor

Resolves #1401

This is the gem show page rendered for gems which are blaclisted
blacklisted_page

@shlok007 shlok007 force-pushed the blacklisted-gem-page branch from 559ad06 to 5194493 Compare September 28, 2016 05:03
Comment thread app/views/rubygems/show.html.erb Outdated
@@ -1,18 +1,24 @@
<% @title = @rubygem.name %>
<% @subtitle = @latest_version.try(:slug) %>
<% if !@blacklisted_name %>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it will be better to use separate template instead of this branching.

Comment thread app/controllers/rubygems_controller.rb Outdated
@latest_version = @rubygem.versions.most_recent
@versions = @rubygem.public_versions(5)
else
@blacklisted_name = params[:id]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

params[:id].downcase

Comment thread app/views/rubygems/show.html.erb Outdated
<%= auto_discovery_link_tag(:atom, rubygem_versions_path(@rubygem, format: "atom"), {title: "#{@rubygem.name} Version Feed"}) %>
<% if @rubygem.versions.present? && @latest_version.indexed %>
<link rel="canonical" href="<%= rubygem_version_url(@rubygem, @latest_version.slug) %>" />
<link rel="canonical" href="<%= rubygem_version_url(@rubygem, @latest_version.slug) %>" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change can be roll-backed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved now 👍

@shlok007 shlok007 force-pushed the blacklisted-gem-page branch from 5c13315 to bd688ca Compare September 28, 2016 14:48
@sonalkr132
Copy link
Copy Markdown
Member

Can we please add a test that GET for a blacklisted gem takes user to correct template and content.

@shlok007
Copy link
Copy Markdown
Contributor Author

I have added the tests. Do let me know when its ready to be merged and I'll squash the commits into one.

Comment thread app/controllers/rubygems_controller.rb Outdated
def show
@latest_version = @rubygem.versions.most_recent
@versions = @rubygem.public_versions(5)
if @rubygem
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional is not obvious (without reading the rest of the code) that if @rubygem is nil, then the gem is blacklisted. Maybe a new before_action to check for blacklisted gems?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a new before_action to set blacklisted gem here and this line has been changed into if @blacklisted_gem

@shlok007
Copy link
Copy Markdown
Contributor Author

shlok007 commented Oct 8, 2016

@dwradcliffe Please let me know if any changes are pending yet :)

@indirect
Copy link
Copy Markdown

indirect commented Nov 6, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants