Skip to content
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

Add/Remove Participant #1

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Conversation

ChrisLubin
Copy link
Collaborator

Allows a user to add or remove anyone from a conversation.

circuitClient.js Outdated Show resolved Hide resolved
circuitClient.js Outdated Show resolved Hide resolved
circuitClient.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved

if (!users.length) {
if (users.length !== 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why you changed this. Can you explain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function searchUsers already checks for

if (!users.length)

and

if (users.length > 1)

so after the function is called, we just have to check if the function returned 1 result.

@@ -61,13 +539,14 @@ app.intent('send.message', async (conv, {target, message}) => {

if (!users.length && !convs.length) {
conv.ask(`I cannot find any user or conversation called ${target}. What's the name?`);
conv.contexts.set('sendmessage_getconv', 5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What this a bug in the existing code? If so what was the bug?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. After a user and conversation could not be found, the Google Assistant would immediately go to the default fallback intent rather than the collect.target followup intent.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@ChrisLubin ChrisLubin requested a review from rogeru March 30, 2019 17:24
circuitClient.js Outdated

// Properties
Object.defineProperty(this, 'user', {
get: _ => { return this.client.loggedOnUser; }
get: _ => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe I should ask Roger but couldn't we just do get: _ => this.client.loggedOnUser

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't have been modified on my end. I reverted the change; you can still ask Roger about it though.

index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@ChrisLubin ChrisLubin self-assigned this Apr 3, 2019
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@ChrisLubin ChrisLubin removed their assignment Jul 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants