Improve factories

app/models/user.rb

@@ -159,6 +159,6 @@ def requires_name?
   end
 
   def requires_organisation?
-    organisation_id_was.present? || role_changed?(to: :organisation_admin)
+    organisation.blank? && (organisation_id_was.present? || role_changed?(to: :organisation_admin))
   end
 end

spec/factories/models/forms.rb

@@ -29,11 +29,11 @@
 
     trait :with_group do
       transient do
-        group { nil }
+        group { association :group }
       end
 
       after(:build) do |form, evaluator|
-        g = evaluator.group || FactoryBot.create(:group)
+        g = evaluator.group
         form.instance_variable_set(:@associated_group, g)
         form.define_singleton_method(:group) { g }
       end
@@ -61,7 +61,7 @@
       end
 
       pages do
-        Array.new(pages_count) { association(:page) }
+        Array.new(pages_count) { build(:page, form: nil) }
       end
 
       after(:build) do |form|
@@ -80,7 +80,7 @@
 
     trait :with_text_page do
       pages do
-        Array.new(1) { association(:page, answer_type: "text", answer_settings: { input_type: %w[single_line long_text].sample }) }
+        Array.new(1) { build(:page, answer_type: "text", answer_settings: { input_type: %w[single_line long_text].sample }, form: nil) }
       end
 
       question_section_completed { true }
@@ -148,7 +148,7 @@
       end
 
       pages do
-        Array.new(pages_count) { association(:page, :with_selection_settings) }
+        Array.new(pages_count) { build(:page, :with_selection_settings, form: nil) }
       end
 
       after(:build) do |form|

spec/factories/models/groups.rb

@@ -1,13 +1,13 @@
 FactoryBot.define do
   factory :group do
     sequence(:name) { |n| "Group #{n}" }
-    organisation { association :organisation, id: 1, slug: "test-org" }
+    organisation { association :organisation }
     creator { association :user, organisation: }
     status { :trial }
     external_id { SecureRandom.base58(8) }
 
     trait :org_has_org_admin do
-      organisation { association :organisation, :with_org_admin, id: 1, slug: "test-org" }
+      organisation { association :organisation, :with_org_admin }
     end
   end
 end

spec/factories/models/memberships.rb

@@ -1,8 +1,8 @@
 FactoryBot.define do
   factory :membership do
-    user
-    group
-    added_by { association :user }
+    user { build :user }
+    group { build :group, organisation: user&.organisation }
+    added_by { build :user, organisation: user&.organisation }
     role { :editor }
   end
 end

spec/factories/models/mou_signatures.rb

@@ -1,13 +1,13 @@
 FactoryBot.define do
   factory :mou_signature do
-    user { create(:user) }
+    user { build(:user) }
     organisation { user.organisation }
     agreement_type { :crown }
     created_at { Time.zone.now }
 
     factory :mou_signature_for_organisation do
       organisation
-      user { create(:user, organisation:) }
+      user { build(:user, organisation:) }
     end
   end
 end

spec/factories/models/organisation_domains.rb

@@ -1,6 +1,6 @@
 FactoryBot.define do
   factory :organisation_domain do
-    organisation { association :organisation, id: 1, slug: "test-org" }
+    organisation { association :organisation }
     domain { Faker::Internet.domain_name }
   end
 end

spec/factories/models/organisations.rb

@@ -1,26 +1,22 @@
 FactoryBot.define do
   factory :organisation do
     govuk_content_id { nil }
-    slug { "test-org" }
+    sequence(:slug) { |n| "test-org-#{n}" }
     name { slug.titleize }
     abbreviation { name.split.collect(&:chr).join }
     internal { false }
 
-    initialize_with do
-      Organisation.create_with(govuk_content_id:, name:).find_or_initialize_by(slug:)
-    end
-
     trait :with_signed_mou do
       after(:build) do |organisation|
-        organisation.mou_signatures << (create :mou_signature_for_organisation, organisation:)
+        organisation.mou_signatures << build(:mou_signature_for_organisation, organisation:)
       end
     end
 
     trait :with_org_admin do
       with_signed_mou
 
       after(:build) do |organisation|
-        organisation.users << create(:organisation_admin_user, organisation:)
+        organisation.users << build(:organisation_admin_user, organisation:)
       end
     end
   end

spec/factories/models/users.rb

@@ -26,10 +26,10 @@
       org_has_signed_mou
     end
 
-    organisation { association :organisation, id: 1, slug: "test-org" }
+    organisation { association :organisation }
 
     trait :org_has_signed_mou do
