Merge pull request #368 from coopdevs/delete_org

sauloperez · web-flow · commit a47597e20937 · 2018-09-20T10:24:31.000+02:00
Delete a organization + organization policy
diff --git a/Gemfile b/Gemfile
@@ -7,7 +7,7 @@ gem 'rails-i18n'
 gem "rdiscount"
 gem 'activeadmin', '~> 1.2.1'
 gem 'has_scope'
-gem 'pundit'
+gem 'pundit', '~> 2.0.0'
 gem 'pg', '0.17.1'
 gem 'hstore_translate'
 gem 'dalli'
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -238,7 +238,7 @@ GEM
     prawn-table (0.2.2)
       prawn (>= 1.3.0, < 3.0.0)
     public_suffix (2.0.5)
-    pundit (0.3.0)
+    pundit (2.0.0)
       activesupport (>= 3.0.0)
     rack (1.6.10)
     rack-protection (2.0.1)
@@ -428,7 +428,7 @@ DEPENDENCIES
   pg (= 0.17.1)
   prawn (~> 2.2.0)
   prawn-table (~> 0.2.2)
-  pundit
+  pundit (~> 2.0.0)
   rails (~> 4.2)
   rails-i18n
   rails_12factor (= 0.0.3)
diff --git a/app/admin/organization.rb b/app/admin/organization.rb
@@ -27,6 +27,19 @@
     f.actions
   end
 
+  controller do
+    def destroy
+      resource.destroy
+
+      if resource == current_organization
+        sign_out(current_user)
+        redirect_to root_path
+      else
+        redirect_to admin_organizations_path
+      end
+    end
+  end
+
   filter :name
   filter :city, as: :select, collection: -> { Organization.pluck(:city).uniq }
   filter :neighborhood
diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb
@@ -1,12 +1,14 @@
 class OrganizationsController < ApplicationController
-  before_filter :load_resource, only: [:show, :edit, :update, :destroy, :set_current]
+  before_filter :load_resource, only: [:show, :edit, :update, :set_current]
 
   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,8 +23,10 @@ def show
   def create
     @organization = Organization.new(organization_params)
 
+    authorize @organization
+
     if @organization.save
-      redirect_to @organization, status: :created
+      redirect_to @organization
     else
       render action: :new, status: :unprocessable_entity
     end
@@ -36,11 +40,6 @@ def update
     end
   end
 
-  def destroy
-    @organization.destroy
-    redirect_to organizations_path, notice: "deleted"
-  end
-
   # POST /organizations/:organization_id/set_current
   #
   def set_current
@@ -54,6 +53,8 @@ def set_current
 
   def load_resource
     @organization = Organization.find(params[:id])
+
+    authorize @organization
   end
 
   def organization_params
diff --git a/app/models/event.rb b/app/models/event.rb
@@ -4,6 +4,7 @@ class Event < ActiveRecord::Base
   belongs_to :post
   belongs_to :member
   belongs_to :transfer
+  has_many :push_notifications, dependent: :destroy
 
   validates :action, presence: true
   validate :resource_presence
diff --git a/app/models/member.rb b/app/models/member.rb
@@ -8,6 +8,7 @@ class Member < ActiveRecord::Base
   belongs_to :organization
   has_one :account, as: :accountable
   has_many :movements, through: :account
+  has_many :events, dependent: :destroy
 
   delegate :balance, to: :account, prefix: true, allow_nil: true
   delegate :gender, :date_of_birth, to: :user, prefix: true, allow_nil: true
diff --git a/app/models/organization.rb b/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
 
diff --git a/app/models/post.rb b/app/models/post.rb
@@ -39,13 +39,12 @@ def self.inherited(child)
   attr_reader :member_id
 
   belongs_to :category
-
   belongs_to :user
   belongs_to :organization
   belongs_to :publisher, class_name: "User", foreign_key: "publisher_id"
-  has_many :user_members, class_name: "Member", through: :user, source: :members
   has_many :transfers
   has_many :movements, through: :transfers
+  has_many :events, dependent: :destroy
 
   delegate :name, to: :category, prefix: true, allow_nil: true
 
diff --git a/app/models/transfer.rb b/app/models/transfer.rb
@@ -14,8 +14,8 @@ class Transfer < ActiveRecord::Base
   attr_accessor :source, :destination, :amount, :hours, :minutes
 
   belongs_to :post
-  belongs_to :operator, class_name: "User"
-  has_many :movements
+  has_many :movements, dependent: :destroy
+  has_many :events, dependent: :destroy
 
   validate :different_source_and_destination
 
diff --git a/app/policies/application_policy.rb b/app/policies/application_policy.rb
@@ -17,7 +17,7 @@ def index?
   end
 
   def show?
-    scope.where(id: record.id).exists?
+    record.class.where(id: record.id).exists?
   end
 
   def create?
@@ -39,23 +39,4 @@ def edit?
   def destroy?
     false
   end
-
-  def scope
-    Pundit.policy_scope!(member, record.class)
-  end
-
-  class Scope
-    attr_reader :member, :user, :organization, :scope
-
-    def initialize(member, scope)
-      @member = member
-      @user = member.user if member
-      @organization = member.organization if member
-      @scope = scope
-    end
-
-    def resolve
-      scope
-    end
-  end
 end
