Skip to content

Initial support for sending SMS from short codes#26

Closed
philnash wants to merge 1 commit intovisualitypl:masterfrom
philnash:short-code
Closed

Initial support for sending SMS from short codes#26
philnash wants to merge 1 commit intovisualitypl:masterfrom
philnash:short-code

Conversation

@philnash
Copy link
Copy Markdown
Contributor

@philnash philnash commented Nov 7, 2016

This should fix #25.

I have included an is_short_code? method that does a basic check on whether a number looks like a short code. In this case, that is simply whether it is a 5 or 6 digit long number. There are more restrictions on short codes than that, however this is a start and the assumption is that users would know what short code they are using along with the restrictions around it (i.e. no international messages).

I've started a Utils class for the is_short_code? method as it was needed in two places. Utils is not a great name but I couldn't think of anything better right now.

@philnash philnash mentioned this pull request Nov 7, 2016
Closed
@rylanb
Copy link
Copy Markdown

rylanb commented Nov 7, 2016

👍 Looks great to me, I can't seem to point at the ref / github in my Gemfile to test locally. Trying by copying into the gem temporarily.

Its early but am I missing something: gem 'textris', github: 'visualitypl/textris', ref: '48935afd28b8d7d96d202482ea334105f0104309' ?

@philnash
Copy link
Copy Markdown
Contributor Author

philnash commented Nov 7, 2016

It's on my fork of the gem right now, so you want github: 'philnash/textris' instead.

@rylanb
Copy link
Copy Markdown

rylanb commented Nov 7, 2016

Works locally just copy/pasting into local files in the gem. 👍

ronin
ronin suggested changes Nov 23, 2016
View reviewed changes
Copy link
Copy Markdown
Contributor

@ronin ronin left a comment

Choose a reason for hiding this comment

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

Hey @philnash. Thanks for this contribution. Would you mind doing little refactoring? I think it will be better to implement Utils as a plain class instead of module that is included in different classes.

May I suggest to use name ShortCode for this class and then just use it like

ShortCode.is_short_code?(phone)
# or
ShortCode.valid?(phone)
# or
ShortCode.plausible?(phone)

And tests for this class will be simpler.

@philnash
Copy link
Copy Markdown
Contributor Author

philnash commented Nov 24, 2016

Hey @ronin, sounds like a good idea. I'll get on that soon.

@nowakov
Copy link
Copy Markdown
Contributor

nowakov commented May 10, 2017

Hey @philnash, what's the status on this one? Let's implement what @ronin suggested and get it merged 😄

@philnash
Copy link
Copy Markdown
Contributor Author

philnash commented May 13, 2017

Oh wow! I completely forgot about this. I'll take a look and update soon.

@espen
Copy link
Copy Markdown

espen commented May 31, 2017

FYI: In Norway short numbers are 4 digits.

Marahin added a commit that referenced this pull request Nov 24, 2017
@Marahin
Add support for alphameric sender ID and short codes
6edc558 
Add support (based on @philnash's [code](#26))
for Short Codes and also fulfill request to bring Alphameric
Sender ID support.
Handle formatting/discovery logic in newly created class,
Textris::PhoneFormatter.
Marahin added a commit that referenced this pull request Nov 24, 2017
@Marahin
Add support for alphameric sender ID and short codes
cabfe92 
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 added a commit that referenced this pull request Nov 24, 2017
@Marahin
Add support for alphameric sender ID and short codes
dddc47b 
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 added a commit that referenced this pull request Nov 24, 2017
@Marahin
Add support for alphameric sender ID and short codes
99a3bda 
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 added a commit that referenced this pull request Nov 24, 2017
@Marahin
Add support for alphameric sender ID and short codes
33537d6 
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 added a commit that referenced this pull request Nov 24, 2017
@Marahin
Add support for alphameric sender ID and short codes
a1a6d19 
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.
@philnash
Copy link
Copy Markdown
Contributor Author

philnash commented Dec 1, 2017

Closing this in favour of #39.

@philnash philnash closed this Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@ronin ronin ronin requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Shortcodes don't work

5 participants

@philnash @rylanb @nowakov @espen @ronin