Added interrupt digits handling.

[jacobkiers]

Hi!

As we think it is not always desirable that a callee can interrupt an interruptible
Node with any digit, we have improved the interrupt digit handling.

All these changes are backwards compatible.

Now you can:

• Make a Node interruptible and not interruptible
• Clear interrupt digits
• Add interrupt digits

The order of interrupt digits is changed to correspond to the ASCII
table. This makes sorting easier.

[marcelog]

mmm i don't like this idea.. have you tried using Node::prePromptMessagesNotInterruptable( ) ?

[marcelog]

I should have mentioned why I'm not so much into this change: Prompts should not change in their definition (essentially). And this adds complexity to the api (and complexity when debugging).

If there's a node that can change interrupt digits, IMHO might mean that you're trying to mix responsabilities into 1 node, where more than 1 could be used.

[jacobkiers]

It may be me, but I don't really understand your reasoning. Let me first explain my reasoning.

The Node already controls whether a prompt is interruptible or not. This PR only changes to which interrupts the Node responds. Therefore, I don't really think the responsibilities are changed.

As it is currently, the Node can be interrupted by any digit. That means that if someone enters a random digit, the sound file will stop playing. This is really surprising behaviour. You ask a callee to enter a specific digit ("press the pound key to listen to this message again"). Then, they accidentally press the 9, and the call ends (as it has no handling defined for the 9 digit).

To be honest, I personally think this is buggy behaviour in the first place.

There may be other ways of doing this, but they are not as elegant. To add a second Node that handles the interrupt basically just moves the "problem" to another place. In both cases, the Node will respond to any digit, which is the main reason for this PR.

As to your argument of complexity of the API and debugging it, I would be willing to add the necessary tests to cover this. I've just not yet been able to find out how to do that. Also, we've verified it does work with Asterisk, and it is production code with us. I do understand of course that this assertion may not be enough :)

Now, as I explained why I (still) think this is a good idea, would you please try explaining again why you think this is not? I'm no trying to have a heated argument, I just want to find out the main reason you object to this. As the state of upstream is important to us, your arguments do carry some weight.

[marcelog]

I understand your point. On one hand, I don't see why you can't just validate the input and play a message like "invalid option".

In your example, where you ask the user to press the pound key, the bug is in the actual node (your code), that's not validating the input correctly, and does not communicate the error to the user.

The validation is still 1 line of code. If you let the user press 9 accidentally, and then continue playing the message as nothing happened, the user will keep waiting to see the effect of his/her action (which is, with your patch, just silence).

So I prefer to let the node being interrupted and validate the input (after all, the user actually pressed something, he/she tried to do something), and your app should validate that input accordingly instead of just ignoring it.

What do you think?

[jacobkiers]

In your example, where you ask the user to press the pound key, the bug is in the actual node (your code), that's not validating the input correctly, and does not communicate the error to the user.
The validation is still 1 line of code. If you let the user press 9 accidentally, and then continue playing the message as nothing happened, the user will keep waiting to see the effect of his/her action (which is, with your patch, just silence).

Correct. With this patch, the input is just ignored. The sound file keeps playing without interruption and ends with the normal $nodeResult->onComplete() action (the one without input).

I see your point regarding actively informing the user of the incorrect input and I will discuss that with the people responsible for the contents of the messages.

I understand your point.(...)

Great! I gather you're still not happy to carry this patch? It does expose some stock Asterisk functionality, after all. If you won't, I'll just maintain them in my own fork.

That said, I do think sending patches upstream is important, so I'll just keep on doing so and working with you to find acceptable solutions.

Thanks again for considering and discussing this.

[marcelog]

Please, Jacob, I'm very happy (and actually enjoy) to discuss these matters, it's all about making things better for both devs and users.

I'll leave this one open, and play a little bit more with your solution.

Although, for me, it's kind of a must to inform the user that he/she did something wrong without having them to wait more, or just trying to figure out what they did wrong (or maybe even thinking that it's the system, that's just lagging behind o having issues).

So in this particular scenario, I think the solution might not be to patch pagi, but to change the way you're handling UX in your app.

Please come back with your conclusions, and I'll do the same (perhaps others might join our chat).

I look forward for to your comments and next PR's, with pleasure :)

[jacobkiers]

(...) Although, for me, it's kind of a must to inform the user that he/she did something wrong without having them to wait more, or just trying to figure out what they did wrong (or maybe even thinking that it's the system, that's just lagging behind o having issues).

So in this particular scenario, I think the solution might not be to patch pagi, but to change the way you're handling UX in your app.

As said, I will take this up with my co-workers, who have to decide on the matter. For new customers another approach may just be considered.

Please come back with your conclusions, and I'll do the same (perhaps others might join our chat).

I look forward for to your comments and next PR's, with pleasure :)

If anything, this serves as a nice reminder (also for myself) that there are different possible use cases outside of your own that may need to be considered in a professional Open Source project.

[reenl]

Hi,

I'm one of Jacob's colleagues.

Apart from the interrupt digit discussion, this patch also has a "interruptablePrompts" function, to reset the state of the node to default.

We created this because we need our nodes to be "uninterruptable" by default (over 90% of all nodes we create should not be interuptable), so when we need an interruptable prompt we call that function explicitly.

<?php
class MyNodeController extends NodeController
{
    public function register($name)
    {
        $result = parent::register($name);
        $result->unInterruptablePrompts();
        return $result;
    }
}

$nc = new MyNodeController();
$nc->register('node')
   ->interruptablePrompts();
   // Etc...

We could create a separate PR since we are trying to fix 2 issues here.