Skip to content

Delete a organization + organization policy #368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Sep 20, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
4 changes: 2 additions & 2 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
13 changes: 13 additions & 0 deletions app/admin/organization.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
17 changes: 9 additions & 8 deletions app/controllers/organizations_controller.rb
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -54,6 +53,8 @@ def set_current

def load_resource
@organization = Organization.find(params[:id])

authorize @organization
end

def organization_params
Expand Down
1 change: 1 addition & 0 deletions app/models/event.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions app/models/member.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@enricostano enricostano Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to add this kind of change to the model tests also? Thanks 😍

e.g. https://github.com/coopdevs/timeoverflow/blob/develop/spec/models/member_spec.rb#L8

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👉 7f6a087


delegate :balance, to: :account, prefix: true, allow_nil: true
delegate :gender, :date_of_birth, to: :user, prefix: true, allow_nil: true
Expand Down
8 changes: 4 additions & 4 deletions app/models/organization.rb
Original file line number Diff line number Diff line change
@@ -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

Expand Down
3 changes: 1 addition & 2 deletions app/models/post.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 2 additions & 2 deletions app/models/transfer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
21 changes: 1 addition & 20 deletions app/policies/application_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand All @@ -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
19 changes: 19 additions & 0 deletions app/policies/organization_policy.rb
Original file line number Diff line number Diff line change
@@ -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
19 changes: 0 additions & 19 deletions app/policies/user_policy.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
class UserPolicy < ApplicationPolicy
def new?
user.admins?(organization)
end

def create?
user.admins?(organization)
end
Expand All @@ -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
10 changes: 3 additions & 7 deletions app/views/organizations/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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) %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same question here

Copy link
Collaborator Author

@markets markets May 28, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to previous comment, this controller doesn't require authentication at this point, so now you can visit the /organizations page without a crash.

<%= 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 %>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we showing all organizations now??

Copy link
Collaborator Author

@markets markets May 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfortunately yes, without pagination 🙊

15 changes: 4 additions & 11 deletions app/views/organizations/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 20 additions & 0 deletions spec/admin/organizations_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -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
55 changes: 49 additions & 6 deletions spec/controllers/organizations_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -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

Expand Down
1 change: 1 addition & 0 deletions spec/models/event_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Expand Down
Loading