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

Hi,

patch commited to CVS.

Thanks,
Alex

Am 22.06.2009 um 18:42 schrieb Alejandro Guerrieri:

> Looks fine, +1. This definitely completes (no longer semi) generic  
> http interface.
>
> Regards,
> --
> Alejandro Guerrieri
> [email protected]
>
>
>
> On 22/06/2009, at 18:35, Franck LAMASUTA wrote:
>
>> Here is the new version...
>>
>>
>>
>>
>>> -----Original Message-----
>>> From: [email protected]
>>> [mailto:[email protected]] On Behalf Of Franck LAMASUTA
>>> Sent: Monday, June 22, 2009 4:53 PM
>>> To: [email protected]
>>> Subject: RE: [PATCH] custom MO Parameters for smsc-http
>>>
>>> Thanks for your comments.
>>>
>>> First comment regarding the variable name:
>>> The new variable is closely related to status-success-regex
>>> which doesn't start with "generic-".
>>> But ok, I will change it if you think it's better...
>>>
>>>
>>> Second comment regarding pmatch[1].rm_eo:
>>> The test seems to be useless according to regexec() documentation.
>>> But it's cheap so it can't hurt.
>>>
>>> I will update userguide.xml also.
>>>
>>> Franck
>>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Alejandro Guerrieri [mailto:[email protected]]
>>>> Sent: Monday, June 22, 2009 12:37 PM
>>>> To: Alexander Malysh
>>>> Cc: [email protected];[email protected]
>>>> Subject: Re: [PATCH] custom MO Parameters for smsc-http
>>>>
>>>> +1 with the changes Alex proposed. I'd rename
>>>> ack_foreign_id_regex as
>>>> well, for clarity's sake.
>>>>
>>>> Regards,
>>>> --
>>>> Alejandro Guerrieri
>>>> [email protected]
>>>>
>>>> On 22/06/2009, at 12:26, Alexander Malysh wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> patch looks ok. Here some minor comments:
>>>>>
>>>>> +    OCTSTR(ack-foreign-id-regex)
>>>>> would it be better to name it something like generic-foreign-id-
>>>>> regex? so user know it belongs only to generic http...
>>>>>
>>>>> +                if (pmatch[1].rm_so != -1) {
>>>>> better to check the end as well -> if (pmatch[1].rm_eo != -1 &&
>>>>> pmatch[1].rm_so != -1)
>>>>>
>>>>> Otherwise patch looks good.
>>>>> Please make patch to userguide and I will commit it to CVS.
>>>>>
>>>>> Thanks,
>>>>> Alex
>>>>>
>>>>> Am 19.06.2009 um 14:40 schrieb Franck LAMASUTA:
>>>>>
>>>>>> Here it is, implemented with second idea...
>>>>>>
>>>>>> I did a small optimization of previous gw_regex_exec() calls in
>>>>>> generic_parse_reply().
>>>>>> I have also fixed 3 debug() calls in generic_receive_sms()
>>>>>> ("smsc.http.kannel" => "smsc.http.generic").
>>>>>>
>>>>>> Waiting for your comments...
>>>>>>
>>>>>> Franck
>>>>>>
>>>>>>
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Alejandro Guerrieri [mailto:[email protected]]
>>>>>>> Sent: Wednesday, June 10, 2009 3:08 PM
>>>>>>> To: Alexander Malysh
>>>>>>> Cc: [email protected];[email protected]
>>>>>>> Subject: Re: [PATCH] custom MO Parameters for smsc-http
>>>>>>>
>>>>>>> Me too, second one is far more flexible and clean.
>>>>>>> --
>>>>>>> Alejandro Guerrieri
>>>>>>> [email protected]
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 10/06/2009, at 14:31, Alexander Malysh wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I'm for the second idea, regex rocks ;)
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Alex
>>>>>>>>
>>>>>>>> Am 10.06.2009 um 12:37 schrieb Franck LAMASUTA:
>>>>>>>>
>>>>>>>>> It's not so urgent but I could try, it could be interesting...
>>>>>>>>>
>>>>>>>>> I have 2 ideas so far...
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> First idea
>>>>>>>>> ==========
>>>>>>>>>
>>>>>>>>> I could add 2 new config parameters to define 2 regex:
>>>>>>>>> - the first one, to find the start of the foreign id.
>>>>>>>>> - the second one, to find the end of the foreign id.
>>>>>>>>>
>>>>>>>>> For Claro, they could be defined like this:
>>>>>>>>> foreign-id-regex-begin = "<transaction-id>"
>>>>>>>>> foreign-id-regex-end = "</transaction-id>"
>>>>>>>>>
>>>>>>>>> For Clickatell, they could be defined like this:
>>>>>>>>> foreign-id-regex-begin = "ID: "
>>>>>>>>> foreign-id-regex-end = " "
>>>>>>>>>
>>>>>>>>> For Brunet, they could be defined like this:
>>>>>>>>> foreign-id-regex-begin = "MessageId="
>>>>>>>>> foreign-id-regex-end = " "
>>>>>>>>>
>>>>>>>>> In generic_parse_reply(), only in case of success,
>>>>>>> regex-begin will
>>>>>>>>> be used to find the beginning of the id. Starting from
>>>>>>> there, regex-
>>>>>>>>> end will be used to find the end of the id. If regex-end
>>>>>>> can't find
>>>>>>>>> a match, the id will be read until the end of text
>>> (body) and an
>>>>>>>>> ending CRLF will be removed.
>>>>>>>>> Then, the id will be used to call dlr_add() (like done in
>>>>>>>>> clickatell_parse_reply()).
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Second idea
>>>>>>>>> ===========
>>>>>>>>>
>>>>>>>>> It's basically the same idea except that only one regex
>>>>>>> will be used.
>>>>>>>>>
>>>>>>>>> For Claro, it could be defined like this:
>>>>>>>>> foreign-id-regex = "<transaction-id>(.*)</transaction-id>"
>>>>>>>>>
>>>>>>>>> For Clickatell, it could be defined like this (if an
>>> id contains
>>>>>>>>> only digits):
>>>>>>>>> foreign-id-regex = "ID: ([0-9]+)"
>>>>>>>>>
>>>>>>>>> The offsets given by gw_regex_exec() in pmatch[1] could be
>>>>>>> used to
>>>>>>>>> extract the id.
>>>>>>>>> However, it could be more difficult to understand for
>>> people who
>>>>>>>>> are not regex experts...
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Any comment, opinion, suggestion or better idea? :)
>>>>>>>>>
>>>>>>>>> Franck
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> From: Alexander Malysh
>>>>>>>>>> Sent: Tuesday, June 09, 2009 6:05 PM
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Am 09.06.2009 um 17:55 schrieb Alejandro Guerrieri:
>>>>>>>>>>
>>>>>>>>>>> Franck,
>>>>>>>>>>>
>>>>>>>>>>> The dlr part is inherited from the "kannel" smsc
>>>> format. In the
>>>>>>>>>>> actual implementation there's no way to grab the remote
>>>>>>> message id
>>>>>>>>>>> from a request, and it'd make a lot of sense to be
>>>> able to do
>>>>>>>>>>> it.
>>>>>>>>>>>
>>>>>>>>>>> I can try adding it, but don't refrain to do it so
>>>>>>> yourself if you
>>>>>>>>>>> feel the urge (you'll learn a lot about the code in the
>>>>>>> process!).
>>>>>>>>>>
>>>>>>>>>> yep, Alex is right here :) and we need more contributors :)
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Opinions?
>>>>>>>>>>> --
>>>>>>>>>>> Alejandro Guerrieri
>>>>>>>>>>> [email protected]
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 09/06/2009, at 17:47, Franck LAMASUTA wrote:
>>>>>>>>>>>
>>>>>>>>>>>> Hi Alejandro ,
>>>>>>>>>>>>
>>>>>>>>>>>> I work with Hervé who posted to this list the 25th of May
>>>>>>>>>>>> (Claro
>>>>>>>>>>>> http communication).
>>>>>>>>>>>> It seems the patch you've done on the generic part
>>> could be
>>>>>>>>>>>> very
>>>>>>>>>>>> useful for us to manage our communications with Claro. It
>>>>>>>>>> could be
>>>>>>>>>>>> better than a specific implementation for Claro.
>>>>>>>>>>>>
>>>>>>>>>>>> However, I don't understand how your patch manages a DLR:
>>>>>>>>>>>> In generic_receive_sms(), you call dlr_find() (line
>>>> 1674 of the
>>>>>>>>>>>> patched source) to retrieve a previous record stored
>>>> in the DLR
>>>>>>>>>>>> storage.
>>>>>>>>>>>> It seems fine except that dlr_add() is never called in
>>>>>>>>>>>> generic_parse_reply(), so I guess dlr_find() will always
>>>>>>>>>> return NULL.
>>>>>>>>>>>> Am I wrong???
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> In fact, when we submit a MT SMS to Claro with this request
>>>>>>>>>>>>
>>>>>>>>>>
>>> http://retail.mds.claro.com.br/MSE/api?profile=xxxx&pwd=xxxx&m
>>>>>>>>> ode=assync-delivery&ANUM=3333&BNUM=1234567&TEXT=test
>>>>>>>>>>>>
>>>>>>>>>>>> we get a HTTP code 200 with this body:
>>>>>>>>>>>>
>>>>>>>>>>>> <?xml version="1.0" encoding="UTF-8" ?>
>>>>>>>>>>>> <mse-response>
>>>>>>>>>>>> <status-code>0</status-code>
>>>>>>>>>>>> <profile>profleID</profile>
>>>>>>>>>>>> <transaction-id>1020606201622668099</transaction-id>
>>>>>>>>>>>> </mse-response>
>>>>>>>>>>>>
>>>>>>>>>>>> So I guess that the transaction-id should be used in
>>>>>>>>>>>> generic_parse_reply() to call dlr_add().
>>>>>>>>>>>> Unfortunately, the current implementation does not allow
>>>>>>>>>> to manage
>>>>>>>>>>>> such an id, it only allows to search if the request was
>>>>>>>>>> successful
>>>>>>>>>>>> or not through the regex.
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> If all my hypothesis are right... :-)
>>>>>>>>>>>> I could try to patch generic_parse_reply() to manage
>>>> also the
>>>>>>>>>>>> id
>>>>>>>>>>>> through another regex. It will be my first Kannel
>>> patch! ;-)
>>>>>>>>>>>> If you prefer to do it, no problem, just let me know.
>>>>>>>>>>>>
>>>>>>>>>>>> If I'm wrong, please clarify how it works.
>>>>>>>>>>>>
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Franck
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> <kannel_http-generic.patch>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>
>>>
>>>
>> <kannel_http-generic_2.patch>
>
>