Make purchase helper pick default storage location (#4722)

* Make purchase helper pick default storage location

Changed the purchase helper to use the default storage location
of the organization first, before using any other storage locations
that are available.

Also added a (failing) spec to check that we can see the option
on the page. Should be fixed in the next commit

* Fix test by using extended format and `subject`

Fixed the failing test by using the extended format
of the test expectation and used `subject` to make
an assertion regarding the response body.

* Add test that new donation has default storage location

Added a test that when we begin to make a new donation,
we have a default storage location set.

* Switch to use helper to set default storage location

Switched to use the helper method from PurchasesHelper
to set the default storage location for a donation

* Address review feedback

Addressed review feedback namely:
* Renaming new_purchase_default_location to set_default_location
and moving it to ApplicationHelper. This allows us to use across
both the donation and purchase forms.
* Updated the tests and forms accordingly
* Added specs to ApplicationHelper to test that we are able
to successfully set the default storage location for purchases
and donations.

* Rename method, have only one test

Renamed the method as per review suggestion
and keep only one test since we only need to test
that the default storage location is successfully set

* Update set_default_location to default_location

* Update method references in view code

* Update spec to match new behaviour

Updated spec to match the new behaviour of
the donation form.

app/helpers/application_helper.rb

@@ -124,4 +124,8 @@ def storage_location_for_source(source_object)
     end
     current_organization.default_storage_location
   end
+
+  def default_location(source_object)
+    current_organization.default_storage_location || source_object.storage_location_id.presence || current_organization.intake_location
+  end
 end

app/helpers/purchases_helper.rb

@@ -3,8 +3,4 @@ module PurchasesHelper
   def purchased_from(purchase)
     purchase.purchased_from.nil? ? "" : "(#{purchase.purchased_from})"
   end
-
-  def new_purchase_default_location(purchase)
-    purchase.storage_location_id.presence || current_organization.intake_location
-  end
 end

app/views/donations/_donation_form.html.erb

@@ -39,7 +39,7 @@
       <div class="col-xs-8 col-md-6 col-lg-3">
         <%= f.association :product_drive_participant,
                           collection: @product_drive_participants,
-                          selected: donation_form.product_drive_participant_id,
+                          selected: default_location(@donation),
                           include_blank: true,
                           label_method: lambda { |x| "#{x.try(:business_name) }" },
                           label: "Product Drive Participant",

app/views/purchases/_purchase_form.html.erb

@@ -18,7 +18,7 @@
                           collection: @storage_locations,
                           label: "Storage Location",
                           error: "Where is it being stored?",
-                          selected: new_purchase_default_location(@purchase),
+                          selected: default_location(@purchase),
                           wrapper: :input_group %>
       </div>
     </div>

spec/helpers/application_helper_spec.rb

@@ -134,4 +134,51 @@ def current_organization; end
       end
     end
   end
+
+  describe "#set_default_location for purchase" do
+    helper do
+      def current_organization; end
+    end
+
+    before(:each) do
+      allow(helper).to receive(:current_organization).and_return(organization)
+    end
+
+    context "returns storage_location_id if present" do
+      let(:purchase) { build(:purchase, storage_location_id: 2) }
+      subject { helper.default_location(purchase) }
+
+      it { is_expected.to eq(2) }
+    end
+
+    context "returns current_organization intake_location if storage_location_id is not present" do
+      let(:organization) { build(:organization, intake_location: 1) }
+      let(:purchase) { build(:purchase, storage_location_id: nil) }
+
+      before do
+        allow(helper).to receive(:current_organization).and_return(organization)
+      end
+
+      subject { helper.default_location(purchase) }
+
+      it { is_expected.to eq(1) }
+    end
+  end
+
+  describe "#default_location for source_object" do
+    helper do
+      def current_organization; end
+    end
+
+    before(:each) do
+      allow(helper).to receive(:current_organization).and_return(organization)
+    end
+
+    context "returns storage_location_id if present" do
+      let(:donation) { build(:donation, storage_location_id: 2) }
+      subject { helper.default_location(donation) }
+
+      it { is_expected.to eq(2) }
+    end
+  end
 end

spec/helpers/purchases_helper_spec.rb

@@ -4,25 +4,5 @@
       def current_organization
       end
     end
-
-    context "returns purchase storage_location_id if present" do
-      let(:purchase) { build(:purchase, storage_location_id: 2) }
-      subject { helper.new_purchase_default_location(purchase) }
-
-      it { is_expected.to eq(2) }
-    end
-
-    context "returns current_organization intake_location if purchase storage_location_id is not present" do
-      let(:organization) { build(:organization, intake_location: 1) }
-      let(:purchase) { build(:purchase, storage_location_id: nil) }
-
-      before do
-        allow(helper).to receive(:current_organization).and_return(organization)
-      end
-
-      subject { helper.new_purchase_default_location(purchase) }
-
-      it { is_expected.to eq(1) }
-    end
   end
 end

spec/requests/donations_requests_spec.rb

@@ -1,5 +1,6 @@
 RSpec.describe "Donations", type: :request do
   let(:organization) { create(:organization) }
+  let(:storage_location) { create(:storage_location, name: "Pawane Location", organization: organization) }
   let(:user) { create(:user, organization: organization) }
   let(:organization_admin) { create(:organization_admin, organization: organization) }
 
@@ -86,6 +87,19 @@
       end
     end
 
+    describe "GET #new" do
+      subject do
+        organization.update!(default_storage_location: storage_location)
+        get new_donation_path
+        response
+      end
+
+      it { is_expected.to be_successful }
+      it "should include the storage location name" do
+        expect(subject.body).to include("Pawane Location")
+      end
+    end
+
     describe "GET #print" do
       let(:item) { create(:item) }
       let!(:donation) { create(:donation, :with_items, item: item) }
@@ -254,7 +268,7 @@
         expect(response.body).to include("<li class=\"breadcrumb-item\">\n            <a href=\"#\">Editing #{original_source}")
         expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_source}\">#{edited_source}</option>")
         expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_source_drive.id}\">#{edited_source_drive_name}</option>")
-        expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_source_drive_participant.id}\">#{edited_source_drive_participant_business_name}</option>")
+        expect(response.body).to include("<option value=\"#{edited_source_drive_participant.id}\">#{edited_source_drive_participant_business_name}</option>")
         expect(response.body).to include("<option selected=\"selected\" value=\"#{edited_storage_location.id}\">#{edited_storage_location_name}</option>")
         expect(response.body).to include(edited_comment)
         expect(response.body).to include("value=\"#{edited_money}\" type=\"text\" name=\"donation[money_raised_in_dollars]")

spec/requests/purchases_requests_spec.rb

@@ -1,5 +1,6 @@
 RSpec.describe "Purchases", type: :request do
   let(:organization) { create(:organization) }
+  let(:storage_location) { create(:storage_location, name: "Pawane Location", organization: organization) }
   let(:user) { create(:user, organization: organization) }
   let(:organization_admin) { create(:organization_admin, organization: organization) }
 
@@ -48,11 +49,15 @@
 
     describe "GET #new" do
       subject do
+        organization.update!(default_storage_location: storage_location)
         get new_purchase_path
         response
       end
 
       it { is_expected.to be_successful }
+      it "should include the storage location name" do
+        expect(subject.body).to include("Pawane Location")
+      end
     end
 
     describe "POST#create" do