marcelog / PAGI

PHP AGI ( Asterisk Gateway Interface ) facade, with CDR ( Call Detail Record ), Call spool and schedule auto dial, Send and Receive Fax, Channel Variables, and Caller ID management
http://marcelog.github.com/PAGI
Apache License 2.0
191 stars 77 forks source link

Ability to execute a Node only when a precondition is met #8

Closed jacobkiers closed 11 years ago

jacobkiers commented 12 years ago

Execute Node conditionally

NOTE BEFORE DISCUSSION: I am willing to implement this myself.

However, before starting implementation, I would like to know if this idea has any change of being merged eventually. As you probably found out in earlier discussions, I'm a strong proponent of sending patches upstream.

Furthermore, I would like some feedback on the architectural side of it.

The issue

Currently, Nodes have an executeBeforeRun callback, but this one (rightly) seems to be unable to influence the actual running of the Node.

What we need in our applications is a way to perform checks before a Node is ran, and act upon the result of the check. Examples of checks to be performed are:

The main issue is that we have IVR scripts which should perform different actions based on a state that has nothing to do like the state of the current call, like user input. We could of course solve this with adding if-then-else blocks all over the place and completely passing over the NodeController (and possible Nodes themselves). However, adding this- relatively simple, but very powerful - functionality to NodeController would give us the ability to perform some 90% of our requirements within the PAGI framework.

Furthermore, I think it would be a nice addition to PAGI itself, which is the primary reason I actually added this issue at all. If I was convinced that we were the only ones with such requirements, I would not have brought it up.

Requirements

The most logical place to add a precondition seems to be in the NodeController, since that is where the overall logic is being handled.

The idea is to add the ability to add the precondition to the NodeController together with the Node. Then, when jump()ing to a Node, before executing it, the precondition should be checked.

When the precondition fails, the NodeController checks whether the precondition has conditional actions defined based upon the status of the preconditions. These actions should be Nodes themselves.

Of course, when the precondition is met, it's business as usual, i.e. the Node is ran.

marcelog commented 12 years ago

Hi Jacob!

Sounds good, and I also agree that the precondition should be check in the NodeController (because it's the component that automatically run the nodes, the other option is to run the nodes manually, implying that you could check this kind of things right in your code before calling run() on them).

Just a thing, and before going any further, have you tried using jumpAfterEval( \Closure $callback ) ? I think it's pretty similar to what you need, like:

$nodeController->registerResult('nodeName')
->onComplete()
->jumpAfterEval(function (Node $node) { .... }

The result of this callback is a string which is the next node to jump to.

This can be used on all nodes except the one where your application starts (where you programatically jump to begin your application). If you already tried this, what would it take for this feature to satisfy your requirements?

jacobkiers commented 12 years ago

Hi Marcelo,

Thanks again for your quick response! I'd read the docs, but it didn't click yet, so thanks for pointing me to that function.

Since I'm currently still in the planning phase, and have a short vacation after today, I can't test today. However, I will test it next week and inform you of the results.

Thanks for your great work!

marcelog commented 12 years ago

Hi!

There's a comment on it in this article: http://marcelog.github.com/articles/making_your_ivr_nodes_call_flow_with_pagi_and_php_asterisk.html

And here's the api for NodeActionCommand: http://ci.marcelog.name:8080/job/PAGI/javadoc/db_Node_NodeActionCommand.html

I've noticed that the current generated PAGI doc did not include NodeActionCommand in the namespace/package section, looks like I've forgot to add the class and file comments in NodeActionCommand.php. This docblock was commited a few days ago, so this is may be a reason for you to missed that api. I will generate a new build to include it.

I look forward to your comments about it, I hope you can use it for this particular requirement. If not, we can discuss if we could extend this or work on a different/new feature.

Thank you! For your interest and for using PAGI :) Enjoy your vacation! :)

marcelog commented 12 years ago

Hi Jacob! Did you get the chance to try it?

jacobkiers commented 12 years ago

Hi Marcelo, I've barely had the chance to even look at it this past week, so no, I've no answer as of yet. Don't worry though, I'll certainly follow up on this when I've a chance to test it (this is a bit of a side-project).

marcelog commented 12 years ago

Ok Jacob, no worries, take your time :)

jacobkiers commented 12 years ago

Hi Marcelo, my colleage just informed me that the 'jumpAfterEval' function does not fully satisfy our requirements. We really need to have preconditions. I'll therefore start implementing my original proposal shortly.

Do you have any comments (besides that you agree it should be added into the NodeController)?

marcelog commented 12 years ago

Hi Jacob!

I dont have any suggestions yet. I'd like to know an example use case for it first, and maybe I can give you a hint then (other than I 'd like to keep things really simple when using the nodes).

jacobkiers commented 12 years ago

So, you're basically saying: just show me some code, and then I'll remark, right? ;) That's okay with me, by the way.

On Wed, Jun 6, 2012 at 3:41 PM, Marcelo Gornstein < reply@reply.github.com

wrote:

Hi Jacob!

I dont have any suggestions yet. I'd like to know an example use case for it first, and maybe I can give you a hint then (other than I 'd like to keep things really simple when using the nodes).


