Bl/waitlist notify #254

brucethesloth · 2017-04-02T21:34:53Z

This PR correspond to the scenario where a text message is sent to the first person on the wait list for a tool when a tool is returned.

… autoload the module

… after a tool is checked back in

…phonenumber in the system yet

…rwarding and it seems to be security related. Need to discuss it with people more knowledgable before implemeting this feature

ChaseBro · 2017-04-03T17:20:12Z

app/controllers/checkouts_controller.rb

@@ -181,5 +179,11 @@ def checkin_bak
      end
    end
  end
+
+  # def reply


You should finish or remove this commented out code before merge

We are talking to Israel to make sure to resolve our concern about security. We will remove this if security is not an issue, then we can uncomment and implement this section. If we do not finish the keyword process feature, this will be a good starting point for next year's team.

I hear what you're saying, but checking in commented out code is always bad practice. If you really want to save this for next year I suggest creating a wiki page or some other documentation about what your aims were including any un-finished code.

An alternative is to move the commented out code to a new feature branch. That way we don't have commented out code in master but the WIP code is still around for work next year.

ChaseBro · 2017-04-03T17:20:22Z

app/models/checkout.rb

@@ -31,10 +33,15 @@ def card_number
    @card_number
  end

+  # def reply_to(message_content)


ChaseBro · 2017-04-03T17:20:53Z

app/models/checkout.rb

+    end
+  end
+
+  # def process_text(message_content)


ChaseBro · 2017-04-03T17:22:42Z

app/models/checkout.rb

+  private
+
+  def notify
+    if (self.checked_in_at != nil)


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.

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?

Opened issue #256 about this issue

ChaseBro · 2017-04-03T17:22:51Z

config/routes.rb

@@ -63,6 +63,12 @@
    post 'uncheckin', on: :member
  end

+  # resource :checkouts do


ChaseBro · 2017-04-03T17:23:28Z

lib/messenger.rb

+      )
+    rescue Twilio::REST::RequestError => e
+      case e.code
+        when 21610


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

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.

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?

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)

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

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.

@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

…st trying to merge this code to goat branch

sclark · 2017-04-09T20:23:26Z

lib/messenger.rb

+      )
+    rescue Twilio::REST::RequestError => e
+      case e.code
+        when 21610


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.

sclark · 2017-04-09T20:28:44Z

app/models/checkout.rb

+  private
+
+  def notify
+    if (self.checked_in_at != nil)


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?

sclark

Alright looks good

coveralls · 2017-04-09T22:49:22Z

Coverage increased (+0.05%) to 14.207% when pulling 51e9ce2 on bl/waitlist_notify into ddca290 on 17/goat/main.

* updated the gemfile to include single test gem * Changed participant test for a current student in integration, added relationship to document test * Created Units tests for Charge model * Unit tests- shifts, events, documents (#228) * Created intial files for event and event type tests, modified documents relationships test * Document unit tests done, besides one clarification issue with search scope * Event unit tests done, added event and event type in factories.rb * Finished unit tests for FAQ * Fixed last error in document, have shift, shift type, and shift participant test passing * Unit Test - Tools (#229) * Created intial files for event and event type tests, modified documents relationships test * Document unit tests done, besides one clarification issue with search scope * Event unit tests done, added event and event type in factories.rb * Finished unit tests for FAQ * Fixed last error in document, have shift, shift type, and shift participant test passing * Made time to now instad of local in shift formatted_name test, finished toop type unit test and scope for tool test. Need to finish tool methods and tool_waitlist unit tests * FInished all unit test assigned to me besides tool and tool waitlist. Once tool waitlist is done, finish tool waitlist method in the tool unit test * Finished tool_waitlist test and finished the last method test in tool_test (now complete) * Test Cases for several models (#231) * created unit test for organization model. almost done * switching to goat branch to checkout akshay's commit * begining to modify test class for organization_alias_class. trying to merge rumby's modification on factories into my test branch * finished creating test class for organization_alias model. Everything functions properly * updated test for organization_category model. Everything functions properly * created new factory for organization_status_type. created a new test class for orgniazation_status but have been getting error message: factory not registered. Will come back to this later * fixed error! added organization_status to the factory file. test for organization_status now passed! * checking Rumby's PR. commiting before switching to goat * fixed previous file name error. Test for organization_status_type passed * finished up organization_timeline_entry test. removed duplicate schema comment in the status_type * updated organization test to include dependency testing * did some refactoring on organization_alias model and test * created test function for downtime methods * fixed syntax for org_category test. added test case for dependency * fixed minor syntax in test case for org_status and org_status_type * added test case for membership * added test case for judge, judgement, judgement_category * Added authorization checks - Home Controller & fixed Event okay/un-okay button (#234) * Added authorization checks so that anyone who isnt logged in cannot see downtime, charges overview, or hardhats * Changed the okay button to correct type of HTTP request so that the events ok/unokay works * Cleaned code (moved authorization from views to controllers) for charges (now participants can't read charges pages from views) * Commented shift test out, opened it up as a bug though so we know to go back and figure out the CI error on it * Added Dependency Tests for Participant, Coverage for Other Models & Bugfix (#237) * Added tests for charge_type model * Added tests for role model * Added tests for StoreItem model * Added tests for StorePurchase model * Added tests for Checkout model * Fixed charge model coverage * Added tests to charge_type, checkout, and partly participant * Added full test coverage for Task model * Added fixes to participant model tests * Added dependency tests for participant * Fixed duplicate cart items bug * Edited factory file to get rid of merge conflict * Added schema.rb file * Added Placeholder value for receiving participant when updating charges (#239) * created unit test for organization model. almost done * switching to goat branch to checkout akshay's commit * begining to modify test class for organization_alias_class. trying to merge rumby's modification on factories into my test branch * finished creating test class for organization_alias model. Everything functions properly * updated test for organization_category model. Everything functions properly * created new factory for organization_status_type. created a new test class for orgniazation_status but have been getting error message: factory not registered. Will come back to this later * fixed error! added organization_status to the factory file. test for organization_status now passed! * checking Rumby's PR. commiting before switching to goat * fixed previous file name error. Test for organization_status_type passed * finished up organization_timeline_entry test. removed duplicate schema comment in the status_type * updated organization test to include dependency testing * did some refactoring on organization_alias model and test * created test function for downtime methods * fixed syntax for org_category test. added test case for dependency * fixed minor syntax in test case for org_status and org_status_type * added test case for membership * added test case for judge, judgement, judgement_category * somehow event_type_test got updated with a new schema entry. Saving change before checking out akshay's unit tests * updated TravisCI's timezone to US Eastern time, which should match with the built in timezone indicated in application.rb * changed TRAVIS CI Setting to eastern time * trying to see if this fix for time works * travis yml syntax incorrect in the previous commit. just fixed it now. Hopefully this works * travis could not load timezone data. trying a different method * both attempts failed. reverted back to master branch version of travis configuration. Will create an issue * added redirect back to organization as a temporary solution for bug #201 * made modification to the charge param and fixed odd behavior, described in issue #202 * added placeholder value so that the receiving_participant does not seem to disappear * Update .travis.yml * Update event_type_test.rb * Cleaned Code for charge, charge_type, checkouts, and documents (#242) * Cleaned code for charges and charge type * Cleaned code for checkout * Cleaned code for documents besides the new * Cleaned views for events and event types * cleaned code for faqs * Added timezone on CI server config to correct CI server shift error (#245) * Reproducing issue #235 * Changing CI timezone to fix issue #235 * Ak/twilio notifications2 (#253) * added twilio messenger module, also tweaked the application config to autoload the module * Added 2 scenarios for notifications * Fixed organization timeline entry twilio notification * Fixed bugs for watch shift twilio notifications * Added assigned field to events and notifications for assignees when note is assigned to them * Switched notification delays to production times * Bl/waitlist notify (#254) * added twilio messenger module, also tweaked the application config to autoload the module * tried to modified messenger with text processing but failed * added code to send out text alert to the next person on the wait list after a tool is checked back in * added a safeguard for phone number in case someone has not put their phonenumber in the system yet * updated messenger module with try-rescue block * commented out code for reply forwarding. There is an issue for the forwarding and it seems to be security related. Need to discuss it with people more knowledgable before implemeting this feature * removed commented code. Will probably add it to a seperate branch. Just trying to merge this code to goat branch * modified messenger to keep track of error code * New features branch (#252) * Prevent participants from skipping saftey video * Added user certifications * Add certification check to tools checkout * Only show certifications required by tool type if they exist * Added cancan checks around certification creation and deletion * Added wristband field to participant view to help with wristband distribution #220 * Added digital signature to waiver #173 * Cleaned waiver code * Changed store items price datatype to support fractional prices #168 * Added rake db:migrate to travis script so it can build with migrations * Added rake db:migrate to travis script so it can build with migrations * Fixing style on PR #248 * Removing nil check for PR #248 * Changed error message after skipping the saftey video * Using migration technique from http://stackoverflow.com/questions/17150529 * Reverting to previous 9V battery price * Fixing spelling of 'safety' * Define wristband colors as constants * Added load_and_authorize_resource to CertificationsController * Changed building_status to is_building in db seeds file * Fixed waiver playing in bg for admin and allow admin to skip video * Added scissor lift wristband color * Updated safety video * Cleaned up waiver code and let user skip waiver on error from previous error * Fixed ForbiddenAttribute error on certification creation * Fixed messed up sidebar on SCC member new waiver * Removed duplicate gems in Gemfile * Added delayed_job bin to git (platanus/capistrano3-delayed-job#21) * Fixed db migration for is_building field in organization_category * Added requested legal clauses to waiver * Fixed scissor lift wristband color * Bl/waitlist notify (#259) * added twilio messenger module, also tweaked the application config to autoload the module * tried to modified messenger with text processing but failed * added code to send out text alert to the next person on the wait list after a tool is checked back in * added a safeguard for phone number in case someone has not put their phonenumber in the system yet * updated messenger module with try-rescue block * commented out code for reply forwarding. There is an issue for the forwarding and it seems to be security related. Need to discuss it with people more knowledgable before implemeting this feature * removed commented code. Will probably add it to a seperate branch. Just trying to merge this code to goat branch * modified messenger to keep track of error code * made change to charges controller as requested * Ak/twilio notifications2 (#258) * added twilio messenger module, also tweaked the application config to autoload the module * Added 2 scenarios for notifications * Fixed organization timeline entry twilio notification * Fixed bugs for watch shift twilio notifications * Added assigned field to events and notifications for assignees when note is assigned to them * Switched notification delays to production times * Added tweaks stephen requested, including showing preview of the event in text, fixing the scc exec member drop down, and sending a text when everyone is confirmed for a watch shift * Added production times for shift notifications * Added proper messenger code * Fixed up gemfile for merge * Fixed delayed_job mis-spelling * Update participant_test.rb * Updated safety video

Bruce Lin added 4 commits March 25, 2017 19:17

added twilio messenger module, also tweaked the application config to…

cafe725

… autoload the module

tried to modified messenger with text processing but failed

edf81c3

added code to send out text alert to the next person on the wait list…

27b44a8

… after a tool is checked back in

added a safeguard for phone number in case someone has not put their …

1edbd5a

…phonenumber in the system yet

brucethesloth requested review from sclark, AkshayGoradia and a user April 2, 2017 21:34

Bruce Lin added 2 commits April 2, 2017 20:17

updated messenger module with try-rescue block

5cd7d5e

commented out code for reply forwarding. There is an issue for the fo…

d11daf3

…rwarding and it seems to be security related. Need to discuss it with people more knowledgable before implemeting this feature

ChaseBro reviewed Apr 3, 2017

View reviewed changes

app/models/checkout.rb Outdated

@@ -31,10 +33,15 @@ def card_number

@card_number

end

# def reply_to(message_content)

Copy link

Member

ChaseBro Apr 3, 2017

Choose a reason for hiding this comment

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

same here

ChaseBro reviewed Apr 3, 2017

View reviewed changes

app/models/checkout.rb Outdated

end

end

# def process_text(message_content)

Copy link

Member

ChaseBro Apr 3, 2017

Choose a reason for hiding this comment

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

And here

ChaseBro reviewed Apr 3, 2017

View reviewed changes

config/routes.rb Outdated

@@ -63,6 +63,12 @@

post 'uncheckin', on: :member

end

# resource :checkouts do

Copy link

Member

ChaseBro Apr 3, 2017

Choose a reason for hiding this comment

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

And here

ChaseBro reviewed Apr 3, 2017

View reviewed changes

removed commented code. Will probably add it to a seperate branch. Ju…

06d5cab

…st trying to merge this code to goat branch

sclark suggested changes Apr 9, 2017

View reviewed changes

sclark requested review from AkshayGoradia and a user and removed request for AkshayGoradia and a user April 9, 2017 20:29

modified messenger to keep track of error code

f191563

sclark mentioned this pull request Apr 9, 2017

Tool waitlist text notification button? #256

Open

sclark approved these changes Apr 9, 2017

View reviewed changes

Merge branch '17/goat/main' into bl/waitlist_notify

51e9ce2

ghost merged commit 8dc4dc7 into 17/goat/main Apr 9, 2017

sclark deleted the bl/waitlist_notify branch April 10, 2017 03:42

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bl/waitlist notify #254

Bl/waitlist notify #254

brucethesloth commented Apr 2, 2017

ChaseBro Apr 3, 2017

brucethesloth Apr 3, 2017

ChaseBro Apr 3, 2017

pkoenig10 Apr 3, 2017

ChaseBro Apr 3, 2017

ChaseBro Apr 3, 2017

ChaseBro Apr 3, 2017

sclark Apr 9, 2017

sclark Apr 9, 2017

ChaseBro Apr 3, 2017

ChaseBro Apr 3, 2017

brucethesloth Apr 4, 2017

ChaseBro Apr 4, 2017

ChaseBro Apr 4, 2017

ChaseBro Apr 4, 2017

sclark Apr 9, 2017

brucethesloth Apr 9, 2017

sclark Apr 9, 2017

sclark Apr 9, 2017

sclark left a comment

coveralls commented Apr 9, 2017

Bl/waitlist notify #254

Bl/waitlist notify #254

Conversation

brucethesloth commented Apr 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sclark left a comment

Choose a reason for hiding this comment

coveralls commented Apr 9, 2017