Bs 182 SMS Error Code REST API#2967
Conversation
| uri MEDIUMTEXT NOT NULL, | ||
| smpp_message_id MEDIUMTEXT | ||
| smpp_message_id MEDIUMTEXT, | ||
| error_code BIGINT |
There was a problem hiding this comment.
i would use a text based type instead
| * @author mariafarooq | ||
| * | ||
| */ | ||
| public enum Error { |
There was a problem hiding this comment.
I find this name too generic. Maybe SmsError is more suitable?
There was a problem hiding this comment.
agree,its even collisioning with existing class in JDK
| } | ||
| } | ||
|
|
||
| protected void writeError(final Error error, final HierarchicalStreamWriter writer) { |
There was a problem hiding this comment.
If this is SMS-specific error, why did we create these methods in an generic converter?
There was a problem hiding this comment.
agree this converter is getting loaded with specific entity details that will be rarely reused on diff converters,move to specific converter instead
| logger.warning("responseMessageId=" + smppMessageId + " was never received!"); | ||
| } else { | ||
| this.storage.getSmsMessagesDao().updateSmsMessage(sms.setSmppMessageId(null).setStatus(deliveryStatus)); | ||
| sms.setSmppMessageId(null); |
There was a problem hiding this comment.
CAREFUL!! each setter returns a brand new SMS object
There was a problem hiding this comment.
ummm,right this is a bug for sure. I would say lets create a builder with copy constructor instead, this is very dangerous idiom we are using.
| return status; | ||
| } | ||
|
|
||
| private Error parseRestcommErrorCode(String errCode) { |
There was a problem hiding this comment.
Duplicated logic in ErrorCodeMapper, with difference that code is a String. Can't we maintain consistency in single place?
There was a problem hiding this comment.
not really duplicated.one maping is for DLR err codes,the other for submitResp.they are diff mapping as documented
2 participants
What this PR does / why we need it:
Exposes SMS error codes in REST API
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes # Bs 182
Special notes for your reviewer: