Merge #​2654 "Fix access to the version history of organization-level roles"

app/models/admin_ability.rb

@@ -99,6 +99,8 @@ class AdminAbility
     can :manage, Conference, organization_id: org_ids_for_organization_admin
     can [:index, :show], Role
 
+    can [:index, :revert_object, :revert_attribute], PaperTrail::Version, organization_id: org_ids_for_organization_admin
+
     signed_in_with_organizer_role(user, conf_ids_for_organization_admin)
   end
 

app/models/role.rb

@@ -5,7 +5,9 @@ class Role < ApplicationRecord
   has_many :users_roles
   has_many :users, through: :users_roles
 
-  has_paper_trail on: [:create, :update], only: [:name, :description], meta: { conference_id: :resource_id }
+  has_paper_trail on:   [:create, :update],
+                  only: [:name, :description],
+                  meta: { conference_id: :conference_id, organization_id: :organization_id }
 
   before_destroy :cancel
   scopify
@@ -14,6 +16,14 @@ class Role < ApplicationRecord
 
   validates :name, uniqueness: { scope: :resource }
 
+  def conference_id
+    resource_type == 'Conference' ? resource_id : nil
+  end
+
+  def organization_id
+    resource_type == 'Organization' ? resource_id : nil
+  end
+
   private
 
   # Needed to ensure that removing all user from role doesn't remove role.

app/models/users_role.rb

@@ -4,11 +4,8 @@ class UsersRole < ApplicationRecord
   belongs_to :role
   belongs_to :user
 
-  has_paper_trail on: [:create, :destroy], meta: { conference_id: :conference_id }
+  delegate :conference_id, :organization_id, to: :role
 
-  private
-
-  def conference_id
-    role.resource_id
-  end
+  has_paper_trail on:   [:create, :destroy],
+                  meta: { conference_id: :conference_id, organization_id: :organization_id }
 end

app/views/admin/versions/_object_desc_and_link.html.haml

@@ -15,18 +15,14 @@
   - role = current_or_last_object_state('Role', object.role_id)
   - role_name = role.try(:name) || PaperTrail::Version.where(item_type: 'Role', item_id: object.role_id).last.changeset[:name].second
   role
-  - if role_name == 'organization_admin'
-    - if Organization.find_by(id: version.conference_id)
-      -# organization_admin belongs to organization and not conferences
-      - organization = Organization.find_by(id: version.conference_id)
-      = link_if_alive version, role_name,
-            admins_admin_organization_path(organization), organization
-    - else
-      (Deleted Organization)
-  - else
+  - if version.conference_id
     - conference = Conference.find_by(id: version.conference_id)
     - conference_short_title = conference.try(:short_title) || current_or_last_object_state('Conference', version.conference_id).try(:short_title) || ' '
     = link_if_alive version, role.try(:name), admin_conference_role_path(conference_short_title,role.try(:name) || ' '), conference
+  - elsif version.organization_id
+    - organization = Organization.find(version.organization_id)
+    = link_if_alive version, role_name,
+          admins_admin_organization_path(organization), organization
 
   = version.event == 'create' ? 'to' : 'from'
   user
@@ -133,19 +129,15 @@
 - when 'Role'
   role
   - role_name = object.try(:name) || PaperTrail::Version.where(item_type: 'Role', item_id: version.item_id).last.changeset[:name].second
-  - if role_name == 'organization_admin'
-    - if Organization.find_by(id: version.conference_id)
-      -# organization_admin belongs to organization and not conferences
-      - organization = Organization.find_by(id: version.conference_id)
-      = link_if_alive version, role_name,
-            admins_admin_organization_path(organization), organization
-    - else
-      (Role Deleted)
-  - else
+  - if version.conference_id
     - conference = Conference.find_by(id: version.conference_id)
     - conference_short_title = conference.try(:short_title) || current_or_last_object_state('Conference', version.conference_id).try(:short_title) || ' '
     = link_if_alive version, role_name,
             admin_conference_role_path(conference_short_title, role_name), conference
