Merge pull request #652 from metamaps/feature/explore-controller

refactor map controller

app/controllers/explore_controller.rb

@@ -0,0 +1,115 @@
+# frozen_string_literal: true
+class ExploreController < ApplicationController
+  before_action :authorize_explore
+  after_action :verify_authorized
+  after_action :verify_policy_scoped
+
+  respond_to :html, :json, :csv
+
+  # TODO: remove?
+  # autocomplete :map, :name, full: true, extra_data: [:user_id]
+
+  # GET /explore/active
+  def active
+    page = params[:page].present? ? params[:page] : 1
+    @maps = policy_scope(Map).order('updated_at DESC')
+                             .page(page).per(20)
+
+    respond_to do |format|
+      format.html do
+        # root url => main/home. main/home renders maps/activemaps view.
+        redirect_to(root_url) && return if authenticated?
+        respond_with(@maps, @user)
+      end
+      format.json { render json: @maps.to_json }
+    end
+  end
+
+  # GET /explore/featured
+  def featured
+    page = params[:page].present? ? params[:page] : 1
+    @maps = policy_scope(
+      Map.where('maps.featured = ? AND maps.permission != ?',
+                true, 'private')
+    ).order('updated_at DESC').page(page).per(20)
+
+    respond_to do |format|
+      format.html { respond_with(@maps, @user) }
+      format.json { render json: @maps.to_json }
+    end
+  end
+
+  # GET /explore/mine
+  def mine
+    unless authenticated?
+      skip_policy_scope
+      return redirect_to explore_active_path
+    end
+
+    page = params[:page].present? ? params[:page] : 1
+    @maps = policy_scope(
+      Map.where('maps.user_id = ?', current_user.id)
+    ).order('updated_at DESC').page(page).per(20)
+
+    respond_to do |format|
+      format.html { respond_with(@maps, @user) }
+      format.json { render json: @maps.to_json }
+    end
+  end
+
+  # GET /explore/shared
+  def shared
+    unless authenticated?
+      skip_policy_scope
+      return redirect_to explore_active_path
+    end
+
+    page = params[:page].present? ? params[:page] : 1
+    @maps = policy_scope(
+      Map.where('maps.id IN (?)', current_user.shared_maps.map(&:id))
+    ).order('updated_at DESC').page(page).per(20)
+
+    respond_to do |format|
+      format.html { respond_with(@maps, @user) }
+      format.json { render json: @maps.to_json }
+    end
+  end
+
+  # GET /explore/starred
+  def starred
+    unless authenticated?
+      skip_policy_scope
+      return redirect_to explore_active_path
+    end
+
+    page = params[:page].present? ? params[:page] : 1
+    stars = current_user.stars.map(&:map_id)
+    @maps = policy_scope(
+      Map.where('maps.id IN (?)', stars)
+    ).order('updated_at DESC').page(page).per(20)
+
+    respond_to do |format|
+      format.html { respond_with(@maps, @user) }
+      format.json { render json: @maps.to_json }
+    end
+  end
+
+  # GET /explore/mapper/:id
+  def mapper
+    page = params[:page].present? ? params[:page] : 1
+    @user = User.find(params[:id])
+    @maps = policy_scope(Map.where(user: @user))
+            .order('updated_at DESC').page(page).per(20)
+
+    respond_to do |format|
+      format.html { respond_with(@maps, @user) }
+      format.json { render json: @maps.to_json }
+    end
+  end
+
+  private
+
+  def authorize_explore
+    authorize :Explore
+  end
+end

app/controllers/main_controller.rb

@@ -17,7 +17,7 @@ class MainController < ApplicationController
         if !authenticated?
           render 'main/home'
         else
-          render 'maps/activemaps'
+          render 'explore/active'
         end
       end
     end

app/controllers/maps_controller.rb

@@ -1,108 +1,30 @@
 # frozen_string_literal: true
 class MapsController < ApplicationController
-  before_action :require_user, only: [:create, :update, :access, :star, :unstar, :screenshot, :events, :destroy]
-  after_action :verify_authorized, except: [:activemaps, :featuredmaps, :mymaps, :sharedmaps, :starredmaps, :usermaps]
-  after_action :verify_policy_scoped, only: [:activemaps, :featuredmaps, :mymaps, :sharedmaps, :starredmaps, :usermaps]
+  before_action :require_user, only: [:create, :update, :destroy, :access, :events, :screenshot, :star, :unstar]
+  before_action :set_map, only: [:show, :update, :destroy, :access, :contains, :events, :export, :screenshot, :star, :unstar]
+  after_action :verify_authorized
 
   respond_to :html, :json, :csv
 
   autocomplete :map, :name, full: true, extra_data: [:user_id]
 
-  # GET /explore/active
-  def activemaps
-    page = params[:page].present? ? params[:page] : 1
-    @maps = policy_scope(Map).order('updated_at DESC')
-                             .page(page).per(20)
-
+  # GET maps/:id
+  def show
     respond_to do |format|
       format.html do
-        # root url => main/home. main/home renders maps/activemaps view.
-        redirect_to(root_url) && return if authenticated?
-        respond_with(@maps, @user)
+        @allmappers = @map.contributors
+        @allcollaborators = @map.editors
+        @alltopics = policy_scope(@map.topics)
+        @allsynapses = policy_scope(@map.synapses)
+        @allmappings = policy_scope(@map.mappings)
+        @allmessages = @map.messages.sort_by(&:created_at)
+        @allstars = @map.stars
+
+        respond_with(@allmappers, @allcollaborators, @allmappings, @allsynapses, @alltopics, @allmessages, @allstars, @map)
       end
-      format.json { render json: @maps.to_json }
-    end
-  end
-
-  # GET /explore/featured
-  def featuredmaps
-    page = params[:page].present? ? params[:page] : 1
-    @maps = policy_scope(
-      Map.where('maps.featured = ? AND maps.permission != ?',
-                true, 'private')
-    ).order('updated_at DESC').page(page).per(20)
-
-    respond_to do |format|
-      format.html { respond_with(@maps, @user) }
-      format.json { render json: @maps.to_json }
-    end
-  end
-
-  # GET /explore/mine
-  def mymaps
-    unless authenticated?
-      skip_policy_scope
-      return redirect_to explore_active_path
-    end
-
-    page = params[:page].present? ? params[:page] : 1
-    @maps = policy_scope(
-      Map.where('maps.user_id = ?', current_user.id)
-    ).order('updated_at DESC').page(page).per(20)
-
-    respond_to do |format|
-      format.html { respond_with(@maps, @user) }
-      format.json { render json: @maps.to_json }
-    end
-  end
-
-  # GET /explore/shared
-  def sharedmaps
-    unless authenticated?
-      skip_policy_scope
-      return redirect_to explore_active_path
-    end
-
-    page = params[:page].present? ? params[:page] : 1
-    @maps = policy_scope(
-      Map.where('maps.id IN (?)', current_user.shared_maps.map(&:id))
-    ).order('updated_at DESC').page(page).per(20)
-
-    respond_to do |format|
-      format.html { respond_with(@maps, @user) }
-      format.json { render json: @maps.to_json }
-    end
-  end
-
-  # GET /explore/starred
-  def starredmaps
-    unless authenticated?
-      skip_policy_scope
-      return redirect_to explore_active_path
-    end
-
-    page = params[:page].present? ? params[:page] : 1
-    stars = current_user.stars.map(&:map_id)
-    @maps = policy_scope(
-      Map.where('maps.id IN (?)', stars)
-    ).order('updated_at DESC').page(page).per(20)
-
-    respond_to do |format|
-      format.html { respond_with(@maps, @user) }
-      format.json { render json: @maps.to_json }
-    end
-  end
-
-  # GET /explore/mapper/:id
-  def usermaps
-    page = params[:page].present? ? params[:page] : 1
-    @user = User.find(params[:id])
-    @maps = policy_scope(Map.where(user: @user))
-            .order('updated_at DESC').page(page).per(20)
-
-    respond_to do |format|
-      format.html { respond_with(@maps, @user) }
-      format.json { render json: @maps.to_json }
+      format.json { render json: @map }
+      format.csv { redirect_to action: :export, format: :csv }
+      format.xls { redirect_to action: :export, format: :xls }
     end
   end
 
@@ -120,34 +42,79 @@ class MapsController < ApplicationController
     end
   end
 
-  # GET maps/:id
-  def show
-    @map = Map.find(params[:id])
+  # POST maps
+  def create
+    @user = current_user
+    @map = Map.new(create_map_params)
+    @map.user = @user
+    @map.arranged = false
+
+    if params[:topicsToMap].present?
+      create_topics!
+      create_synapses! if params[:synapsesToMap].present?
+      @map.arranged = true
+    end
+
     authorize @map
 
     respond_to do |format|
-      format.html do
-        @allmappers = @map.contributors
-        @allcollaborators = @map.editors
-        @alltopics = @map.topics.to_a.delete_if { |t| !policy(t).show? }
-        @allsynapses = @map.synapses.to_a.delete_if { |s| !policy(s).show? }
-        @allmappings = @map.mappings.to_a.delete_if { |m| !policy(m).show? }
-        @allmessages = @map.messages.sort_by(&:created_at)
-        @allstars = @map.stars
-
-        respond_with(@allmappers, @allcollaborators, @allmappings, @allsynapses, @alltopics, @allmessages, @allstars, @map)
+      if @map.save
+        format.json { render json: @map }
+      else
+        format.json { render json: 'invalid params' }
       end
-      format.json { render json: @map }
-      format.csv { redirect_to action: :export, format: :csv }
-      format.xls { redirect_to action: :export, format: :xls }
+    end
+  end
+
+  # PUT maps/:id
+  def update
+    respond_to do |format|
+      if @map.update_attributes(update_map_params)
+        format.json { head :no_content }
+      else
+        format.json { render json: @map.errors, status: :unprocessable_entity }
+      end
+    end
+  end
+
+  # DELETE maps/:id
+  def destroy
+    @map.delete
+
+    respond_to do |format|
+      format.json do
+        head :no_content
+      end
+    end
+  end
+
+  # POST maps/:id/access
+  def access
+    user_ids = params[:access] || []
+
+    added = @map.add_new_collaborators(user_ids)
+    added.each do |user_id|
+      MapMailer.invite_to_edit_email(@map, current_user, User.find(user_id)).deliver_later
+    end
+    @map.remove_old_collaborators(user_ids)
+
+    respond_to do |format|
+      format.json do
+        render json: { message: 'Successfully altered edit permissions' }
+      end
+    end
+  end
+
+  # GET maps/:id/contains
+  def contains
+    respond_to do |format|
+      format.json { render json: @map.contains(current_user) }
     end
   end
 
   # GET maps/:id/export
   def export
-    map = Map.find(params[:id])
-    authorize map
-    exporter = MapExportService.new(current_user, map)
+    exporter = MapExportService.new(current_user, @map)
     respond_to do |format|
       format.json { render json: exporter.json }
       format.csv { send_data exporter.csv }
@@ -157,156 +124,37 @@ class MapsController < ApplicationController
 
   # POST maps/:id/events/:event
   def events
-    map = Map.find(params[:id])
-    authorize map
-
     valid_event = false
     if params[:event] == 'conversation'
-      Events::ConversationStartedOnMap.publish!(map, current_user)
+      Events::ConversationStartedOnMap.publish!(@map, current_user)
       valid_event = true
     elsif params[:event] == 'user_presence'
-      Events::UserPresentOnMap.publish!(map, current_user)
+      Events::UserPresentOnMap.publish!(@map, current_user)
       valid_event = true
     end
 
     respond_to do |format|
       format.json do
-        head :ok if valid_event
         head :bad_request unless valid_event
+        head :ok
       end
     end
   end
 
-  # GET maps/:id/contains
-  def contains
-    @map = Map.find(params[:id])
-    authorize @map
-
-    @allmappers = @map.contributors
-    @allcollaborators = @map.editors
-    @alltopics = @map.topics.to_a.delete_if { |t| !policy(t).show? }
-    @allsynapses = @map.synapses.to_a.delete_if { |s| !policy(s).show? }
-    @allmappings = @map.mappings.to_a.delete_if { |m| !policy(m).show? }
-
-    @json = {}
-    @json['map'] = @map
-    @json['topics'] = @alltopics
-    @json['synapses'] = @allsynapses
-    @json['mappings'] = @allmappings
-    @json['mappers'] = @allmappers
-    @json['collaborators'] = @allcollaborators
-    @json['messages'] = @map.messages.sort_by(&:created_at)
-    @json['stars'] = @map.stars
-
-    respond_to do |format|
-      format.json { render json: @json }
-    end
-  end
-
-  # POST maps
-  def create
-    @user = current_user
-    @map = Map.new
-    @map.name = params[:name]
-    @map.desc = params[:desc]
-    @map.permission = params[:permission]
-    @map.user = @user
-    @map.arranged = false
-
-    if params[:topicsToMap]
-      @all = params[:topicsToMap]
-      @all = @all.split(',')
-      @all.each do |topic|
-        topic = topic.split('/')
-        mapping = Mapping.new
-        mapping.map = @map
-        mapping.user = @user
-        mapping.mappable = Topic.find(topic[0])
-        mapping.xloc = topic[1]
-        mapping.yloc = topic[2]
-        authorize mapping, :create?
-        mapping.save
-      end
-
-      if params[:synapsesToMap]
-        @synAll = params[:synapsesToMap]
-        @synAll = @synAll.split(',')
-        @synAll.each do |synapse_id|
-          mapping = Mapping.new
-          mapping.map = @map
-          mapping.user = @user
-          mapping.mappable = Synapse.find(synapse_id)
-          authorize mapping, :create?
-          mapping.save
-        end
-      end
-
-      @map.arranged = true
-    end
-
-    authorize @map
+  # POST maps/:id/upload_screenshot
+  def screenshot
+    @map.base64_screenshot(params[:encoded_image])
 
     if @map.save
-      respond_to do |format|
-        format.json { render json: @map }
-      end
+      render json: { message: 'Successfully uploaded the map screenshot.' }
     else
-      respond_to do |format|
-        format.json { render json: 'invalid params' }
-      end
-    end
-  end
-
-  # PUT maps/:id
-  def update
-    @map = Map.find(params[:id])
-    authorize @map
-
-    respond_to do |format|
-      if @map.update_attributes(map_params)
-        format.json { head :no_content }
-      else
-        format.json { render json: @map.errors, status: :unprocessable_entity }
-      end
-    end
-  end
-
-  # POST maps/:id/access
-  def access
-    @map = Map.find(params[:id])
-    authorize @map
-    userIds = params[:access] || []
-    added = userIds.select do |uid|
-      user = User.find(uid)
-      if user.nil? || (current_user && user == current_user)
-        false
-      else
-        !@map.collaborators.include?(user)
-      end
-    end
-    removed = @map.collaborators.select { |user| !userIds.include?(user.id.to_s) }.map(&:id)
-    added.each do |uid|
-      UserMap.create(user_id: uid.to_i, map_id: @map.id)
-      user = User.find(uid.to_i)
-      MapMailer.invite_to_edit_email(@map, current_user, user).deliver_later
-    end
-    removed.each do |uid|
-      @map.user_maps.select { |um| um.user_id == uid }.each(&:destroy)
-    end
-
-    respond_to do |format|
-      format.json do
-        render json: { message: 'Successfully altered edit permissions' }
-      end
+      render json: { message: 'Failed to upload image.' }
     end
   end
 
   # POST maps/:id/star
   def star
-    @map = Map.find(params[:id])
-    authorize @map
-    star = Star.find_by_map_id_and_user_id(@map.id, current_user.id)
-    star = Star.create(map_id: @map.id, user_id: current_user.id) unless star
+    star = Star.find_or_create_by(map_id: @map.id, user_id: current_user.id)
 
     respond_to do |format|
       format.json do
@@ -317,9 +165,7 @@ class MapsController < ApplicationController
 
   # POST maps/:id/unstar
   def unstar
-    @map = Map.find(params[:id])
-    authorize @map
-    star = Star.find_by_map_id_and_user_id(@map.id, current_user.id)
+    star = Star.find_by(map_id: @map.id, user_id: current_user.id)
     star&.delete
 
     respond_to do |format|
@@ -329,44 +175,47 @@ class MapsController < ApplicationController
     end
   end
 
-  # POST maps/:id/upload_screenshot
-  def screenshot
-    @map = Map.find(params[:id])
-    authorize @map
-
-    png = Base64.decode64(params[:encoded_image]['data:image/png;base64,'.length..-1])
-    StringIO.open(png) do |data|
-      data.class.class_eval { attr_accessor :original_filename, :content_type }
-      data.original_filename = 'map-' + @map.id.to_s + '-screenshot.png'
-      data.content_type = 'image/png'
-      @map.screenshot = data
-    end
-
-    if @map.save
-      render json: { message: 'Successfully uploaded the map screenshot.' }
-    else
-      render json: { message: 'Failed to upload image.' }
-    end
-  end
-
-  # DELETE maps/:id
-  def destroy
-    @map = Map.find(params[:id])
-    authorize @map
-
-    @map.delete
-
-    respond_to do |format|
-      format.json do
-        head :no_content
-      end
-    end
-  end
-
   private
 
-  # Never trust parameters from the scary internet, only allow the white list through.
-  def map_params
+  def set_map
+    @map = Map.find(params[:id])
+    authorize @map
+  end
+
+  def create_map_params
+    params.permit(:name, :desc, :permission)
+  end
+
+  def update_map_params
     params.require(:map).permit(:id, :name, :arranged, :desc, :permission)
   end
+
+  def create_topics!
+    topics = params[:topicsToMap]
+    topics = topics.split(',')
+    topics.each do |topic|
+      topic = topic.split('/')
+      mapping = Mapping.new
+      mapping.map = @map
+      mapping.user = @user
+      mapping.mappable = Topic.find(topic[0])
+      mapping.xloc = topic[1]
+      mapping.yloc = topic[2]
+      authorize mapping, :create?
+      mapping.save
+    end
+  end
+
+  def create_synapses!
+    @synAll = params[:synapsesToMap]
+    @synAll = @synAll.split(',')
+    @synAll.each do |synapse_id|
+      mapping = Mapping.new
+      mapping.map = @map
+      mapping.user = @user
+      mapping.mappable = Synapse.find(synapse_id)
+      authorize mapping, :create?
+      mapping.save
+    end
+  end
 end

app/models/map.rb

@@ -16,11 +16,12 @@ class Map < ApplicationRecord
   has_many :events, -> { includes :user }, as: :eventable, dependent: :destroy
 
   # This method associates the attribute ":image" with a file attachment
-  has_attached_file :screenshot, styles: {
-    thumb: ['188x126#', :png]
-    #:full => ['940x630#', :png]
-  },
-                                 default_url: 'https://s3.amazonaws.com/metamaps-assets/site/missing-map-white.png'
+  has_attached_file :screenshot,
+    styles: {
+      thumb: ['188x126#', :png]
+      #:full => ['940x630#', :png]
+    },
+    default_url: 'https://s3.amazonaws.com/metamaps-assets/site/missing-map-white.png'
 
   validates :name, presence: true
   validates :arranged, inclusion: { in: [true, false] }
@@ -31,7 +32,7 @@ class Map < ApplicationRecord
   validates_attachment_content_type :screenshot, content_type: /\Aimage\/.*\Z/
 
   def mappings
-    topicmappings + synapsemappings
+    topicmappings.or(synapsemappings)
   end
 
   def mk_permission
@@ -108,4 +109,49 @@ class Map < ApplicationRecord
     self.screenshot = data
     save
   end
