Merge remote-tracking branch 'origin/main' into sethherr/hartford

Author: sethherr

app/services/bike_services/builder.rb

@@ -10,7 +10,13 @@ def build(b_param, new_attrs = nil)
       bike = Bike.new(cycle_type: "bike")
       # passed_organization is assigned unless b_param has an organization
       passed_organization = new_attrs.delete(:organization)
-      bike.attributes = b_param.safe_bike_attrs(new_attrs)
+      # One at a time, so a value the model refuses to take (an out of range enum, say)
+      # becomes an error on the bike rather than a 500, and the rest still land
+      b_param.safe_bike_attrs(new_attrs).each do |key, value|
+        bike.attributes = {key => value}
+      rescue ArgumentError
+        bike.errors.add(key, "is not valid")
+      end
 
       bike.address_record&.bike = bike # Kinda gross, but gotta get it both ways!
 

spec/requests/bikes/create_request_spec.rb

@@ -229,6 +229,18 @@ def expect_created_stolen_bike(bike_params: nil, stolen_params: {})
       end
     end
   end
+  context "invalid frame_material" do
+    # Form filling bots submit "1" for every field - the enum assignment used to raise
+    let(:bike_params) { basic_bike_params.merge(frame_material: "1") }
+    it "renders the error" do
+      expect {
+        post base_url, params: {bike: bike_params}
+      }.to change(Bike, :count).by(0)
+      b_param = BParam.last
+      expect(b_param.bike_errors).to eq(["Frame material is not valid"])
+      expect(response).to redirect_to(new_bike_url(b_param_token: b_param.id_token))
+    end
+  end
   context "no existing b_param, impounded" do
     let(:bike_params) { basic_bike_params }
     context "impound_record" do

spec/requests/organizations_request_spec.rb

@@ -231,6 +231,18 @@
         expect(bike.status).to eq "status_stolen"
         expect(bike.owner_email).to eq(b_param_attrs[:bike][:owner_email])
       end
+      context "with an invalid enum value" do
+        let(:b_param_attrs) do
+          {bike: {owner_email: "someemail@stuff.com", frame_material: "1",
+                  creation_organization_id: current_organization.id.to_s}}
+        end
+        it "renders" do
+          get "#{base_url}/#{current_organization.id}/embed_extended?b_param_id_token=#{b_param.id_token}"
+          expect(response.code).to eq("200")
+          expect(response).to render_template(:embed_extended)
+          expect(assigns(:bike).frame_material).to be_blank
+        end
+      end
     end
   end
 

spec/requests/register_request_spec.rb

@@ -702,6 +702,24 @@ def address_street_field
         end
       end
 
+      # This flow permits no enum a bot could poison - cycle_type is friendly_found and
+      # status is gated by BParam#status - but it creates the bike from whatever the
+      # registration already holds, so a bad value can still arrive at Builder
+      context "registration holding an invalid enum value" do
+        let(:b_param) do
+          BParam.create(origin: "register_flow",
+            params: {bike: {owner_email:, manufacturer_id: "Trek", frame_material: "1"}}.as_json)
+        end
+
+        it "sends them back to step 2 with the error, rather than raising" do
+          expect {
+            patch base_url, params: {b_param_token: b_param.id_token, bike: bike_details}
+          }.to_not change(Bike, :count)
+          expect(flash[:error]).to match(/frame material/i)
+          expect(response).to redirect_to register_path(b_param_token: b_param.id_token, step: 2)
+        end
+      end
+
       context "registering to their own address" do
         let(:owner_email) { current_user.email }
 

spec/services/bike_services/builder_spec.rb

@@ -127,6 +127,16 @@
       end
     end
 
+    context "with an invalid enum value" do
+      let(:b_param_params) { {bike: {frame_material: "1", handlebar_type: "9", frame_model: "Cool model"}} }
+      it "adds an error rather than raising" do
+        expect(bike.errors.full_messages).to eq(["Frame material is not valid", "Handlebar type is not valid"])
+        expect(bike.frame_material).to be_blank
+        expect(bike.handlebar_type).to be_blank
+        expect(bike.frame_model).to eq "Cool model"
+      end
+    end
+
     context "with organization" do
       let(:new_attrs) { {organization:} }