Feature 1792 - users can choose to recieve password resets via SMS

[xihai01]

What github issue is this PR for, if any?

Resolves #1792

What changed, and why?

CASA users can now send password resets via SMS in addition to email.
Users can have the flexible options of getting a reset link via:

• Just email
• Just phone number
• Both email and phone number

To achieve this:

• A phone number field is added in the front end
• A passwords controller with a create action is implemented to override Devise's passwords controller. This is needed to add in the functionality of sending via SMS.

How is this tested? (please write tests!) 💖💪

• A view spec is created to test front end aspect (i.e additional text and phone number field is present on page)
• A system spec is created to test user flow (forgot password -> enter email and/or phone number -> click send -> redirect with a notice) and rendering of any form validation errors
• A request spec is created to test our custom route that it calls short io and twilio services
  and redirects if no form errors occur

Screenshots please :)

SMS received via my phone (short link was visible thanks to @7riumph 's branded domain)

Feelings gif (optional)

I really enjoyed this issue - it allowed me to dive deeper into the internals of Devise

[xihai01]

@compwron @harsohailB @7riumph rspec is failing because the db is missing a "user reminder times" table and the lib/tasks tests depend on it. Looking at the schema, it seems to me someone refactored the "users reminder times" to "user case contact types reminders". Any clue to what's going on here?

[xihai01]

I'll work on fixing the code climate issues. Please don't merge yet or until everything is green. :)

[harsohailB]

it seems to me someone refactored the "users reminder times" to "user case contact types reminders"

@xihai01 It should be the opposite. I refactored "user case contact types reminders" table to "users reminder times" to make it re-usable for all reminder times. #3735

[harsohailB]

Really nice job! Glad you enjoyed this because I was having a really hard time wrapping my head around Devise. Just a few comments about code separation and reusability in the passwords controller.

[harsohailB]

Can we do something like this instead so its more readable than indexing:

phone_number_is_valid, error_message = valid_phone_number(phone_number)

if phone_number_is_valid
    error_message = "Phone number not found" if User.find_by(phone_number: phone_number)
end

if error_message
    resource.errors.add(:phone_number, error_message)
end

[harsohailB]

Seems like you are already querying User.find_by(phone_number: phone_number) and User.find_by(email: email) above in the file to add error if the queries doesn't find anything. Maybe put these in a variable so you only call them once and reuse them if the query does find something?

[harsohailB]

I think you should have 2 clear paths for email and sms based on what is filled out. Right now, its hard to follow the logic as we have a lot if statements that keep checking email.blank? and phone_number.blank?. Maybe:

if !email.blank? && valid_email
   # run this code which  only has to do with the email
end

if !phone_number.blank? && valid_phone_number
   # run this code which  only has to do with the sms
end

[xihai01]

TO DO STILL:

• create action is apparently still too complex for a human to understand -> need refactoring extra credit to @harsohailB for the help
• migrate request spec to a controller spec to pass that spec checker 💩
• fix error with failing lib/tasks tests; once fixed should resolve factory bot lint check too - 2 🐦 1 🪨

[compwron]

This is exciting!
CI is failing on lint and spec

[xihai01]

This is exciting! CI is failing on lint and spec

fixed! I drop the db and recreated it. It seemed the schema I had was outdated with some missing tables.

[compwron]

eek

[compwron]

for devise ?

[xihai01]

yes, for devise. The devise view calls some internal methods like resource and resource_name. So we need to map these to use them in our controllers and specs

[7riumph]

🔥🔥🔥