Merge pull request #2478 from UKGovernmentBEIS/chore/refactor-active-…

…deactivated-scopes

Refactor user scopes

app/controllers/users_controller.rb

@@ -4,11 +4,10 @@ class UsersController < BaseController
   def index
     authorize :user, :index?
     @user_state = params[:user_state]
-    @users = policy_scope(User).includes(:organisation).joins(:organisation).order("organisations.name ASC, users.name ASC")
     @users = if @user_state == "active"
-      @users.active
+      policy_scope(User).all_active
     else
-      @users.deactivated
+      policy_scope(User).all_deactivated
     end
   end
 

app/models/user.rb

@@ -15,7 +15,14 @@ class User < ApplicationRecord
   }.freeze
 
   scope :active, -> { where(deactivated_at: nil) }
-  scope :deactivated, -> { where.not(deactivated_at: nil).order(deactivated_at: :asc) }
+  scope :deactivated, -> { where.not(deactivated_at: nil) }
+
+  scope :all_active, -> {
+    active.includes(:organisation).joins(:organisation).order("organisations.name ASC, users.name ASC")
+  }
+  scope :all_deactivated, -> {
+    deactivated.includes(:organisation).joins(:organisation).order("users.deactivated_at ASC, organisations.name ASC, users.name ASC")
+  }
 
   delegate :service_owner?, :partner_organisation?, to: :organisation
 

spec/features/beis_users_can_view_other_users_spec.rb

@@ -45,30 +45,6 @@
       expect(page).to have_content("Mobile number confirmed for authentication? Yes")
     end
 
-    scenario "users are grouped by their organisation name in alphabetical order" do
-      a_organisation = create(:partner_organisation, name: "A Organisation")
-      b_organisation = create(:partner_organisation, name: "B Organisation")
-
-      a1_user = create(:administrator, organisation: a_organisation)
-      a2_user = create(:administrator, organisation: a_organisation)
-      b1_user = create(:administrator, organisation: b_organisation)
-      b2_user = create(:administrator, organisation: b_organisation)
-
-      # Navigate from the landing page
-      visit organisation_path(user.organisation)
-      click_on(t("page_title.users.index"))
-
-      expected_array = [
-        a1_user.organisation.name,
-        a2_user.organisation.name,
-        b1_user.organisation.name,
-        b2_user.organisation.name,
-        user.organisation.name
-      ].sort
-
-      expect(page.all("td.organisation").collect(&:text)).to match_array(expected_array)
-    end
-
     scenario "an inactive user can be viewed" do
       another_user = create(:inactive_user, deactivated_at: DateTime.now - 1.hour)
 

spec/models/user_spec.rb

@@ -111,27 +111,66 @@
   end
 
   describe "scopes" do
-    it "shows active users for active scope" do
-      create(:administrator, deactivated_at: nil)
-      create(:administrator, deactivated_at: nil)
-      create(:administrator, deactivated_at: DateTime.yesterday)
+    describe "#active" do
+      it "shows only active users for active scope" do
+        active_user = create(:partner_organisation_user, deactivated_at: nil)
+        deactivated_user = create(:partner_organisation_user, deactivated_at: DateTime.yesterday)
 
-      expect(User.active.size).to eq(2)
-      expect(User.active.last.active).to eq(true)
+        scoped_users = User.active
+
+        expect(scoped_users).to include active_user
+        expect(scoped_users).not_to include deactivated_user
+      end
     end
 
     describe "#deactivated" do
-      it "shows deactivated users order by ago, oldest first then name" do
-        create(:partner_organisation_user, name: "A User", deactivated_at: DateTime.yesterday)
-        create(:partner_organisation_user, name: "B User", deactivated_at: DateTime.now - 1.week)
-        create(:partner_organisation_user, name: "C User", deactivated_at: DateTime.now - 1.year)
-        create(:partner_organisation_user, name: "Z User", deactivated_at: DateTime.now - 1.year)
+      it "shows only deactivated users" do
+        active_user = create(:partner_organisation_user, deactivated_at: nil)
+        deactivated_user = create(:partner_organisation_user, deactivated_at: DateTime.yesterday)
 
         scoped_users = User.deactivated
 
-        expect(scoped_users.first.name).to eql "C User"
-        expect(scoped_users.second.name).to eql "Z User"
+        expect(scoped_users).to include deactivated_user
+        expect(scoped_users).not_to include active_user
+      end
+    end
+
+    describe "#index_active" do
+      it "shows only active users sorted by the organisation and then name" do
+        first_organisation = create(:partner_organisation, name: "A Organisation")
+        last_organisation = create(:partner_organisation, name: "B Organisation")
+
+        create(:partner_organisation_user, name: "A User", organisation: last_organisation)
+        create(:partner_organisation_user, name: "B User", organisation: last_organisation)
+        create(:partner_organisation_user, name: "Z User", organisation: first_organisation)
+
+        scoped_users = User.all_active
+
+        expect(scoped_users.first.name).to eql "Z User"
+        expect(scoped_users.second.name).to eql "A User"
         expect(scoped_users.third.name).to eql "B User"
+      end
+    end
+
+    describe "#index_deactivated" do
+      it "shows only deactivated users sorted by the deactivated at time, then organisation and then name" do
+        first_organisation = create(:partner_organisation, name: "A Organisation")
+        last_organisation = create(:partner_organisation, name: "B Organisation")
+
+        travel_to(DateTime.now) do
+          create(:partner_organisation_user, name: "A User", deactivated_at: DateTime.yesterday)
+          create(:partner_organisation_user, name: "B User", deactivated_at: DateTime.now - 1.week)
+          create(:partner_organisation_user, name: "C User", organisation: last_organisation, deactivated_at: DateTime.now - 1.year)
+          create(:partner_organisation_user, name: "D User", organisation: first_organisation, deactivated_at: DateTime.now - 1.year)
+          create(:partner_organisation_user, name: "Z User", deactivated_at: DateTime.now - 2.year)
+        end
+
+        scoped_users = User.all_deactivated
+
+        expect(scoped_users.first.name).to eql "Z User"
+        expect(scoped_users.second.name).to eql "D User"
+        expect(scoped_users.third.name).to eql "C User"
+        expect(scoped_users.fourth.name).to eql "B User"
         expect(scoped_users.last.name).to eql "A User"
       end
     end