Closed vetss closed 7 years ago
@abdulazizali77 please provide estimates
@scottbarstow my estimate should at least be double of @vetss , this would be inclusive of setting up a local environment and testing
Hello @abdulazizali77
I have checked your update https://github.com/RestComm/smscgateway/pull/183/commits, thanks for it. I have added your commit with my little fixes. My comments:
1) removeTlvParameter(MProcMessage message, short tag); must have an parameter message for processing of a concrete message. I have adding this. Sorry for I have not added the parameter into an initial because I was sure that you will reuse of templates from other methods.
2) Having of a parameter tlvValueToMatch as byte[] is rather complicated to manege from GUI / CLI from my point of view. I do suggest to have a String value. Then it will be easy to have getter / setter, CLI / GUI processing. But a next problem is that we need to compare values that may be different for different TVL types. To overcome it I suggest to have different implementations for following different TLV types:
I have commented (not removed) current implementation (in order to not affect of current behaviour). Please uncomment of the code and and finish of implementation.
3) GUI / manual parts are not updated so far. We need to finish it. This is an example of implementation: https://github.com/RestComm/smscgateway/commit/df8dd6501c5841f00d3d599a7bfbd71e3a6d83ad
4) we need to remove code like if (SmppConstants.TAG_NAME_MAP.containsKey(tag)) { .... } because we will the most probably reuse of custome parameters. Please fix it.
Fixed by: https://github.com/RestComm/smscgateway/commit/0ad0472394bda823b1b217fa36575c4942741781 https://github.com/RestComm/smscgateway/commit/e2417e5020939589ebb3b11cc84827e9eb5d1f2e https://github.com/RestComm/smscgateway/commit/a22563f1bdcde615bb84ad37b83b5e47ca29c3ac https://github.com/RestComm/smscgateway/commit/424325035b0051aed9d241dc38c9f2fbb2c30c32 https://github.com/RestComm/smscgateway/commit/06d4556ae108c4aebdc60f816582d679c4d4ee5b https://github.com/RestComm/smscgateway/commit/294c8b4fef65fd8c7bb9867ba6e58ebb4abc5c74
Thanks @vetss. I will address the additional fixes tomorrow/next few days For 4) Should we maintain our own hashmap of allowable custom parameters OR should we allow any and all adhoc parameters set by the user?
@abdulazizali77
for 4) I think we cal allow any tag value (not from some a specified list). If you asked something esle please comment
thanks @vetss you answered my question
@vetss, One thought/question for 2) should we allow limited regex patterns for tlvTagToMatch and or tlvValueToMatch ? eg tlvbyte. tlv_byte_1. tlv_byte_1. tlv_byte_1.. Just a thought, its not easy to implement
@abdulazizali77
I do not think we need a regex pattern for a TLV tag matchng. I beleive we will know a concrete value of TLV tag. Or you have scenarious when we need it ?
Check also my comment in https://github.com/RestComm/smscgateway/issues/180
Please let me know if you have some doubts of desired approach.
Hello @abdulazizali77
I checked your update in https://github.com/RestComm/smscgateway/pull/190 and have some comments:
1) Your solution is: short tlvTagToMatchString = -1; String tlvValueToMatchString = ""; short tlvTagToMatchInt = -1; int tlvValueToMatchInt = -1; short tlvTagToMatchByte = -1; byte tlvValueToMatchByte = -1; I think it is simpler to have following values: short tlvTagToMatch = -1; TlvValueType tlvValueTypeToMatch = null; String tlvValueToMatch = null; and enum TlvValueType { byte, int, String }; When making of matching we need cast byte / int TLV values into String firstly and then compare it with tlvValueToMatch This solution seems to me more clear.
2) getRuleParameters() - a) when you are describing a condition (like "tlv_byte_11 333") you need to put it into a condition section (before newNetworkId in your case). "tlvTagToRemove" - at the end because it is an action. We have firtsly all conditions and then all actions. b) we need to have "tlvTagToMatchString" parameters names like we have in CLI interface "tlvString_11" or "tlvByte_12" or "tlvInt_13" in our case, you can check how other name correlate to CLI name. Here we need to have names like CLI commands (not like tlvTagToMatch / tlvValueTypeToMatch / tlvValueToMatch)
3) we need to add getters / setters for GUI. For "remove_tlv 11" it is similar like others (one getter / setter). But for conditions like "tlv_byte_11 333" we need 3 getters / setters (for tlvTagToMatch / tlvValueTypeToMatch / tlvValueToMatch). We need to add them into MProcRuleDefaultImpl and MProcRuleDefault interface.
4) finally we need to update GUI and manual
May I ask you to refactor code and provide a new update.
Thanks @vetss , i have updated the PR, please have a look. Issues: -Confused/Uncertain. Should the conditions/commands be "tlv_byte_11 333" OR "tlvbyte_11 333" ? I have left it at "tlv_byte_11 333" -Could have defined TlvValueType in a separate enum class file or defined elsewhere. -I would like to update the GUI and docs in a separate PR
Hello @abdulazizali77
I have added your commit and add my fix. My fix is for default values, values checking and a way how to load / store ENUM values.
-Confused/Uncertain. Should the conditions/commands be "tlv_byte_11 333" OR "tlvbyte_11 333" ? I have left it at "tlv_byte_11 333"
Let it be "tlv_byte_11 333" as we have now
-Could have defined TlvValueType in a separate enum class file or defined elsewhere.
Check my update, I fixed it.
-I would like to update the GUI and docs in a separate PR
Please go ahead to finish the issue and make testing at a live server.
thanks @vetss
Hello @abdulazizali77
have you finished all updates for this issue ? Have you tested you work at a live server ? Does GUI / CLI management work for new parameters ? (if not all is implemented we need to finish it) We need also to update the manual - this chapter : http://documentation.telestax.com/core/smsc/SMSC_Gateway_Admin_Guide.html#_mproc_rules_fundamentals
apologies for the late update @vetss -we have tested on live staging server, Default mproc rule works fine. -have pushed GUI CLI and docs PR in #205
Hello @abdulazizali77
the work is finished by https://github.com/RestComm/smscgateway/pull/205 Thanks for your update.
thank you @vetss
We need to implement functionality when SMSC GW will change networkId depending on a value of some TVL field.
We need to implement:
We need to update default mptoc rules by: