Skip to content

Feature/1784 - All users recieve account activation SMS#3621

Merged
compwron merged 36 commits into
rubyforgood:mainfrom
xihai01:f/1784
Jun 15, 2022
Merged

Feature/1784 - All users recieve account activation SMS#3621
compwron merged 36 commits into
rubyforgood:mainfrom
xihai01:f/1784

Conversation

@xihai01
Copy link
Copy Markdown
Collaborator

@xihai01 xihai01 commented Jun 6, 2022

What github issue is this PR for, if any?

Resolves #1784

What changed, and why?

  • Within application controller, a function called deliver_sms_to() is created to instantiate a new Twilio client and send a SMS
  • Refactored spec/support/webmock_helper.rb by creating each stub in their own functions within a module. This is so each function can return the stub that we can use in the tests (for example, asserting how many times a stub was called)
  • In /config/initializers/constants.rb, a module is created to hold the body messages of SMS notifications to be sent.

To do (features)

  • Implement @harsohailB suggestion to re-structure webmock for clarity
  • Research how Twilio handles errors (ex. incorrect phone numbers) and apply a fix
  • Refactor

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

  • Request specs are written for volunteer/supervisor/admin controllers where: phone number is provided, phone number is not provided and when Twilio responds back with an error. In each case, we look for if the correct stub was called, redirect and the appropriate flash notices

Screenshots please :)

Here is an example of a supervisor created successfully with a SMS sent.
Screenshot from 2022-06-09 23-06-57
SMS

Here is a case when the user makes a typo in their phone number. User still gets created, but SMS not sent.
SMS not sent

Feelings gif (optional)

My 💰 don't 🤹 🤹 it folds
I like to see you wiggle wiggle for sure

Feedback please? (optional)

We are very interested in your feedback! Please give us some :) https://forms.gle/1D5ACNgTs2u9gSdh9

@xihai01 xihai01 added zzz_archived: Tests no-skin-tone zzz_archived: 📱 SMS work relating to SMS notifications #1017 codethechange code.the.change developers labels Jun 6, 2022
@xihai01 xihai01 self-assigned this Jun 6, 2022
@github-actions github-actions Bot added the ruby Touches Ruby code label Jun 6, 2022
@compwron
Copy link
Copy Markdown
Collaborator

compwron commented Jun 8, 2022

Please fix lint and the tests :)

@github-actions github-actions Bot added the erb Touches ERB templates label Jun 10, 2022
@compwron
Copy link
Copy Markdown
Collaborator

Please fit the git conflicts :)

@xihai01
Copy link
Copy Markdown
Collaborator Author

xihai01 commented Jun 10, 2022

Please fit the git conflicts :)

ok, will do

@xihai01
Copy link
Copy Markdown
Collaborator Author

xihai01 commented Jun 10, 2022

50% done issue. Still working. Will fix and refactor tests, lints, you name it near the end

@compwron
Copy link
Copy Markdown
Collaborator

Codeclimate is failing on duplicate code blocks
Screen Shot 2022-06-14 at 6 08 01 PM

@compwron
Copy link
Copy Markdown
Collaborator

Also there are some git conflicts to resolve...

@github-actions github-actions Bot removed the erb Touches ERB templates label Jun 15, 2022
@xihai01
Copy link
Copy Markdown
Collaborator Author

xihai01 commented Jun 15, 2022

Also there are some git conflicts to resolve...

yes, I will work on the conflicts asap. Once everything is good, I'll open this issue.

Comment thread spec/support/webmock_helper.rb Outdated
}
)
.to_return(status: 200, body: "", headers: {})
module TwilioAPI
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put these stubbed request modules in their own files for easier maintainability?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so instead of modules, you want to put twilio stubs and short io stubs into their own separate files?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can still have them as modules but have a hierarchy as follows for clarity and seperation:

-- support
    -- subbed_requests
        -- twilio_api.rb
        -- short_io_api.rb

A file such as twilio_api.rb would look like this:

# twilio_api.rb

module StubbedRequests
    module TwilioAPI
        def self.allowed_sites(blacklist)
             lambda { |uri|
                 blacklist.none? { |site| uri.host.include?(site) }
             }
        end

        # stubs
    end
end

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that idea. Very clear.

@xihai01
Copy link
Copy Markdown
Collaborator Author

xihai01 commented Jun 15, 2022

fixed codeclimate issue and rspec tests are passing. Before I lint and open this issue for review, I will do the following:

  • - Implement @harsohailB changes to webmock structure
  • - Implement error handling for the case where user enters a correctly formatted phone number, but invalid

@xihai01 xihai01 marked this pull request as ready for review June 15, 2022 20:46
@xihai01 xihai01 requested a review from harsohailB June 15, 2022 20:47
Comment thread app/models/casa_admin.rb
# updated_at :datetime not null
# casa_org_id :bigint not null
# invited_by_id :bigint
# invited_by_id :integer
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these extraneous comment changes are not ideal but they are common. Don't worry about them for now.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats weird.. i never touched any model files


RSpec.describe "/casa_admins", type: :request do
# stub the domains within blacklist for testing
blacklist = ["api.twilio.com", "api.short.io"]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO replace all "blacklist" with "disallowlist"

@compwron compwron merged commit 254ea21 into rubyforgood:main Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codethechange code.the.change developers ruby Touches Ruby code 🧪 Tests Tests zzz_archived: 📱 SMS work relating to SMS notifications #1017

Projects

None yet

Development

Successfully merging this pull request may close these issues.

all users receive account activation sms notification

4 participants