refactor: remove more test order dependence (#4273)

* fix: remove more test order dependence

As discussed here #4264,
some tests modify global variables and expect them to
be reset before each test. This makes it more difficult
to move forward with removal of pre-seeding.

This commit refactors certain tests to be less brittle to
changes in global state and to rely less on assumptions
on global state. Ex: not assuming that there are 0 records
of a certain type when a test starts, etc.

* fix: minor cleanup / lint

spec/controllers/application_controller_spec.rb

@@ -1,4 +1,4 @@
-describe ApplicationController do
+RSpec.describe ApplicationController do
   describe "current_organization" do
     before(:each) do
       allow(controller).to receive(:current_user).and_return(user)

spec/jobs/reminder_deadline_job_spec.rb

@@ -1,6 +1,6 @@
 require 'rails_helper'
 
-describe ReminderDeadlineJob, type: :job do
+RSpec.describe ReminderDeadlineJob, type: :job do
   describe '#perform' do
     subject { -> { described_class.perform_now } }
     let(:partner) { create(:partner) }

spec/pdfs/distribution_pdf_spec.rb

@@ -1,6 +1,6 @@
 # avoid Rubocop failing with an infinite loop when it checks this cop
 # rubocop:disable Layout/ArrayAlignment
-describe DistributionPdf do
+RSpec.describe DistributionPdf do
   let(:distribution) { FactoryBot.create(:distribution) }
   let(:item1) { FactoryBot.create(:item, name: "Item 1", package_size: 50, value_in_cents: 100) }
   let(:item2) { FactoryBot.create(:item, name: "Item 2", value_in_cents: 200) }

spec/requests/distributions_by_county_spec.rb

@@ -2,7 +2,7 @@
 
 RSpec.describe "DistributionsByCounties", type: :request do
   let(:default_params) do
-    {organization_name: @organization.to_param}
+    {organization_name: organization.to_param}
   end
 
   include_examples "distribution_by_county"
@@ -15,11 +15,11 @@
 
     context "While signed in as bank" do
       before do
-        sign_in(@user)
+        sign_in(user)
       end
 
       it "shows 'Unspecified 100%' if no served_areas" do
-        create(:distribution, :with_items, item: item_1, organization: @user.organization)
+        create(:distribution, :with_items, item: item_1, organization: organization)
         get distributions_by_county_report_path(default_params)
         expect(response.body).to include("Unspecified")
         expect(response.body).to include("100")
@@ -28,8 +28,8 @@
 
       context "basic behaviour with served areas" do
         it "handles multiple partners with overlapping service areas properly" do
-          create(:distribution, :with_items, item: item_1, organization: @user.organization, partner: partner_1, issued_at: issued_at_present)
-          create(:distribution, :with_items, item: item_1, organization: @user.organization, partner: partner_2, issued_at: issued_at_present)
+          create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_1, issued_at: issued_at_present)
+          create(:distribution, :with_items, item: item_1, organization: organization, partner: partner_2, issued_at: issued_at_present)
 
           get distributions_by_county_report_path(default_params)
 

spec/requests/distributions_requests_spec.rb

@@ -1,21 +1,25 @@
 require 'rails_helper'
 
 RSpec.describe "Distributions", type: :request do
+  let(:organization) { create(:organization, skip_items: true) }
+  let(:user) { create(:user, organization: organization) }
+  let(:organization_admin) { create(:organization_admin, organization: organization) }
+
   let(:default_params) do
-    { organization_name: @organization.to_param }
+    { organization_name: organization.to_param }
   end
 
   let(:secret_key) { "HI MOM THIS IS ME AND I'M CODING" }
   let(:crypt) { ActiveSupport::MessageEncryptor.new(secret_key) }
-  let(:hashed_id) { CGI.escape(crypt.encrypt_and_sign(@organization.id)) }
+  let(:hashed_id) { CGI.escape(crypt.encrypt_and_sign(organization.id)) }
   before(:each) do
     allow(Rails.application).to receive(:secret_key_base).and_return(secret_key)
     allow(DistributionPdf).to receive(:new).and_return(double("DistributionPdf", compute_and_render: "PDF"))
   end
 
   context "While signed in" do
     before do
-      sign_in(@user)
+      sign_in(user)
     end
 
     describe "GET #itemized_breakdown" do
@@ -51,22 +55,22 @@
 
     describe "GET #reclaim" do
       it "returns http success" do
