diff --git a/Gemfile.lock b/Gemfile.lock index 11cf4639..bdf6e8db 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -190,6 +190,7 @@ GEM date net-protocol net-pop (0.1.2) + net-protocol net-protocol (0.2.2) timeout net-scp (4.0.0) diff --git a/app/controllers/organizations/base_controller.rb b/app/controllers/organizations/base_controller.rb index 1595b6cd..8d1fcfe9 100644 --- a/app/controllers/organizations/base_controller.rb +++ b/app/controllers/organizations/base_controller.rb @@ -16,6 +16,10 @@ class Organizations::BaseController < ApplicationController # raise Pundit::NotAuthorizedError unless @current_membership.admin? # end + def authorize_organization_owner! + redirect_to organization_path(@organization), alert: "You are not authorized to perform this action." unless @organization.owner?(current_user) + end + private def set_organization diff --git a/app/controllers/organizations/transfers_controller.rb b/app/controllers/organizations/transfers_controller.rb new file mode 100644 index 00000000..0c2bb56f --- /dev/null +++ b/app/controllers/organizations/transfers_controller.rb @@ -0,0 +1,14 @@ +class Organizations::TransfersController < Organizations::BaseController + before_action :authorize_organization_owner! + + def show + end + + def update + if @organization.transfer_ownership(params[:user_id]) + redirect_to @organization, notice: "Ownership transferred successfully." + else + render :show, status: :unprocessable_entity + end + end +end diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index 2900d7d6..f8c33aad 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -22,6 +22,7 @@ def edit # POST /organizations def create @organization = Organization.new(organization_params) + @organization.owner = current_user @organization.memberships.build(user: current_user, role: Membership.roles[:admin]) if @organization.save diff --git a/app/models/organization.rb b/app/models/organization.rb index f457ca8e..40df40fd 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -1,6 +1,10 @@ class Organization < ApplicationRecord has_many :memberships, dependent: :destroy has_many :users, through: :memberships + belongs_to :owner, class_name: "User" + + include Transfer + has_many :inboxes, dependent: :destroy validates :name, presence: true diff --git a/app/models/organization/transfer.rb b/app/models/organization/transfer.rb new file mode 100644 index 00000000..9b0a01db --- /dev/null +++ b/app/models/organization/transfer.rb @@ -0,0 +1,28 @@ +module Organization::Transfer + extend ActiveSupport::Concern + + def transfer_ownership(user_id) + # previous_owner = owner + + membership = memberships.find_by(user_id: user_id) + return false unless membership + + new_owner = membership.user + + ApplicationRecord.transaction do + membership.update!(role: Membership.roles[:admin]) + update!(owner: new_owner) + end + rescue => e + Rails.logger.error("Ownership transfer failed: #{e.message}") + false + end + + def owner?(user) + owner_id == user.id + end + + def can_transfer?(user) + owner?(user) && users.size >= 2 + end +end diff --git a/app/models/user.rb b/app/models/user.rb index d6aee37d..8914e0ac 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -5,4 +5,5 @@ class User < ApplicationRecord :recoverable, :rememberable, :validatable has_many :memberships, dependent: :destroy has_many :organizations, through: :memberships + has_many :owned_organizations, class_name: "Organization", foreign_key: :owner_id, inverse_of: :owner, dependent: :destroy end diff --git a/app/views/organizations/show.html.erb b/app/views/organizations/show.html.erb index 9f3dea74..faff242f 100644 --- a/app/views/organizations/show.html.erb +++ b/app/views/organizations/show.html.erb @@ -1,5 +1,6 @@ <%= render PageComponent.new(title: "Organizations / #{@organization.name}") do |component| %> <% component.with_action_list do %> + <%= link_to "Transfer", organization_transfer_path(@organization), class: "btn btn-danger" if @organization.can_transfer?(current_user) %> <%= link_to "Edit", edit_organization_path(@organization), class: "btn btn-secondary" if policy(@organization).edit? %>
<%= button_to "Destroy", @organization, method: :delete, data: { turbo_confirm: "Are you sure?" }, class: "btn btn-danger" if policy(@organization).destroy? %> diff --git a/app/views/organizations/transfers/_form.html.erb b/app/views/organizations/transfers/_form.html.erb new file mode 100644 index 00000000..aaf97f75 --- /dev/null +++ b/app/views/organizations/transfers/_form.html.erb @@ -0,0 +1,11 @@ +<%= form_with url: organization_transfer_path(current_organization), method: :patch, class: "contents space-y-4" do |form| %> +
+ <%= form.label :user_id %> + <%= form.collection_select :user_id, current_organization.users.where.not(users: { id: current_user.id }), :id, :email, { include_blank: true, required: true }, { class: "form-input" } %> +
+ +
+ <%#= form.button, data: { turbo_confirm: "Are you sure?" } %> + <%= form.submit class: "btn btn-primary" %> +
+<% end %> diff --git a/app/views/organizations/transfers/show.html.erb b/app/views/organizations/transfers/show.html.erb new file mode 100644 index 00000000..4abf5002 --- /dev/null +++ b/app/views/organizations/transfers/show.html.erb @@ -0,0 +1,4 @@ +<%= render PageComponent.new(title: "Transfer organization", content_container: true, full_width: false) do |component| %> + <%= render "organizations/transfers/form" %> + <%= link_to "Cancel", organization_path(current_organization), class: "btn btn-secondary" %> +<% end %> diff --git a/config/routes.rb b/config/routes.rb index 1e85ca7e..05751e98 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -3,6 +3,7 @@ resources :organizations do resources :memberships, module: :organizations, except: %i[show] + resource :transfer, module: :organizations, only: %i[show update] resources :inboxes, module: :organizations end diff --git a/db/migrate/20241208203403_add_owner_to_organizations.rb b/db/migrate/20241208203403_add_owner_to_organizations.rb new file mode 100644 index 00000000..5f582a80 --- /dev/null +++ b/db/migrate/20241208203403_add_owner_to_organizations.rb @@ -0,0 +1,12 @@ +class AddOwnerToOrganizations < ActiveRecord::Migration[8.0] + def change + add_reference :organizations, :owner, foreign_key: { to_table: :users } + + Organization.all.each do |organization| + user = organization.memberships.admin.first&.user + organization.update(owner: user) + end + + change_column_null :organizations, :owner_id, false + end +end diff --git a/db/schema.rb b/db/schema.rb index 2e62bf1f..74e5d6e0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 2024_11_02_205048) do +ActiveRecord::Schema[8.0].define(version: 2024_12_08_203403) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -65,6 +65,8 @@ t.string "name" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.bigint "owner_id", null: false + t.index ["owner_id"], name: "index_organizations_on_owner_id" end create_table "users", force: :cascade do |t| @@ -95,4 +97,5 @@ add_foreign_key "inboxes", "organizations" add_foreign_key "memberships", "organizations" add_foreign_key "memberships", "users" + add_foreign_key "organizations", "users", column: "owner_id" end diff --git a/test/controllers/organizations/transfers_controller_test.rb b/test/controllers/organizations/transfers_controller_test.rb new file mode 100644 index 00000000..708f7d2c --- /dev/null +++ b/test/controllers/organizations/transfers_controller_test.rb @@ -0,0 +1,52 @@ +require "test_helper" + +class Organizations::TransfersControllerTest < ActionDispatch::IntegrationTest + setup do + @user = users(:one) + @organization = organizations(:one) + @membership = memberships(:one) + + @user2 = users(:two) + end + + test "#show" do + sign_in @user + + get organization_transfer_path(@organization) + assert_response :success + end + + test "admin can transfer organization" do + @membership2 = @organization.memberships.create!(user: @user2, role: Membership.roles[:member]) + sign_in @user + + patch organization_transfer_path(@organization), params: { user_id: @user2.id } + assert_redirected_to organization_path(@organization) + assert_equal @user2, @organization.reload.owner + end + + test "regular_user cannot transfer organization" do + @membership2 = @organization.memberships.create!(user: @user2, role: Membership.roles[:member]) + sign_in @user2 + + patch organization_transfer_path(@organization), params: { user_id: @user2.id } + assert_redirected_to organization_path(@organization) + assert_equal @user, @organization.reload.owner + end + + test "transfer organization to non-member" do + sign_in @user + + patch organization_transfer_path(@organization), params: { user_id: @user2.id } + assert_response :unprocessable_entity + assert_equal @user, @organization.reload.owner + end + + test "transfer organization that user is not a member of" do + sign_in @user2 + + patch organization_transfer_path(@organization), params: { user_id: @user2.id } + assert_response :not_found + assert_equal @user, @organization.reload.owner + end +end diff --git a/test/fixtures/organizations.yml b/test/fixtures/organizations.yml index b0bf4fa5..8f5acb13 100644 --- a/test/fixtures/organizations.yml +++ b/test/fixtures/organizations.yml @@ -2,6 +2,8 @@ one: name: Organization 1 + owner: one two: name: Organization 2 + owner: two diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index f276e3f9..3d8aadcb 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -9,3 +9,6 @@ one: two: email: hello@superails.com + +unassociated: + email: unassociated@superails.com \ No newline at end of file diff --git a/test/models/organization/transfer_test.rb b/test/models/organization/transfer_test.rb new file mode 100644 index 00000000..2e13ca96 --- /dev/null +++ b/test/models/organization/transfer_test.rb @@ -0,0 +1,16 @@ +require "test_helper" + +class OrganizationTransferTest < ActiveSupport::TestCase + test "transfer_ownership" do + organization = organizations(:one) + user = users(:two) + + assert_not organization.transfer_ownership(user.id) + assert_not organization.owner?(user) + + membership = organization.memberships.create!(user: user, role: Membership.roles[:member]) + assert organization.transfer_ownership(user.id) + assert organization.owner?(user) + assert membership.reload.admin? + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index ed4fa037..45908d16 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -2,11 +2,11 @@ class UserTest < ActiveSupport::TestCase # TODO: memberships with associated downstream records should not be deletable - test "destroying a user destroys associated memberships" do + test "destroying organization owner destroys organization" do organization = organizations(:one) user = users(:one) - assert_no_difference "Organization.count" do + assert_difference "Organization.count", -1 do assert_difference "Membership.count", -1 do user.destroy end @@ -15,4 +15,18 @@ class UserTest < ActiveSupport::TestCase # TODO: this should not be a valid state! assert organization.memberships.none? end + + test "destroying organization member (non-owner) does not destroy organization" do + organization = organizations(:one) + user = users(:unassociated) + organization.memberships.create!(user:, role: Membership.roles[:admin]) + + assert_no_difference "Organization.count" do + assert_difference "Membership.count", -1 do + user.destroy + end + end + + assert organization.memberships.any? + end end