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

Instagram handler #392

Merged
merged 21 commits into from
Jan 13, 2022
Merged

Conversation

matmsa27
Copy link
Contributor

@matmsa27 matmsa27 commented Dec 17, 2021

According to issue and PR opened on Rapidpro repository.

@norkans7
Copy link
Contributor

@matmsa27 I see most codes are copied from the Facebook app channel type?
What challenges are there to use the same app for Facebook and Instagram?
So the app will need to have all the required permissions for both?

Could that simplify the Instagram handler and possible make a few adjustment to the facebook app one to accommodate that as well?

Refactor response field to external ID
@Robi9
Copy link
Contributor

Robi9 commented Dec 20, 2021

What challenges are there to using the same app for Facebook and Instagram?

Yes, most of the code is very similar to Facebook. I don't see that it would have a problem to use the same application, it would just leave us stuck in having to use the same for both, we can have rapidpro hosts that are only interested in one of the channels so it would be interesting to have this application distinction.

So the app will need to have all the required permissions for both?

In case it is just one app for both types, it would be necessary to have all permissions for Facebook and Instagram, but if it is just an app for Instagram, it would not need to have all permissions, it would only be some that the Facebook channel uses and Instagram permissions.

Could that simplify the Instagram handler and possibly make a few adjustments to the Facebook app one to accommodate that as well?

Could you explain better what you thought about simplifying the Instagram handler if the same app is used for both?

@norkans7
Copy link
Contributor

@Robi9 I think an approach as the way we handle Twilio types that share a lot of code
https://github.com/nyaruka/courier/blob/main/handlers/twiml/twiml.go#L58

so the Facebookapp type will be the one with the handler but we can initiate the IG channel type are well in the file

all incoming messages will be receive on /c/fba/receive and we switch the channel we look for using the payload object value in the request we receive

That way I think we remain with less code so easier to maintain

@Robi9
Copy link
Contributor

Robi9 commented Dec 20, 2021

@norkans7 Understood, I will adapt the Facebookapp handler to support the IG type.

@Robi9
Copy link
Contributor

Robi9 commented Dec 27, 2021

@norkans7 I decided to separate receiving messages, so Facebook's keep arriving at c/fba/receive and Instagram's arrive at c/ig/receive, we can do that since the application allows us to indicate callback endpoints for types of different payload objects.

@matmsa27
Copy link
Contributor Author

Hi, @norkans7 We are already ready to receive the feedbacks about this PR.

@norkans7
Copy link
Contributor

norkans7 commented Jan 3, 2022

@Robi9 good to have two webhooks configured, now I think we need to have the signature validation and webhook validation work for both.
And what is the use of InstagramApplicationSecret I think the app will be the same as the facebook one, correct?

@codecov
Copy link

codecov bot commented Jan 3, 2022

Codecov Report

Merging #392 (43b982c) into main (3163cff) will increase coverage by 0.03%.
The diff coverage is 92.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #392      +/-   ##
==========================================
+ Coverage   71.70%   71.74%   +0.03%     
==========================================
  Files          95       95              
  Lines        8257     8268      +11     
==========================================
+ Hits         5921     5932      +11     
  Misses       1747     1747              
  Partials      589      589              
Impacted Files Coverage Δ
handlers/facebookapp/facebookapp.go 80.81% <92.59%> (+0.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3163cff...43b982c. Read the comment docs.


var SendTestCasesIG = []ChannelSendTestCase{
{Label: "Plain Send",
Text: "Simple Message", URN: "facebook:12345",
Copy link
Contributor

@norkans7 norkans7 Jan 5, 2022

Choose a reason for hiding this comment

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

These tests should use instagram URNs

Copy link
Contributor

Choose a reason for hiding this comment

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

not facebook URNs

Copy link
Contributor

@norkans7 norkans7 left a comment

Choose a reason for hiding this comment

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

Adjust the instagram tests

@@ -56,18 +56,20 @@ const (
payloadKey = "payload"
)

func newHandler(channelType courier.ChannelType, name string, validateSignatures bool) courier.ChannelHandler {
Copy link
Contributor

@norkans7 norkans7 Jan 5, 2022

Choose a reason for hiding this comment

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

validateSignatures is misleading as that parameter is for using channel UUIDs routes or not

urn urns.URN
metadata map[string]string
}{
{"facebook:1337", map[string]string{"name": "John Doe"}},
Copy link
Contributor

Choose a reason for hiding this comment

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

instagram URN

return
}

// no name
w.Write([]byte(`{ "first_name": "", "last_name": ""}`))
w.Write([]byte(`{ "name": ""}`))
}))
graphURL = server.URL

return server
}

func TestDescribe(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is best to have 2 separate tests one for FBA and the other for IG instead of the for loop on a slice of slices and the if condition

@norkans7 norkans7 requested a review from rowanseymour January 6, 2022 07:29
}

// no entries? ignore this request
if len(payload.Entry) == 0 {
return nil, fmt.Errorf("no entries found")
}

pageID := payload.Entry[0].ID
EntryID := payload.Entry[0].ID
Copy link
Member

Choose a reason for hiding this comment

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

entryID

}]
}`

var unkownMessagingEntry = `{
var unkownMessagingEntryFBA = `{
Copy link
Member

Choose a reason for hiding this comment

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

unknown.. not your typo but you can fix while you're here

@rowanseymour rowanseymour merged commit bacb70e into nyaruka:main Jan 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants