- add some missing "dependent: :destroy" in Organization model: fixes #…

…359, removes all organization memberships and activity when destroying an org instance

- introduce OrganizationPolicy to properly define an access control strategy for Organizations

app/controllers/organizations_controller.rb

@@ -1,12 +1,14 @@
 class OrganizationsController < ApplicationController
-  before_filter :load_resource, only: [:show, :edit, :update, :destroy]
+  before_filter :load_resource, only: [:show, :edit, :update]
 
   def new
     @organization = Organization.new
+
+    authorize @organization
   end
 
   def index
-    @organizations = Organization.all
+    @organizations = Organization.all.page(params[:page]).per(25)
   end
 
   def show
@@ -21,6 +23,8 @@ def show
   def create
     @organization = Organization.new(organization_params)
 
+    authorize @organization
+
     if @organization.save
       redirect_to @organization, status: :created
     else
@@ -36,11 +40,6 @@ def update
     end
   end
 
-  def destroy
-    @organization.destroy
-    redirect_to organizations_path, notice: "deleted"
-  end
-
   def set_current
     if current_user
       session[:current_organization_id] = @organization.id
@@ -52,6 +51,8 @@ def set_current
 
   def load_resource
     @organization = Organization.find(params[:id])
+
+    authorize @organization
   end
 
   def organization_params

app/models/organization.rb

@@ -1,15 +1,15 @@
 class Organization < ActiveRecord::Base
   has_many :members, dependent: :destroy
   has_many :users, -> { order "members.created_at DESC" }, through: :members
-  has_many :all_accounts, class_name: "Account", inverse_of: :organization
+  has_many :all_accounts, class_name: "Account", inverse_of: :organization, dependent: :destroy
   has_many :all_movements, class_name: "Movement", through: :all_accounts, source: :movements
   has_many :all_transfers, class_name: "Transfer", through: :all_movements, source: :transfer
-  has_one :account, as: :accountable
+  has_one :account, as: :accountable, dependent: :destroy
   has_many :member_accounts, through: :members, source: :account
-  has_many :posts
+  has_many :posts, dependent: :destroy
   has_many :offers
   has_many :inquiries
-  has_many :documents, as: :documentable
+  has_many :documents, as: :documentable, dependent: :destroy
 
   validates :name, presence: true, uniqueness: true
 

app/models/transfer.rb

@@ -15,7 +15,7 @@ class Transfer < ActiveRecord::Base
 
   belongs_to :post
   belongs_to :operator, class_name: "User"
-  has_many :movements
+  has_many :movements, dependent: :destroy
 
   validate :different_source_and_destination
 

app/policies/organization_policy.rb

@@ -0,0 +1,19 @@
+class OrganizationPolicy < ApplicationPolicy
+  alias_method :organization, :record
+
+  def index?
+    true
+  end
+
+  def show?
+    user&.active?(organization)
+  end
+
+  def create?
+    user&.superadmin?
+  end
+
+  def update?
+    user&.admins?(organization)
+  end
+end

app/views/organizations/index.html.erb

@@ -27,20 +27,16 @@
         <td><%= link_to org.name, org %></td>
         <td><%= org.users.count %></td>
         <td class="hover-actions">
-          <% if current_user.admins?(org) %>
+          <% if current_user&.admins?(org) %>
             <%= link_to edit_organization_path(org), class: 'action' do %>
               <%= glyph :pencil %>
               <%= t 'global.edit' %>
             <% end %>
-            <%= link_to organization_path(org),
-                        data: { method: :delete },
-                        class: 'action' do %>
-              <%= glyph :trash %>
-              <%= t 'global.borrar' %>
-            <% end %>
           <% end %>
         </td>
       </tr>
     <% end %>
   </tbody>
 </table>
+
+<%= paginate @organizations %>

app/views/organizations/show.html.erb

@@ -100,15 +100,6 @@
           <% end %>
         </li>
       <% end %>
-      <% if superadmin? %>
-        <li>
-          <%= link_to organization_path(@organization),
-                      data: { method: :delete, confirm: t("are_you_sure") } do %>
-            <%= glyph :trash %>
-            <%= t "global.delete" %>
-          <% end %>
-        </li>
-      <% end %>
       <li>
         <%= link_to new_transfer_path(id: @organization, destination_account_id: @organization.account.id) do %>
           <%= glyph :time %>

config/routes.rb

@@ -33,7 +33,7 @@
     get :give_time, on: :member
   end
 
-  resources :organizations, concerns: :accountable do
+  resources :organizations, concerns: :accountable, except: :destroy do
     member do
       post :set_current
     end

spec/controllers/organizations_controller_spec.rb

@@ -3,8 +3,11 @@
 describe OrganizationsController do
   describe '#show' do
     let(:organization) { Fabricate(:organization) }
+    let(:member) { Fabricate(:member, organization: organization) }
 
     it 'links to new_transfer_path' do
+      login(member.user)
+
       get 'show', id: organization.id
       expect(response.body).to include(
         "<a href=\"/transfers/new?destination_account_id=#{organization.account.id}&amp;id=#{organization.id}\">"