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

Already on GitHub? Sign in to your account

AO3-4736 Rails 4.0.13 Upgrade #2917

Merged
merged 61 commits into from May 15, 2017

Conversation

Projects
None yet
6 participants
Contributor

davidstump commented May 12, 2017

Pull Request Checklist

  • Have you read How to write the perfect pull request?
  • Have you read through the contributor guidelines?
  • Have you added tests for any changed functionality?
  • Have you added the JIRA issue number as the first thing in your pull request title (eg: AO3-1234 Fix thing)
  • Have you updated the JIRA issue with the information below?

Issue

https://otwarchive.atlassian.net/browse/AO3-4736

Purpose

This PR upgrades the AO3 application to run on Rails 4.0.13. This encompasses many changes including scopes, finders, observers and many other adjustments.

Testing

Due to the wide scope of these changes, a full run-through of site features is required to validate this upgrade.

References

No explicit external references

Credit

David Stump - Littlelines

What pronouns do you prefer (she/her, he/him, zie/hir etc)?

he/him

davidstump and others added some commits Apr 22, 2017

Adds 'skip_degist: true' to view cache
`expire_fragment` will not find a cache with a digest appended, and the cache
automatically adds a digest without skip_digest
Uses expire_fragment with skip_digest: true for kudos sweeper
Instead of dealing with an invisible digest that doesn't matter and just causes
confusion.
Adds `skip_digest: true` to all cache method calls in app/views
This ensures that all cache calls are bahving the same way now as they were
before the Rails 4 update.
:through => :taggings,
:source => :tagger,
:source_type => 'Tag',
- :before_remove => :remove_filter_tagging,
- :conditions => "tags.type = 'Category'"
+ :before_remove => :remove_filter_tagging
@houndci-bot

houndci-bot May 12, 2017

Use the new Ruby 1.9 hash syntax.

has_many :warnings,
+ -> { where("tags.type = 'Warning'") },
@houndci-bot

houndci-bot May 12, 2017

Align the parameters of a method call if they span more than one line.

:through => :taggings,
:source => :tagger,
:source_type => 'Tag',
- :before_remove => :remove_filter_tagging,
- :conditions => "tags.type = 'Warning'"
+ :before_remove => :remove_filter_tagging
@houndci-bot

houndci-bot May 12, 2017

Use the new Ruby 1.9 hash syntax.

has_many :fandoms,
+ -> { where("tags.type = 'Fandom'") },
@houndci-bot

houndci-bot May 12, 2017

Align the parameters of a method call if they span more than one line.

:through => :taggings,
:source => :tagger,
:source_type => 'Tag',
- :before_remove => :remove_filter_tagging,
- :conditions => "tags.type = 'Fandom'"
+ :before_remove => :remove_filter_tagging
@houndci-bot

houndci-bot May 12, 2017

Use the new Ruby 1.9 hash syntax.

has_many :relationships,
+ -> { where("tags.type = 'Relationship'") },
@houndci-bot

houndci-bot May 12, 2017

Align the parameters of a method call if they span more than one line.

:through => :taggings,
:source => :tagger,
:source_type => 'Tag',
- :before_remove => :remove_filter_tagging,
- :conditions => "tags.type = 'Relationship'"
+ :before_remove => :remove_filter_tagging
@houndci-bot

houndci-bot May 12, 2017

Use the new Ruby 1.9 hash syntax.

has_many :characters,
+ -> { where("tags.type = 'Character'") },
@houndci-bot

houndci-bot May 12, 2017

Align the parameters of a method call if they span more than one line.

:through => :taggings,
:source => :tagger,
:source_type => 'Tag',
- :before_remove => :remove_filter_tagging,
- :conditions => "tags.type = 'Character'"
+ :before_remove => :remove_filter_tagging
@houndci-bot

houndci-bot May 12, 2017

Use the new Ruby 1.9 hash syntax.

has_many :freeforms,
+ -> { where("tags.type = 'Freeform'") },
@houndci-bot

houndci-bot May 12, 2017

Align the parameters of a method call if they span more than one line.

:through => :taggings,
:source => :tagger,
:source_type => 'Tag',
- :before_remove => :remove_filter_tagging,
- :conditions => "tags.type = 'Freeform'"
+ :before_remove => :remove_filter_tagging
@houndci-bot

