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

only one last thing:

+        msg->sms.account = octstr_duplicate(account);
+        msg->sms.binfo = octstr_duplicate(binfo);
+        if (ret == -1) {
+            retmsg = octstr_create("Not accepted");
+            retstatus = fm->status_error;
+        } else {
+            retmsg = urltrans_fill_escape_codes(fm->message_sent, msg);
+            retstatus = fm->status_sent;
+        }
+        ret = bb_smscconn_receive(conn, msg);
+    }

are you sure you want check ret before bb_smscconn_receive ? ;)

Thanks,
Alex

Am 09.06.2009 um 13:23 schrieb Alejandro Guerrieri:

> Ok, done!
>
> http://www.blogalex.com/wp-content/uploads/2009/06/kannel-http-mo-params1.patch
>
> I've fixed the dlr-url bug _only_ on the generic part. If no  
> objections, I'm doing a second patch to fix it on the kannel smsc  
> (since it's in fact a "separate" issue).
>
> I've also fixed a small glitch on the dlrmsg response code (I was  
> using the "error" status code for successful submits as well).
>
> Last but not least, I've added url translation to the response  
> message, so now you can include escape codes on the response, which  
> may come handy on many cases (for example, to return kannel's  
> message id on the requests). Userguide part updated accordingly.
>
> Regards,
> --
> Alejandro Guerrieri
> [email protected]
>
>
>
> On 09/06/2009, at 11:27, Alexander Malysh wrote:
>
>>
>> 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]
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>