Reply to this email directly or view it on GitHub: https://github.com/marcelog/PAGI/issues/8#issuecomment-6150818

marcelog commented 12 years ago

hehe well, actually, I want to know more about the feature in question first. Why is it that you need exactly that? How would you use it? Why do you conclude that this is the way it should be implemented? Why jumpAfterEval does not satisfy your requirements?

Then, I'd like to see some code, yeah :) But I'd like to be sure that we're talking about the same thing first, and that this feature is really a new needed feature that cant be satisfied with today's implementation. Honestly, I'm using PAGI with very complex ivr's here, and did not have that need, so I just want to understand the problem first

jacobkiers commented 12 years ago

I have updated the description (primarily under the "The Issue" heading). Would you please re-read it and ask specific clarifications if so needed? I agree we need to be on the same page, so we can have a fruitful discussion.

marcelog commented 12 years ago

Hi!

I'll play devil's advocate, just to be sure to understand where are the limitations you're facing with PAGI, please bare with me :) I'll show some code that could do what you need, and maybe you can write the code you would like to write by using these preconditions.

How about:

$nodeController->registerResult('main')  // this might as well be a dummy/empty node where your app starts
    ->onComplete()  
    ->jumpAfterEval(function (Node $node) use ($businessHoursService) {  
        $time = new \DateTime();
        $shouldAnswer = $businessHoursService->areWeAvailableAt($time);
        if ($shouldAnswer) {
            return "takeCallNode";
        } else {
            return "sorryNotAvailableNowNode";
        }
    });

// Start application.
$nodeController->jumpTo('main');

Regarding this point, nodes can carry state on their own, by using the registry, see: saveCustomData(), hasCustomData(), getCustomData(), and delCustomData(). The registry is basically a key/value store inside the node designed specifically to carry state:

$myNextNodeWhereStateIsUsed = $node;
$myNextNodeWhereStateIsUsed->executeBeforeRun(function ($node) use ($nextNode) {
        $state  = $node ->getCustomData('state');
        $client = $node->getClient();
        if ($state == 'thisCustomerIsBanned') {
            ....
        } else {
            $dialResult = $client->dial(...);
            $nextNode->saveCustomData($dialResult);
        }
});

$nodeController->registerResult('someNode')
    ->onComplete()
    ->execute(function (Node $node) use ($myNextNodeWhereStateIsUsed) {
        $node->saveCustomData('state', 'thisCustomerIsBanned');
    });

This could be pretty much done with jumpAfterEval() and/or executeBeforeRun()/executeAfterRun(), like in the above two examples. As a matter of fact, we're doing this exactly for our prepaid telephony product to discriminate where the ivr starts, depending on if the system "knows" your ANI or not.

What are your thoughts? What is the code you imagine to write? Perhaps there could be an implementation that uses this technique, just as a syntactic sugar, instead of writing a whole new feature.

jacobkiers commented 12 years ago

Don't worry about playing the devil's advocate. As it is said: "Iron sharpens iron, and one man sharpens another."

I need some time to review your examples and see how they apply to our services. I've planned some time later this week (provided that I'm not called away on other projects first). Anyway, it looks like I better read the complete API docs again ;-)

It may be time I give you a little background on where I'm coming from. We operate an automated reminder service, reminding people of things like hospital appointments, debts, service messages for ISPs and so on. Our current platform is mainly developed in-house, but the call servers are still based on a proprietary PBX, of which we hit the limits more and more.

Therefore, I've been tasked to investigate a replacement. We already decided to use Asterisk, and I'm currently inclined to use the PAGI and PAMI libraries to interface with our call servers.

Since we are basically to switch our core business to a completely different platform, I'm careful to assess if our needs are met with existing technology, and what we need to develop ourselves. Control over the technology stack is a major concern for us.

marcelog commented 12 years ago

Well sounds pretty interesting, and I'm glad you liked PAMI and PAGI enough to be considering them :) And I agree with your point about being careful, sounds like a major move there.

I'm also careful from my side, kind of "the other side of the desk", trying to not add a lot of features, because they will have to be mantained. So that's why I'm taking this as far as possible, pushing the current features up to the limit. If I can clearly see this limit, we can then discuss about the best way to implement the new features :) I just hope this "process" (going back and forth) is inline with your own schedule there. Anyway, I think we're getting there :)

Please take your time, and feel free to "ping" me if you have any questions while trying things out.

marcelog commented 12 years ago

Hi! Just giving this a bump. Did you get the chance to try this stuff?

jacobkiers commented 12 years ago

I'm sorry to report that I haven't gotten around to work on this any further. Other projects (some also including PAMI and/or PAGI) have taken priority.

Thanks for asking, though.

On Thu, Jul 12, 2012 at 4:07 PM, Marcelo Gornstein < reply@reply.github.com

wrote:

Hi! Just giving this a bump. Did you get the chance to try this stuff?


Reply to this email directly or view it on GitHub: https://github.com/marcelog/PAGI/issues/8#issuecomment-6935954

jacobkiers commented 11 years ago

Hi! For now, we've decided on using Node the way you described. Therefore, I will close this issue. If we see the need to reconsider this, I will reopen it or create another one.

Thanks for your feedback.