New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Intermediate Delivery Reports #62

Closed
faizann opened this Issue Mar 21, 2016 · 12 comments

Comments

Projects
None yet
3 participants
@faizann
Collaborator

faizann commented Mar 21, 2016

There is usually a need on SMPP clients to get extra information about status of an SMS for reporting purposes. This requires having intermediate delivery reports for every temporary failures.
We need a way to send intermedia DLRs for such failures so that SMPP client side knows what is really going on with an SMS.

Suggestion is to loop through sms in SmsSet in case there are no failured sms and create intermediary DLR. The MessageUtil.java needs to accept an extra param tempFailure in createReceiptSms for identifying if a DLR is for temporary (ENROUTE/BUFFERED) or final in case of failure.

@deruelle deruelle added this to the 3.0.0 milestone Mar 21, 2016

@deruelle

This comment has been minimized.

Member

deruelle commented Mar 21, 2016

Thanks @faizann. Do you want to contribute this one and provide a Pull Request for this ?

@faizann

This comment has been minimized.

Collaborator

faizann commented Mar 21, 2016

I will do so.

On Mon, 21 Mar 2016 at 17:59 Jean Deruelle notifications@github.com wrote:

Thanks @faizann https://github.com/faizann. Do you want to contribute
this one and provide a Pull Request for this ?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#62 (comment)

@vetss

This comment has been minimized.

Collaborator

vetss commented Mar 22, 2016

Some comments:

  • this feature must be configurable and disabled for default
  • implementaion of this demands a major processing in methods like onDeliveryError() and a very accurate implementing. Now delivery receipts are generated only for messages in "lstFailured" (so for a permananet failure only). There are also other actions that are applied for such messages. We will need to change this logic - like adding of extra list "lstTempFailured" and processing of it for receipts purposes. Also we have several places for receipt generating code...

@vetss vetss removed this from the 3.0.0 milestone Mar 22, 2016

@faizann

This comment has been minimized.

Collaborator

faizann commented Mar 22, 2016

Yeah indeed it is not that simple. I have only looked at onDeliveryError
part in SS7 side. I think MProc rules should be skipped for temp failures.
lstFailured will have to stay independent.

Give me a few days and I will send a pull request. I will be working on it.

On Tue, 22 Mar 2016 at 08:54 vetss notifications@github.com wrote:

Some comments:

  • this feature must be configurable and disabled for default
  • implementaion of this demands a major processing in methods like
    onDeliveryError() and a very accurate implementing. Now delivery receipts
    are generated only for messages in "lstFailured" (so for a permananet
    failure only). There are also other actions that are applied for such
    messages. We will need to change this logic - like adding of extra list
    "lstTempFailured" and processing of it for receipts purposes. Also we have
    several places for receipt generating code...


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#62 (comment)

@deruelle

This comment has been minimized.

Member

deruelle commented Apr 21, 2016

@faizann just wondered if you had anything blocking to complete this that we could help you with ?

@faizann

This comment has been minimized.

Collaborator

faizann commented Apr 22, 2016

Hi
I have nothing blocking. Just very busy with work. I will try to submit a
pull request next week.

On Thu, 21 Apr 2016 at 13:36 Jean Deruelle notifications@github.com wrote:

@faizann https://github.com/faizann just wondered if you had anything
blocking to complete this that we could help you with ?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#62 (comment)

@deruelle

This comment has been minimized.

Member

deruelle commented Apr 29, 2016

Thanks @faizann looking forward to it !

@deruelle deruelle added this to the 3.0.0 milestone May 6, 2016

@deruelle

This comment has been minimized.

Member

deruelle commented May 6, 2016

hi @faizann, just wondering if you were able to move forward on that ?

@faizann

This comment has been minimized.

Collaborator

faizann commented May 9, 2016

Hi. I had messed up a lot of code on my local machine so had to cleanup.
Here is the pull request #77

@vetss

This comment has been minimized.

Collaborator

vetss commented May 11, 2016

Hi @faizann,

I have committed your update, thanks for your work.

But I have some remarks for your work.

  • your update looks like a rough patch. We need to some more elegant solution like preparing a special list lstIntermediateReceipts like we have lstFailured and fill it during message processing. I understand that this is more complicated task but it looks more proper because we need to have a readable code. For general code refactoring I have created a special issue #80
  • I fixed one I guees bug - when in MT part we are sending IntermediateReceipts even when on originator does not ask it.
  • usage of "lstFailured.indexOf(sms)" can lead some delays if we have many messages for delivering. This is an often case when sending to SMPP. So it is better to refactor this (if we refactor remark 1)
@vetss

This comment has been minimized.

Collaborator

vetss commented May 11, 2016

Fixed by:
6b67993
998c419

It is better to refactor code to have more clear code.

@vetss vetss closed this May 11, 2016

@deruelle

This comment has been minimized.

Member

deruelle commented May 11, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment