feat(service): Add Meta Workplace #413
Conversation
Signed-off-by: Melvin Hillsman <mhillsma@redhat.com>
Codecov ReportBase: 64.34% // Head: 58.69% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #413 +/- ##
==========================================
- Coverage 64.34% 58.69% -5.65%
==========================================
Files 32 33 +1
Lines 1091 1196 +105
==========================================
Hits 702 702
- Misses 324 429 +105
Partials 65 65
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| #### Please consider the following: | ||
| * Meta Workplace service does not allow sending messages to group chats | ||
| * Meta Workplace service does not allow sending messages to group posts | ||
| * Meta Workplace service does not allow sending messages to users that are not already in a thread/chat with you | ||
| * Testing still needs to be added; the scenarios below will all fail. Those without a user/thread id should fail. Those |
There was a problem hiding this comment.
Hi @mrhillsman,
first of all thank you for your efforts and the contribution. Really stoked to get this service in.
On first glance everything seems very well made and basically ready to merge. However, on second check I do have some smaller things and questions.
-
Could you clarify why the service does not support the things you've listed here? This would help with setting future goals and todos for this service.
-
Are you planning to implement the missing tests? As you've probably noticed (since you've added the go:generate directive) you can use
make mockto generate a mock of this service that should help with testing. We're trying to achieve a high standard in quality with Notify and tests are defenitely a foundation for that. It would be very much appreciated!
Also, please check the linter errors and warnings. I'd ask you to fix those too. You can use make lint and make fmt to get them out of the way.
Thanks again for your efforts! Please let me know if anything needs clarification.
There was a problem hiding this comment.
Hi @nikoksr,
The items listed as not being supported are limitations on the Meta Workplace API not that they couldn't be implemented. Unfortunately I was not able to find a way to get group chat/posts and you can send a message to a user that is not already in a chat/thread; I probably just typed that in error.
I hope I can get time to work on the tests. Work got a bit busy right after I committed to at least getting the service implementation done but I hope to write the tests as well.
There was a problem hiding this comment.
Hi @mrhillsman,
I'm so sorry for coming back this late to you. I did not get notified about your reply and just saw it now by accident!
Thank you for clarifying that. That sounds acceptable to me then. The package design you came up with seems to allow for future expansion and addition of the currently missing features, so I'm okay with leaving them out for now.
Let me know if there's anything I can do to help you out with! Appreciate your efforts a lot.
|
|
||
| const ( | ||
| // ENDPOINT is the base URL of the Workplace API to send messages. | ||
| ENDPOINT = "https://graph.facebook.com/me/messages" |
There was a problem hiding this comment.
| ENDPOINT = "https://graph.facebook.com/me/messages" | |
| endpoint = "https://graph.facebook.com/me/messages" |
Let's follow the common naming conventions for constants in Go here.
|
|
||
| err := serviceConfig.ValidateConfig() | ||
| if err != nil { | ||
| log.Fatalf("failed to validate Meta Workplace service configuration: %v", err) |
There was a problem hiding this comment.
| log.Fatalf("failed to validate Meta Workplace service configuration: %v", err) | |
| return nil, errors.Wrap(err, "validate config") |
Following common practice of the Notify codebase here.
| client: &http.Client{ | ||
| Timeout: 10 * time.Second, | ||
| }, |
There was a problem hiding this comment.
Not a requirement but it would be nice tfor the HTTP client to be configurable by the user. A sane default like you chose plus a MetaWorkplaceService.WithHTTPClient(client *http.Client) method would be preferable. I do understand however, that your time is limited, so I don't see this as a hard requirement.
| // | ||
| //go:generate mockery --name=metaWorkplaceService --output=. --case=underscore --inpackage | ||
| type metaWorkplaceService interface { | ||
| send(payload interface{}) *MetaWorkplaceResponse |
There was a problem hiding this comment.
| send(payload interface{}) *MetaWorkplaceResponse | |
| send(payload any) *MetaWorkplaceResponse |
|
|
||
| data, err = io.ReadAll(res.Body) | ||
| if err != nil { | ||
| log.Printf("failed to read response body: %v", err) |
There was a problem hiding this comment.
| log.Printf("failed to read response body: %v", err) | |
| return nil, errors.Wrap(err, "read response body") |
|
|
||
| err = json.Unmarshal(data, &response) | ||
| if err != nil { | ||
| log.Printf("failed to unmarshal response body: %v", err) |
There was a problem hiding this comment.
| log.Printf("failed to unmarshal response body: %v", err) | |
| return nil, errors.Wrap(err, "unmarshal response body") |
| if len(mw.threadIDs) != 0 { | ||
| for _, threadID := range mw.threadIDs { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| default: | ||
| payload := metaWorkplaceThreadPayload{ | ||
| Message: metaWorkplaceMessage{Text: message}, | ||
| Recipient: metaWorkplaceThread{ThreadID: threadID}, | ||
| } | ||
| err := mw.send(payload) | ||
| if err.Error != nil { | ||
| log.Printf("%+v\n", err.Error) | ||
| return errors.New("failed to send message to Workplace thread: " + threadID) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
| if len(mw.threadIDs) != 0 { | |
| for _, threadID := range mw.threadIDs { | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| default: | |
| payload := metaWorkplaceThreadPayload{ | |
| Message: metaWorkplaceMessage{Text: message}, | |
| Recipient: metaWorkplaceThread{ThreadID: threadID}, | |
| } | |
| err := mw.send(payload) | |
| if err.Error != nil { | |
| log.Printf("%+v\n", err.Error) | |
| return errors.New("failed to send message to Workplace thread: " + threadID) | |
| } | |
| } | |
| } | |
| } | |
| for _, threadID := range mw.threadIDs { | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| default: | |
| payload := metaWorkplaceThreadPayload{ | |
| Message: metaWorkplaceMessage{Text: message}, | |
| Recipient: metaWorkplaceThread{ThreadID: threadID}, | |
| } | |
| resp, err := mw.send(payload) | |
| if err != nil { | |
| return nil, errors.Wrapf(err, "send message to thread %q: %+v", threadID, resp) | |
| } | |
| if resp.Error != nil { | |
| return nil, errors.Errorf("send message to thread %q: %+v", threadID, resp) | |
| } | |
| } | |
| } |
The length check here is obsolete here since an empty slice would result in a skipped loop execution body. Also, applying the error handling changes that would come implicitly with the expanded send method.
| if len(mw.userIDs) != 0 { | ||
| for _, userID := range mw.userIDs { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| default: | ||
| payload := metaWorkplaceUserPayload{ | ||
| Message: metaWorkplaceMessage{Text: message}, | ||
| Recipient: metaWorkplaceUsers{UserIDs: []string{userID}}, | ||
| } | ||
| err := mw.send(payload) | ||
| if err.Error != nil { | ||
| log.Printf("%+v\n", err.Error) | ||
| return errors.New("failed to send message to Workplace user: " + userID) | ||
| } | ||
|
|
||
| // Capture the thread ID for the user and add it to the thread ID list. Subsequent | ||
| // messages will be sent to the thread instead of creating a new thread for the user. | ||
| mw.threadIDs = append(mw.threadIDs, err.ThreadKey) | ||
| } | ||
| } |
There was a problem hiding this comment.
| if len(mw.userIDs) != 0 { | |
| for _, userID := range mw.userIDs { | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| default: | |
| payload := metaWorkplaceUserPayload{ | |
| Message: metaWorkplaceMessage{Text: message}, | |
| Recipient: metaWorkplaceUsers{UserIDs: []string{userID}}, | |
| } | |
| err := mw.send(payload) | |
| if err.Error != nil { | |
| log.Printf("%+v\n", err.Error) | |
| return errors.New("failed to send message to Workplace user: " + userID) | |
| } | |
| // Capture the thread ID for the user and add it to the thread ID list. Subsequent | |
| // messages will be sent to the thread instead of creating a new thread for the user. | |
| mw.threadIDs = append(mw.threadIDs, err.ThreadKey) | |
| } | |
| } | |
| for _, userID := range mw.userIDs { | |
| select { | |
| case <-ctx.Done(): | |
| return ctx.Err() | |
| default: | |
| payload := metaWorkplaceUserPayload{ | |
| Message: metaWorkplaceMessage{Text: message}, | |
| Recipient: metaWorkplaceUsers{UserIDs: []string{userID}}, | |
| } | |
| resp, err := mw.send(payload) | |
| if err != nil { | |
| return nil, errors.Wrapf(err, "send message to user %q: %+v", userID, resp) | |
| } | |
| if resp == nil { | |
| continue // All following actions require a non-nil response | |
| } | |
| if resp.Error != nil { | |
| return nil, errors.Errorf("send message to user %q: %+v", userID, resp) | |
| } | |
| // Capture the thread ID for the user and add it to the thread ID list. Subsequent | |
| // messages will be sent to the thread instead of creating a new thread for the user. | |
| mw.threadIDs = append(mw.threadIDs, resp.ThreadKey) | |
| } | |
| } |
| } | ||
|
|
||
| // Clear the user ID list. Subsequent messages will be sent to the thread instead of creating a new thread for the user. | ||
| mw.userIDs = []string{} |
There was a problem hiding this comment.
| mw.userIDs = []string{} | |
| mw.userIDs = make([]string, 0) |
Let's also free up the allocated memory here.
Signed-off-by: Melvin Hillsman mhillsma@redhat.com
Description
Add service for Meta Workplace
Motivation and Context
Hacktoberfest
Resolves #278
How Has This Been Tested?
A sample project was created locally for testing and a README.md file was added showing that code.
Screenshots / Output (if appropriate):
Types of changes
Checklist: