Skip to content

Messaging service support - #51

Open
YReshetko wants to merge 10 commits into
kevinburke:masterfrom
YReshetko:master
Open

Messaging service support#51
YReshetko wants to merge 10 commits into
kevinburke:masterfrom
YReshetko:master

Conversation

@YReshetko

Copy link
Copy Markdown

Hello @kevinburke

We would like to have messaging service support in the framework
https://www.twilio.com/docs/sms/services/api

@kevinburke

Copy link
Copy Markdown
Owner

Hi, thanks very much for submitting this, apologies I haven't gotten to this yet.

Comment thread http.go Outdated
return c
}

// NewServiceResource returns a new Client to use Service Resource API

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

use the*

Can you link to documentation here?

Comment thread service_resource_test.go Outdated
t.Parallel()
client, server := getServer(messagingResourceResponse)
defer server.Close()
msr, err := client.Resource.ServiceResourceService.Fetch(context.Background(), "MGXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Could we change this to client.Messaging.ServiceResources.Fetch or similar?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Unfortunately Messaging field exists in Client structure, so I changed ServiceResourceService to just ServiceResources.
I understand that Resource name is very generic one but I could not fine suitable name.

Maybe we can use Copilot name as it is main feature of the Messaging services. What do you think?

Comment thread service_resource.go Outdated
// Fetch a service resource
//
// https://www.twilio.com/docs/sms/services/api#fetch-a-service-resource
func (s *MessagingService) Fetch(ctx context.Context, sid string) (*MessagingResource, error) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This should be just Get().

Comment thread service_resource.go Outdated
// Fetch a phone number resource for messaging service
//
// https://www.twilio.com/docs/sms/services/api/phonenumber-resource#fetch-a-phonenumber-resource
func (s *PhoneNumberService) Fetch(ctx context.Context, serviceSID, sid string) (*PhoneNumberResource, error) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ditto here, we don't use Fetch() anywhere else in the API.

Comment thread service_resource.go Outdated
AlphaSenders []*AlphaSenderResource `json:"alpha_senders"`
}

// Create a alpha sender resource for messaging service

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

an alpha*

Comment thread http.go Outdated
c.APIVersion = ServiceResourceVersion
c.ServiceResourceService = &ServiceResourceService{
MessagingService: &MessagingService{c},
PhoneNumber: &PhoneNumberService{c},

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Generally these are plural elsewhere in the API... PhoneNumbers, AlphaSenders, ShortCodes.

Comment thread service_resource_test.go
@@ -0,0 +1,140 @@
package twilio

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you for adding tests!

@YReshetko

Copy link
Copy Markdown
Author

Hi @kevinburke
Thank you for review.

I've updated the PR according to your comments apart from Messaging Service client name Resource could you have a look at my comment and advise appropriate name, please?

@yinzara

yinzara commented Aug 20, 2020

Copy link
Copy Markdown

Could we get this updated to master and something finalized? I really want to use this in a terraform provider and I'm having to use a custom version of everything.

@kevinburke

Copy link
Copy Markdown
Owner

I'll try to take a look at this, but to be clear, I don't get paid to maintain this project so I review and merge changes when I have time.

@yinzara

yinzara commented Aug 20, 2020

Copy link
Copy Markdown

Just out of curiosity, have you ever approached Twilio developer evangelists about them providing you with official support?

@kevinburke

Copy link
Copy Markdown
Owner

Yes, we chat about it every six months or so, they haven't expressed any interest in doing it.

Kevin

@tmc

tmc commented Feb 12, 2021

Copy link
Copy Markdown

@YReshetko can you resolve the conflicts here?

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