GRN2-xx: Made role priority unique scoped to provider (#942)

* Made role priority unique scoped to provider

* Fixed issues related to update_role after making role priority unique

app/controllers/concerns/populator.rb

@@ -74,7 +74,7 @@ module Populator
   def shared_user_list
     roles_can_appear = []
     Role.where(provider: @user_domain).each do |role|
-      roles_can_appear << role.name if role.get_permission("can_appear_in_share_list") && role.name != "super_admin"
+      roles_can_appear << role.name if role.get_permission("can_appear_in_share_list") && role.priority >= 0
     end
 
     initial_list = User.where.not(uid: current_user.uid)

app/controllers/concerns/rolify.rb

@@ -119,17 +119,32 @@ module Rolify
       return false if role.priority <= current_user_role.priority || role.provider != @user_domain
     end
 
-    # Update the roles priority including the user role
+    # Get the priority of the current user's role and start with 1 higher
-    top_priority = 0
+    new_priority = [current_user_role.priority, 0].max + 1
 
-    role_to_update.each_with_index do |id, index|
+    begin
-      new_priority = index + [current_user_role.priority, 0].max + 1
+      # Save the old priorities incase something fails
-      top_priority = new_priority
+      old_priority = Role.where(id: role_to_update).select(:id, :priority).index_by(&:id)
-      Role.where(id: id).update_all(priority: new_priority)
+
+      # Set all the priorities to nil to avoid unique column issues
+      Role.where(id: role_to_update).update_all(priority: nil)
+
+      # Starting at the starting priority, increase by 1 every time
+      role_to_update.each_with_index do |id, index|
+        Role.find(id).update_attribute(:priority, new_priority + index)
+      end
+
+      true
+    rescue => e
+      # Reset to old prorities
+      role_to_update.each_with_index do |id, _index|
+        Role.find(id).update_attribute(:priority, old_priority[id.to_i].priority)
+      end
+
+      logger.error "#{current_user} failed to update role priorities: #{e}"
+
+      false
     end
-
-    user_role.priority = top_priority + 1
-    user_role.save!
   end
 
   # Update Permissions

app/models/role.rb

@@ -38,8 +38,8 @@ class Role < ApplicationRecord
       send_demoted_email: true, can_edit_site_settings: true, can_manage_rooms_recordings: true,
       can_edit_roles: true, can_manage_users: true)
     Role.create(name: "pending", provider: provider, priority: -1, colour: "#17a2b8").update_all_role_permissions