houndci-bot May 12, 2017

Use the new Ruby 1.9 hash syntax.

@@ -154,7 +154,7 @@ def cast_tags
characters = self.characters.by_name || []
relationships = self.relationships.by_name || []
return [] if relationships.empty? && characters.empty?
- canonical_relationships = Relationship.canonical.by_name.find(:all, :conditions => {:id => relationships.collect(&:merger_id).compact.uniq})
+ canonical_relationships = Relationship.canonical.by_name.where(id: relationships.collect(&:merger_id).compact.uniq)
@houndci-bot

houndci-bot May 12, 2017

Prefer map over collect.

# set parents
if skin_content.match(/PARENTS:\s*(.*)\s*\*\//)
parent_string = $1
set_parents(skin, parent_string)
end
end
-
+
@houndci-bot

houndci-bot May 12, 2017

Extra empty line detected at block body end.

@@ -153,15 +153,15 @@ namespace :skins do
desc "Cache all site skins"
task(:cache_all_site_skins => :environment) do
- Skin.where(cached: true).each{|skin| skin.cache!}
+ Skin.where(cached: true).each{|skin| skin.cache!}
@houndci-bot

houndci-bot May 12, 2017

Space missing to the left of {.
Space between { and | missing.
Pass &:cache! as an argument to each instead of a block.
Space missing inside }.

@@ -30,7 +30,7 @@ def get_stat(statistic, work_id)
end
def get_database_stat(statistic, work_id)
- StatCounter.where(:work_id => work_id).value_of(statistic).first || 0
+ StatCounter.where(:work_id => work_id).pluck(statistic).first || 0
@houndci-bot

houndci-bot May 12, 2017

Use the new Ruby 1.9 hash syntax.

@@ -206,7 +206,7 @@ function setupToggled(){
if (node.hasClass('open')) {
close_toggles.each(function(){$j(this).show();});
- open_toggles.each(function(){$j(this).hide();});
+ open_toggles.each(function(){$j(this).hide();});
@houndci-bot

houndci-bot May 12, 2017

Identifier 'open_toggles' is not in camel case.

Gemfile
#gem 'daemon-spawn', :require => 'daemon_spawn'
gem 'tire'
gem 'elasticsearch'
gem 'aws-sdk'
gem 'css_parser'
gem 'cocaine'
-gem 'paperclip'
+gem 'paperclip', '4.3.6'
# for looking up image dimensions quickly
gem 'fastimage'
# Gems for authentication
gem 'devise', '~> 3.0' # Lock on version 3 until we update to Rails 4
@zz9pzza

zz9pzza May 12, 2017

Contributor

Given the comment should we not lock this to version 3 now ?

@davidstump

davidstump May 12, 2017

Contributor

Sorry - didn't see that comment. I can bump devise is that is the preference.

@zz9pzza

zz9pzza May 12, 2017

Contributor

"Devise 4.0 works with Rails 4.1 onwards. You can add it to your Gemfile with:..."

So looks like not yet :) sorry

@davidstump

davidstump May 12, 2017

Contributor

Oh no worries - I noticed 3.5 or so works with 4.0 so I bumped it as far as was compatible on its own branch. I'll let it run and see how the tests go. If nothing major breaks, we can do a minor bump with 4.0 if that works.

+ def set_preferred_locale
+ # Loading the current locale
+ if session[:locale] && @loaded_locales.detect { |loc| loc.iso == session[:locale]}
+ set_locale session[:locale].to_sym
@zz9pzza

zz9pzza May 12, 2017

Contributor

This bit makes me a bit nervous given we have both partial page caching and full page caching. I would prefer we got the locale from the url myself.

@davidstump

davidstump May 12, 2017

Contributor

That part came from my upstream merge - I don't believe it has anything to do with the actual 4.0 upgrade. Interesting that it came through as an addition? Could this have been a deletion upstream?

@zz9pzza

zz9pzza May 12, 2017

Contributor

I can see it on a really old branch of mine. Why it came back I don't know.

@ariana-paris

ariana-paris May 12, 2017

Contributor

That sounds likely as that code was removed in commit a42de43 (PR 2834) a couple of weeks ago.

@davidstump

davidstump May 12, 2017

Contributor

Perfect, thanks. I'll remove the code to mesh with master

@@ -372,8 +372,8 @@ def admin_deleted_work_notification(user, work)
# Sends email to authors when a creation is hidden by an Admin
def admin_hidden_work_notification(creation_id, user_id)
- @user = User.find_by_id(user_id)
- @work = Work.find_by_id(creation_id)
+ @user = User.find_by(id: user_id)
@zz9pzza

zz9pzza May 12, 2017

Contributor

is find_by(id: ... ) better than find(....)

@davidstump

davidstump May 12, 2017

Contributor

find_by_id was used previously so I kept the same concept (return nil vs raise exception). If @user is required later, either would be just fine.

@zz9pzza

zz9pzza May 12, 2017

Contributor

Ah sorry nil, v exception ok :)

@@ -9,7 +9,7 @@
<!--/subnav-->
<!--main content-->
- <%= form_tag url_for(:controller => 'user_invite_requests', :action => 'update') do %>
+ <%= form_tag url_for(:controller => 'user_invite_requests', :action => 'update'), method: :put do %>
@zz9pzza

zz9pzza May 12, 2017

Contributor

Just curious why this is a put and not a patch ?

@davidstump

davidstump May 12, 2017

Contributor

I believe the error raise mentioned put, however either should work.

@davidstump

davidstump May 12, 2017

Contributor

I'll update this to patch as well

app/views/works/edit_multiple.html.erb
@@ -13,7 +13,7 @@
<%= render "works/work_abbreviated_list", :works => @works %>
-<%= form_for :work, :url => update_multiple_user_works_path(@user), :html => { :method => :put, :class => "post" } do |form| %>
+<%= form_for :work, :url => update_multiple_user_works_path(@user), method: :put, :html => { :class => "post" } do |form| %>
@zz9pzza

zz9pzza May 12, 2017

Contributor

put ?

@davidstump

davidstump May 12, 2017

Contributor

Routes generate both and put was used previously. Can certainly use either.

@zz9pzza

zz9pzza May 12, 2017

Contributor

It's a question David, I saw patch for the first time and read that it was the prefered way for updates however I don't know what is the right thing to do and this view is an edit.

@davidstump

davidstump May 12, 2017

Contributor

You are correct patch is the preferred method - I'll update this.

@@ -8,7 +8,7 @@
config.cache_classes = true
# Log error messages when you accidentally call methods on nil.
- config.whiny_nils = true
+ # config.whiny_nils = true
@zz9pzza

zz9pzza May 12, 2017

Contributor

Should this be set to catch more errors ?

@davidstump

davidstump May 12, 2017

Contributor

whiny_nils is deprecated as of 4.0 I believe.

@zz9pzza

zz9pzza May 12, 2017

Contributor

You are right, I thought I saw it being set in config/environments/development.rb but I was wrong sorry :)

db/structure.sql
INSERT INTO schema_migrations (version) VALUES ('20160706031054');
-INSERT INTO schema_migrations (version) VALUES ('20160724234958');
+INSERT INTO schema_migrations (version) VALUES ('20170322092920');
@zz9pzza

zz9pzza May 12, 2017

Contributor

This makes me nervous, I will look later.

@davidstump

davidstump May 12, 2017

Contributor

Ack. Should not have been pushed. Apologies.

features/step_definitions/user_steps.rb
assert user.active?
end
Then /^I should see the current user's preferences in the console$/ do
puts User.current_user.preference.inspect
end
+When(/^I pry$/) do
+ binding.pry
@zz9pzza

zz9pzza May 12, 2017

Contributor

We have this in

features/step_definitions/generic_steps.rb

@davidstump

davidstump May 12, 2017

Contributor

Ah awesome - can remove.

@@ -8,54 +8,54 @@ def self.included(taggable)
has_many :filter_taggings, :as => :filterable
has_many :filters, :through => :filter_taggings
- has_many :direct_filter_taggings, :class_name => "FilterTagging", :as => :filterable, :conditions => "inherited = 0"
+ has_many :direct_filter_taggings, -> { where(inherited: 0) }, :class_name => "FilterTagging", :as => :filterable
@houndci-bot

houndci-bot May 12, 2017

Use the new Ruby 1.9 hash syntax.

has_many :direct_filters, :source => :filter, :through => :direct_filter_taggings
has_many :taggings, :as => :taggable, :dependent => :destroy
has_many :tags, :through => :taggings, :source => :tagger, :source_type => 'Tag'
has_many :ratings,
+ -> { where("tags.type = 'Rating'") },
@houndci-bot

houndci-bot May 12, 2017

Align the parameters of a method call if they span more than one line.

:through => :taggings,
:source => :tagger,
:source_type => 'Tag',
- :before_remove => :remove_filter_tagging,
- :conditions => "tags.type = 'Rating'"
+ :before_remove => :remove_filter_tagging
@houndci-bot

houndci-bot May 12, 2017

Use the new Ruby 1.9 hash syntax.

has_many :categories,
+ -> { where("tags.type = 'Category'") },
@houndci-bot

houndci-bot May 12, 2017

Align the parameters of a method call if they span more than one line.

davidstump added some commits May 12, 2017

This looks really good! I just have a few questions to make sure I understand what's going on in a couple of places.

@@ -50,7 +50,7 @@ def update
flash[:notice] = ts('Challenge was successfully updated.')
# expire the cache on the signup form
- expire_fragment(:controller => 'challenge_signups', :action => 'new')
+ ActionController::Base.new.expire_fragment('challenge_signups/new')
@sarken

sarken May 14, 2017

Owner

Google was failing me on this, and I'm curious: why do we need ActionController::Base.new. here?

@davidstump

davidstump May 15, 2017

Contributor

Upgrading to Rails 4 causes an exception: undefined method 'expire_fragment' that is solved by adding the ActionController::Base.new prefix. A number of relevant threads, but this one is concise: http://stackoverflow.com/questions/33499384/expire-fragment-caching-in-rails-4

@@ -732,19 +732,19 @@ def update_multiple
@works = Work.joins(pseuds: :user).where('users.id = ?', @user.id).where(id: params[:work_ids]).readonly(false)
@errors = []
# to avoid overwriting, we entirely trash any blank fields and also any unchecked checkboxes
- work_params = params[:work].reject { |_key, value| value.blank? || value == '0' }
+ updated_work_params = work_params.reject { |_key, value| value.blank? || value == '0' }
@sarken

sarken May 14, 2017

Owner

I just want to double-check that I understand what's going on here: we have to switch the variable to updated_work_params because strong parameters prevent us from modifying work_params?

@davidstump

davidstump May 15, 2017

Contributor

Yep.

@@ -25,7 +25,7 @@ def admin_posts
@admin_posts = AdminPost.non_translated.for_homepage.all
else
@admin_posts = Rails.cache.fetch("home/index/home_admin_posts", expires_in: 20.minutes) do
- AdminPost.non_translated.for_homepage.all
+ AdminPost.non_translated.for_homepage
@sarken

sarken May 14, 2017

Owner

We originally needed the alls because this wasn't getting the actual AdminPost (or Reading) objects and the caching wasn't working -- does this mean this less-than-ideal behavior is fixed in Rails 4.0 and onward?

@davidstump

davidstump May 15, 2017

Contributor

Using all results in a deprecation warning. Without all, it returns an ActiveRecord::Relation object. With all it is returning an array. If the array is the desired output, we can use to_a to get the same result without a deprecation warning. Does that work?

@@ -589,7 +599,7 @@ def self.find_or_create_by_name(new_name)
elsif tag
self.find_or_create_by_name(new_name + " - " + self.to_s)
else
- self.create(:name => new_name)
+ self.create(name: new_name, type: self.to_s)
@sarken

sarken May 14, 2017

Owner

Why does it need type now?

@davidstump

davidstump May 15, 2017

Contributor

This is due to the bug we found in 4.0 with STI. If you run Media.create(params) it would result in a Fandom object getting created. If you explicitly add in the type param, it would maintain the proper type. Ideally, this could go away at 4.2 (ish) if the bug is resolved at that point.

@@ -1,6 +1,6 @@
class TagSweeper < ActionController::Caching::Sweeper
@sarken

sarken May 14, 2017

Owner

I noticed tag_sweeper is commented out in several places now that we've moved its contents into the model. Are we still using this file rather than the model in some places?

@davidstump

davidstump May 15, 2017

Contributor

We are not that I know of. I left the file in place as I wanted to make sure others weighed in on moving away from Sweepers slowly.

@@ -25,7 +25,7 @@
<fieldset>
<legend><%= ts('Management') %></legend>
- <%= render "tag_set_form_management", :form => tag_set_form %>
+ <%= render partial: "tag_set_form_management", locals: {form: tag_set_form} %>
@sarken

sarken May 14, 2017

Owner

We've slowly been doing away with partial and locals and moving to the simpler <%= render "tag_set_form_management", form: tag_set_form %> -- is there a specific reason we need to use this syntax and the hash rockets here?

@davidstump

davidstump May 15, 2017

Contributor

I think it complained about it at some point - but the simpler syntax should work fine so I am reverting and pushing. Hopefully some other fix was the cause of the error when I was working on this one.

Given /^the basic warnings exist$/ do
- Warning.find_or_create_by_name_and_canonical("No Archive Warnings Apply", true)
- Warning.find_or_create_by_name_and_canonical("Choose Not To Use Archive Warnings", true)
+ Warning.find_or_create_by_name("No Archive Warnings Apply").update(canonical: true)
@sarken

sarken May 14, 2017

Owner

Why do Warning and Category tags use find_or_create_by_name and then update the canonical attribute while other tag types use find_by(name: value, canonical: value).first_or_create? Were you running into issues with Warning and Category tags existing but not being canonical?

@davidstump

davidstump May 15, 2017

Contributor

Warning and Category inherit from Tag which has a custom defined find_or_create_by_name method. Calling find_by(name: directly skips this method (and some important logic). I was keeping all Tag children using the custom method. In this case, it might be simply using the find_or_create logic. Perhaps that method should be renamed to avoid confusion with Rails provided finders?

features/step_definitions/tag_steps.rb
puts tag.inspect
end
+
+Given(/^a media exists with name: "([^"]*)", canonical: true$/) do |media|
@sarken

sarken May 14, 2017

Owner

World's second smallest nitpick: could you move this to the top with the other given definitions?

@davidstump

davidstump May 15, 2017

Contributor

No problem. Done

script/gem_security.sh
-bundle-audit update
-bundle-audit check --ignore OSVDB-96425 OSVDB-131677 > $TMP
+bundle-audit update
+# Ignoring issues related to outdated gems required for 4.0 step. To be updated at earliest convinience
@sarken

sarken May 14, 2017

Owner

World's actual smallest nitpick: there's a typo in "convenience." (You don't have to fix it, but since I labeled another comment the world's second smallest nitpick, I figured I might as well follow through and let you know what the smallest was ;P)

@davidstump

davidstump May 15, 2017

Contributor

Heh. Also fixed :)

@@ -38,7 +38,7 @@ def update
skin.cache! unless skin.cached?
skin.update_attribute(:in_chooser, true)
end
- skin.update_attribute(:admin_note, params[:skin_admin_note]["#{skin.id}"]) if params[:skin_admin_note] && params[:skin_admin_note]["#{skin.id}"]
+ skin.update_attribute(:admin_note, params[:skin_admin_note]["#{skin.id}"]) if params[:skin_admin_note] && params[:skin_admin_note]["#{skin.id}"]
@houndci-bot

houndci-bot May 15, 2017

Prefer to_s over string interpolation.

signup = ChallengeSignup.find(signup_id)
- pmatches = return_requests ?
+ pmatches = return_requests ?
@houndci-bot

houndci-bot May 15, 2017

Avoid multi-line ternary operators, use if or unless instead.

if current_user == @user
- if params[:collection_id] && (@collection = Collection.find_by_name(params[:collection_id]))
+ if params[:collection_id] && (@collection = Collection.find_by(name: params[:collection_id]))
@houndci-bot

houndci-bot May 15, 2017

Use the return of the conditional for variable assignment and comparison.

@@ -300,7 +300,7 @@ def gift_exchange_to_csv
csv_array
end
-
+
@houndci-bot

houndci-bot May 15, 2017

Extra blank line detected.

@check_ownership_of = @user
end
def index
if @user && current_user == @user
@external_authors = @user.external_authors
elsif logged_in? && current_user.archivist
- @external_authors = ExternalCreatorship.find_all_by_archivist_id(current_user).collect(&:external_author).uniq
+ @external_authors = ExternalCreatorship.where(archivist_id: current_user).collect(&:external_author).uniq
@houndci-bot

houndci-bot May 15, 2017

Prefer map over collect.

@@ -15,7 +15,7 @@ def fetch
@external_work = ExternalWork.where(:url => url).first
end
respond_to do |format|
- format.js
+ format.json { render 'fetch.js.erb' }
@houndci-bot

houndci-bot May 15, 2017

Use 2 (not 1) spaces for indentation.

AND common_taggings.filterable_type = 'Tag'"
end
end
@fandoms = Fandom.joins(join_string).
where(conditions).
order(params[:sort] == 'count' ? "count DESC" : "sortable_name ASC").
with_count.
- paginate(:page => params[:page], :per_page => 250)
+ paginate(:page => params[:page], :per_page => 250)
@houndci-bot

houndci-bot May 15, 2017

Align paginate with Fandom.joins(join_string). on line 56.
Use the new Ruby 1.9 hash syntax.

@@ -1,9 +1,9 @@
class GiftsController < ApplicationController
-
+
@houndci-bot