diff --git a/app/policies/organization_policy.rb b/app/policies/organization_policy.rb
@@ -0,0 +1,19 @@
+class OrganizationPolicy < ApplicationPolicy
+  alias_method :organization, :record
+
+  def index?
+    true
+  end
+
+  def create?
+    user&.superadmin?
+  end
+
+  def update?
+    user&.superadmin? || user&.admins?(organization)
+  end
+
+  def set_current?
+    user&.as_member_of(organization)
+  end
+end
diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb
@@ -1,8 +1,4 @@
 class UserPolicy < ApplicationPolicy
-  def new?
-    user.admins?(organization)
-  end
-
   def create?
     user.admins?(organization)
   end
@@ -14,19 +10,4 @@ def update?
       user.admins?(organization)
     )
   end
-
-  class Scope < ApplicationPolicy::Scope
-    attr_reader :member, :user, :organization, :scope
-
-    def initialize(user, scope)
-      @member = member
-      @user = member.user if member
-      @organization = member.organization if member
-      @scope = scope
-    end
-
-    def resolve
-      scope
-    end
-  end
 end
diff --git a/app/views/organizations/index.html.erb b/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 %>
diff --git a/app/views/organizations/show.html.erb b/app/views/organizations/show.html.erb
@@ -100,21 +100,14 @@
           <% end %>
         </li>
       <% end %>
-      <% if superadmin? %>
+      <% if current_user&.active?(@organization) %>
         <li>
-          <%= link_to organization_path(@organization),
-                      data: { method: :delete, confirm: t("are_you_sure") } do %>
-            <%= glyph :trash %>
-            <%= t "global.delete" %>
+          <%= link_to new_transfer_path(id: @organization, destination_account_id: @organization.account.id) do %>
+            <%= glyph :time %>
+            <%= t "global.give_time" %>
           <% end %>
         </li>
       <% end %>
-      <li>
-        <%= link_to new_transfer_path(id: @organization, destination_account_id: @organization.account.id) do %>
-          <%= glyph :time %>
-          <%= t "global.give_time" %>
-        <% end %>
-      </li>
     </ul>
   </div>
 </div>
diff --git a/config/routes.rb b/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
diff --git a/spec/admin/organizations_controller_spec.rb b/spec/admin/organizations_controller_spec.rb
@@ -0,0 +1,20 @@
+require 'spec_helper'
+
+RSpec.describe Admin::OrganizationsController, type: :controller do
+  let(:organization) { Fabricate(:organization) }
+  let(:member) { Fabricate(:member, organization: organization) }
+  let(:user) { member.user }
+
+  before do
+    login(user)
+    allow(controller).to receive(:authenticate_superuser!).and_return(true)
+  end
+
+  describe "DELETE #destroy" do
+    it "sign out if current user is logged to organization deleted" do
+      expect {
+        delete :destroy, id: organization.id
+      }.to change { controller.current_user }.to nil
+    end
+  end
+end
diff --git a/spec/controllers/organizations_controller_spec.rb b/spec/controllers/organizations_controller_spec.rb
@@ -1,16 +1,59 @@
 require 'spec_helper'
 
 RSpec.describe OrganizationsController do
-  let(:organization) { Fabricate(:organization) }
+  let!(:organization) { Fabricate(:organization) }
   let(:member) { Fabricate(:member, organization: organization) }
   let(:user) { member.user }
 
-  describe '#show' do
-    it 'links to new_transfer_path' do
+  describe 'GET #show' do
+    it 'displays the organization page' do
       get 'show', id: organization.id
-      expect(response.body).to include(
-        "<a href=\"/transfers/new?destination_account_id=#{organization.account.id}&amp;id=#{organization.id}\">"
-      )
+
+      expect(assigns(:organization)).to eq(organization)
+      expect(response.status).to eq(200)
+      expect(response.body).to include(organization.name)
+    end
+  end
+
+  describe 'GET #index' do
+    it 'populates and array of organizations' do
+      get :index
+
+      expect(assigns(:organizations)).to eq([organization])
+    end
+  end
+
+  describe 'POST #create' do
+    it 'only superdamins are authorized create to new organizations' do
+      login(member.user)
+
+      expect {
+        post :create, organization: { name: 'New cool organization' }
+      }.not_to change { Organization.count }
+    end
+  end
+
+  describe 'POST #update' do
+    context 'with a logged user (admins organization)' do
+      let(:member) { Fabricate(:member, organization: organization, manager: true) }
+
+      it 'allows to update organization' do
+        login(member.user)
+
+        post :update, id: organization.id, organization: { name: 'New org name' }
+
+        organization.reload
+        expect(organization.name).to eq('New org name')
+      end
+    end
+
+    context 'without a logged user' do
+      it 'does not allow to update organization' do
+        post :update, id: organization.id, organization: { name: 'New org name' }
+
+        expect(response).to redirect_to(root_path)
+        expect(flash[:error]).to eq('You are not authorized to perform this action.')
+      end
     end
   end
 
diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb
@@ -35,6 +35,7 @@
     it { is_expected.to belong_to(:post) }
     it { is_expected.to belong_to(:member) }
     it { is_expected.to belong_to(:transfer) }
+    it { is_expected.to have_many(:push_notifications) }
 
     it { is_expected.to have_db_column(:post_id) }
     it { is_expected.to have_db_column(:member_id) }
diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb
diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb
diff --git a/spec/models/transfer_spec.rb b/spec/models/transfer_spec.rb
diff --git a/spec/views/organizations/show.html.erb_spec.rb b/spec/views/organizations/show.html.erb_spec.rb