+
+  # user param helps determine what records are visible
+  def contains(user)
+    {
+      map: self,
+      topics: Pundit.policy_scope(user, topics).to_a,
+      synapses: Pundit.policy_scope(user, synapses).to_a,
+      mappings: Pundit.policy_scope(user, mappings).to_a,
+      mappers: contributors,
+      collaborators: editors,
+      messages: messages.sort_by(&:created_at),
+      stars: stars
+    }
+  end
+
+  def add_new_collaborators(user_ids)
+    users = User.where(id: user_ids)
+    current_collaborators = collaborators + [user]
+    added = users.map do |new_user|
+      next nil if current_collaborators.include?(new_user)
+      UserMap.create(user_id: new_user.id, map_id: id)
+      new_user.id
+    end
+    added.compact
+  end
+
+  def remove_old_collaborators(user_ids)
+    current_collaborators = collaborators + [user]
+    removed = current_collaborators.map(&:id).map do |old_user_id|
+      next nil if user_ids.include?(old_user_id)
+      user_maps.where(user_id: old_user_id).find_each(&:destroy)
+      old_user_id
+    end
+    removed.compact
+  end
+
+  def base64_screenshot(encoded_image)
+    png = Base64.decode64(encoded_image['data:image/png;base64,'.length..-1])
+    StringIO.open(png) do |data|
+      data.class.class_eval { attr_accessor :original_filename, :content_type }
+      data.original_filename = 'map-' + @map.id.to_s + '-screenshot.png'
+      data.content_type = 'image/png'
+      @map.screenshot = data
+    end
+  end
 end

app/policies/explore_policy.rb

@@ -0,0 +1,25 @@
+class ExplorePolicy < ApplicationPolicy
+  def active?
+    true
+  end
+
+  def featured?
+    true
+  end
+
+  def mine?
+    true
+  end
+
+  def shared?
+    true
+  end
+
+  def starred?
+    true
+  end
+
+  def mapper?
+    true
+  end
+end

app/policies/map_policy.rb

@@ -41,10 +41,6 @@ class MapPolicy < ApplicationPolicy
     user.present? && record.user == user
   end
 
-  def activemaps?
-    user.blank? # redirect to root url if authenticated for some reason
-  end
-
   def contains?
     show?
   end
@@ -57,14 +53,6 @@ class MapPolicy < ApplicationPolicy
     show?
   end
 
-  def featuredmaps?
-    true
-  end
-
-  def mymaps?
-    user.present?
-  end
-
   def star?
     unstar?
   end
@@ -76,8 +64,4 @@ class MapPolicy < ApplicationPolicy
   def screenshot?
     update?
   end
-
-  def usermaps?
-    true
-  end
 end

config/routes.rb

@@ -57,12 +57,14 @@ Metamaps::Application.routes.draw do
   post 'maps/:id/star', to: 'maps#star', defaults: { format: :json }
   post 'maps/:id/unstar', to: 'maps#unstar', defaults: { format: :json }
 
-  get 'explore/active', to: 'maps#activemaps'
-  get 'explore/featured', to: 'maps#featuredmaps'
-  get 'explore/mine', to: 'maps#mymaps'
-  get 'explore/shared', to: 'maps#sharedmaps'
-  get 'explore/starred', to: 'maps#starredmaps'
-  get 'explore/mapper/:id', to: 'maps#usermaps'
+  namespace :explore do
+    get 'active'
+    get 'featured'
+    get 'mine'
+    get 'shared'
+    get 'starred'
+    get 'mapper/:id', action: 'mapper'
+  end
 
   devise_for :users, skip: :sessions, controllers: {
     registrations: 'users/registrations',

spec/controllers/explore_controller_spec.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+require 'rails_helper'
+
+RSpec.describe ExploreController, type: :controller do
+  before :each do
+    sign_in create(:user)
+  end
+
+  describe 'GET explore/active' do
+    context 'always returns an array' do
+      it 'with 0 records' do
+        Map.delete_all
+        get :active, format: :json
+        expect(JSON.parse(response.body)).to eq []
+      end
+      it 'with 1 record' do
+        map = create(:map)
+        get :active, format: :json
+        expect(JSON.parse(response.body).class).to be Array
+      end
+    end
+  end
+end

spec/controllers/maps_controller_spec.rb

@@ -9,21 +9,6 @@ RSpec.describe MapsController, type: :controller do
     sign_in create(:user)
   end
 
-  describe 'GET #activemaps' do
-    context 'always returns an array' do
-      it 'with 0 records' do
-        Map.delete_all
-        get :activemaps, format: :json
-        expect(JSON.parse(response.body)).to eq []
-      end
-      it 'with 1 record' do
-        map = create(:map)
-        get :activemaps, format: :json
-        expect(JSON.parse(response.body).class).to be Array
-      end
-    end
-  end
-
   describe 'POST #create' do
     context 'with valid params' do
       it 'creates a new Map' do