Merge pull request #5334 from GreenGogh47/fix_request_fulfilled_issue

Fix request fulfilled issue

Author: awwaiid

app/controllers/requests_controller.rb

@@ -37,9 +37,14 @@ def show
   # pre-filled distribution containing all the requested items.
   def start
     request = Request.find(params[:id])
-    request.status_started!
-    flash[:notice] = "Request started"
-    redirect_to new_distribution_path(request_id: request.id)
+    begin
+      request.status_started!
+      flash[:notice] = "Request started"
+      redirect_to new_distribution_path(request_id: request.id)
+    rescue ActiveRecord::RecordInvalid
+      flash[:alert] = request.errors.full_messages.to_sentence
+      redirect_to request_path(request)
+    end
   end
 
   def print_picklist

app/models/request.rb

@@ -37,6 +37,7 @@ class Request < ApplicationRecord
   validates :distribution_id, uniqueness: true, allow_nil: true
   validate :item_requests_uniqueness_by_item_id
   validate :not_completely_empty
+  validate :cannot_change_status_once_fulfilled, on: :update
 
   after_validation :sanitize_items_data
 
@@ -86,4 +87,10 @@ def not_completely_empty
       errors.add(:base, "completely empty request")
     end
   end
+
+  def cannot_change_status_once_fulfilled
+    if status_changed? && status_was == "fulfilled"
+      errors.add(:status, "cannot be changed once fulfilled")
+    end
+  end
 end

db/migrate/20250826223940_backfill_fulfilled_requests.rb

@@ -0,0 +1,21 @@
+class BackfillFulfilledRequests < ActiveRecord::Migration[8.0]
+  def up
+    # This migration fixes requests that should be marked as "fulfilled" 
+    # but are currently marked as "started"
+    # A request should be "fulfilled" if its associated distribution is "complete"
+    Request.joins(:distribution)
+      .where(status: "started", distributions: { state: "complete" })
+      .update_all(status: "fulfilled")
+  end
+
+  def down
+    # This is a data correction migration, so we make it irreversible
+    # to prevent accidental data corruption
+    raise ActiveRecord::IrreversibleMigration
+  end
+end
+
+# Migration Context:
+# Previously, fulfilled requests could be incorrectly changed back to "started" status.
+# This has been fixed in the Request model with validation, but this migration 
+# corrects existing data that may be in an inconsistent state.

spec/migrations/backfill_fulfilled_requests_spec.rb

@@ -0,0 +1,65 @@
+require "rails_helper"
+require Rails.root.join("db/migrate/20250826223940_backfill_fulfilled_requests")
+
+RSpec.describe BackfillFulfilledRequests, type: :migration do
+  let(:migration) { described_class.new }
+  let(:organization) { create(:organization, :with_items) }
+  let(:partner) { create(:partner, organization: organization) }
+
+  it "updates started requests with complete distributions to fulfilled" do
+    # Create a distribution with complete state
+    distribution = create(:distribution, organization: organization, partner: partner, state: "complete")
+
+    # Create a request with started status and link it to the distribution
+    request = create(:request, :started, organization: organization, partner: partner, distribution: distribution)
+
+    expect {
+      migration.up
+    }.to change { request.reload.status }.from("started").to("fulfilled")
+  end
+
+  it "does not change requests without complete distributions" do
+    # Create a distribution with scheduled state (not complete)
+    distribution = create(:distribution, organization: organization, partner: partner, state: "scheduled")
+
+    # Create a request with started status and link it to the distribution
+    request = create(:request, :started, organization: organization, partner: partner, distribution: distribution)
+
+    expect {
+      migration.up
+    }.not_to change { request.reload.status }
+  end
+
+  it "does not change requests without any distribution" do
+    # Create a request with started status but no distribution
+    request = create(:request, :started, organization: organization, partner: partner, distribution: nil)
+
+    expect {
+      migration.up
+    }.not_to change { request.reload.status }
+  end
+
+  it "does not change fulfilled requests even if they have complete distributions" do
+    # Create a distribution with complete state
+    distribution = create(:distribution, organization: organization, partner: partner, state: "complete")
+
+    # Create a request that's already fulfilled
+    request = create(:request, :fulfilled, organization: organization, partner: partner, distribution: distribution)
+
+    expect {
+      migration.up
+    }.not_to change { request.reload.status }
+  end
+
+  it "does not change pending requests even if they have complete distributions" do
+    # Create a distribution with complete state
+    distribution = create(:distribution, organization: organization, partner: partner, state: "complete")
+
+    # Create a request with pending status
+    request = create(:request, :pending, organization: organization, partner: partner, distribution: distribution)
+
+    expect {
+      migration.up
+    }.not_to change { request.reload.status }
+  end
+end

spec/models/request_spec.rb

@@ -29,6 +29,21 @@
         expect(Request.status_started).to eq([request_started])
         expect(Request.status_fulfilled).to eq([request_fulfilled])
       end
+
+      it "does not regress from fulfilled to started" do
+        expect { request_fulfilled.status_started! }
+          .to raise_error(ActiveRecord::RecordInvalid, /cannot be changed once fulfilled/)
+      end
+
+      it "does not regress from fulfilled to pending" do
+        expect { request_fulfilled.status_pending! }
+          .to raise_error(ActiveRecord::RecordInvalid, /cannot be changed once fulfilled/)
+      end
+
+      it "allows normal transitions" do
+        expect { request_pending.status_started! }.not_to raise_error
+        expect { request_started.status_fulfilled! }.not_to raise_error
+      end
     end
   end