feat(sms): return country code from /sms/status #1766
Conversation
|
While integrating, I thought of a case that might be worthwhile (or might be a corner case to be ignored). For testing, we allow a For E2E testing it would be great if the verification link could be opened with a This would allow testers in countries that are enabled in prod, for example, Romania hasn't been given legal approval yet, but Softvision testers would be able to open the verification link with |
|
Sounds fine to me, @shane-tomlinson. So concretely, you want to specify |
|
|
@shane-tomlinson, new and improved! Like old PR but with more query param! Choose PR #1766 for all your country code needs! Tell your friends! |
|
There were two stupid things I'd done in here that were breaking this for @shane-tomlinson:
Fixed now. |
philbooth
force-pushed the
phil/sms-status-return-country-code
branch
from
6619336
to
6c7da33
philbooth
force-pushed the
phil/sms-status-return-country-code
branch
from
6c7da33
to
f463b31
philbooth
force-pushed the
phil/sms-status-return-country-code
branch
from
f463b31
to
dd3e6c1
|
I haven't tested it yet and it's too late at night to start down that road, but this looks good @philbooth. I had a question about whether one more test case should be present (country query param specified that doesn't have support) and whether the /sms/status remote tests work for you and I in GB, and if so, how. I'll test this tomorrow. |
| @@ -589,3 +589,103 @@ describe('/sms/status with disabled geo-ip lookup', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| describe('/sms/status with query param and enabled geo-ip lookup', () => { | |||
shane-tomlinson
Mar 28, 2017
Member
+ 💯 to these tests!
+
| @@ -589,3 +589,103 @@ describe('/sms/status with disabled geo-ip lookup', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| describe('/sms/status with query param and enabled geo-ip lookup', () => { | |||
shane-tomlinson
Mar 28, 2017
Member
Is there a case to be made for a test that's with query param that's not enabled? For example, if some goon queries the endpoint with country=AU will it return ok: false?
Is there a case to be made for a test that's with query param that's not enabled? For example, if some goon queries the endpoint with country=AU will it return ok: false?
philbooth
Mar 28, 2017
Author
Contributor
In terms of paths through the code, the REGIONS.has has check is independently variable to the query param check, so I see it as already covered.
I know some people prefer a comprehensive/completionist approach though, were you asking from that perspective? I can add one if so.
In terms of paths through the code, the REGIONS.has has check is independently variable to the query param check, so I see it as already covered.
I know some people prefer a comprehensive/completionist approach though, were you asking from that perspective? I can add one if so.
shane-tomlinson
Mar 29, 2017
Member
I know some people prefer a comprehensive/completionist approach though, were you asking from that perspective?
I guess it's the difference between code coverage and path coverage. Both are reasonable to strive for, though path coverage is far more difficult to attain for complex functions.
Someone is almost certainly going to try to hit the endpoint with a bunk country, what happens in that situation, will there be a 400 error and blocked with joi validation, or will this code be exercised? If this code is exercised, it'd be good to be confident it doesn't blow up with bunk countries.
The front end allows a specific list of countries and will error if a country isn't on the list, though that'll only mitigate potential problems for people that use the front end.
I know some people prefer a comprehensive/completionist approach though, were you asking from that perspective?
I guess it's the difference between code coverage and path coverage. Both are reasonable to strive for, though path coverage is far more difficult to attain for complex functions.
Someone is almost certainly going to try to hit the endpoint with a bunk country, what happens in that situation, will there be a 400 error and blocked with joi validation, or will this code be exercised? If this code is exercised, it'd be good to be confident it doesn't blow up with bunk countries.
The front end allows a specific list of countries and will error if a country isn't on the list, though that'll only mitigate potential problems for people that use the front end.
shane-tomlinson
Mar 29, 2017
Member
FWIW, if I allow any country on the front end and hit the backend with a bunk country, it behaves as expected.
FWIW, if I allow any country on the front end and hit the backend with a bunk country, it behaves as expected.
| .then(status => { | ||
| assert.ok(status) | ||
| assert.equal(typeof status.ok, 'boolean') | ||
| assert.equal(status.country, 'US') |
shane-tomlinson
Mar 28, 2017
Member
Will this work for you and I in the UK?
Will this work for you and I in the UK?
philbooth
Mar 28, 2017
Author
Contributor
Exactly, yep.
Exactly, yep.
philbooth
force-pushed the
phil/sms-status-return-country-code
branch
from
dd3e6c1
to
8ef2dfa
|
this tests well locally @philbooth! I'll do more testing with it on a remote server that does geolookup later. I did a bit of testing remotely, but not thoroughly. r+ |
|
Thanks @shane-tomlinson! |
@shane-tomlinson, sorry it took me so long to get round to this, flow event stuff is a bit of a nightmare at the moment. Was there an issue for this? Is it what you wanted?