From b8b6784067b24a362e73f6e71ae38afbea321910 Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Tue, 9 May 2017 18:23:19 -0700 Subject: [PATCH 1/3] Added rubocop and updated for all the violations --- .rubocop.yml | 1 + .rubocop_todo.yml | 253 +++++++++++++++++++++++++++ Gemfile | 3 + Gemfile.lock | 18 +- app/controllers/invite_controller.rb | 53 +++--- bin/setup | 2 +- bin/spring | 2 +- config/application.rb | 2 +- 8 files changed, 303 insertions(+), 31 deletions(-) create mode 100644 .rubocop.yml create mode 100644 .rubocop_todo.yml diff --git a/.rubocop.yml b/.rubocop.yml new file mode 100644 index 00000000..18b0dda5 --- /dev/null +++ b/.rubocop.yml @@ -0,0 +1 @@ +inherit_from: .rubocop_todo.yml \ No newline at end of file diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml new file mode 100644 index 00000000..dc77f4bf --- /dev/null +++ b/.rubocop_todo.yml @@ -0,0 +1,253 @@ +Style/PercentLiteralDelimiters: + Enabled: false + +# kind_of? is a good way to check a type +Style/ClassCheck: + EnforcedStyle: kind_of? + +Style/FrozenStringLiteralComment: + Enabled: false + +# This doesn't work with older versions of Ruby (pre 2.4.0) +Style/SafeNavigation: + Enabled: false + +# This doesn't work with older versions of Ruby (pre 2.4.0) +Performance/RegexpMatch: + Enabled: false + +# This suggests use of `tr` instead of `gsub`. While this might be more performant, +# these methods are not at all interchangable, and behave very differently. This can +# lead to people making the substitution without considering the differences. +Performance/StringReplacement: + Enabled: false + +# .length == 0 is also good, we don't always want .zero? +Style/NumericPredicate: + Enabled: false + +# this would cause errors with long lanes +Metrics/BlockLength: + Enabled: false + +# this is a bit buggy +Metrics/ModuleLength: + Enabled: false + +# certificate_1 is an okay variable name +Style/VariableNumber: + Enabled: false + +# This is used a lot across the fastlane code base for config files +Style/MethodMissing: + Enabled: false + +# +# File.chmod(0777, f) +# +# is easier to read than +# +# File.chmod(0o777, f) +# +Style/NumericLiteralPrefix: + Enabled: false + +# +# command = (!clean_expired.nil? || !clean_pattern.nil?) ? CLEANUP : LIST +# +# is easier to read than +# +# command = !clean_expired.nil? || !clean_pattern.nil? ? CLEANUP : LIST +# +Style/TernaryParentheses: + Enabled: false + +# sometimes it is useful to have those empty methods +Style/EmptyMethod: + Enabled: false + +# It's better to be more explicit about the type +Style/BracesAroundHashParameters: + Enabled: false + +# specs sometimes have useless assignments, which is fine +Lint/UselessAssignment: + Exclude: + - '**/spec/**/*' + +# We could potentially enable the 2 below: +Style/IndentHash: + Enabled: false + +Style/AlignHash: + Enabled: false + +# HoundCI doesn't like this rule +Style/DotPosition: + Enabled: false + +# We allow !! as it's an easy way to convert ot boolean +Style/DoubleNegation: + Enabled: false + +# Prevent to replace [] into %i +Style/SymbolArray: + Enabled: false + +# We still support Ruby 2.0.0 +Style/IndentHeredoc: + Enabled: false + +# This cop would not work fine with rspec +Style/MixinGrouping: + Exclude: + - '**/spec/**/*' + +# Sometimes we allow a rescue block that doesn't contain code +Lint/HandleExceptions: + Enabled: false + +# Cop supports --auto-correct. +Lint/UnusedBlockArgument: + Enabled: false + +Lint/AmbiguousBlockAssociation: + Enabled: false + +# Needed for $verbose +Style/GlobalVars: + Enabled: false + +# We want to allow class Fastlane::Class +Style/ClassAndModuleChildren: + Enabled: false + +# $? Exit +Style/SpecialGlobalVars: + Enabled: false + +Metrics/AbcSize: + Enabled: false + +Metrics/MethodLength: + Enabled: false + +Metrics/CyclomaticComplexity: + Enabled: false + +# The %w might be confusing for new users +Style/WordArray: + MinSize: 19 + +# raise and fail are both okay +Style/SignalException: + Enabled: false + +# Better too much 'return' than one missing +Style/RedundantReturn: + Enabled: false + +# Having if in the same line might not always be good +Style/IfUnlessModifier: + Enabled: false + +# and and or is okay +Style/AndOr: + Enabled: false + +# Configuration parameters: CountComments. +Metrics/ClassLength: + Max: 320 + + +# Configuration parameters: AllowURI, URISchemes. +Metrics/LineLength: + Max: 370 + +# Configuration parameters: CountKeywordArgs. +Metrics/ParameterLists: + Max: 17 + +Metrics/PerceivedComplexity: + Max: 18 + +# Sometimes it's easier to read without guards +Style/GuardClause: + Enabled: false + +# We allow both " and ' +Style/StringLiterals: + Enabled: false + +# something = if something_else +# that's confusing +Style/ConditionalAssignment: + Enabled: false + +# Better to have too much self than missing a self +Style/RedundantSelf: + Enabled: false + +# e.g. +# def self.is_supported?(platform) +# we may never use `platform` +Lint/UnusedMethodArgument: + Enabled: false + +# the let(:key) { ... } +Lint/ParenthesesAsGroupedExpression: + Exclude: + - '**/spec/**/*' + +# This would reject is_ in front of methods +# We use `is_supported?` everywhere already +Style/PredicateName: + Enabled: false + +# We allow the $ +Style/PerlBackrefs: + Enabled: false + +# Disable '+ should be surrounded with a single space' for xcodebuild_spec.rb +Style/SpaceAroundOperators: + Exclude: + - '**/spec/actions_specs/xcodebuild_spec.rb' + +AllCops: + Include: + - '**/fastlane/Fastfile' + Exclude: + - '**/lib/assets/custom_action_template.rb' + - './vendor/**/*' + +# They have not to be snake_case +Style/FileName: + Exclude: + - '**/Dangerfile' + - '**/Brewfile' + - '**/Gemfile' + - '**/Podfile' + - '**/Rakefile' + - '**/Fastfile' + - '**/Deliverfile' + - '**/Snapfile' + +# We're not there yet +Style/Documentation: + Enabled: false + +# Added after upgrade to 0.38.0 +Style/MutableConstant: + Enabled: false + +# length > 0 is good +Style/ZeroLengthPredicate: + Enabled: false + +# Adds complexity +Style/IfInsideElse: + Enabled: false + +# Sometimes we just want to 'collect' +Style/CollectionMethods: + Enabled: false diff --git a/Gemfile b/Gemfile index f1bc2463..f3b2c6e6 100644 --- a/Gemfile +++ b/Gemfile @@ -36,4 +36,7 @@ group :development, :test do # Spring speeds up development by keeping your application running in the background. Read more: https://github.com/rails/spring gem 'spring' + + # make sure our code is spick and span + gem 'rubocop', require: false end diff --git a/Gemfile.lock b/Gemfile.lock index 2a829957..123e53e9 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -39,6 +39,7 @@ GEM addressable (2.5.1) public_suffix (~> 2.0, >= 2.0.2) arel (6.0.4) + ast (2.3.0) autoprefixer-rails (6.7.7.2) execjs babosa (1.0.2) @@ -164,7 +165,10 @@ GEM nokogiri (1.7.1) mini_portile2 (~> 2.1.0) os (0.9.6) - plist (3.3.0) + parser (2.4.0.0) + ast (~> 2.2) + plist (3.2.0) + powerpack (0.1.1) public_suffix (2.0.5) puma (3.8.2) rack (1.6.5) @@ -199,11 +203,20 @@ GEM activesupport (= 4.2.8) rake (>= 0.8.7) thor (>= 0.18.1, < 2.0) + rainbow (2.2.2) + rake rake (12.0.0) representable (2.3.0) uber (~> 0.0.7) retriable (2.1.0) - rouge (2.0.7) + rouge (1.11.1) + rubocop (0.48.1) + parser (>= 2.3.3.1, < 3.0) + powerpack (~> 0.1) + rainbow (>= 1.99.1, < 3.0) + ruby-progressbar (~> 1.7) + unicode-display_width (~> 1.0, >= 1.0.1) + ruby-progressbar (1.8.1) rubyzip (1.2.1) sass (3.4.23) sass-rails (5.0.6) @@ -273,6 +286,7 @@ DEPENDENCIES puma rails (~> 4.2.8) rails_12factor + rubocop sass-rails (~> 5.0) spring uglifier (>= 1.3.0) diff --git a/app/controllers/invite_controller.rb b/app/controllers/invite_controller.rb index b4889b8b..5dd7b82b 100644 --- a/app/controllers/invite_controller.rb +++ b/app/controllers/invite_controller.rb @@ -3,7 +3,7 @@ class InviteController < ApplicationController before_action :set_app_details before_action :check_disabled_text - skip_before_filter :verify_authenticity_token + skip_before_action :verify_authenticity_token def index if boarding_service.user and boarding_service.password @@ -39,7 +39,7 @@ def submit if domains.count == 1 @message = "Sorry! Early access is currently restricted to people within the #{domains.first} domain." else - @message = "Sorry! Early access is currently restricted to people within the following domains: (#{domains.join(", ")})" + @message = "Sorry! Early access is currently restricted to people within the following domains: (#{domains.join(', ')})" end @type = "warning" render :index @@ -87,34 +87,35 @@ def submit end private - def create_and_add_tester(email, first_name, last_name) - add_tester_response = boarding_service.add_tester(email, first_name, last_name) - @message = add_tester_response.message - @type = add_tester_response.type - end - def boarding_service - BOARDING_SERVICE - end + def create_and_add_tester(email, first_name, last_name) + add_tester_response = boarding_service.add_tester(email, first_name, last_name) + @message = add_tester_response.message + @type = add_tester_response.type + end - def app_metadata - Rails.cache.fetch('appMetadata', expires_in: 10.minutes) do - { - icon_url: boarding_service.app.app_icon_preview_url, - title: boarding_service.app.name - } - end - end + def boarding_service + BOARDING_SERVICE + end - def set_app_details - @metadata = app_metadata - @title = @metadata[:title] + def app_metadata + Rails.cache.fetch('appMetadata', expires_in: 10.minutes) do + { + icon_url: boarding_service.app.app_icon_preview_url, + title: boarding_service.app.name + } end + end - def check_disabled_text - if boarding_service.itc_closed_text - @message = boarding_service.itc_closed_text - @type = "warning" - end + def set_app_details + @metadata = app_metadata + @title = @metadata[:title] + end + + def check_disabled_text + if boarding_service.itc_closed_text + @message = boarding_service.itc_closed_text + @type = "warning" end + end end diff --git a/bin/setup b/bin/setup index acdb2c13..d3bb9976 100755 --- a/bin/setup +++ b/bin/setup @@ -2,7 +2,7 @@ require 'pathname' # path to your application root. -APP_ROOT = Pathname.new File.expand_path('../../', __FILE__) +APP_ROOT = Pathname.new File.expand_path('../../', __FILE__) Dir.chdir APP_ROOT do # This script is a starting point to setup your application. diff --git a/bin/spring b/bin/spring index 7b45d374..5e7ecc47 100755 --- a/bin/spring +++ b/bin/spring @@ -7,7 +7,7 @@ unless defined?(Spring) require "rubygems" require "bundler" - if match = Bundler.default_lockfile.read.match(/^GEM$.*?^ (?: )*spring \((.*?)\)$.*?^$/m) + if (match = Bundler.default_lockfile.read.match(/^GEM$.*?^ (?: )*spring \((.*?)\)$.*?^$/m)) Gem.paths = { "GEM_PATH" => [Bundler.bundle_path.to_s, *Gem.path].uniq } gem "spring", match[1] require "spring/binstub" diff --git a/config/application.rb b/config/application.rb index 1c3f5f3c..20f9f7d2 100644 --- a/config/application.rb +++ b/config/application.rb @@ -22,7 +22,7 @@ class Application < Rails::Application # The default locale is :en and all translations from config/locales/*.rb,yml are auto loaded. # config.i18n.load_path += Dir[Rails.root.join('my', 'locales', '*.{rb,yml}').to_s] # config.i18n.default_locale = :de - + # Allows iframe from other origins config.action_dispatch.default_headers = { 'X-Frame-Options' => 'ALLOWALL' From b821cfa48920a416a18e3f17aed6621d56b66177 Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Wed, 10 May 2017 10:47:16 -0700 Subject: [PATCH 2/3] Adds new line to end of file --- .rubocop.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.rubocop.yml b/.rubocop.yml index 18b0dda5..cc32da4b 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1 +1 @@ -inherit_from: .rubocop_todo.yml \ No newline at end of file +inherit_from: .rubocop_todo.yml From e653c3d178c0a4313f9a45d06fdaf70ef2c33b07 Mon Sep 17 00:00:00 2001 From: Joshua Liebowitz Date: Wed, 24 May 2017 09:53:18 -0700 Subject: [PATCH 3/3] Rebase + run the rules again --- app/controllers/invite_controller.rb | 4 +- app/services/boarding_service.rb | 152 +++++++++++++-------------- 2 files changed, 77 insertions(+), 79 deletions(-) diff --git a/app/controllers/invite_controller.rb b/app/controllers/invite_controller.rb index 5dd7b82b..09185c85 100644 --- a/app/controllers/invite_controller.rb +++ b/app/controllers/invite_controller.rb @@ -28,7 +28,7 @@ def submit render :index return end - + email = params[:email] first_name = params[:first_name] last_name = params[:last_name] @@ -55,7 +55,7 @@ def submit return end end - + if email.length == 0 render :index return diff --git a/app/services/boarding_service.rb b/app/services/boarding_service.rb index 23d16df2..2afacd4c 100644 --- a/app/services/boarding_service.rb +++ b/app/services/boarding_service.rb @@ -3,7 +3,7 @@ class AddTesterResponse attr_accessor :message attr_accessor :type -end +end class BoardingService include AbstractController::Translation @@ -18,9 +18,9 @@ class BoardingService attr_accessor :itc_closed_text def initialize(app_id: ENV["ITC_APP_ID"], - user: ENV["ITC_USER"] || ENV["FASTLANE_USER"], - password: ENV["ITC_PASSWORD"] || ENV["FASTLANE_PASSWORD"], - tester_group_names: ENV["ITC_APP_TESTER_GROUPS"]) + user: ENV["ITC_USER"] || ENV["FASTLANE_USER"], + password: ENV["ITC_PASSWORD"] || ENV["FASTLANE_PASSWORD"], + tester_group_names: ENV["ITC_APP_TESTER_GROUPS"]) @app_id = app_id @user = user @password = password @@ -70,7 +70,6 @@ def add_tester(email, first_name, last_name) Rails.logger.info "Successfully added tester to the default tester group in app: #{app.name}" end end - rescue => ex Rails.logger.error "Could not add #{tester.email} to app: #{app.name}" raise ex @@ -81,87 +80,86 @@ def add_tester(email, first_name, last_name) private - def create_tester(email: nil, first_name: nil, last_name: nil, app: nil) - current_user = Spaceship::Members.find(Spaceship::Tunes.client.user) - if current_user.admin? - tester = Spaceship::Tunes::Tester::External.create!(email: email, - first_name: first_name, - last_name: last_name) - Rails.logger.info "Successfully added tester: #{email} to your account" - elsif current_user.app_manager? - - Spaceship::TestFlight::Tester.create_app_level_tester(app_id: app.apple_id, - first_name: first_name, - last_name: last_name, - email: email) - tester = Spaceship::Tunes::Tester::External.find_by_app(app.apple_id, email) - Rails.logger.info "Successfully added tester: #{email} to app: #{app.name}" - else - raise "Current account doesn't have permission to create a tester" - end - - return tester - rescue => ex - Rails.logger.error "Could not create tester #{email}" - raise ex + def create_tester(email: nil, first_name: nil, last_name: nil, app: nil) + current_user = Spaceship::Members.find(Spaceship::Tunes.client.user) + if current_user.admin? + tester = Spaceship::Tunes::Tester::External.create!(email: email, + first_name: first_name, + last_name: last_name) + Rails.logger.info "Successfully added tester: #{email} to your account" + elsif current_user.app_manager? + + Spaceship::TestFlight::Tester.create_app_level_tester(app_id: app.apple_id, + first_name: first_name, + last_name: last_name, + email: email) + tester = Spaceship::Tunes::Tester::External.find_by_app(app.apple_id, email) + Rails.logger.info "Successfully added tester: #{email} to app: #{app.name}" + else + raise "Current account doesn't have permission to create a tester" end - def find_app_tester(email: nil, app: nil) - current_user = Spaceship::Members.find(Spaceship::Tunes.client.user) - if current_user.admin? - tester = Spaceship::Tunes::Tester::Internal.find(email) - tester ||= Spaceship::Tunes::Tester::External.find(email) - elsif current_user.app_manager? - unless app - raise "Account #{current_user.email_address} is only an 'App Manager' and therefore you must also define what app this tester (#{email}) should be added to" - end - tester = Spaceship::Tunes::Tester::Internal.find_by_app(app.apple_id, email) - tester ||= Spaceship::Tunes::Tester::External.find_by_app(app.apple_id, email) - else - raise "Account #{current_user.email} doesn't have a role that is allowed to administer app testers, current roles: #{current_user.roles}" - tester = nil - end + return tester + rescue => ex + Rails.logger.error "Could not create tester #{email}" + raise ex + end - if tester - Rails.logger.info "Found existing tester #{email}" + def find_app_tester(email: nil, app: nil) + current_user = Spaceship::Members.find(Spaceship::Tunes.client.user) + if current_user.admin? + tester = Spaceship::Tunes::Tester::Internal.find(email) + tester ||= Spaceship::Tunes::Tester::External.find(email) + elsif current_user.app_manager? + unless app + raise "Account #{current_user.email_address} is only an 'App Manager' and therefore you must also define what app this tester (#{email}) should be added to" end + tester = Spaceship::Tunes::Tester::Internal.find_by_app(app.apple_id, email) + tester ||= Spaceship::Tunes::Tester::External.find_by_app(app.apple_id, email) + else + raise "Account #{current_user.email} doesn't have a role that is allowed to administer app testers, current roles: #{current_user.roles}" + end - return tester + if tester + Rails.logger.info "Found existing tester #{email}" end - def ensure_values - error_message = [] - - error_message << "Environment variable `ITC_APP_ID` required" if @app_id.to_s.length == 0 - error_message << "Environment variable `ITC_USER` or `FASTLANE_USER` required" if @user.to_s.length == 0 - error_message << "Environment variable `ITC_PASSWORD` or `FASTLANE_PASSWORD`" if @password.to_s.length == 0 - raise error_message.join("\n") if error_message.length > 0 - - spaceship = Spaceship::Tunes.login(@user, @password) - spaceship.select_team - - @app ||= Spaceship::Tunes::Application.find(@app_id) - raise "Could not find app with ID #{app_id}" if @app.nil? - - if tester_group_names - test_flight_groups = Spaceship::TestFlight::Group.filter_groups(app_id: @app.apple_id) - test_flight_group_names = test_flight_groups.map { |group| group.name }.to_set - tester_group_names.select do |group_name| - next if test_flight_group_names.include?(group_name) - error_message << "TestFlight missing group `#{group_name}`, You need to first create this group in iTunes Connect." - end - end - raise error_message.join("\n") if error_message.length > 0 + return tester + end + + def ensure_values + error_message = [] + + error_message << "Environment variable `ITC_APP_ID` required" if @app_id.to_s.length == 0 + error_message << "Environment variable `ITC_USER` or `FASTLANE_USER` required" if @user.to_s.length == 0 + error_message << "Environment variable `ITC_PASSWORD` or `FASTLANE_PASSWORD`" if @password.to_s.length == 0 + raise error_message.join("\n") if error_message.length > 0 + + spaceship = Spaceship::Tunes.login(@user, @password) + spaceship.select_team + + @app ||= Spaceship::Tunes::Application.find(@app_id) + raise "Could not find app with ID #{app_id}" if @app.nil? + + if tester_group_names + test_flight_groups = Spaceship::TestFlight::Group.filter_groups(app_id: @app.apple_id) + test_flight_group_names = test_flight_groups.map(&:name).to_set + tester_group_names.select do |group_name| + next if test_flight_group_names.include?(group_name) + error_message << "TestFlight missing group `#{group_name}`, You need to first create this group in iTunes Connect." + end end - def testing_is_live? - app.build_trains.each do |version, train| - if train.external_testing_enabled - train.builds.each do |build| - return true if build.external_testing_enabled - end - end + raise error_message.join("\n") if error_message.length > 0 + end + + def testing_is_live? + app.build_trains.each do |version, train| + next unless train.external_testing_enabled + train.builds.each do |build| + return true if build.external_testing_enabled end - return false end + return false + end end