+  - elsif version.organization_id
+    - organization = Organization.find(version.organization_id)
+    = link_if_alive version, role_name,
+            admins_admin_organization_path(organization), organization
 
 - when 'Venue'
   venue
@@ -203,20 +195,16 @@
     = link_to_user(version.item_id)
 
 - unless %w(Conference Subscription Registration User Organization).include?(version.item_type)
-  - if (version.item_type == 'Role' && role_name == 'organization_admin') || (version.item_type == 'UsersRole' && role_name == 'organization_admin')
-    in organization
-    - if Organization.find_by(id: version.conference_id)
-      -# organization_admin belongs to organization and not conferences
-      - organization = Organization.find_by(id: version.conference_id)
-      = link_to_organization(version.conference_id)
-    - else
-      (Organization Deleted)
-  - elsif version.item_type == 'Commercial'
+  - if version.item_type == 'Commercial'
     - commercial = current_or_last_object_state(version.item_type, version.item_id)
     - commercialable = current_or_last_object_state(commercial.commercialable_type, commercial.commercialable_id)
     - unless commercial.commercialable_type == 'Conference'
       in conference
       = link_to_conference(version.conference_id)
+  - elsif version.organization_id
+    - organization = Organization.find(version.organization_id)
+    in organization
+    = link_to_organization(version.organization_id)
   - else
     in conference
     = link_to_conference(version.conference_id)

db/migrate/20200331214534_add_organization_to_versions.rb

@@ -0,0 +1,35 @@
+class AddOrganizationToVersions < ActiveRecord::Migration[5.2]
+  def up
+    add_reference :versions, :organization
+    deconflate
+  end
+
+  def down
+    conflate
+    remove_reference :versions, :organization
+  end
+
+  private
+
+  def conflate
+    say 'conflate'
+
+    PaperTrail::Version.where.not(organization_id: nil).each do |version|
+      version.update_attributes conference_id: version.organization_id
+    end
+  end
+
+  def deconflate
+    say 'deconflate'
+
+    PaperTrail::Version.where.not(conference_id: nil).where(item_type: %[Role UsersRole]).each do |version|
+      id = version.conference_id
+
+      if Organization.exists?(id)
+        raise "version #{version.id} conflates organization #{id} with conference #{id}" if Conference.exists?(id)
+
+        version.update_attributes conference_id: nil, organization_id: id
+      end
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20181113195810) do
+ActiveRecord::Schema.define(version: 2020_03_31_214534) do
 
   create_table "answers", force: :cascade do |t|
     t.string "title"
@@ -639,7 +639,9 @@ ActiveRecord::Schema.define(version: 20181113195810) do
     t.text "object_changes"
     t.datetime "created_at"
     t.integer "conference_id"
+    t.integer "organization_id"
     t.index ["item_type", "item_id"], name: "index_versions_on_item_type_and_item_id"
+    t.index ["organization_id"], name: "index_versions_on_organization_id"
   end
 
   create_table "votes", force: :cascade do |t|

spec/factories/users.rb

@@ -59,6 +59,17 @@ FactoryBot.define do
     biography { '<div id="divInjectedElement"></div>' }
   end
 
+  factory :organization_admin, parent: :user do
+    transient do
+      organization { create(:organization) }
+    end
+
+    after(:create) do |user, evaluator|
+      user.roles << Role.find_or_create_by(name: 'organization_admin', resource: evaluator.organization)
+      user.save!
+    end
+  end
+
   factory :organizer, parent: :user do
     transient do
       resource { create(:resource) }

spec/features/versions_spec.rb

@@ -318,17 +318,14 @@ feature 'Version' do
   end
 
   context 'organization role', feature: true, versioning: true, js: true do
+    let!(:organization_admin) { create(:organization_admin, organization: conference.organization) }
     let!(:user) { create(:user) }
-    let!(:role) do
-      Role.find_by(
-        resource_id:   conference.organization.id,
-        resource_type: 'Organization'
-      )
-    end
 
     setup do
       user.add_role :organization_admin, conference.organization
       user.remove_role :organization_admin, conference.organization
+
+      sign_in organization_admin
       visit admin_revision_history_path
     end