houndci-bot May 15, 2017

Extra empty line detected at class body beginning.

- :collection_ids,
+ :bookmarkable_complete,
+ :bookmarkable_language_id,
+ :collection_ids,
@houndci-bot

houndci-bot May 15, 2017

Align the parameters of a method call if they span more than one line.

@@ -153,13 +153,13 @@ def set_parent_fields!
elsif self.bookmarks_parent.is_a?(Pseud)
options[:pseud_ids] = [self.bookmarks_parent.id]
elsif self.bookmarks_parent.is_a?(User)
- options[:pseud_ids] = self.bookmarks_parent.pseuds.value_of(:id)
+ options[:pseud_ids] = self.bookmarks_parent.pseuds.pluck(:id)
@houndci-bot

houndci-bot May 15, 2017

Redundant self detected.

@@ -262,7 +262,7 @@ def summary
end
end
unless all_tag_ids.empty?
- tags << Tag.where(:id => all_tag_ids).value_of(:name).join(", ")
+ tags << Tag.where(:id => all_tag_ids).pluck(:name).join(", ")
@houndci-bot

houndci-bot May 15, 2017

Use the new Ruby 1.9 hash syntax.

@@ -18,7 +18,7 @@ def signups_match
if self.sent_at.nil? &&
self.request_signup.present? &&
self.offer_signup.present? &&
- !self.request_signup.request_potential_matches.value_of(:offer_signup_id).include?(self.offer_signup_id)
+ !self.request_signup.request_potential_matches.pluck(:offer_signup_id).include?(self.offer_signup_id)
@houndci-bot

