Problem/Motivation

When working with contexts and Dialogflow I came across some issues with getIntentAttribute and setIntentAttribute as follows:

  • getIntentAttribute assumes the context is keyed by name, it isn't
  • setIntentAttribute doesn't work if you need to set an array of values - e.g you might need to set some-context.param1 and some-context.param2 - the current code sets them as two separate contexts, one of which is dropped by the Dialogflow API

For my immediate needs I ended up working directly with the original request object, but will circle back for a permanent fix.

Proposed resolution

Failing tests
Fixes

Remaining tasks

The lot

User interface changes

None

API changes

None

Data model changes

(Database or configuration data changes that would make stored data on an existing site incompatible with the site's updated codebase, including changes to hook_schema(), configuration schema or keys, or the expected format of stored data, etc.)

CommentFileSizeAuthor
#11 2919609-11.patch6.84 KBgambry
#11 interdiff-9-11.txt761 bytesgambry
#9 2919609-9.patch6.78 KBgambry
#9 interdiff-4-9.txt3.15 KBgambry
#4 2919609-4.patch4.59 KBgambry
#4 2919609-4-test-only.patch2.91 KBgambry

Comments

larowlan created an issue. See original summary.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Project: Dialogflow (Api.AI) Webhook » Chatbot API
Issue summary: View changes

Moving this to Chatbot API as the problem is on the chatbot_api_apiai driver's proxy classes.

I had a quick look and I can see there is no way to set 2 context parameters.

the current code sets them as two separate contexts

That should be possible. By looking at the code, the second call should override the first one (so param1 is lost). But as I said I haven't tried myself.

We'll have a look ASAP, trying to provide a failing test first.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: +DrupalSouth 2017

I will aim to work on this next week at the sprint

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Status: Active » Needs review
StatusFileSize
new 2919609-4-test-only.patch2.91 KB
new 2919609-4.patch4.59 KB

Failing test and approach to fix IntentRequestApiAiProxy attached.

I think fixing IntentResponseApiAiProxy will require some re-factoring of the iboldurev/api-ai-php models. We currently hosts Webhook modules in Dialogflow Webhook contrib, but our models have been pushed in latest version of iboldurev/api-ai-php so we need to make sure if we do any refactoring we PR the changes there too.

Setting Needs Review for letting the testbot test the patches, issue currently still needs work.

The last submitted patch, 4: 2919609-4-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Status: Needs review » Needs work

The last submitted patch, 4: 2919609-4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Issue tags: -DrupalSouth 2017 
+++ b/modules/chatbot_api_apiai/src/IntentRequestApiAiProxy.php
@@ -76,17 +76,21 @@ class IntentRequestApiAiProxy implements IntentRequestInterface {
+        $value = isset($params[$this->getParameterName($name)]) ? $params[$this->getParameterName($name)] : $default;

could we break/return early when we get a match?

Thanks for picking this up, forgot all about it

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Issue tags: +blocker

Tests against patch fail on testSetIntentAttribute() too. This is not expected - and it didn't happen locally yesterday -.
I will have a look later.

could we break/return early when we get a match?

Yes of course, staying in the loop doesn't make sense if there is a match.

Thanks for picking this up, forgot all about it

No problem at all! I had a bit of time for contributing this weekend.

This is currently the only blocker for a stable release.

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-4-9.txt3.15 KB
new 2919609-9.patch6.78 KB

Patch implements early return on context getter as per #7 and fix setter, including additional testing coverage.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented 
+++ b/modules/chatbot_api_apiai/src/IntentRequestApiAiProxy.php
@@ -76,17 +76,24 @@ class IntentRequestApiAiProxy implements IntentRequestInterface {
     return $value;

Will we ever have $value now?

Other than that, rtbc in my book

gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new interdiff-9-11.txt761 bytes
new 2919609-11.patch6.84 KB

$value is always $default, so we can directly return the latter.

RTBCing as per #10 to trigger the tests. If green I will commit.

Thanks @larowlan for your reviews!

  • gambry authored bc98ea2 on 8.x-1.x 
    Issue #2919609 by gambry, larowlan: getIntentAttribute/...
gambry’s picture

gambry
Primary language Italian
Location Milan
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed. I'm going to review if this is really the only blocker, if it is will release a RC.

Thanks.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

congratulations!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.