improve specs. add caching for resource

Author: compwron

app/controllers/users/passwords_controller.rb

@@ -95,6 +95,7 @@ def empty_fields_error
   end
 
   def invalid_phone_number_error(error_message)
+    @resource ||= resource
     @resource.errors.add(:phone_number, error_message)
 
     false

spec/requests/users/passwords_spec.rb

@@ -4,17 +4,17 @@
   let!(:org) { create(:casa_org) }
   let!(:user) { create(:user, phone_number: "+12222222222", casa_org: org) }
 
-  let!(:twillio_service_double) { instance_double(TwilioService) }
+  let!(:twilio_service_double) { instance_double(TwilioService) }
   let!(:short_url_service_double) { instance_double(ShortUrlService) }
 
   before do
     allow(TwilioService).to(
       receive(:new).with(
         org
-      ).and_return(twillio_service_double)
+      ).and_return(twilio_service_double)
     )
 
-    allow(twillio_service_double).to receive(:send_sms)
+    allow(twilio_service_double).to receive(:send_sms)
 
     allow(ShortUrlService).to receive(:new).and_return(short_url_service_double)
 
@@ -37,14 +37,14 @@
 
       it "sends a password reset SMS to existing user" do
         request
-        expect(twillio_service_double).to have_received(:send_sms).once.with(
+        expect(twilio_service_double).to have_received(:send_sms).once.with(
           {From: org.twilio_phone_number, Body: a_string_matching("reset_url"), To: user.phone_number}
         )
       end
 
       it "sends a password reset email to existing user" do
-        expect_any_instance_of(User).to receive(:send_reset_password_instructions).once
-        request
+        expect { request }.to change { ActionMailer::Base.deliveries.count }.by(1)
+        expect(ActionMailer::Base.deliveries.last.to).to include(user.email)
       end
 
       it { is_expected.to redirect_to(user_session_url) }
@@ -60,13 +60,13 @@
         let(:params) { {user: {email: user.email, phone_number: ""}} }
 
         it "sends a password reset email to existing user" do
-          expect_any_instance_of(User).to receive(:send_reset_password_instructions).once
-          request
+          expect { request }.to change { ActionMailer::Base.deliveries.count }.by(1)
+          expect(ActionMailer::Base.deliveries.last.to).to include(user.email)
         end
 
         it "does not send sms with reset password" do
           request
-          expect(twillio_service_double).not_to have_received(:send_sms)
+          expect(twilio_service_double).not_to have_received(:send_sms)
         end
       end
 
@@ -75,14 +75,13 @@
 
         it "sends a password reset SMS to existing user" do
           request
-          expect(twillio_service_double).to have_received(:send_sms).once.with(
+          expect(twilio_service_double).to have_received(:send_sms).once.with(
             {From: org.twilio_phone_number, Body: a_string_matching("reset_url"), To: user.phone_number}
           )
         end
 
         it "does not send email with reset password" do
-          expect_any_instance_of(User).not_to receive(:send_reset_password_instructions)
-          request
+          expect { request }.not_to change { ActionMailer::Base.deliveries.count }
         end
       end
     end
@@ -96,17 +95,35 @@
       end
     end
 
-    context "with wrong parameters" do
+    context "with wrong parameters (non-existent user)" do
       let(:params) { {user: {phone_number: "13333333333"}} }
 
-      it "sets errors correctly" do
+      it "does not reveal if user exists (security)" do
         request
         expect(flash[:notice]).to(
           eq("If the account exists you will receive an email or SMS with instructions on how to reset your password in a few minutes.")
         )
       end
     end
 
+    context "with invalid phone number format" do
+      let(:params) { {user: {email: "", phone_number: "1234"}} }
+
+      it "shows phone number validation error" do
+        request
+        expect(request.parsed_body.to_html).to include("phone_number")
+      end
+    end
+
+    context "with non-numeric phone number" do
+      let(:params) { {user: {email: "", phone_number: "abc"}} }
+
+      it "shows phone number validation error" do
+        request
+        expect(request.parsed_body.to_html).to include("phone_number")
+      end
+    end
+
     context "when twilio is disabled" do
       let(:params) { {user: {email: user.email, phone_number: user.phone_number}} }
 
@@ -115,19 +132,56 @@
       end
 
       it "does not send an sms, only an email" do
-        expect_any_instance_of(User).to receive(:send_reset_password_instructions).once
+        expect { request }.to change { ActionMailer::Base.deliveries.count }.by(1)
+        expect(flash[:notice]).to(
+          eq("If the account exists you will receive an email or SMS with instructions on how to reset your password in a few minutes.")
+        )
+      end
+    end
+
+    context "when email sending times out with Net::ReadTimeout" do
+      let(:params) { {user: {email: user.email, phone_number: user.phone_number}} }
+
+      before do
+        allow(user).to receive(:send_reset_password_instructions).and_raise(Net::ReadTimeout)
+        allow(User).to receive(:find_by).and_return(user)
+      end
+
+      it "handles the timeout gracefully and still shows success message" do
+        expect(Rails.logger).to receive(:error).with(/Password reset email failed to send/)
         request
         expect(flash[:notice]).to(
           eq("If the account exists you will receive an email or SMS with instructions on how to reset your password in a few minutes.")
         )
       end
+
+      it "does not crash the request" do
+        expect { request }.not_to raise_error
+        expect(response).to redirect_to(user_session_url)
+      end
+
+      it "notifies Bugsnag of the error" do
+        expect(Bugsnag).to receive(:notify).with(instance_of(Net::ReadTimeout))
+        request
+      end
+
+      it "generates a fallback token for SMS to use" do
+        expect(user).to receive(:generate_password_reset_token).and_call_original
+        request
+      end
+
+      it "still sends SMS with the fallback token" do
+        request
+        expect(twilio_service_double).to have_received(:send_sms).once
+      end
     end
 
-    context "when email sending times out" do
+    context "when email sending times out with Net::OpenTimeout" do
       let(:params) { {user: {email: user.email, phone_number: ""}} }
 
       before do
-        allow_any_instance_of(User).to receive(:send_reset_password_instructions).and_raise(Net::ReadTimeout)
+        allow(user).to receive(:send_reset_password_instructions).and_raise(Net::OpenTimeout)
+        allow(User).to receive(:find_by).and_return(user)
       end
 
       it "handles the timeout gracefully and still shows success message" do
@@ -142,6 +196,23 @@
         expect { request }.not_to raise_error
         expect(response).to redirect_to(user_session_url)
       end
+
+      it "notifies Bugsnag of the error" do
+        expect(Bugsnag).to receive(:notify).with(instance_of(Net::OpenTimeout))
+        request
+      end
+    end
+
+    context "when SMS sending fails" do
+      let(:params) { {user: {email: "", phone_number: user.phone_number}} }
+
+      before do
+        allow(twilio_service_double).to receive(:send_sms).and_raise(Twilio::REST::TwilioError.new("Service unavailable"))
+      end
+
+      it "raises the error (no rescue in controller)" do
+        expect { request }.to raise_error(Twilio::REST::TwilioError)
+      end
     end
   end
 
@@ -162,12 +233,58 @@
       }
     end
 
-    subject(:submit_reset) { put user_password_path, params: params }
+    subject(:request) { put user_password_path, params: params }
 
-    it "successfully resets the password" do
-      submit_reset
-      expect(response).to redirect_to(new_user_session_path)
-      expect(flash[:notice]).to eq("Your password has been changed successfully.")
+    context "with valid token and password" do
+      it "successfully resets the password" do
+        request
+        expect(response).to redirect_to(new_user_session_path)
+        expect(flash[:notice]).to eq("Your password has been changed successfully.")
+      end
+
+      it "allows user to sign in with new password" do
+        request
+        user.reload
+        expect(user.valid_password?("newpassword123!")).to be true
+      end
+    end
+
+    context "with password mismatch" do
+      let(:params) do
+        {
+          user: {
+            reset_password_token: token,
+            password: "newpassword123!",
+            password_confirmation: "differentpassword123!"
+          }
+        }
+      end
+
+      it "does not reset the password" do
+        old_password_digest = user.encrypted_password
+        request
+        user.reload
+        expect(user.encrypted_password).to eq(old_password_digest)
+      end
+    end
+
+    context "with password too short" do
+      let(:params) do
+        {
+          user: {
+            reset_password_token: token,
+            password: "abc",
+            password_confirmation: "abc"
+          }
+        }
+      end
+
+      it "does not reset the password" do
+        old_password_digest = user.encrypted_password
+        request
+        user.reload
+        expect(user.encrypted_password).to eq(old_password_digest)
+      end
     end
   end
 end