Refactoring controllers (#546)

* include byebug in test group

* refactor feeds controller

* refactor tags and maps controllers

* refactor images controller

* increase maps controller test coverage

Gemfile

@@ -42,11 +42,14 @@ end
 group :test do
   gem 'simplecov', require: false
   gem 'simplecov-cobertura', require: false
-  gem 'test-unit'    
+  gem 'test-unit'
+end
+
+group :development, :test do
+  gem "byebug"
 end
 
 group :development do
-  gem "byebug"
   gem "jshintrb"
   gem "therubyracer"
 end

app/controllers/application_controller.rb

@@ -41,4 +41,11 @@ class ApplicationController < ActionController::Base
       return false
     end
   end
+
+  def save_tags(map)
+    return unless params[:tags].present?
+    params[:tags].tr(' ', ',').split(',').each do |tagname|
+      map.add_tag(tagname.strip, current_user)
+    end
+  end
 end

app/controllers/feeds_controller.rb

@@ -1,4 +1,6 @@
 class FeedsController < ApplicationController
+  before_filter :query, only: %i[clean license]
+
   def all
     # (Warpable.all + Map.all).sort_by(&:created_at)
     @maps = Map.find(:all, order: 'id DESC', limit: 20,
@@ -10,21 +12,12 @@ class FeedsController < ApplicationController
   end
 
   def clean
-    @maps = Map.order(id: :desc)
-               .limit(20)
-               .where(archived: false, password: '')
-               .joins(:warpables)
-               .group('maps.id')
     render layout: false, template: 'feeds/clean'
     response.headers['Content-Type'] = 'application/xml; charset=utf-8'
   end
 
   def license
-    @maps = Map.order(id: :desc)
-               .limit(20)
-               .where(archived: false, password: '', license: params[:id])
-               .joins(:warpables)
-               .group('maps.id')
+    @maps = @maps.where(license: params[:id])
     render layout: false, template: 'feeds/license'
     response.headers['Content-Type'] = 'application/xml; charset=utf-8'
   end
@@ -45,12 +38,20 @@ class FeedsController < ApplicationController
 
   def tag
     @tag = Tag.find_by_name params[:id]
-    if @tag
-      @maps = @tag.maps.paginate(page: params[:page], per_page: 24)
-      render layout: false, template: 'feeds/tag'
-      response.headers['Content-Type'] = 'application/xml; charset=utf-8'
-    else
-      render text: "No maps with tag #{params[:id]}"
-    end
+    @maps = @tag.maps.paginate(page: params[:page], per_page: 24)
+    render layout: false, template: 'feeds/tag'
+    response.headers['Content-Type'] = 'application/xml; charset=utf-8'
+  rescue NoMethodError
+    render text: "No maps with tag #{params[:id]}"
+  end
+
+  private
+
+  def query
+    @maps = Map.order(id: :desc)
+               .limit(20)
+               .where(archived: false, password: '')
+               .joins(:warpables)
+               .group('maps.id')
   end
 end

app/controllers/images_controller.rb

@@ -1,10 +1,8 @@
 require 'open-uri'
 class ImagesController < ApplicationController
-  # avoid raising exceptions for common errors (e.g. file not found)
-  rescue_from Errno::ENOENT, with: :url_upload_not_found
-  rescue_from Errno::ETIMEDOUT, with: :url_upload_not_found
-  rescue_from OpenURI::HTTPError, with: :url_upload_not_found
-  rescue_from Timeout::Error, with: :url_upload_not_found
+  rescue_from Errno::ENOENT, Errno::ETIMEDOUT,
+              OpenURI::HTTPError, Timeout::Error,
+              with: :url_upload_not_found
   protect_from_forgery except: %i[update delete]
   # Convert model to json without including root name. Eg. 'warpable'
   ActiveRecord::Base.include_root_in_json = false
@@ -24,6 +22,7 @@ class ImagesController < ApplicationController
   end
 
   # rubocop:disable LineLength
+  # assign attributes directly after rails update
   def create
     @warpable = Warpable.new
     @warpable.image = params[:uploaded_data]
@@ -60,7 +59,6 @@ class ImagesController < ApplicationController
     end
   end
 
-  # is this used?
   def url_upload_not_found
     flash[:notice] = 'Sorry, the URL you provided was not valid.'
     redirect_to '/map/edit/' + params[:id]
@@ -69,7 +67,7 @@ class ImagesController < ApplicationController
   def show
     @image = Warpable.find params[:id]
     respond_to do |format|
-      format.html # show.html.erb
+      format.html
       format.json { render json: @image.map(&:fup_json) }
     end
   end

app/controllers/maps_controller.rb

@@ -58,15 +58,6 @@ class MapsController < ApplicationController
   def show
     @map.zoom ||= 12
 
-    # stuff for Sparklines resolution graph;
-    # messy, could tuck into model
-    # hist = @map.images_histogram
-    # (0..100).each do |i|
-    # hist[i] = 0 if !hist[i]
-    # end
-    # hist = hist[0..100]
-    # @images_histogram = @map.grouped_images_histogram((hist.length/15).to_i+1)
-
     # this is used for the resolution slider
     @resolution = @map.average_cm_per_pixel.round(4)
     @resolution = 5 if @resolution < 5 # soft-set min res
@@ -111,13 +102,7 @@ class MapsController < ApplicationController
     @map.description = params[:map][:description]
     @map.license =     params[:map][:license] if @map.user_id == current_user.id
 
-    # save new tags
-    if params[:tags]
-      params[:tags].tr(' ', ',').split(',').each do |tagname|
-        @map.add_tag(tagname.strip, current_user)
-      end
-    end
-
+    save_tags(@map)
     @map.save
     redirect_to action: 'show'
   end

app/controllers/tags_controller.rb

@@ -5,16 +5,7 @@ class TagsController < ApplicationController
     @map = Map.find params[:map_id]
 
     if logged_in?
-      # there is identical code in MapsController#update.
-      # TODO: DRY up this functionality.
-
-      # save new tags
-      if params[:tags]
-        params[:tags].tr(' ', ',').split(',').each do |tagname|
-          @map.add_tag(tagname.strip, current_user)
-        end
-      end
-
+      save_tags(@map)
       redirect_to '/maps/' + @map.slug
     else
       flash[:error] = 'You must be logged in to add tags'
@@ -29,11 +20,10 @@ class TagsController < ApplicationController
     render template: 'maps/index'
   end
 
-  # rubocop:disable LineLength
   def destroy
     @tag = Tag.find(params[:id])
 
-    if logged_in? && (@tag.user_id.to_i == current_user.id || current_user.role == 'admin')
+    if logged_in? && current_user.can_delete?(@tag)
       @tag.delete
       flash[:notice] = 'Tag ' + @tag.name + ' deleted.'
       redirect_to @tag.map
@@ -42,5 +32,4 @@ class TagsController < ApplicationController
       redirect_to '/login'
     end
   end
-  # rubocop:enable LineLength
 end

test/fixtures/maps.yml

@@ -11,6 +11,7 @@ saugus:
   created_at: <%= Time.now %>
   license: publicdomain
   user_id: 1
+  author: quentin
 
 cubbon:
   id: 2
@@ -23,6 +24,7 @@ cubbon:
   created_at: <%= Time.now %>
   license: publicdomain
   user_id: 1
+  author: aaron
 
 nairobi:
   id: 3
@@ -35,6 +37,7 @@ nairobi:
   created_at: <% Time.now %>
   license: publicdomain
   user_id: 2
+  author: aaron
 
 village:
   id: 4
@@ -47,3 +50,4 @@ village:
   created_at: <% Time.now %>
   license: publicdomain
   user_id: 2
+  author: aaron

test/fixtures/users.yml

@@ -6,28 +6,22 @@ quentin:
   created_at:                <%= 5.days.ago.to_s :db  %>
   remember_token_expires_at: <%= 1.days.from_now.to_s :db %>
   remember_token:            77de68daecd823babbb58edb1c8e14d7106e83bb
-      
+
 admin:
   id:                        2
   role:                      "admin"
   login:                     aaron
   email:                     aaron@example.com
   created_at:                <%= 1.days.ago.to_s :db %>
-  remember_token_expires_at: 
-  remember_token:            
 
 chris:
   id:                        3
   login:                     chris
   email:                     chris@example.com
   created_at:                <%= 3.days.ago.to_s :db %>
-  remember_token_expires_at: 
-  remember_token:            
 
 joshua:
   id:                        4
   login:                     joshua
   email:                     joshua@example.com
   created_at:                <%= 3.days.ago.to_s :db %>
-  remember_token_expires_at: 
-  remember_token:            

test/functional/feeds_controller_test.rb

@@ -6,7 +6,7 @@ class FeedsControllerTest < ActionController::TestCase
   def setup
     @map = maps(:saugus)
     @tag = tags(:nice)
-  end 
+  end
 
   test "should get main feed (all)" do
     get :all
@@ -39,10 +39,16 @@ class FeedsControllerTest < ActionController::TestCase
   end
 
   test "should get tag feed" do
-    get :tag, :id => "nice"
+    get :tag, id: 'nice'
     assert_response :success
     assert_not_nil :tag
     assert_not_nil :maps
+    assert_template 'feeds/tag'
   end
 
+  test 'rescues if tag not present' do
+    get :tag, id: 'cess'
+    assert_equal 'text/html', @response.content_type
+    assert_equal 'No maps with tag cess', @response.body
+  end
 end

test/functional/maps_controller_test.rb

@@ -5,7 +5,7 @@ class MapsControllerTest < ActionController::TestCase
   # called before every single test
   def setup
     @map = maps(:saugus)
-  end 
+  end
 
   # called after every single test
   def teardown
@@ -50,6 +50,7 @@ class MapsControllerTest < ActionController::TestCase
   test "should get map of maps" do
     get :map
     assert_response :success
+    assert_includes @response.body, @map.slug
   end
 
   test "should get new" do
@@ -107,7 +108,7 @@ class MapsControllerTest < ActionController::TestCase
   test "should create map if not logged in" do
     before_count = Map.count
     post(:create, map: {
-      name: "Coal terminal map", 
+      name: "Coal terminal map",
       slug: "coal-terminal",
       location: "London",
       lat: 42.43823313018592,
@@ -119,13 +120,34 @@ class MapsControllerTest < ActionController::TestCase
     assert_not_equal before_count, Map.count
     assert Map.all.collect(&:name).include?("Coal terminal map")
     assert_nil @map.user
+    assert_equal 'anonymous', @map.author
+  end
+
+  test 'assigns current user as map author if logged in' do
+    before_count = Map.count
+    user = users(:quentin)
+    session[:user_id] = user.id
+    post :create, map: {
+      name: 'Yaya Center',
+      slug: 'yaya-center',
+      location: 'Nairobi',
+      lat: -0.3030988,
+      lon: 36.080026
+    }
+    @map = assigns(:map)
+
+    assert_redirected_to '/maps/'+@map.slug
+    assert_not_equal before_count, Map.count
+    assert Map.all.collect(&:name).include?('Yaya Center')
+    assert_equal user, @map.user
+    assert_equal user.login, @map.author
   end
 
   test "should render new if map not created" do
     session[:user_id] = 1
     before_count = Map.count
     post(:create, map: {
-      name: "Coal terminal map", 
+      name: "Coal terminal map",
       slug: "coal-terminal"
     })
     @map = assigns(:map)
@@ -203,6 +225,11 @@ class MapsControllerTest < ActionController::TestCase
     assert @map.has_tag("bears")
   end
 
+  test 'should not update unless logged in' do
+    put :update, id: 2, map: { name: 'Street 5'}
+    assert_redirected_to '/login?back_to=/map/2'
+  end
+
   test "should display maps by region"do
     get :region , { minlat: 40, maxlat: 50, minlon: -80, maxlon: -60}
     @maps = assigns(:maps)
@@ -228,4 +255,36 @@ class MapsControllerTest < ActionController::TestCase
     assert_response :success
     assert_equal 'application/json', response.content_type
   end
+
+  def export_if_logged_in
+    Rails.stub(:env, ActiveSupport::StringInquirer.new('development')) do
+      session[:user_id] = 1
+      post :export, id: @map.id, resolution: 5
+      assert_response :success
+      assert_equal 'complete', @response.body
+    end
+  end
+
+  def export_anonmymous_map
+    Rails.stub(:env, ActiveSupport::StringInquirer.new('development')) do
+      map = maps(:cubbon)
+      post :export, id: map.id, resolution: 5
+      assert_response :success
+    end
+  end
+
+  test 'returns the exports' do
+    get :exports, id: 1
+    assert_response :success
+  end
+
+  test 'license' do
+    get :license
+    assert_response :success
+  end
+
+  test 'featured' do
+    get :featured
+    assert_response :success
+  end
 end