-
Notifications
You must be signed in to change notification settings - Fork 7
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
Fix "MSISDN invalid" error #8
base: master
Are you sure you want to change the base?
Fix "MSISDN invalid" error #8
Conversation
When the values of the arguments `from` and `to` (that must be a Vodacom phone number) are in `841234567` format as described in the documentation, you receive an `MSISDN invalid` error. The values of these properties must start with the country code `258` so that the error mentioned above does not occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@callebdev Thanks for finding this and coming up with a PR.
The most elegant way to fix it would be to use RegEx, but this workaround should work for now.
I just left a few comments that should be addressed before merging it:
@@ -72,7 +72,11 @@ public Builder amount(double amount) { | |||
} | |||
|
|||
public Builder from(String from) { | |||
this.from = from; | |||
if (from.length() == 9) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking the string's length sounds a bit hacky. Maybe we should check if the string starts with "258" instead?
@@ -72,7 +72,11 @@ public Builder amount(double amount) { | |||
} | |||
|
|||
public Builder from(String from) { | |||
this.from = from; | |||
if (from.length() == 9) { | |||
this.from = 258 + from; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this field is a String, let's use a String literal here
this.from = 258 + from; | |
this.from = "258" + from; |
if (to.length() == 9) { | ||
this.to = 258 + to; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 2 comments I left above also apply here.
@rosariopfernandes Thank you very much for leaving this code review. I will make the proposed changes. |
Closes Issue #7
The values of
to
andfrom
must start with the country code258
so that the error does not occur.With the current changes:
If:
841234567
is passed as the argument offrom
orto
, its value will be258841234567
;258841234567
is passed as the argument offrom
orto
, its value will continue being258841234567
;Thus, if the phone number is valid, regardless of whether its format is
258841234567
or841234567
, the MSISDN error will not occur.