houndci-bot May 15, 2017

Align the operands of a condition in an if statement spanning multiple lines.
Redundant self detected.

@@ -76,45 +76,47 @@ def signups_match
WORKS_JOIN = "INNER JOIN works ON works.id = challenge_assignments.creation_id AND challenge_assignments.creation_type = 'Work'"
WORKS_LEFT_JOIN = "LEFT JOIN works ON works.id = challenge_assignments.creation_id AND challenge_assignments.creation_type = 'Work'"
- scope :fulfilled,
+ scope :fulfilled, -> {
@houndci-bot

houndci-bot May 15, 2017

Use the lambda method for multiline lambdas.

ChallengeAssignment.where(:request_signup_id => ids)
end
# has to be a left join to get assignments that don't have a collection item
- scope :unfulfilled,
+ scope :unfulfilled, -> {
@houndci-bot

houndci-bot May 15, 2017

Use the lambda method for multiline lambdas.

- :conditions => ['users.id = ?', user.id]
- }
+ select('DISTINCT challenge_claims.*')
+ .joins("INNER JOIN users ON challenge_claims.claiming_user_id = users.id")
@houndci-bot

houndci-bot May 15, 2017

Place the . on the previous line, together with the method call receiver.
Use 2 (not 0) spaces for indenting an expression spanning multiple lines.

- }
+ select('DISTINCT challenge_claims.*')
+ .joins("INNER JOIN users ON challenge_claims.claiming_user_id = users.id")
+ .where('users.id = ?', user.id)
@houndci-bot

