Re: [PATCH] custom MO Parameters for smsc-http

Am 09.06.2009 um 11:23 schrieb Alejandro Guerrieri:

> Alex,
>
> Regarding the charsets, I agree it's better to tackle it now that  
> leave it for "later" (aka, "never" ;) ).
>
> I'm fixing the charset issue and the dlr bugs and resending later.
>

thanks alex!

> Regards,
> --
> Alejandro Guerrieri
> [email protected]
>
>
>
> On 09/06/2009, at 11:17, Alexander Malysh wrote:
>
>>
>> Am 09.06.2009 um 11:05 schrieb Alejandro Guerrieri:
>>
>>> Alex,
>>>
>>> Commenting inline below.
>>>
>>> Regards,
>>> --
>>> Alejandro Guerrieri
>>> [email protected]
>>>
>>>
>>> On 09/06/2009, at 10:44, Alexander Malysh wrote:
>>>
>>>> Hi,
>>>>
>>>> Seems you don't handle charset at all. Do you assume to receive  
>>>> UTF-8 in any case? I don't think it's
>>>> applicable to generic interface. At least alt-charset should be  
>>>> taken in account...
>>>>
>>>
>>> I'm not adding anything new here, the MO handling is built on the  
>>> original code for kannel_receive_sms (which was the default  
>>> interface for MO on the generic interface anyway). There's no  
>>> charset handling in there either. I agree it makes sense to have  
>>> charset handling, since an http interface should be as flexible as  
>>> possible, but is it a showstopper at this point?
>>>
>>
>> It is not really show stopper but if it will not be added now it  
>> will remain so for ages. It is not a problem for kannel_receive_sms  
>> because it's
>> well defined interface to smsbox which uses UTF-8.
>> I would prefer to see it implemented (there only few line of  
>> code ;) )
>>
>>>
>>>> some comments to patch below...
>>>>
>>>> why is dlrurl required for DLR? We have dlrurl stored in DLR  
>>>> storage...
>>>>
>>>> +    }
>>>> +    else if (dlrurl != NULL && dlrmask != 0 && dlrmid != NULL) {
>>>> +        /* we got a DLR, and we don't require additional values */
>>>> +        Msg *dlrmsg;
>>>>
>>>
>>> Idem... why is it needed for the kannel interface then?
>>
>> hmm, seems both buggy then. We don't need dlr-url for DLR. Could  
>> you please fix both interfaces?
>>
>>>
>>>> hmm this check is not complete. it should be udh_len +  
>>>> msgdata_len but msgdata_len depends in coding...
>>>>
>>>> +    else if (udh != NULL && octstr_len(udh) > MAX_SMS_OCTETS) {
>>>> +        error(0, "HTTP[%s]: UDH field is too long, rejected",
>>>> +              octstr_get_cstr(conn->id));
>>>> +        retmsg = octstr_create("UDH field is too long, rejected");
>>>> +        retstatus = fm->status_error;
>>>> +    }
>>>>
>>>
>>> Idem... this is from kannel_receive_sms as well. If it's wrong  
>>> here, then it's wrong on the "kannel" smsc as well.
>>
>> Hmm seems, I'm wrong here. This check checks for the right UDH but  
>> allows long MO messages.
>>
>>>
>>>>
>>>> why do you always load generic config options ?
>>>>
>>>> static void generic_send_sms(SMSCConn *conn, Msg *sms)
>>>> {
>>>> @@ -1635,6 +1934,7 @@
>>>>  cfg_get_bool(&conndata->no_sep, cfg, octstr_imm("no-sep"));
>>>>  conndata->proxy = cfg_get(cfg, octstr_imm("system-id"));
>>>>  conndata->alt_charset = cfg_get(cfg, octstr_imm("alt-charset"));
>>>> +    conndata->fieldmap = generic_get_field_map(cfg);
>>>>
>>>> ---> conndata->fieldmap = NULL:
>>>>
>>>>  if (conndata->send_url == NULL)
>>>>      panic(0, "HTTP[%s]: Sending not allowed. No 'send-url'  
>>>> specified.",
>>>> @@ -1693,7 +1993,7 @@
>>>>                octstr_get_cstr(conn->id));
>>>>          goto error;
>>>>      }
>>>>
>>>> --->    conndata->fieldmap = generic_get_field_map(cfg);
>>>>
>>>> -        conndata->receive_sms = kannel_receive_sms; /* emulate  
>>>> sendsms interface */
>>>> +        conndata->receive_sms = generic_receive_sms; /* emulate  
>>>> sendsms interface */
>>>>      conndata->send_sms = generic_send_sms;
>>>>      conndata->parse_reply = generic_parse_reply;
>>>
>>> True, it's a waste of CPU cycles if you're using other smsc-types  
>>> instead. Fixing it.
>>>
>>>>
>>>> Thanks,
>>>> Alex
>>>>
>>>> Am 09.06.2009 um 10:14 schrieb Alejandro Guerrieri:
>>>>
>>>>> Ok, here's a new version of my patch to add support for custom  
>>>>> MO parameters on the generic http-smsc.
>>>>>
>>>>> New features:
>>>>>
>>>>> 1. Support for setting the numeric response code for successful  
>>>>> and failed requests (as it is now, it always returns 202  
>>>>> HTTP_ACCEPTED).
>>>>> 2. Support for setting the text response for successful requests  
>>>>> (right now it returns "Sent.").
>>>>> 3. Some code cleanups (extra lines, parameter expansion after  
>>>>> authorization).
>>>>> 4. Documentation.
>>>>>
>>>>> http://www.blogalex.com/archives/192
>>>>>
>>>>> I can commit if no objections.
>>>>>
>>>>> Regards,
>>>>> --
>>>>> Alejandro Guerrieri
>>>>> [email protected]
>>>>>
>>>>>
>>>>>
>>>>> On 01/06/2009, at 22:50, Alexander Malysh wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> here some comments...
>>>>>>
>>>>>> in generic_receive_sms you first receive all values than  
>>>>>> converts it and only
>>>>>> after this done check authorization. Make no sense... first  
>>>>>> check auth then convert anything.
>>>>>>
>>>>>> +    else if (from == NULL || to == NULL || text == NULL) {
>>>>>> +
>>>>>> +        error(0, "HTTP[%s]: Insufficient args",
>>>>>>
>>>>>> why extra line?
>>>>>>
>>>>>> I think retmsg should also be configurable. Because not all  
>>>>>> gateways will accept 'Sent'  as response.
>>>>>> I think even HTTP error code for not accepted MOs/DLRs should  
>>>>>> be configurable.
>>>>>>
>>>>>> Thanks,
>>>>>> Alex
>>>>>>
>>>>>> P.S. And userguide part would be nice ;)
>>>>>>
>>>>>> Am 28.05.2009 um 23:00 schrieb Alejandro Guerrieri:
>>>>>>
>>>>>>> HI,
>>>>>>>
>>>>>>> I've finished my patch to configure the parameter names for MO  
>>>>>>> on the generic http-smsc.
>>>>>>>
>>>>>>> To use it you just add a few entries on the smsc definition,  
>>>>>>> only with the parameters you want to rename.
>>>>>>>
>>>>>>> e.g:
>>>>>>>
>>>>>>> generic-param-from = "phoneNumber"
>>>>>>> generic-param-to = "shortCode"
>>>>>>> generic-param-text = "message"
>>>>>>>
>>>>>>> More details and the patch here:
>>>>>>>
>>>>>>> http://www.blogalex.com/archives/171
>>>>>>>
>>>>>>> Of course I'm writing the userguide part if this goes forward :)
>>>>>>>
>>>>>>> Regards,
>>>>>>> --
>>>>>>> Alejandro Guerrieri
>>>>>>> [email protected]
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>