-      organisation { association :organisation, :with_signed_mou, id: 1, slug: "test-org" }
+      organisation { association :organisation, :with_signed_mou }
     end
 
     after(:build) do |user|

spec/features/form/create_or_edit_a_form_spec.rb

@@ -2,7 +2,7 @@
 
 feature "Create or edit a form", type: :feature do
   let(:form) { create :form, name: "Apply for a juggling license", created_at: "2024-10-08T07:31:15.762Z" }
-  let(:group) { create :group, name: "Group 1" }
+  let(:group) { create :group, name: "Group 1", organisation: standard_user.organisation }
 
   before do
     login_as_standard_user

spec/features/mou/upgrade_user_changes_mou_spec.rb

@@ -3,7 +3,7 @@
 describe "Assign an organisation to a user with a signed MOU", type: :feature do
   let(:user) { create :user, name: "Test User", organisation: nil }
   let!(:mou_signature) { create(:mou_signature, user:, organisation: nil, created_at: Time.zone.parse("September 1, 2023")) }
-  let(:organisation) { create :organisation }
+  let(:organisation) { test_org }
 
   it "a logged in user can sign an MOU" do
     login_as_super_admin_user

spec/input_objects/forms/group_select_spec.rb

@@ -31,17 +31,12 @@
       end
 
       context "when the organisation admin user is logged in" do
-        let(:org_admin) { build(:organisation_admin_user) }
-
-        it "returns only groups in the user's organisation" do
-          create_list(:group, 3) do |g|
-            g.organisation = group.organisation
-            g.save!
-          end
-          create(:group, organisation: build(:organisation)) # Group 5
+        it "returns only groups in the current group's organisation" do
+          create_list(:group, 3, organisation: group.organisation)
+          create(:group, organisation: build(:organisation)) # Group in a different organisation
 
           expect(group_select.groups).not_to include(group)
-          expect(group_select.groups.count).to eq(4)
+          expect(group_select.groups.count).to eq(3)
         end
       end
 

spec/integration/create_a_form_prevent_double_click_spec.rb

@@ -5,7 +5,7 @@
   let(:unwanted_form) { create :form, name: "Test form", created_at: form.created_at + 0.1 }
 
   let(:group) do
-    group = create :group, name: "Test group", creator: user
+    group = create :group, name: "Test group", creator: user, organisation: user.organisation
     create :membership, group:, user:, added_by: user
     group
   end

spec/integration/organisation_factory_spec.rb

@@ -1,13 +1,6 @@
 require "rails_helper"
 
 RSpec.describe "organisation factory" do
-  it "does not duplicate organisations with same slug" do
-    org1 = create :organisation, slug: "org-slug"
-    org2 = create :organisation, slug: "org-slug"
-
-    expect(org1.id).to eq org2.id
-  end
-
   it "does not persist the organisation" do
     org = build :organisation
 

spec/lib/tasks/users.rake_spec.rb

@@ -142,7 +142,7 @@
       let(:user_in_group) { users.fifth }
 
       before do
-        group = create :group, creator: users.first
+        group = create :group, creator: users.first, organisation: user_in_group.organisation
         create :membership, group:, user: user_in_group, added_by: users.first
       end
 
@@ -181,7 +181,7 @@
       end
 
       let(:user_in_group) { users.fifth }
-      let(:group_with_user) { create :group, creator: users.first }
+      let(:group_with_user) { create :group, creator: users.first, organisation: user_in_group.organisation }
 
       before do
         create :membership, group: group_with_user, user: user_in_group, added_by: users.first

spec/models/group_spec.rb

@@ -126,9 +126,10 @@
 
   describe "associations" do
     it "destroys associated memberships" do
-      group = create :group
-      user = create :user
-      added_by = create :user
+      organisation = create :organisation
+      group = create :group, organisation: organisation
+      user = create :user, organisation: organisation
+      added_by = create :user, organisation: organisation
       create(:membership, group:, user:, added_by:)
 
       expect { group.destroy }.to change(Membership, :count).by(-1)
@@ -151,11 +152,12 @@
     describe "#memberships" do
       describe "#ordered" do
         it "orders the membership records by name of the associated user" do
-          group = create :group
+          organisation = create :organisation
+          group = create :group, organisation: organisation
           users = [
-            create(:user, name: "Barbara User"),
-            create(:user, name: "Alfred User"),
-            create(:user, name: "Charlie User"),
+            create(:user, name: "Barbara User", organisation: organisation),
+            create(:user, name: "Alfred User", organisation: organisation),
+            create(:user, name: "Charlie User", organisation: organisation),
           ]
 
           users.each do |user|
