Fix unexpected issued_at behavior when date field empty. (#4849)

coalest · web-flow · commit 9811a97d363a · 2024-12-17T19:57:36.000-05:00
* Fix unexpected issued_at behavior when date field empty.

Previously before_save and before_create callbacks set the value for issued_at
created_at end of day if no datetime was provided. This prevented records being
saved with no issue date. But it caused unexpected behavior when creating or
updating a donation/purchase/distribution with an empty issued_at field,
silently changing the issued_at date to something potentially incorrect.

* Fix missing partner dropdown options of distribution update fail

* Revert partner list fix
diff --git a/app/models/concerns/issued_at.rb b/app/models/concerns/issued_at.rb
@@ -5,23 +5,18 @@ module IssuedAt
   extend ActiveSupport::Concern
 
   included do
-    before_create :initialize_issued_at
-    before_save :initialize_issued_at
     scope :by_issued_at, ->(issued_at) { where(issued_at: issued_at.beginning_of_month..issued_at.end_of_month) }
     scope :for_year, ->(year) { where("extract(year from issued_at) = ?", year) }
+    validates :issued_at, presence: true
     validate :issued_at_cannot_be_before_2000
     validate :issued_at_cannot_be_further_than_1_year
   end
 
   private
 
-  def initialize_issued_at
-    self.issued_at ||= created_at&.end_of_day
-  end
-
   def issued_at_cannot_be_before_2000
     if issued_at.present? && issued_at < Date.new(2000, 1, 1)
-      errors.add(:issued_at, "Cannot be before 2000")
+      errors.add(:issued_at, "cannot be before 2000")
     end
   end
 
diff --git a/app/views/distributions/edit.html.erb b/app/views/distributions/edit.html.erb
@@ -24,7 +24,7 @@
 
 <section class="content">
 
-  <% unless @distribution.future? %>
+  <% if @distribution.issued_at && !@distribution.future? %>
     <div class="alert alert-warning" role="alert">
       The current date is past the date this distribution was scheduled for.
       Please be very careful when editing this record;
diff --git a/config/locales/en.yml b/config/locales/en.yml
@@ -29,4 +29,12 @@
 # To learn more, please read the Rails Internationalization guide
 # available at http://guides.rubyonrails.org/i18n.html.
 
-en:
+en:
+  activerecord:
+    attributes:
+      donation:
+        issued_at: "Issue date"
+      purchase:
+        issued_at: "Purchase date"
+      distribution:
+        issued_at: "Distribution date and time"
diff --git a/spec/controllers/distributions_controller_spec.rb b/spec/controllers/distributions_controller_spec.rb
@@ -18,6 +18,7 @@
             organization_name: organization.id,
             distribution: {
               partner_id: partner.id,
+              issued_at: Date.yesterday,
               storage_location_id: first_storage_location.id,
               line_items_attributes:
               {
@@ -42,6 +43,7 @@
               distribution: {
                 partner_id: partner.id,
                 storage_location_id: second_storage_location.id,
+                issued_at: Date.yesterday,
                 line_items_attributes:
                   {
                     "0": { item_id: second_storage_location.items.first.id, quantity: 18 }
@@ -66,6 +68,7 @@
             distribution: {
               partner_id: partner.id,
               storage_location_id: storage_location.id,
+              issued_at: Date.yesterday,
               line_items_attributes:
                 {
                   "0": { item_id: storage_location.items.first.id, quantity: 18 }
@@ -91,6 +94,7 @@
               distribution: {
                 partner_id: partner.id,
                 storage_location_id: storage_location.id,
+                issued_at: Date.yesterday,
                 line_items_attributes:
                   {
                     "0": { item_id: storage_location.items.first.id, quantity: 18 },
@@ -134,6 +138,7 @@
             distribution: {
               partner_id: partner.id,
               storage_location_id: storage_location.id,
+              issued_at: Date.yesterday,
               line_items_attributes:
                 {
                   "0": { item_id: item1.id, quantity: 18 },
@@ -172,6 +177,7 @@
             distribution: {
               partner_id: partner.id,
               storage_location_id: storage_location.id,
+              issued_at: Date.yesterday,
               line_items_attributes:
                 {
                   "0": { item_id: item1.id, quantity: 18 },
@@ -199,6 +205,7 @@
             distribution: {
               partner_id: partner.id,
               storage_location_id: storage_location.id,
+              issued_at: Date.yesterday,
               line_items_attributes:
                 {
                   "0": { item_id: storage_location.items.first.id, quantity: 0 }
@@ -235,6 +242,7 @@
             id: distribution.id,
             distribution: {
               storage_location_id: distribution.storage_location.id,
+              issued_at: Date.yesterday,
               line_items_attributes:
                 {
                   "0": { item_id: item1.id, quantity: 18 },
@@ -309,6 +317,7 @@
             id: distribution.id,
             distribution: {
               storage_location_id: distribution.storage_location.id,
+              issued_at: Date.yesterday,
               line_items_attributes:
                 {
                   "0": { item_id: item1.id, quantity: 4 },
diff --git a/spec/controllers/donations_controller_spec.rb b/spec/controllers/donations_controller_spec.rb
@@ -34,6 +34,7 @@
           donation: { storage_location_id: storage_location.id,
                       donation_site_id: donation_site.id,
                       source: "Donation Site",
+                      issued_at: Date.yesterday,
                       line_items: line_items }
         }
         expect(response).to redirect_to(donations_path)
diff --git a/spec/factories/distributions.rb b/spec/factories/distributions.rb
@@ -22,7 +22,7 @@
     storage_location
     partner
     organization { Organization.try(:first) || create(:organization) }
-    issued_at { nil }
+    issued_at { Time.current }
     delivery_method { :pick_up }
     state { :scheduled }
 
diff --git a/spec/factories/donations.rb b/spec/factories/donations.rb
@@ -23,7 +23,7 @@
     comment { "It's a fine day for diapers." }
     storage_location
     organization { Organization.try(:first) || create(:organization) }
-    issued_at { nil }
+    issued_at { Time.current }
 
     factory :manufacturer_donation do
       manufacturer
diff --git a/spec/factories/purchases.rb b/spec/factories/purchases.rb
@@ -24,7 +24,7 @@
     purchased_from { "Google" }
     storage_location
     organization { Organization.try(:first) || create(:organization) }
-    issued_at { nil }
+    issued_at { Time.current }
     amount_spent_in_cents { 10_00 }
     vendor { Vendor.try(:first) || create(:vendor) }
 
diff --git a/spec/models/distribution_spec.rb b/spec/models/distribution_spec.rb
@@ -240,17 +240,6 @@
   end
 
   context "Callbacks >" do
-    it "initializes the issued_at field to default to midnight if it wasn't explicitly set" do
-      yesterday = 1.day.ago
-      today = Time.zone.today
-
-      distribution = create(:distribution, created_at: yesterday, issued_at: today)
-      expect(distribution.issued_at.to_date).to eq(today)
-
-      distribution = create(:distribution, created_at: yesterday)
-      expect(distribution.issued_at).to eq(distribution.created_at.end_of_day)
-    end
-
     context "#before_save" do
       context "#reset_shipping_cost" do
         context "when delivery_method is other then shipped" do
diff --git a/spec/models/donation_spec.rb b/spec/models/donation_spec.rb
@@ -61,17 +61,6 @@
   end
 
   context "Callbacks >" do
-    it "inititalizes the issued_at field to default to midnight if it wasn't explicitly set" do
-      yesterday = 1.day.ago
-      today = Time.zone.today
-
-      donation = create(:donation, created_at: yesterday, issued_at: today)
-      expect(donation.issued_at.to_date).to eq(today)
-
-      donation = create(:donation, created_at: yesterday)
-      expect(donation.issued_at).to eq(donation.created_at.end_of_day)
-    end
-
     it "automatically combines duplicate line_item records when they're created" do
       donation = build(:donation)
       item = create(:item)
diff --git a/spec/models/purchase_spec.rb b/spec/models/purchase_spec.rb
@@ -87,17 +87,6 @@
   end
 
   context "Callbacks >" do
-    it "inititalizes the issued_at field to default to midnight if it wasn't explicitly set" do
-      yesterday = 1.day.ago
-      today = Time.zone.today
-
-      purchase = create(:purchase, created_at: yesterday, issued_at: today)
-      expect(purchase.issued_at.to_date).to eq(today)
-
-      purchase = create(:purchase, created_at: yesterday)
-      expect(purchase.issued_at).to eq(purchase.created_at.end_of_day)
-    end
-
     it "automatically combines duplicate line_item records when they're created" do
       purchase = build(:purchase)
       item = create(:item)
diff --git a/spec/requests/distributions_requests_spec.rb b/spec/requests/distributions_requests_spec.rb
@@ -211,8 +211,9 @@
     describe "POST #create" do
       let!(:storage_location) { create(:storage_location, organization: organization) }
       let!(:partner) { create(:partner, organization: organization) }
+      let(:issued_at) { Time.current }
       let(:distribution) do
-        { storage_location_id: storage_location.id, partner_id: partner.id, delivery_method: :delivery }
+        { storage_location_id: storage_location.id, partner_id: partner.id, issued_at:, delivery_method: :delivery }
       end
 
       it "redirects to #show on success" do
@@ -261,6 +262,17 @@
           expect(response.body).to include("Active Partner")
         end
       end
+
+      context "with missing issued_at field" do
+        let(:issued_at) { "" }
+
+        it "fails and returns validation error message" do
+          post distributions_path(distribution:, format: :turbo_stream)
+
+          expect(response).to have_http_status(400)
+          expect(flash[:error]).to include("Distribution date and time can't be blank")
+        end
+      end
     end
 
     describe "GET #new" do
@@ -514,6 +526,26 @@
         expect(response.status).to redirect_to(distribution_path(distribution.to_param))
       end
 
+      context "with invalid issued_at field" do
+        let(:distribution_params) do
+          { id: distribution.id,
+            distribution: {
+              partner_id: partner.id,
+              storage_location_id: location.id,
+              'issued_at(1i)' => issued_at.to_date.year,
+              'issued_at(2i)' => issued_at.to_date.month,
+              'issued_at(3i)' => nil # day part of date missing
+            }}
+        end
+
+        it "fails and returns validation error message" do
+          patch distribution_path(distribution_params)
+
+          expect(flash[:error]).to include("Distribution date and time can't be blank")
+          expect(response).not_to redirect_to(anything)
+        end
+      end
+
       describe "when changing storage location" do
         let(:item) { create(:item, organization: organization) }
         it "updates storage quantity correctly" do
diff --git a/spec/requests/donations_requests_spec.rb b/spec/requests/donations_requests_spec.rb
@@ -100,6 +100,74 @@
       end
     end
 
+    describe "POST #create" do
+      let(:product_drive) { create(:product_drive, organization:) }
+      let(:storage_location) { create(:storage_location, organization:) }
+      let(:manufacturer) { create(:manufacturer, organization:) }
+      let(:source) { Donation::SOURCES[:manufacturer] }
+      let(:issued_at) { Date.yesterday }
+
+      let(:params) do
+        {
+          donation: {
+            source: Donation::SOURCES[:manufacturer],
+            manufacturer_id: manufacturer.id,
+            product_drive_id: product_drive.id,
+            storage_location_id: storage_location.id,
+            money_raised_in_dollars: 5,
+            product_drive_participant_id: nil,
+            comment: "",
+            issued_at: issued_at,
+            line_items_attributes: {}
+          }
+        }
+      end
+
+      it "flashes a success message" do
+        post donations_path(params)
+
+        expect(flash[:notice]).to eq("Donation created and logged!")
+      end
+
+      it "redirects to the index page" do
+        post donations_path(params)
+
+        expect(response).to redirect_to(donations_path)
+      end
+
+      context "with invalid issued_at param" do
+        let(:issued_at) { "" }
+
+        it "flashes the correct validation error" do
+          post donations_path(params)
+
+          expect(flash[:error]).to include("Issue date can't be blank")
+        end
+      end
+    end
+
+    describe "PATCH #update" do
+      let(:donation) { create(:donation, organization:) }
+      let(:item) { create(:item, organization:) }
+      let(:params) { { id: donation.id, donation: donation_params } }
+      let(:donation_params) do
+        {
+          line_items_attributes: {
+            "0": { item_id: item.id, quantity: 5 }
+          }
+        }
+      end
+
+      context "with invalid issued_at param" do
+        it "flashes the correct validation error" do
+          donation_params[:issued_at] = ""
+          put donation_path(params)
+
+          expect(flash[:alert]).to include("Issue date can't be blank")
+        end
+      end
+    end
+
     describe "GET #print" do
       let(:item) { create(:item) }
       let!(:donation) { create(:donation, :with_items, item: item) }
diff --git a/spec/requests/purchases_requests_spec.rb b/spec/requests/purchases_requests_spec.rb
@@ -85,16 +85,16 @@
       let!(:storage_location) { create(:storage_location, organization: organization) }
       let(:line_items) { [attributes_for(:line_item)] }
       let(:vendor) { create(:vendor, organization: organization) }
+      let(:purchase) do
+        { storage_location_id: storage_location.id,
+          purchased_from: "Google",
+          vendor_id: vendor.id,
+          amount_spent: 10,
+          issued_at: Time.current,
+          line_items: line_items }
+      end
 
       context "on success" do
-        let(:purchase) do
-          { storage_location_id: storage_location.id,
-            purchased_from: "Google",
-            vendor_id: vendor.id,
-            amount_spent: 10,
-            line_items: line_items }
-        end
-
         it "redirects to GET#edit" do
           expect { post purchases_path(purchase: purchase) }
             .to change { Purchase.count }.by(1)
@@ -122,6 +122,15 @@
           expect(response).to be_successful # Will render :new
           expect(response.body).to include('Failed to create purchase due to')
         end
+
+        context "with invalid issued_at param" do
+          it "flashes the correct validation error" do
+            issued_at = ""
+            post purchases_path(purchase: purchase.merge(issued_at:))
+
+            expect(flash[:error]).to include("Purchase date can't be blank")
+          end
+        end
       end
     end
 
@@ -152,6 +161,15 @@
                }.by(5)
       end
 
+      context "with invalid issued_at" do
+        it "redirects to index after update" do
+          purchase = create(:purchase, purchased_from: "Google")
+          put purchase_path(id: purchase.id, purchase: { issued_at: "" })
+
+          expect(flash[:alert]).to include("Purchase date can't be blank")
+        end
+      end
+
       describe "when removing a line item" do
         it "updates storage invetory item quantity correctly" do
           purchase = create(:purchase, :with_items, item_quantity: 10)
diff --git a/spec/services/distribution_create_service_spec.rb b/spec/services/distribution_create_service_spec.rb
diff --git a/spec/services/distribution_update_service_spec.rb b/spec/services/distribution_update_service_spec.rb
diff --git a/spec/services/donation_create_service_spec.rb b/spec/services/donation_create_service_spec.rb
diff --git a/spec/services/itemizable_update_service_spec.rb b/spec/services/itemizable_update_service_spec.rb

Original file line number	Diff line number	Diff line change
`@@ -34,6 +34,7 @@`
`34`	`34`	`donation: { storage_location_id: storage_location.id,`
`35`	`35`	`donation_site_id: donation_site.id,`
`36`	`36`	`source: "Donation Site",`
	`37`	`+ issued_at: Date.yesterday,`
`37`	`38`	`line_items: line_items }`
`38`	`39`	`}`
`39`	`40`	`expect(response).to redirect_to(donations_path)`