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.

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

Support reporting raids with SMS #24

Merged
merged 1 commit into from
Nov 5, 2017

Conversation

binyomen
Copy link
Contributor

If a subscribed user sends an SMS starting with "report", the text after
"report" will be interpreted as an address and sent out as an alert to
all subscribers. This operates through the same code path as the alert
form on the site.

Right now, this only supports plain text addresses. We will be able to
integrate it with geocoding in the future to acquire coordinates, as
well as correct typos.

If a subscribed user sends an SMS starting with "report", the text after
"report" will be interpreted as an address and sent out as an alert to
all subscribers. This operates through the same code path as the alert
form on the site.

Right now, this only supports plain text addresses. We will be able to
integrate it with geocoding in the future to acquire coordinates, as
well as correct typos.
@binyomen
Copy link
Contributor Author

Issue #13

@binyomen
Copy link
Contributor Author

Also, I'm still very new to Scala, so if there's anything I did which isn't idiomatic or which could be done better in Scala, let me know :)

Copy link
Collaborator

@joshmcfarlane joshmcfarlane left a comment

Choose a reason for hiding this comment

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

Code looks good - The one concern I have is if we want all subscribers to be able to send out alerts system wide. If you're at the tech meeting we can chat about it there, otherwise I'll bring it up with the rest of the group and update the PR with the notes from the discussion.

@binyomen
Copy link
Contributor Author

binyomen commented Nov 2, 2017

Oh, I 100% agree! I meant this mainly as a stop-gap until we have some sort of whitelist, but I'd also be happy to build in some basic whitelist functionality into this PR.

@binyomen
Copy link
Contributor Author

binyomen commented Nov 2, 2017

And, unfortunately I won't be able to make it to the meeting, but I'll stay in touch!

@binyomen
Copy link
Contributor Author

binyomen commented Nov 5, 2017

Just double checking if this is good to merge. Since this app is still in development, I think it's OK to submit this without a whitelist and wait to do whitelisting correctly in another change, but if we want I can add an unauthenticated whitelist page to the site as part of this PR and only allow whitelisted users to submit reports.

@joshmcfarlane joshmcfarlane merged commit 942181f into ColectivaLegal:master Nov 5, 2017
@joshmcfarlane
Copy link
Collaborator

Thanks for the follow up - I agree we can submit this without whitelisting as we're not using it in production yet. We can follow up in another PR to add it. Merged!

@binyomen binyomen deleted the basic_sms_report branch November 5, 2017 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants