From 186129807e892ee09ea9c5de8686377c14517244 Mon Sep 17 00:00:00 2001 From: Devin Howard Date: Mon, 12 Dec 2016 22:28:10 -0500 Subject: [PATCH] fix spec, bugs, style --- app/controllers/access_controller.rb | 1 - app/controllers/application_controller.rb | 6 ++--- app/controllers/topics_controller.rb | 6 ++--- .../users/registrations_controller.rb | 5 +--- app/helpers/topics_helper.rb | 2 +- app/models/access_request.rb | 9 ++++--- app/models/map.rb | 12 ++++----- app/models/webhooks/slack/base.rb | 4 +-- app/policies/explore_policy.rb | 1 + app/policies/hack_policy.rb | 1 + app/policies/map_policy.rb | 2 +- app/policies/topic_policy.rb | 2 +- app/services/notification_service.rb | 1 - config/initializers/doorkeeper.rb | 8 +++--- config/initializers/rack-attack.rb | 25 +++++++++---------- config/locales/en.yml | 10 +++----- spec/api/v2/mappings_api_spec.rb | 1 - spec/api/v2/maps_api_spec.rb | 5 ++-- spec/api/v2/topics_api_spec.rb | 1 - spec/controllers/synapses_controller_spec.rb | 4 +-- spec/mailers/map_mailer_spec.rb | 5 ++-- spec/models/access_request_spec.rb | 7 +++--- 22 files changed, 54 insertions(+), 64 deletions(-) diff --git a/app/controllers/access_controller.rb b/app/controllers/access_controller.rb index a83fd128..2441aa62 100644 --- a/app/controllers/access_controller.rb +++ b/app/controllers/access_controller.rb @@ -6,7 +6,6 @@ class AccessController < ApplicationController :deny_access, :deny_access_post, :request_access] after_action :verify_authorized - # GET maps/:id/request_access def request_access @map = nil diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 4285682e..4bb5be10 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -22,7 +22,7 @@ class ApplicationController < ActionController::Base helper_method :admin? def handle_unauthorized - if authenticated? and params[:controller] == 'maps' and params[:action] == 'show' + if authenticated? && (params[:controller] == 'maps') && (params[:action] == 'show') redirect_to request_access_map_path(params[:id]) elsif authenticated? redirect_to root_path, notice: "You don't have permission to see that page." @@ -41,13 +41,13 @@ class ApplicationController < ActionController::Base def require_no_user return true unless authenticated? redirect_to edit_user_path(user), notice: 'You must be logged out.' - return false + false end def require_user return true if authenticated? redirect_to sign_in_path, notice: 'You must be logged in.' - return false + false end def require_admin diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index ea56059b..b54cc4f5 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -14,14 +14,14 @@ class TopicsController < ApplicationController @topics = policy_scope(Topic).where('LOWER("name") like ?', term.downcase + '%').order('"name"') @mapTopics = @topics.select { |t| t&.metacode&.name == 'Metamap' } # prioritize topics which point to maps, over maps - @exclude = @mapTopics.length > 0 ? @mapTopics.map(&:name) : [''] + @exclude = @mapTopics.length.positive? ? @mapTopics.map(&:name) : [''] @maps = policy_scope(Map).where('LOWER("name") like ? AND name NOT IN (?)', term.downcase + '%', @exclude).order('"name"') else @topics = [] @maps = [] end - @all= @topics.to_a.concat(@maps.to_a).sort { |a, b| a.name <=> b.name } - + @all = @topics.to_a.concat(@maps.to_a).sort_by(&:name) + render json: autocomplete_array_json(@all).to_json end diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index 7c211f26..44bcb2de 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -21,13 +21,10 @@ class Users::RegistrationsController < Devise::RegistrationsController end end - private def store_location - if params[:redirect_to] - store_location_for(User, params[:redirect_to]) - end + store_location_for(User, params[:redirect_to]) if params[:redirect_to] end def configure_sign_up_params diff --git a/app/helpers/topics_helper.rb b/app/helpers/topics_helper.rb index 58f53a6e..fa26095d 100644 --- a/app/helpers/topics_helper.rb +++ b/app/helpers/topics_helper.rb @@ -20,7 +20,7 @@ module TopicsHelper type: is_map ? metamapMetacode.name : t.metacode.name, typeImageURL: is_map ? metamapMetacode.icon : t.metacode.icon, mapCount: is_map ? 0 : t.maps.count, - synapseCount: is_map ? 0 : t.synapses.count, + synapseCount: is_map ? 0 : t.synapses.count } end end diff --git a/app/models/access_request.rb b/app/models/access_request.rb index e5416fff..fe68ce8f 100644 --- a/app/models/access_request.rb +++ b/app/models/access_request.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true class AccessRequest < ApplicationRecord belongs_to :user belongs_to :map @@ -5,7 +6,7 @@ class AccessRequest < ApplicationRecord def approve self.approved = true self.answered = true - self.save + save Mailboxer::Notification.where(notified_object: self).find_each do |notification| Mailboxer::Receipt.where(notification: notification).update_all(is_read: true) @@ -18,7 +19,7 @@ class AccessRequest < ApplicationRecord def deny self.approved = false self.answered = true - self.save + save Mailboxer::Notification.where(notified_object: self).find_each do |notification| Mailboxer::Receipt.where(notification: notification).update_all(is_read: true) @@ -26,10 +27,10 @@ class AccessRequest < ApplicationRecord end def requested_text - self.map.name + ' - request to edit' + map.name + ' - request to edit' end def approved_text - self.map.name + ' - access approved' + map.name + ' - access approved' end end diff --git a/app/models/map.rb b/app/models/map.rb index 5744b856..899992c6 100644 --- a/app/models/map.rb +++ b/app/models/map.rb @@ -18,11 +18,11 @@ class Map < ApplicationRecord # This method associates the attribute ":image" with a file attachment has_attached_file :screenshot, - styles: { - thumb: ['220x220#', :png] - #:full => ['940x630#', :png] - }, - default_url: 'https://s3.amazonaws.com/metamaps-assets/site/missing-map-square.png' + styles: { + thumb: ['220x220#', :png] + #:full => ['940x630#', :png] + }, + default_url: 'https://s3.amazonaws.com/metamaps-assets/site/missing-map-square.png' validates :name, presence: true validates :arranged, inclusion: { in: [true, false] } @@ -125,6 +125,6 @@ class Map < ApplicationRecord end def invited_text - self.name + ' - invited to edit' + name + ' - invited to edit' end end diff --git a/app/models/webhooks/slack/base.rb b/app/models/webhooks/slack/base.rb index 2274e32c..ba4e9ea5 100644 --- a/app/models/webhooks/slack/base.rb +++ b/app/models/webhooks/slack/base.rb @@ -14,9 +14,7 @@ Webhooks::Slack::Base = Struct.new(:webhook, :event) do 'something' end - def channel - webhook.channel - end + delegate :channel, to: :webhook def attachments [{ diff --git a/app/policies/explore_policy.rb b/app/policies/explore_policy.rb index b4d52fe5..ce17d4f4 100644 --- a/app/policies/explore_policy.rb +++ b/app/policies/explore_policy.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true class ExplorePolicy < ApplicationPolicy def active? true diff --git a/app/policies/hack_policy.rb b/app/policies/hack_policy.rb index b6fbf6ce..bdc9eaab 100644 --- a/app/policies/hack_policy.rb +++ b/app/policies/hack_policy.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true class HackPolicy < ApplicationPolicy def load_url_title? true diff --git a/app/policies/map_policy.rb b/app/policies/map_policy.rb index f670f59e..937d564c 100644 --- a/app/policies/map_policy.rb +++ b/app/policies/map_policy.rb @@ -16,7 +16,7 @@ class MapPolicy < ApplicationPolicy end def show? - record.permission.in?(['commons', 'public']) || + record.permission.in?(%w(commons public)) || record.collaborators.include?(user) || record.user == user end diff --git a/app/policies/topic_policy.rb b/app/policies/topic_policy.rb index 64463b4a..bc80f657 100644 --- a/app/policies/topic_policy.rb +++ b/app/policies/topic_policy.rb @@ -22,7 +22,7 @@ class TopicPolicy < ApplicationPolicy if record.defer_to_map.present? map_policy.show? else - record.permission.in?(['commons', 'public']) || record.user == user + record.permission.in?(%w(commons public)) || record.user == user end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 05a268f4..41202345 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -1,6 +1,5 @@ # frozen_string_literal: true class NotificationService - def self.renderer renderer ||= ApplicationController.renderer.new( http_host: ENV['MAILER_DEFAULT_URL'], diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 433b1c40..21b08bc2 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -9,20 +9,20 @@ Doorkeeper.configure do current_user else store_location_for(User, request.fullpath) - redirect_to(sign_in_url, notice: "Sign In to Connect") + redirect_to(sign_in_url, notice: 'Sign In to Connect') end end # If you want to restrict access to the web interface for adding oauth authorized applications, # you need to declare the block below. admin_authenticator do - if current_user && current_user.admin + if current_user&.admin current_user elsif current_user && !current_user.admin - redirect_to(root_url, notice: "Unauthorized") + redirect_to(root_url, notice: 'Unauthorized') else store_location_for(User, request.fullpath) - redirect_to(sign_in_url, notice: "Try signing in to do that") + redirect_to(sign_in_url, notice: 'Try signing in to do that') end end diff --git a/config/initializers/rack-attack.rb b/config/initializers/rack-attack.rb index 1cb90f0f..0fd76889 100644 --- a/config/initializers/rack-attack.rb +++ b/config/initializers/rack-attack.rb @@ -1,3 +1,4 @@ +# frozen_string_literal: true class Rack::Attack Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new @@ -11,10 +12,8 @@ class Rack::Attack # Throttle POST requests to /login by IP address # # Key: "rack::attack:#{Time.now.to_i/:period}:logins/ip:#{req.ip}" - throttle('logins/ip', :limit => 5, :period => 20.seconds) do |req| - if req.path == '/login' && req.post? - req.ip - end + throttle('logins/ip', limit: 5, period: 20.seconds) do |req| + req.ip if req.path == '/login' && req.post? end # Throttle POST requests to /login by email param @@ -25,17 +24,17 @@ class Rack::Attack # throttle logins for another user and force their login requests to be # denied, but that's not very common and shouldn't happen to you. (Knock # on wood!) - throttle("logins/email", :limit => 5, :period => 20.seconds) do |req| + throttle('logins/email', limit: 5, period: 20.seconds) do |req| if req.path == '/login' && req.post? # return the email if present, nil otherwise req.params['email'].presence end end - throttle('load_url_title/req/5mins/ip', :limit => 300, :period => 5.minutes) do |req| + throttle('load_url_title/req/5mins/ip', limit: 300, period: 5.minutes) do |req| req.ip if req.path == 'hacks/load_url_title' end - throttle('load_url_title/req/1s/ip', :limit => 5, :period => 1.second) do |req| + throttle('load_url_title/req/1s/ip', limit: 5, period: 1.second) do |req| # If the return value is truthy, the cache key for the return value # is incremented and compared with the limit. In this case: # "rack::attack:#{Time.now.to_i/1.second}:load_url_title/req/ip:#{req.ip}" @@ -46,16 +45,16 @@ class Rack::Attack end self.throttled_response = lambda do |env| - now = Time.now - match_data = env['rack.attack.match_data'] + now = Time.now + match_data = env['rack.attack.match_data'] period = match_data[:period] limit = match_data[:limit] - headers = { + headers = { 'X-RateLimit-Limit' => limit.to_s, - 'X-RateLimit-Remaining' => '0', - 'X-RateLimit-Reset' => (now + (period - now.to_i % period)).to_s - } + 'X-RateLimit-Remaining' => '0', + 'X-RateLimit-Reset' => (now + (period - now.to_i % period)).to_s + } [429, headers, ['']] end diff --git a/config/locales/en.yml b/config/locales/en.yml index 46d3db07..c4ada107 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1,8 +1,4 @@ -# Sample localization file for English. Add more files in this directory for other locales. -# See https://github.com/svenfuchs/rails-i18n/tree/master/rails%2Flocale for starting points. - en: - activerecord: - attributes: - user: - joinedwithcode: "Access code" + mailboxer: + notification_mailer: + subject: "%{subject}" diff --git a/spec/api/v2/mappings_api_spec.rb b/spec/api/v2/mappings_api_spec.rb index 6f225c6a..f8854e91 100644 --- a/spec/api/v2/mappings_api_spec.rb +++ b/spec/api/v2/mappings_api_spec.rb @@ -18,7 +18,6 @@ RSpec.describe 'mappings API', type: :request do it 'GET /api/v2/mappings/:id' do get "/api/v2/mappings/#{mapping.id}", params: { access_token: token } - expect(response).to have_http_status(:success) expect(response).to match_json_schema(:mapping) expect(JSON.parse(response.body)['data']['id']).to eq mapping.id diff --git a/spec/api/v2/maps_api_spec.rb b/spec/api/v2/maps_api_spec.rb index a7edeef2..fbf07903 100644 --- a/spec/api/v2/maps_api_spec.rb +++ b/spec/api/v2/maps_api_spec.rb @@ -1,4 +1,5 @@ -#t frozen_string_literal: true +# frozen_string_literal: true +# t frozen_string_literal: true require 'rails_helper' RSpec.describe 'maps API', type: :request do @@ -35,7 +36,7 @@ RSpec.describe 'maps API', type: :request do expect(response).to match_json_schema(:map) expect(JSON.parse(response.body)['data']['id']).to eq map.id end - + it 'POST /api/v2/maps' do post '/api/v2/maps', params: { map: map.attributes, access_token: token } diff --git a/spec/api/v2/topics_api_spec.rb b/spec/api/v2/topics_api_spec.rb index 31d93b87..ac2fb56a 100644 --- a/spec/api/v2/topics_api_spec.rb +++ b/spec/api/v2/topics_api_spec.rb @@ -18,7 +18,6 @@ RSpec.describe 'topics API', type: :request do it 'GET /api/v2/topics/:id' do get "/api/v2/topics/#{topic.id}" - expect(response).to have_http_status(:success) expect(response).to match_json_schema(:topic) expect(JSON.parse(response.body)['data']['id']).to eq topic.id diff --git a/spec/controllers/synapses_controller_spec.rb b/spec/controllers/synapses_controller_spec.rb index 511971ad..7abeb2ee 100644 --- a/spec/controllers/synapses_controller_spec.rb +++ b/spec/controllers/synapses_controller_spec.rb @@ -53,9 +53,9 @@ RSpec.describe SynapsesController, type: :controller do expect(response.status).to eq 422 end it 'does not create a synapse' do - expect { + expect do post :create, format: :json, params: { synapse: invalid_attributes } - }.to change { + end.to change { Synapse.count }.by 0 end diff --git a/spec/mailers/map_mailer_spec.rb b/spec/mailers/map_mailer_spec.rb index 5fed48f5..a920746b 100644 --- a/spec/mailers/map_mailer_spec.rb +++ b/spec/mailers/map_mailer_spec.rb @@ -1,10 +1,11 @@ +# frozen_string_literal: true require 'rails_helper' RSpec.describe MapMailer, type: :mailer do describe 'access_request_email' do - let(:request) { create(:access_request) } let(:map) { create(:map) } - let(:mail) { described_class.access_request_email(request, map) } + let(:request) { create(:access_request, map: map) } + let(:mail) { described_class.access_request_email(request) } it { expect(mail.from).to eq ['team@metamaps.cc'] } it { expect(mail.to).to eq [map.user.email] } diff --git a/spec/models/access_request_spec.rb b/spec/models/access_request_spec.rb index 98490bf7..e8db280b 100644 --- a/spec/models/access_request_spec.rb +++ b/spec/models/access_request_spec.rb @@ -1,8 +1,7 @@ +# frozen_string_literal: true require 'rails_helper' RSpec.describe AccessRequest, type: :model do - include ActiveJob::TestHelper # enqueued_jobs - let(:access_request) { create(:access_request) } describe 'approve' do @@ -13,7 +12,7 @@ RSpec.describe AccessRequest, type: :model do it { expect(access_request.approved).to be true } it { expect(access_request.answered).to be true } it { expect(UserMap.count).to eq 1 } - it { expect(enqueued_jobs.count).to eq 1 } + it { expect(Mailboxer::Notification.count).to eq 1 } end describe 'deny' do @@ -24,6 +23,6 @@ RSpec.describe AccessRequest, type: :model do it { expect(access_request.approved).to be false } it { expect(access_request.answered).to be true } it { expect(UserMap.count).to eq 0 } - it { expect(enqueued_jobs.count).to eq 0 } + it { expect(Mailboxer::Notification.count).to eq 0 } end end