-    Role.create(name: "denied", provider: provider, priority: -1, colour: "#343a40").update_all_role_permissions
+    Role.create(name: "denied", provider: provider, priority: -2, colour: "#343a40").update_all_role_permissions
-    Role.create(name: "super_admin", provider: provider, priority: -2, colour: "#cd201f")
+    Role.create(name: "super_admin", provider: provider, priority: -3, colour: "#cd201f")
         .update_all_role_permissions(can_create_rooms: true,
       send_promoted_email: true, send_demoted_email: true, can_edit_site_settings: true,
       can_edit_roles: true, can_manage_users: true, can_manage_rooms_recordings: true,
@@ -56,8 +56,8 @@ class Role < ApplicationRecord
     role.priority = user_role.priority
     user_role.priority += 1
 
-    role.save!
     user_role.save!
+    role.save!
 
     role
   end

app/models/user.rb

@@ -238,9 +238,9 @@ class User < ApplicationRecord
   end
 
   def self.all_users_highest_priority_role
-    User.joins("INNER JOIN (SELECT user_id, role_id, min(roles.priority) FROM users_roles " \
+    User.joins("INNER JOIN (SELECT user_id, min(roles.priority) as role_priority FROM users_roles " \
       "INNER JOIN roles ON users_roles.role_id = roles.id GROUP BY user_id) as a ON " \
-      "a.user_id = users.id INNER JOIN roles ON roles.id = a.role_id " \
+      "a.user_id = users.id INNER JOIN roles ON roles.priority = a.role_priority " \
       " INNER JOIN role_permissions ON roles.id = role_permissions.role_id").distinct
   end
 

app/views/admins/components/_menu_buttons.html.erb

@@ -21,13 +21,15 @@
       <span class="icon mr-3"><i class="fas fa-users"></i></span><%= t("administrator.users.title") %>
     <% end %>
   <% end %>
-  <% if highest_role.get_permission("can_edit_site_settings") || highest_role.name == "super_admin" %>
+  <% if highest_role.get_permission("can_manage_rooms_recordings") || highest_role.name == "super_admin" %>
     <%= link_to admin_rooms_path, class: "list-group-item list-group-item-action dropdown-item #{"active" if active_page == "server_rooms"}" do %>
       <span class="icon mr-4"><i class="fas fa-binoculars"></i></span><%= t("administrator.rooms.title") %>
     <% end %>
     <%= link_to admin_recordings_path, class: "list-group-item list-group-item-action dropdown-item #{"active" if active_page == "server_recordings"}" do %>
       <span class="icon mr-4"><i class="fas fa-video"></i></span><%= t("administrator.recordings.title") %>
     <% end %>
+  <% end %>
+  <% if highest_role.get_permission("can_edit_site_settings") || highest_role.name == "super_admin" %>
     <%= link_to admin_site_settings_path, class: "list-group-item list-group-item-action dropdown-item #{"active" if active_page == "site_settings"}" do %>
       <span class="icon mr-4"><i class="fas fa-cogs"></i></span><%= t("administrator.site_settings.title") %>
     <% end %>

app/views/shared/_header.html.erb

@@ -63,6 +63,10 @@
                 <%= link_to admins_path, class: "dropdown-item" do %>
                   <i class="dropdown-icon fas fa-user-tie mr-3"></i><%= t("header.dropdown.account_settings") %>
                 <% end %>
+              <% elsif highest_role.get_permission("can_manage_rooms_recordings")%>
+                <%= link_to admin_rooms_path, class: "dropdown-item" do %>
+                  <i class="dropdown-icon fas fa-user-tie mr-3"></i><%= t("header.dropdown.account_settings") %>
+                <% end %>
               <% elsif highest_role.get_permission("can_edit_site_settings") %>
                 <%= link_to admin_site_settings_path, class: "dropdown-item" do %>
                   <i class="dropdown-icon fas fa-user-tie mr-3"></i><%= t("header.dropdown.account_settings") %>

db/migrate/20200130144841_change_role_priority_to_unique.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+class MigrationProduct < ActiveRecord::Base
+  self.table_name = :roles
+end
+
+class ChangeRolePriorityToUnique < ActiveRecord::Migration[5.2]
+  def change
+    reversible do |dir|
+      dir.up do
+        MigrationProduct.where("priority < 0").where.not(name: "pending").each do |role|
+          role.decrement!(:priority)
+        end
+
+        add_index MigrationProduct, [:priority, :provider], unique: true
+      end
+
+      dir.down do
+        remove_index MigrationProduct, [:priority, :provider]
+
+        MigrationProduct.where("priority < 0").where.not(name: "pending").each do |role|
+          role.increment!(:priority)
+        end
+      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: 2019_11_28_212935) do
+ActiveRecord::Schema.define(version: 2020_01_30_144841) do
 
   create_table "features", force: :cascade do |t|
     t.integer "setting_id"
@@ -58,6 +58,7 @@ ActiveRecord::Schema.define(version: 2019_11_28_212935) do
     t.datetime "updated_at", null: false
     t.index ["name", "provider"], name: "index_roles_on_name_and_provider", unique: true
     t.index ["name"], name: "index_roles_on_name"
+    t.index ["priority", "provider"], name: "index_roles_on_priority_and_provider", unique: true
   end
 
   create_table "rooms", force: :cascade do |t|

spec/controllers/admins_controller_spec.rb

@@ -497,6 +497,7 @@ describe AdminsController, type: :controller do
     context "PATCH #change_role_order" do
       before do
         Role.create_default_roles("provider1")
+        @user.roles.delete(Role.find_by(name: "user", provider: "greenlight"))
       end
 
       it "should fail if user attempts to change the order of the admin or user roles" do
@@ -512,35 +513,9 @@ describe AdminsController, type: :controller do
       end
 
       it "should fail if a user attempts to edit a role with a higher priority than their own" do
-        Role.create(name: "test1", priority: 1, provider: "greenlight")
+        new_role3 = Role.create_new_role("test3", "provider1")
-        new_role2 = Role.create(name: "test2", priority: 2, provider: "greenlight")
+        new_role2 = Role.create_new_role("test2", "provider1")
         new_role2.update_permission("can_edit_roles", "true")
-        new_role3 = Role.create(name: "test3", priority: 3, provider: "greenlight")
-        user_role = Role.find_by(name: "user", provider: "greenlight")
-
-        user_role.priority = 4
-        user_role.save!
-
-        @user.roles << new_role2
-        @user.save!
-
-        @request.session[:user_id] = @user.id
-
-        patch :change_role_order, params: { role: [new_role3.id, new_role2.id] }
-
-        expect(flash[:alert]).to eq(I18n.t("administrator.roles.invalid_order"))
-        expect(response).to redirect_to admin_roles_path
-      end
-
-      it "should fail if a user attempts to edit a role with a higher priority than their own" do
-        Role.create(name: "test1", priority: 1, provider: "greenlight")
-        new_role2 = Role.create(name: "test2", priority: 2, provider: "greenlight")
-        new_role2.update_permission("can_edit_roles", "true")
-        new_role3 = Role.create(name: "test3", priority: 3, provider: "greenlight")
-        user_role = Role.find_by(name: "user", provider: "greenlight")
-
-        user_role.priority = 4
-        user_role.save!
 
         @user.roles << new_role2
         @user.save!
@@ -554,10 +529,11 @@ describe AdminsController, type: :controller do
       end
 
       it "should update the role order" do
+        user_role = Role.find_by(name: "user", provider: "provider1")
+        user_role.update_attribute(:priority, 4)
         new_role1 = Role.create(name: "test1", priority: 1, provider: "provider1")
         new_role2 = Role.create(name: "test2", priority: 2, provider: "provider1")
         new_role3 = Role.create(name: "test3", priority: 3, provider: "provider1")
-        user_role = Role.find_by(name: "user", provider: "provider1")
 
         @request.session[:user_id] = @admin.id
 
@@ -578,16 +554,15 @@ describe AdminsController, type: :controller do
     context 'POST #update_role' do
       before do
         Role.create_default_roles("provider1")
+        @user.roles.delete(Role.find_by(name: "user", provider: "greenlight"))
       end
 
       it "should fail to update a role with a lower priority than the user" do
+        user_role = Role.find_by(name: "user", provider: "provider1")
+        user_role.update_attribute(:priority, 3)
         new_role1 = Role.create(name: "test1", priority: 1, provider: "provider1")
         new_role2 = Role.create(name: "test2", priority: 2, provider: "provider1")
         new_role2.update_permission("can_edit_roles", "true")
-        user_role = Role.find_by(name: "user", provider: "greenlight")
-
-        user_role.priority = 3
-        user_role.save!
 
         @user.roles << new_role2
         @user.save!
@@ -601,7 +576,7 @@ describe AdminsController, type: :controller do
       end
 
       it "should fail to update if there is a duplicate name" do
-        new_role = Role.create(name: "test2", priority: 1, provider: "provider1")
+        new_role = Role.create(name: "test2", priority: 2, provider: "provider1")
         new_role.update_permission("can_edit_roles", "true")
 
         @request.session[:user_id] = @admin.id
@@ -613,7 +588,7 @@ describe AdminsController, type: :controller do
       end
 
       it "should update role permisions" do
-        new_role = Role.create(name: "test2", priority: 1, provider: "provider1")
+        new_role = Role.create(name: "test2", priority: 2, provider: "provider1")
         new_role.update_permission("can_edit_roles", "true")
 
         @request.session[:user_id] = @admin.id
@@ -658,7 +633,7 @@ describe AdminsController, type: :controller do
       end
 
       it "should successfully delete the role" do
-        new_role = Role.create(name: "test2", priority: 1, provider: "provider1")
+        new_role = Role.create(name: "test2", priority: 2, provider: "provider1")
         new_role.update_permission("can_edit_roles", "true")
 
         @request.session[:user_id] = @admin.id

spec/controllers/users_controller_spec.rb

@@ -312,7 +312,7 @@ describe UsersController, type: :controller do
 
         user_role.save!
 
-        tmp_role = Role.create(name: "test", priority: -2, provider: "greenlight")
+        tmp_role = Role.create(name: "test", priority: -4, provider: "greenlight")
 
         params = random_valid_user_params
         patch :update, params: params.merge!(user_uid: user, user: { role_ids: tmp_role.id.to_s })
@@ -354,9 +354,9 @@ describe UsersController, type: :controller do
 
         @request.session[:user_id] = admin.id
 
-        tmp_role1 = Role.create(name: "test1", priority: 1, provider: "greenlight")
+        tmp_role1 = Role.create(name: "test1", priority: 2, provider: "greenlight")
         tmp_role1.update_permission("send_promoted_email", "true")
-        tmp_role2 = Role.create(name: "test2", priority: 2, provider: "greenlight")
+        tmp_role2 = Role.create(name: "test2", priority: 3, provider: "greenlight")
 
         params = random_valid_user_params
         params = params.merge!(user_uid: user, user: { role_ids: "#{tmp_role1.id} #{tmp_role2.id}" })
@@ -375,7 +375,7 @@ describe UsersController, type: :controller do
 
         admin.add_role :admin
 
-        tmp_role1 = Role.create(name: "test1", priority: 1, provider: "greenlight")
+        tmp_role1 = Role.create(name: "test1", priority: 2, provider: "greenlight")
         tmp_role1.update_permission("send_demoted_email", "true")
         user.roles << tmp_role1
         user.save!