Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bl/waitlist notify #254

Merged
9 commits merged into from
Apr 9, 2017
Merged

Bl/waitlist notify #254

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cafe725
added twilio messenger module, also tweaked the application config to…
Mar 25, 2017
edf81c3
tried to modified messenger with text processing but failed
Mar 26, 2017
27b44a8
added code to send out text alert to the next person on the wait list…
Mar 27, 2017
1edbd5a
added a safeguard for phone number in case someone has not put their …
Apr 2, 2017
5cd7d5e
updated messenger module with try-rescue block
Apr 3, 2017
d11daf3
commented out code for reply forwarding. There is an issue for the fo…
Apr 3, 2017
06d5cab
removed commented code. Will probably add it to a seperate branch. Ju…
Apr 4, 2017
f191563
modified messenger to keep track of error code
Apr 9, 2017
51e9ce2
Merge branch '17/goat/main' into bl/waitlist_notify
Apr 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions app/controllers/checkouts_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,6 @@ def uncheckin
format.html { redirect_to tool_path(checkout.tool), notice: "Error" }
end
end


end


Expand Down Expand Up @@ -181,5 +179,6 @@ def checkin_bak
end
end
end

end

22 changes: 21 additions & 1 deletion app/models/checkout.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
#

class Checkout < ActiveRecord::Base
include Messenger

# For lookups
def card_number=( card_number )
@card_number = card_number
Expand All @@ -35,6 +37,7 @@ def card_number
validates_associated :tool, :organization, :participant

before_save :checked_out_at, :presence => true
after_update :notify

belongs_to :participant, :touch => true
belongs_to :organization, :touch => true
Expand All @@ -44,4 +47,21 @@ def card_number
scope :old, -> { where('checked_in_at IS NOT NULL') }
scope :current, -> { where('checked_in_at IS NULL') }

end
private

def notify
if (self.checked_in_at != nil)
Copy link
Member

Choose a reason for hiding this comment

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

You're calling this after_update, but is the only reason this could ever get updated a check-in? Otherwise you'll be sending this randomly. I think it would be much better to make this part of the controller. That way you could even add a "notify next" button on the frontend and have the controller action half-wired up.

Copy link
Member

Choose a reason for hiding this comment

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

I see what you are saying here @ChaseBro, but if I'm not mistaken, as it stands after the check in is the only reason this should be updated. Given the proximity to Carnival, I think we should go with it and open a high priority issue / bug. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Opened issue #256 about this issue

toolCategory = self.tool.tool_type
waitlist = ToolWaitlist.for_tool_type(toolCategory.id).by_wait_start_time
if (waitlist.count != 0)
nextPerson = waitlist.first.participant
unless (nextPerson.phone_number.blank?)
number = nextPerson.phone_number
content = "#{toolCategory.name} is now available at the trailer. Please come pick it up within 5 minutes!"
send_sms(number, content)
end
end
end
end

end
2 changes: 2 additions & 0 deletions config/application.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ class Application < Rails::Application
# Do not swallow errors in after_commit/after_rollback callbacks.
config.active_record.raise_in_transactional_callbacks = true

config.autoload_paths += %W(#{config.root}/lib)

WillPaginate::ViewHelpers.pagination_options[:inner_window] = 1
WillPaginate::ViewHelpers.pagination_options[:outer_window] = 0
end
Expand Down
29 changes: 29 additions & 0 deletions lib/messenger.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
module Messenger

def send_sms(number, content)
sid = ENV["TWILIO_ACCT_SID"]
auth = ENV["TWILIO_AUTH"]

@client = Twilio::REST::Client.new sid, auth

from = "+14123854063"

# The following try-rescue block is needed in case user unsubscribe
# if the user ubsubscribe and we attempt to message them
# the api will report an error

begin
message = @client.account.messages.create(
:from => from,
:to => '+1'+number,
:body => content
)
rescue Twilio::REST::RequestError => e
case e.code
when 21610
Copy link
Member

Choose a reason for hiding this comment

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

If this is the un-sub error we should unsub them locally as well rather than continuing to try to message them and continuing to fail

Copy link
Author

Choose a reason for hiding this comment

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

This is the error code in case that a client unsubscribe from getting twilio message. The API reports a blacklist error and need to be rescued. There're several lines of comments in this file to explain what it is.

Copy link
Member

Choose a reason for hiding this comment

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

I think you're missing my point. If this error means that a user un-subscribed from Twilio, we should probably make note of that locally so that we don't try to message that user again in the future, no?

Copy link
Member

Choose a reason for hiding this comment

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

I totally agree we should have the try/rescue I just think we should do more in the rescue beyond just logging the error, especially if we know what the error means and how to prevent it in the future (which isn't the case for every type of error, but is for this one)

Copy link
Member

Choose a reason for hiding this comment

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

Also, can we make 21610 a constant in this file called something like USER_UNSUBSCRIBED_FROM_TWILIO_ERROR_CODE or something similarly meaningful, would probably remove the need for the comment as a side-benefit

Copy link
Member

Choose a reason for hiding this comment

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

I agree, lets just do USER_UNSUBSCRIBED_FROM_TWILIO_ERROR_CODE = 21610 at the top of the file, and then do a when USER_UNSUBSCRIBED_FROM_TWILIO_ERROR_CODE right here.

Copy link
Author

Choose a reason for hiding this comment

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

@ChaseBro There is no need to locally keep track of whoever unsubscribed. Twilio API keeps track of it and if a user blacklist our number, Twilio will not send out a message to them. Here is documentation: https://support.twilio.com/hc/en-us/articles/223133627--The-message-From-To-pair-violates-a-blacklist-rule-when-sending-messages

puts e.message
end
end
end

end