houndci-bot May 15, 2017

Place the . on the previous line, together with the method call receiver.
Use 2 (not 0) spaces for indenting an expression spanning multiple lines.

}
- scope :in_collection, lambda {|collection| {:conditions => ['challenge_claims.collection_id = ?', collection.id] }}
+ scope :in_collection, lambda {|collection|
@houndci-bot

houndci-bot May 15, 2017

Space between { and | missing.

@@ -58,46 +58,48 @@ def self.order_by_offering_pseud(dir="ASC")
WORKS_JOIN = "INNER JOIN works ON works.id = challenge_claims.creation_id AND challenge_claims.creation_type = 'Work'"
WORKS_LEFT_JOIN = "LEFT JOIN works ON works.id = challenge_claims.creation_id AND challenge_claims.creation_type = 'Work'"
- scope :fulfilled,
+ scope :fulfilled, -> {
@houndci-bot

houndci-bot May 15, 2017

Use the lambda method for multiline lambdas.

posted_ids.empty? ? in_collection(collection) : in_collection(collection).where("challenge_claims.creation_id IS NULL OR challenge_claims.id NOT IN (?)", posted_ids)
end
# has to be a left join to get works that don't have a collection item
- scope :unfulfilled,
+ scope :unfulfilled, -> {
@houndci-bot

houndci-bot May 15, 2017

Use the lambda method for multiline lambdas.

all_claims.where("challenge_claims.id NOT IN (?)", posted_ids)
end
def get_collection_item
return nil unless self.creation
- CollectionItem.find(:first, :conditions => ["collection_id = ? AND item_id = ? AND item_type = ?", self.collection_id, self.creation_id, self.creation_type])
+ CollectionItem.where('collection_id = ? AND item_id = ? AND item_type = ?', self.collection_id, self.creation_id, self.creation_type).first
@houndci-bot

houndci-bot May 15, 2017

Redundant self detected.

# Scopes used to include extra data when loading.
- scope :with_request_tags, includes(
+ scope :with_request_tags, -> { includes(
@houndci-bot

houndci-bot May 15, 2017

Use the lambda method for multiline lambdas.
Block body expression is on the same line as the block start.

requests: [tag_set: :tags, optional_tag_set: :tags]
- )
- scope :with_offer_tags, includes(
+ ) }
@houndci-bot

houndci-bot May 15, 2017

Expression at 61, 5 should be on its own line.

- )
- scope :with_offer_tags, includes(
+ ) }
+ scope :with_offer_tags, -> { includes(
@houndci-bot

houndci-bot May 15, 2017

Use the lambda method for multiline lambdas.
Block body expression is on the same line as the block start.

offers: [tag_set: :tags, optional_tag_set: :tags]
- )
+ ) }
@houndci-bot

houndci-bot May 15, 2017

Expression at 64, 5 should be on its own line.

@@ -14,24 +14,24 @@ class Chapter < ActiveRecord::Base
acts_as_commentable
has_many :kudos, :as => :commentable
- validates_length_of :title, :allow_blank => true, :maximum => ArchiveConfig.TITLE_MAX,
+ validates_length_of :title, :allow_blank => true, :maximum => ArchiveConfig.TITLE_MAX,
@houndci-bot

houndci-bot May 15, 2017

Use the new Ruby 1.9 hash syntax.

-
- validates_length_of :summary, :allow_blank => true, :maximum => ArchiveConfig.SUMMARY_MAX,
+
+ validates_length_of :summary, :allow_blank => true, :maximum => ArchiveConfig.SUMMARY_MAX,
@houndci-bot

houndci-bot May 15, 2017

Use the new Ruby 1.9 hash syntax.

:too_long => ts("must be less than %{max} characters long.", :max => ArchiveConfig.SUMMARY_MAX)
- validates_length_of :notes, :allow_blank => true, :maximum => ArchiveConfig.NOTES_MAX,
+ validates_length_of :notes, :allow_blank => true, :maximum => ArchiveConfig.NOTES_MAX,
@houndci-bot

houndci-bot May 15, 2017

Use the new Ruby 1.9 hash syntax.

:too_long => ts("must be less than %{max} characters long.", :max => ArchiveConfig.NOTES_MAX)
- validates_length_of :endnotes, :allow_blank => true, :maximum => ArchiveConfig.NOTES_MAX,
+ validates_length_of :endnotes, :allow_blank => true, :maximum => ArchiveConfig.NOTES_MAX,
@houndci-bot

houndci-bot May 15, 2017

Use the new Ruby 1.9 hash syntax.

:too_long => ts("must be less than %{max} characters long.", :max => ArchiveConfig.NOTES_MAX)
validates_presence_of :content
- validates_length_of :content, :minimum => ArchiveConfig.CONTENT_MIN,
+ validates_length_of :content, :minimum => ArchiveConfig.CONTENT_MIN,
@houndci-bot

houndci-bot May 15, 2017

Use the new Ruby 1.9 hash syntax.

:too_short => ts("must be at least %{min} characters long.", :min => ArchiveConfig.CONTENT_MIN)
- validates_length_of :content, :maximum => ArchiveConfig.CONTENT_MAX,
+ validates_length_of :content, :maximum => ArchiveConfig.CONTENT_MAX,
@houndci-bot

houndci-bot May 15, 2017

Use the new Ruby 1.9 hash syntax.

# make em-dashes into html code
# def clean_emdashes
# self.content.gsub!(/\xE2\x80\"/, '&#8212;')
-# end
+# end
@houndci-bot

houndci-bot May 15, 2017

Incorrect indentation detected (column 0 instead of 2).

# end
-
+
@houndci-bot

houndci-bot May 15, 2017

Extra empty line detected at class body end.

+ has_many :owners, -> { where('collection_participants.participant_role = ?', CollectionParticipant::OWNER) }, through: :collection_participants, source: :pseud
+ has_many :moderators, -> { where('collection_participants.participant_role = ?', CollectionParticipant::MODERATOR) }, through: :collection_participants, source: :pseud
+ has_many :members, -> { where('collection_participants.participant_role = ?', CollectionParticipant::MEMBER) }, through: :collection_participants, source: :pseud
+ has_many :posting_participants, -> { where('collection_participants.participant_role in (?)', [CollectionParticipant::MEMBER,CollectionParticipant::MODERATOR, CollectionParticipant::OWNER ] ) }, through: :collection_participants, source: :pseud
@houndci-bot

houndci-bot May 15, 2017

Space missing after comma.
Space inside square brackets detected.
Space inside parentheses detected.

@@ -199,13 +196,14 @@ def self.signup_open(challenge_type)
where("collections.title LIKE ?", '%' + title + '%')
}
- scope :with_item_count,
+ scope :with_item_count, -> {
@houndci-bot

houndci-bot May 15, 2017

Use the lambda method for multiline lambdas.

@@ -216,7 +214,7 @@ def self.orphan(pseuds, collections, default=true)
for pseud in pseuds
for collection in collections
if pseud && collection && collection.owners.include?(pseud)
- orphan_pseud = default ? User.orphan_account.default_pseud : User.orphan_account.pseuds.find_or_create_by_name(pseud.name)
+ orphan_pseud = default ? User.orphan_account.default_pseud : User.orphan_account.pseuds.find_or_create_by(name: pseud.name)
@houndci-bot

houndci-bot May 15, 2017

Avoid more than 3 levels of block nesting.

@@ -276,12 +274,12 @@ def all_participants
end
def all_items
- CollectionItem.where(collection_id: ([self.id] + self.children.value_of(:id)))
+ CollectionItem.where(collection_id: ([self.id] + self.children.pluck(:id)))
@houndci-bot

houndci-bot May 15, 2017

Redundant self detected.

end
def all_approved_works
work_ids = all_items.where(item_type: "Work", user_approval_status: CollectionItem::APPROVED,
- collection_approval_status: CollectionItem::APPROVED).value_of(:item_id)
+ collection_approval_status: CollectionItem::APPROVED).pluck(:item_id)
@houndci-bot

houndci-bot May 15, 2017

Align the elements of a hash literal if they span more than one line.

@zz9pzza zz9pzza merged commit 8e80a4e into otwcode:master May 15, 2017

3 checks passed

Scrutinizer 83 new issues, 14 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
hound 433 violations found.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment