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 for Short Codes and Alphameric sender ID #39

Merged
merged 6 commits into from Dec 6, 2017

Conversation

@Marahin
Copy link
Contributor

@Marahin Marahin commented Nov 24, 2017

Tested on example app:

Sent text to +48 12 545 67 89
Texter: User#welcome
Date: 2017-11-24 15:39:54 +0100
From: Team Sp.z.o.o
To: +48 12 545 67 89
Rendered user_texter/welcome.text.erb (0.2ms)

Keep in mind that in order to use Alphanumeric sender ID you have to declare it this way: from: 'My Team <Team Inc.>', where 'My Team' will be equal to name, and Team Inc. to Sender ID (.from_phone).

Gemfile Outdated
@@ -1,3 +1,5 @@
source 'https://rubygems.org'

gemspec

gem 'pry'

This comment has been minimized.

@Lackoftactics

Lackoftactics Nov 24, 2017
Contributor

I am not sure if pry should be here, or add it as development dependency.

if (matches = from.to_s.match(/(.*)\<(.*)\>\s*$/).to_a).size == 3 &&
Phony.plausible?(matches[2])
matches = from.to_s.match(/(.*)\<(.*)\>\s*$/).to_a
return if matches.size != 3

This comment has been minimized.

@Lackoftactics

Lackoftactics Nov 24, 2017
Contributor

magic number, what 3 represents here?

This comment has been minimized.

@Marahin

Marahin Nov 24, 2017
Author Contributor

@Lackoftactics https://github.com/visualitypl/textris/blame/ab770cb966218d6a4c2225a7a7ab360d590698ae/lib/textris/message.rb#L88

But:

irb(main):001:0> "Jan Matusz <+48123456789>".match(/(.*)\<(.*)\>\s*$/).to_a
=> ["Jan Matusz <+48123456789>", "Jan Matusz ", "+48123456789"]

So basicly it "splits" the sender string (which allows us to normalize/set the sender ID and sender name explicitly).

@Marahin Marahin force-pushed the short-codes-alphamerics branch from cabfe92 to dddc47b Nov 24, 2017
@@ -81,15 +81,20 @@ def parse_from(from)
end

def parse_from_dual(from)
if (matches = from.to_s.match(/(.*)\<(.*)\>\s*$/).to_a).size == 3 &&
Phony.plausible?(matches[2])
matches = from.to_s.match(/(.*)\<(.*)\>\s*$/).to_a

This comment has been minimized.

@Lackoftactics

Lackoftactics Nov 24, 2017
Contributor

use captures instead of converting to array
from.to_s.match(/(.*)\<(.*)\>\s*$/).captures

This comment has been minimized.

@Marahin

Marahin Nov 24, 2017
Author Contributor

@Lackoftactics done, but this required me to use the short try notation (&.).

@Marahin Marahin force-pushed the short-codes-alphamerics branch 2 times, most recently from 99a3bda to 33537d6 Nov 24, 2017
Phony.plausible?(matches[2])
matches = from.to_s.match(/(.*)\<(.*)\>\s*$/).to_a
return if matches.size != 3
if Phony.plausible?(matches[2]) || PhoneFormatter.is_a_short_code?(matches[2])

This comment has been minimized.

@Lackoftactics

Lackoftactics Nov 24, 2017
Contributor

maybe some simple helper method, that will be readable for getting name and number from the dual field

philnash and others added 3 commits Nov 7, 2016
.idea folder contains JetBrains' IDE data and configurations,
it should not be added to the project.
Continue support introduction (based on @philnash's code:
#26)
for Short Codes and fulfill request to bring Alphameric
Sender ID support. Handle formatting/discovery logic in newly
created class, Textris::PhoneFormatter.
@Marahin Marahin force-pushed the short-codes-alphamerics branch 2 times, most recently from 261ff84 to ab5ffd9 Nov 24, 2017
Phony.plausible?(matches[2])
[matches[1].strip, Phony.normalize(matches[2])]
matches = from.match(/(.*)\<(.*)\>\s*$/)
return if matches.nil?

This comment has been minimized.

@Lackoftactics

Lackoftactics Nov 24, 2017
Contributor

checking nil? maybe better to

return if !matches

What do you think?

This comment has been minimized.

@Marahin

Marahin Nov 24, 2017
Author Contributor

@Lackoftactics maybe return unless matches?

`&.` breaks backward compatibility - instead add explicit calls
that check if there are any matches; and if there are - select the captures.
@Marahin Marahin force-pushed the short-codes-alphamerics branch from ab5ffd9 to 3b66337 Nov 24, 2017
# that cannot be resolved as a short code or a phone
# number, then it is Alphameric Sender ID
def is_alphameric?(phone)
!is_a_phone_number?(phone) && !is_a_short_code?(phone)

This comment has been minimized.

@philnash

philnash Nov 27, 2017
Contributor

The rules for an alphanumeric ID are:

You may use any combination of 1 to 11 letters, A-Z and numbers, 0-9. Both lowercase and uppercase characters are supported as well as spaces. 1 letter and no more than 11 alphanumeric characters may be used.

I messed around with a regex to match this constraint and came up with:

\A(?=.*[a-zA-Z])[a-zA-z\d]{1,11}\z

Broken down, this is:

\A               # Start of the string
(?=.*[a-zA-Z])   # Lookahead to ensure there is at least one letter in the entire string
[a-zA-z\d]{1,11} # Between 1 and 11 characters in the string
\z               # End of the string

So we could rewrite this as:

def is_alphanumeric?(phone)
  !!phone.to_s.match(/\A(?=.*[a-zA-Z])[a-zA-z\d]{1,11}\z/)
end

This comment has been minimized.

@Marahin

Marahin Nov 27, 2017
Author Contributor

Awesome!

end

it 'treat non-short code non-phone number numbers as alphamerics' do
['8945', '894', '89', '8', '8945467', '89454678', '894546789'].each do |number|

This comment has been minimized.

@philnash

philnash Nov 27, 2017
Contributor

These tests are incorrect, alphanumeric sender IDs are required to have at least one letter.

I'd recommend tests that ensured these numbers were not considered as alphanumeric sender IDs, but that the strings: a, 1a, 1aaaaaaaaaa and ZZZZZZZZZZ9 are all considered valid alphanumeric IDs.

We should also ensure that alphanumeric sender IDs are not more than 11 characters long.

This comment has been minimized.

@Marahin

Marahin Nov 27, 2017
Author Contributor

This test case was not supposed to check whether it's classified as alphameric, but rather 'treated' as it was like it (so no additional formatting), if I remember correctly :) but yes, this should be replaced with a proper alphamerics tests, I agree.

@Marahin Marahin merged commit 66ed742 into master Dec 6, 2017
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@Marahin Marahin deleted the short-codes-alphamerics branch Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.