Add OrganizationMembership model (#2471)

* Remove deprecated analytics role

* Make anchor tag a bit more readable

* Add OrganizationMembership table and model

* Add organization_membership associations

* Add unique indices for user_id and org_id

* Wrap update and create calls in transactions

* Remove specs of deprecated analytics_beta_tester

app/controllers/organizations_controller.rb

@@ -8,7 +8,10 @@ class OrganizationsController < ApplicationController
     @organization = Organization.new(organization_params)
     authorize @organization
     if @organization.save
-      current_user.update(organization_id: @organization.id, org_admin: true)
+      ActiveRecord::Base.transaction do
+        current_user.update(organization_id: @organization.id, org_admin: true)
+        OrganizationMembership.create(organization_id: @organization.id, user_id: current_user.id, type_of_user: "admin")
+      end
       redirect_to "/settings/organization", notice:
         "Your organization was successfully created and you are an admin."
     else

app/controllers/users_controller.rb

@@ -95,7 +95,10 @@ class UsersController < ApplicationController
   def join_org
     authorize User
     if (@organization = Organization.find_by(secret: params[:org_secret]))
-      current_user.update(organization_id: @organization.id)
+      ActiveRecord::Base.transaction do
+        current_user.update(organization_id: @organization.id)
+        OrganizationMembership.create(user_id: current_user.id, organization_id: current_user.organization_id, type_of_user: "member")
+      end
       redirect_to "/settings/organization",
                   notice: "You have joined the #{@organization.name} organization."
     else
@@ -105,6 +108,8 @@ class UsersController < ApplicationController
 
   def leave_org
     authorize User
+    type_of_user = current_user.org_admin ? "admin" : "member"
+    OrganizationMembership.find_by(organization_id: current_user.organization_id, user_id: current_user.id, type_of_user: type_of_user)&.delete
     current_user.update(organization_id: nil, org_admin: nil)
     redirect_to "/settings/organization",
                 notice: "You have left your organization."
@@ -114,7 +119,9 @@ class UsersController < ApplicationController
     user = User.find(params[:user_id])
     authorize user
     user.update(org_admin: true)
-    user.add_role :analytics_beta_tester if user.organization.approved
+    org_membership = OrganizationMembership.find_or_initialize_by(user_id: user.id, organization_id: user.organization_id)
+    org_membership.type_of_user = "admin"
+    org_membership.save
     redirect_to "/settings/organization",
                 notice: "#{user.name} is now an admin."
   end
@@ -123,6 +130,9 @@ class UsersController < ApplicationController
     user = User.find(params[:user_id])
     authorize user
     user.update(org_admin: false)
+    org_membership = OrganizationMembership.find_or_initialize_by(user_id: user.id, organization_id: user.organization_id)
+    org_membership.type_of_user = "member"
+    org_membership.save
     redirect_to "/settings/organization",
                 notice: "#{user.name} is no longer an admin."
   end
@@ -130,6 +140,8 @@ class UsersController < ApplicationController
   def remove_from_org
     user = User.find(params[:user_id])
     authorize user
+    type_of_user = user.org_admin ? "admin" : "member"
+    OrganizationMembership.find_by(organization_id: current_user.organization_id, user_id: current_user.id, type_of_user: type_of_user)&.delete
     user.update(organization_id: nil)
     redirect_to "/settings/organization",
                 notice: "#{user.name} is no longer part of your organization."

app/models/organization_membership.rb

@@ -0,0 +1,5 @@
+class OrganizationMembership < ApplicationRecord
+  validates :user_id, :organization_id, :type_of_user, presence: true
+  validates :user_id, uniqueness: { scope: :organization_id }
+  validates :type_of_user, inclusion: { in: %w[admin member guest] }
+end

app/models/role.rb

@@ -18,7 +18,6 @@ class Role < ApplicationRecord
                 trusted
                 banned
                 warned
-                analytics_beta_tester
                 switch_between_orgs
                 triple_unicorn_member
                 level_4_member

app/models/user.rb

@@ -323,10 +323,6 @@ class User < ApplicationRecord
     has_role?(:workshop_pass) && valid_pass
   end
 
-  def analytics
-    has_role? :analytics_beta_tester
-  end
-
   def comment_banned
     has_role? :comment_banned
   end
@@ -354,10 +350,6 @@ class User < ApplicationRecord
   end
   handle_asynchronously :subscribe_to_mailchimp_newsletter
 
-  def can_view_analytics?
-    has_any_role?(:super_admin, :analytics_beta_tester)
-  end
-
   def a_sustaining_member?
     monthly_dues.positive?
   end

app/policies/article_policy.rb

@@ -23,10 +23,6 @@ class ArticlePolicy < ApplicationPolicy
     true
   end
 
-  def analytics_index?
-    (user_is_author? && user_can_view_analytics?) || user_org_admin?
-  end
-
   def permitted_attributes
     %i[title body_html body_markdown main_image published canonical_url
        description allow_small_edits allow_big_edits tag_list publish_under_org
@@ -46,8 +42,4 @@ class ArticlePolicy < ApplicationPolicy
   def user_org_admin?
     user.org_admin && user.organization_id == record.organization_id
   end
-
-  def user_can_view_analytics?
-    user.can_view_analytics?
-  end
 end

app/services/membership_service.rb

@@ -72,7 +72,6 @@ class MembershipService
   def assign_membership_role
     # change role names here, in role.rb, users_controller#handle_settings_tab => @membership_names
     remove_all_membership_roles
-    user.add_role :analytics_beta_tester
     if monthly_dues >= 100_000
       user.add_role :triple_unicorn_member
     elsif monthly_dues > 2500
@@ -87,8 +86,7 @@ class MembershipService
   end
 
   def remove_all_membership_roles
-    tiers = %i[ triple_unicorn_member level_4_member level_3_member level_2_member level_1_member
-                analytics_beta_tester]
+    tiers = %i[triple_unicorn_member level_4_member level_3_member level_2_member level_1_member]
     tiers.each { |t| user.remove_role(t) }
   end
 

app/views/users/edit.html.erb

@@ -76,16 +76,18 @@
 
   <div class="settings-hub">
     <% @tab_list.each do |possible_tab| %>
-      <% if @tab == possible_tab.downcase.gsub(" ", "-") %>
+      <% if @tab == possible_tab.downcase.tr(" ", "-") %>
         <div class="single-settings-tab selected">
           <%= possible_tab %>
         </div>
       <% else %>
-        <a class="single-settings-tab" href="/settings/<%= possible_tab.downcase.gsub(" ", "-") %>"><%= possible_tab %></a>
+        <a class="single-settings-tab" href="/settings/<%= possible_tab.downcase.tr(" ", "-") %>">
+          <%= possible_tab %>
+        </a>
       <% end %>
     <% end %>
   </div>
   <div class="settings-form">
-    <%= render "users/" + @tab.gsub("-", "_"), tab: @tab %>
+    <%= render "users/" + @tab.tr("-", "_"), tab: @tab %>
   </div>
 </div>

db/migrate/20190415194929_create_organization_memberships.rb

@@ -0,0 +1,13 @@
+class CreateOrganizationMemberships < ActiveRecord::Migration[5.2]
+  def change
+    create_table :organization_memberships do |t|
+      t.bigint :user_id, null: false
+      t.bigint :organization_id, null: false
+      t.string :user_title
+      t.string :type_of_user, null: false
+      t.timestamps null: false
+    end
+
+    add_index :organization_memberships, %i[user_id organization_id], unique: true
+  end
+end

db/schema.rb

@@ -491,6 +491,16 @@ ActiveRecord::Schema.define(version: 2019_04_17_171019) do
     t.index ["user_id"], name: "index_notifications_on_user_id"
   end
 
+  create_table "organization_memberships", force: :cascade do |t|
+    t.datetime "created_at", null: false
+    t.bigint "organization_id", null: false
+    t.string "type_of_user", null: false
+    t.datetime "updated_at", null: false
+    t.bigint "user_id", null: false
+    t.string "user_title"
+    t.index ["user_id", "organization_id"], name: "index_organization_memberships_on_user_id_and_organization_id", unique: true
+  end
+
   create_table "organizations", id: :serial, force: :cascade do |t|
     t.string "address"
     t.boolean "approved", default: false

spec/factories/organization_memberships.rb

@@ -0,0 +1,8 @@
+FactoryBot.define do
+  factory :organization_membership do
+    # TODO: replace with `user` and `organization` since this currently doesn't have a belongs_to relationship
+    user_id { rand(6) }
+    organization_id { rand(6) }
+    type_of_user { "member" }
+  end
+end

spec/factories/users.rb

@@ -50,10 +50,6 @@ FactoryBot.define do
       end
     end
 
-    trait :analytics do
-      after(:build) { |user| user.add_role(:analytics_beta_tester) }
-    end
-
     trait :pro do
       after(:build) { |user| user.add_role :pro }
     end

spec/models/organization_membership_spec.rb

@@ -0,0 +1,13 @@
+require "rails_helper"
+
+RSpec.describe OrganizationMembership, type: :model do
+  describe "validations" do
+    subject { create(:organization_membership) }
+
+    it { is_expected.to validate_presence_of(:user_id) }
+    it { is_expected.to validate_presence_of(:organization_id) }
+    it { is_expected.to validate_presence_of(:type_of_user) }
+    it { is_expected.to validate_uniqueness_of(:user_id).scoped_to(:organization_id) }
+    it { is_expected.to validate_inclusion_of(:type_of_user).in_array(%w[admin member guest]) }
+  end
+end

spec/models/user_spec.rb

@@ -622,18 +622,6 @@ RSpec.describe User, type: :model do
     end
   end
 
-  describe "#can_view_analytics?" do
-    it "returns true for users with :super_admin role" do
-      user.add_role(:super_admin)
-      expect(user.can_view_analytics?).to be true
-    end
-
-    it "returns true for users with :analytics_beta_tester role" do
-      user.add_role(:analytics_beta_tester)
-      expect(user.can_view_analytics?).to be true
-    end
-  end
-
   describe "#destroy" do
     it "successfully destroys a user" do
       user.destroy

spec/policies/article_policy_spec.rb

@@ -20,13 +20,13 @@ RSpec.describe ArticlePolicy do
     let(:user) { build(:user) }
 
     it { is_expected.to permit_actions(%i[new create preview]) }
-    it { is_expected.to forbid_actions(%i[update delete_confirm destroy analytics_index]) }
+    it { is_expected.to forbid_actions(%i[update delete_confirm destroy]) }
 
     context "with banned status" do
       before { user.add_role :banned }
 
       it { is_expected.to permit_actions(%i[new preview]) }
-      it { is_expected.to forbid_actions(%i[create update delete_confirm destroy analytics_index]) }
+      it { is_expected.to forbid_actions(%i[create update delete_confirm destroy]) }
     end
   end
 
@@ -48,24 +48,4 @@ RSpec.describe ArticlePolicy do
 
     it { is_expected.to permit_actions(%i[update new create delete_confirm destroy preview]) }
   end
-
-  context "when a user with analytics tries to view someone else's article" do
-    let(:user)          { create(:user, :analytics) }
-    let(:other_user)    { create(:user, :analytics) }
-    let(:article)       { create(:article, user_id: user.id) }
-    let(:article2)      { create(:article, user_id: other_user.id) }
-
-    it "forbids the first user from viewing the other user's analytics via their article" do
-      expect(described_class.new(user, article2)).to forbid_action(:analytics_index)
-    end
-
-    it "forbids the other user from viewing the first user's analytics" do
-      expect(described_class.new(other_user, article)).to forbid_action(:analytics_index)
-    end
-
-    it "forbids them from viewing another person's analytics even if their article is first" do
-      articles = Article.all
-      expect(described_class.new(user, articles)).to forbid_action(:analytics_index)
-    end
-  end
 end

spec/requests/user_organization_spec.rb

@@ -4,60 +4,100 @@ RSpec.describe "UserOrganization", type: :request do
   let(:user)          { create(:user) }
   let(:organization)  { create(:organization, secret: "SECRET", approved: true) }
 
-  def add_org_admin
-    user.update(organization_id: organization.id, org_admin: true)
-    user2 = create(:user, organization_id: organization.id)
-    post "/users/add_org_admin", params: { user_id: user2.id }
+  context "when joining an org" do
+    before { sign_in user }
+
+    it "joins org with proper secret" do
+      post "/users/join_org", params: { org_secret: organization.secret }
+      expect(user.organization_id).to eq(organization.id)
+    end
+
+    it "creates an organization_membership association" do
+      post "/users/join_org", params: { org_secret: organization.secret }
+      org_membership = OrganizationMembership.first
+      expect(org_membership.persisted?).to eq true
+      expect(org_membership.user_id).to eq user.id
+      expect(org_membership.organization_id).to eq organization.id
+      expect(org_membership.type_of_user).to eq "member"
+    end
+
+    it "returns 404 if secret is wrong" do
+      expect { post "/users/join_org", params: { org_secret: "NOT SECRET" } }.
+        to raise_error ActiveRecord::RecordNotFound
+    end
   end
 
-  before do
-    sign_in user
+  context "when leaving an org" do
+    let(:org_member) { create(:user, :org_member) }
+
+    before { sign_in org_member }
+
+    it "leaves org" do
+      post "/users/leave_org"
+      expect(org_member.organization_id).to eq(nil)
+    end
+
+    it "deletes the org_membership association" do
+      create(:organization_membership, user_id: org_member.id, organization_id: org_member.organization_id)
+      post "/users/leave_org"
+      expect(OrganizationMembership.count).to eq 0
+    end
   end
 
-  it "joins org with proper secret" do
-    organization
-    post "/users/join_org", params: { org_secret: "SECRET" }
-    expect(user.organization_id).to eq(organization.id)
+  context "when adding an org admin" do
+    let(:org_admin) { create(:user, :org_admin) }
+    let(:user2) { create(:user, organization_id: org_admin.organization_id) }
+
+    def add_org_admin
+      org_admin
+      sign_in org_admin
+      post "/users/add_org_admin", params: { user_id: user2.id }
+    end
+
+    it "adds org admin" do
+      add_org_admin
+      expect(User.last.org_admin).to eq(true)
+    end
+
+    it "creates the org_membership association" do
+      add_org_admin
+      org_membership = OrganizationMembership.first
+      expect(org_membership.persisted?).to eq true
+      expect(org_membership.user_id).to eq user2.id
+      expect(org_membership.organization_id).to eq org_admin.organization_id
+      expect(org_membership.type_of_user).to eq "admin"
+    end
+
+    it "raises if user not org_admin" do
+      user.update(organization_id: organization.id)
+      expect { post "/users/add_org_admin", params: { user_id: user2.id } }.
+        to raise_error Pundit::NotAuthorizedError
+    end
   end
 
-  it "returns 404 if secret is wrong" do
-    expect { post "/users/join_org", params: { org_secret: "NOT SECRET" } }.
-      to raise_error ActiveRecord::RecordNotFound
-  end
+  context "when removing an org admin" do
+    let(:org_admin) { create(:user, :org_admin) }
 
-  it "leaves org" do
-    post "/users/leave_org"
-    expect(user.organization_id).to eq(nil)
-  end
+    before { sign_in org_admin }
 
-  it "adds org admin" do
-    add_org_admin
-    expect(User.last.org_admin).to eq(true)
-  end
+    it "removes org admin" do
+      user2 = create(:user, organization_id: org_admin.organization_id, org_admin: true)
+      post "/users/remove_org_admin", params: { user_id: user2.id }
+      expect(User.last.org_admin).to eq(false)
+    end
 
-  it "adds analytics role when adding org admin if org is approved" do
-    add_org_admin
-    expect(User.last.has_role?(:analytics_beta_tester)).to eq(true)
-  end
+    it "updates the correct org_membership association to a member level" do
+      user2 = create(:user, organization_id: org_admin.organization_id, org_admin: true)
+      org_membership = create(:organization_membership, organization_id: user2.organization_id, user_id: user2.id, type_of_user: "admin")
+      post "/users/remove_org_admin", params: { user_id: user2.id }
+      expect(org_membership.reload.type_of_user).to eq "member"
+    end
 
-  it "raises if user not org_admin" do
-    user.update(organization_id: organization.id)
-    user2 = create(:user, organization_id: organization.id, org_admin: true)
-    expect { post "/users/add_org_admin", params: { user_id: user2.id } }.
-      to raise_error Pundit::NotAuthorizedError
-  end
-
-  it "removes org admin" do
-    user.update(organization_id: organization.id, org_admin: true)
-    user2 = create(:user, organization_id: organization.id, org_admin: true)
-    post "/users/remove_org_admin", params: { user_id: user2.id }
-    expect(User.last.org_admin).to eq(false)
-  end
-
-  it "remove_org_admin raises if user not org_admin" do
-    user.update(organization_id: organization.id)
-    user2 = create(:user, organization_id: organization.id, org_admin: true)
-    expect { post "/users/remove_org_admin", params: { user_id: user2.id } }.
-      to raise_error Pundit::NotAuthorizedError
+    it "remove_org_admin raises if user not org_admin" do
+      user.update(organization_id: organization.id)
+      user2 = create(:user, organization_id: organization.id, org_admin: true)
+      expect { post "/users/remove_org_admin", params: { user_id: user2.id } }.
+        to raise_error Pundit::NotAuthorizedError
+    end
   end
 end