-        get distributions_path(default_params.merge(organization_name: @organization, id: create(:distribution).id))
+        get distributions_path(default_params.merge(organization_name: organization, id: create(:distribution).id))
         expect(response).to be_successful
       end
     end
 
     describe "GET #index" do
-      let(:item) { create(:item) }
-      let!(:distribution) { create(:distribution, :with_items, :past, item: item, item_quantity: 10) }
+      let(:item) { create(:item, organization: organization) }
+      let!(:distribution) { create(:distribution, :with_items, :past, item: item, item_quantity: 10, organization: organization) }
 
       it "returns http success" do
         get distributions_path(default_params)
         expect(response).to be_successful
       end
 
       it "sums distribution totals accurately" do
-        create(:distribution, :with_items, item_quantity: 5)
+        create(:distribution, :with_items, item_quantity: 5, organization: organization)
         create(:line_item, :distribution, itemizable_id: distribution.id, quantity: 7)
         get distributions_path(default_params)
         expect(assigns(:total_items_all_distributions)).to eq(22)
@@ -101,8 +105,8 @@
     end
 
     describe "POST #create" do
-      let!(:storage_location) { create(:storage_location) }
-      let!(:partner) { create(:partner) }
+      let!(:storage_location) { create(:storage_location, organization: organization) }
+      let!(:partner) { create(:partner, organization: organization) }
       let(:distribution) do
         { distribution: { storage_location_id: storage_location.id, partner_id: partner.id, delivery_method: :delivery } }
       end
@@ -128,10 +132,10 @@
     end
 
     describe "GET #new" do
-      let!(:partner) { create(:partner) }
-      let(:request) { create(:request, partner: partner) }
-      let(:storage_location) { create(:storage_location, :with_items) }
-      let(:default_params) { { organization_name: @organization.to_param, request_id: request.id } }
+      let!(:partner) { create(:partner, organization: organization) }
+      let(:request) { create(:request, partner: partner, organization: organization) }
+      let(:storage_location) { create(:storage_location, :with_items, organization: organization) }
+      let(:default_params) { { organization_name: organization.to_param, request_id: request.id } }
 
       it "returns http success" do
         get new_distribution_path(default_params)
@@ -143,7 +147,7 @@
 
       context "with org default but no partner default" do
         it "selects org default" do
-          @organization.update!(default_storage_location: storage_location.id)
+          organization.update!(default_storage_location: storage_location.id)
           get new_distribution_path(default_params)
           expect(response).to be_successful
           page = Nokogiri::HTML(response.body)
@@ -154,7 +158,7 @@
       context "with partner default" do
         it "selects partner default" do
           location2 = create(:storage_location, :with_items)
-          @organization.update!(default_storage_location: location2.id)
+          organization.update!(default_storage_location: location2.id)
           partner.update!(default_storage_location_id: storage_location.id)
           get new_distribution_path(default_params)
           expect(response).to be_successful
@@ -165,11 +169,11 @@
     end
 
     describe "GET #show" do
-      let(:item) { create(:item) }
-      let!(:distribution) { create(:distribution, :with_items, item: item, item_quantity: 1) }
+      let(:item) { create(:item, organization: organization) }
+      let!(:distribution) { create(:distribution, :with_items, item: item, item_quantity: 1, organization: organization) }
 
       it "sums distribution totals accurately" do
-        distribution = create(:distribution, :with_items, item_quantity: 1)
+        distribution = create(:distribution, :with_items, item_quantity: 1, organization: organization)
 
         item_quantity = 6
         package_size = 2
@@ -219,15 +223,15 @@
         page = Nokogiri::HTML(response.body)
         url = page.at_css('#copy-calendar-button').attributes['data-url'].value
         hash = url.match(/\?hash=(.*)&/)[1]
-        expect(crypt.decrypt_and_verify(CGI.unescape(hash))).to eq(@organization.id)
+        expect(crypt.decrypt_and_verify(CGI.unescape(hash))).to eq(organization.id)
       end
     end
 
     describe 'PATCH #picked_up' do
       subject { patch picked_up_distribution_path(default_params.merge(id: distribution.id)) }
 
       context 'when the distribution is successfully updated' do
-        let(:distribution) { create(:distribution, state: :scheduled) }
+        let(:distribution) { create(:distribution, state: :scheduled, organization: organization) }
 
         it "updates the state to 'complete'" do
           subject
@@ -247,10 +251,10 @@
       end
 
       it "correctly sums the item counts from distributions" do
-        first_item = create(:item)
-        second_item = create(:item)
-        first_distribution = create(:distribution)
-        second_distribution = create(:distribution)
+        first_item = create(:item, organization: organization)
+        second_item = create(:item, organization: organization)
+        first_distribution = create(:distribution, organization: organization)
+        second_distribution = create(:distribution, organization: organization)
         create(:line_item, :distribution, item_id: first_item.id, itemizable_id: first_distribution.id, quantity: 7)
         create(:line_item, :distribution, item_id: first_item.id, itemizable_id: second_distribution.id, quantity: 4)
         create(:line_item, :distribution, item_id: second_item.id, itemizable_id: second_distribution.id, quantity: 5)
@@ -261,10 +265,10 @@
       end
 
       it "correctly sums the item package counts from distributions" do
-        first_item = create(:item, package_size: 2)
-        second_item = create(:item, package_size: 3)
-        first_distribution = create(:distribution)
-        second_distribution = create(:distribution)
+        first_item = create(:item, package_size: 2, organization: organization)
+        second_item = create(:item, package_size: 3, organization: organization)
+        first_distribution = create(:distribution, organization: organization)
+        second_distribution = create(:distribution, organization: organization)
 
         create(:line_item, :distribution, item_id: first_item.id, itemizable_id: first_distribution.id, quantity: 7)
         create(:line_item, :distribution, item_id: first_item.id, itemizable_id: second_distribution.id, quantity: 4)
@@ -282,10 +286,10 @@
     end
 
     describe "POST #update" do
-      let(:location) { create(:storage_location) }
-      let(:partner) { create(:partner) }
+      let(:location) { create(:storage_location, organization: organization) }
+      let(:partner) { create(:partner, organization: organization) }
 
-      let(:distribution) { create(:distribution, partner: partner) }
+      let(:distribution) { create(:distribution, partner: partner, organization: organization) }
       let(:issued_at) { distribution.issued_at }
       let(:distribution_params) do
         default_params.merge(
@@ -306,11 +310,11 @@
       end
 
       describe "when changing storage location" do
-        let(:item) { create(:item) }
+        let(:item) { create(:item, organization: organization) }
         it "updates storage quantity correctly" do
-          new_storage_location = create(:storage_location)
-          create(:donation, :with_items, item: item, item_quantity: 30, storage_location: new_storage_location)
-          distribution = create(:distribution, :with_items, item: item, item_quantity: 10)
+          new_storage_location = create(:storage_location, organization: organization)
+          create(:donation, :with_items, item: item, item_quantity: 30, storage_location: new_storage_location, organization: organization)
+          distribution = create(:distribution, :with_items, item: item, item_quantity: 10, organization: organization)
           original_storage_location = distribution.storage_location
           line_item = distribution.line_items.first
           line_item_params = {
@@ -330,9 +334,9 @@
 
         # TODO this test is invalid in event-world since it's handled by the aggregate
         it "rollsback updates if quantity would go below 0" do
-          next if Event.read_events?(@organization)
+          next if Event.read_events?(organization)
 
-          distribution = create(:distribution, :with_items, item_quantity: 10)
+          distribution = create(:distribution, :with_items, item_quantity: 10, organization: organization)
           original_storage_location = distribution.storage_location
 
           # adjust inventory so that updating will set quantity below 0
@@ -387,8 +391,8 @@
     end
 
     describe "GET #edit" do
-      let(:location) { create(:storage_location) }
-      let(:partner) { create(:partner) }
+      let(:location) { create(:storage_location, organization: organization) }
+      let(:partner) { create(:partner, organization: organization) }
 
       let(:distribution) { create(:distribution, partner: partner) }
 
@@ -400,7 +404,7 @@
 
       it "should show a warning if there is an inteverning audit" do
         distribution.update!(created_at: 1.week.ago)
-        create(:audit, storage_location: distribution.storage_location)
+        create(:audit, storage_location: distribution.storage_location, organization: organization)
         get edit_distribution_path(default_params.merge(id: distribution.id))
         expect(response.body).to include("You’ve had an audit since this distribution was started.")
       end
@@ -428,7 +432,7 @@
       context 'with a correct hash id' do
         it 'should render the calendar' do
           get distributions_calendar_path(hash: hashed_id)
-          expect(CalendarService).to have_received(:calendar).with(@organization.id)
+          expect(CalendarService).to have_received(:calendar).with(organization.id)
           expect(response.media_type).to include('text/calendar')
           expect(response.body).to eq('SOME ICS STRING')
         end