@@ -260,9 +262,10 @@
   describe "scopes" do
     describe ".for_user" do
       it "returns groups that the user is a member of" do
-        user = create :user
-        group1 = create :group
-        group2 = create :group
+        organisation = create :organisation
+        user = create :user, organisation: organisation
+        group1 = create :group, organisation: organisation
+        group2 = create :group, organisation: organisation
         create :group
         create :membership, user:, group: group1
         create :membership, user:, group: group2
@@ -278,8 +281,9 @@
       end
 
       it "does not return groups that the user is not a member of" do
-        user = create :user
-        group1 = create :group
+        organisation = create :organisation
+        user = create :user, organisation: organisation
+        group1 = create :group, organisation: organisation
         create :group
         create :group
         create :membership, user:, group: group1

spec/models/membership_spec.rb

@@ -35,16 +35,18 @@
   end
 
   it "is invalid if the user is already a member of the group" do
-    user = create :user
-    group = create :group
+    organisation = create :organisation
+    user = create :user, organisation: organisation
+    group = create :group, organisation: organisation
     create(:membership, user:, group:)
     membership = build(:membership, user:, group:)
     expect(membership).not_to be_valid
   end
 
   it "raises a DB error if the user is already a member of the group" do
-    user = create :user
-    group = create :group
+    organisation = create :organisation
+    user = create :user, organisation: organisation
+    group = create :group, organisation: organisation
     create(:membership, user:, group:)
     membership = build(:membership, user:, group:)
     expect { membership.save(validate: false) }.to raise_error(ActiveRecord::RecordNotUnique)

spec/models/organisation_spec.rb

@@ -9,19 +9,6 @@
     }.to raise_error ActiveRecord::RecordNotUnique
   end
 
-  describe "factory" do
-    it "does not create organisation if already exists" do
-      existing_organisation = create(:organisation, slug: "duplicate-org")
-      new_organisation = nil
-
-      expect {
-        new_organisation = create(:organisation, slug: "duplicate-org")
-      }.not_to raise_error
-
-      expect(new_organisation).to eq(existing_organisation)
-    end
-  end
-
   describe "versioning", :versioning do
     it "enables paper trail" do
       expect(described_class.new).to be_versioned

spec/models/user_spec.rb

@@ -58,6 +58,16 @@
       end
     end
 
+    context "when reassigning to a new, unsaved organisation" do
+      let(:user) { create(:user, organisation: create(:organisation)) }
+
+      it "is valid even though the new organisation is not yet persisted" do
+        user.organisation = build(:organisation)
+
+        expect(user).to be_valid
+      end
+    end
+
     context "when updating name" do
       let(:user) { create :user, :with_no_name }
 
@@ -128,7 +138,7 @@
   describe "associations" do
     it "destroys associated memberships" do
       user = create(:user)
-      group = create(:group)
+      group = create(:group, organisation: user.organisation)
 
       membership = create(:membership, user:, group:)
       user.destroy!
@@ -364,7 +374,7 @@
 
     it "returns false when user had an organisation" do
       user = create(:user)
-      user.update!(organisation: build(:organisation))
+      user.update!(organisation: create(:organisation))
       expect(user).not_to be_given_organisation
     end
   end
@@ -380,9 +390,9 @@
 
     context "when the user is an organisation admin" do
       it "returns true" do
-        user = create(:organisation_admin_user)
+        user = create(:organisation_admin_user, organisation: create(:organisation, :with_signed_mou))
 
-        expect(user.can_administer_org?(organisation)).to be(true)
+        expect(user.can_administer_org?(user.organisation)).to be(true)
       end
     end
 
@@ -414,7 +424,7 @@
       context "and the user's organisation is not the same as given" do
         it "returns false" do
           user = create(:organisation_admin_user)
-          other_org = build(:organisation, id: 2)
+          other_org = build(:organisation)
 
           expect(user.is_organisations_admin?(other_org)).to be(false)
         end

spec/policies/form_policy_spec.rb

@@ -184,13 +184,13 @@
     end
 
     context "when the user is a super admin" do
-      let(:user) { build :super_admin_user }
+      let(:user) { build :super_admin_user, organisation: }
 
       it { is_expected.to permit_action(:can_administer_group) }
     end
 
     context "when the user is an organisation admin" do
-      let(:user) { build :organisation_admin_user }
+      let(:user) { build :organisation_admin_user, organisation: }
 
       it { is_expected.to permit_action(:can_administer_group) }
     end