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

[GH-1309] Correct Attachments documentation #1324

Merged
merged 9 commits into from
Jan 15, 2024

Conversation

Omar8345
Copy link
Contributor

@Omar8345 Omar8345 commented Dec 30, 2023

Summary

This pull request corrects misinformation in the attachments documentation, in the end of the documentation, it instructs the developer to add an attachments key in the props field, which won't work, I have edited the documentation and the curl and HTTPS examples to add the attachments key in the top-level of the post's JSON.

Ticket Link

Fixes #1309

@mattermost-build
Copy link
Contributor

Hello @Omar8345,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@cwarnermm cwarnermm added the 1: Dev Review Requires review by a core commiter label Jan 8, 2024
@cwarnermm
Copy link
Member

cwarnermm commented Jan 8, 2024

Thanks, @Omar8345, for this developer documentation PR. Your proposed changes will be technically reviewed by Engineering.

Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @Omar8345! I think there might be some confusion going on here because the syntax in those examples matches how POST /api/v4/posts expects the payload to look.

When you tried this, were you perhaps using an incoming webhook? I think the payload sent to a webhook uses the attachments field without props like in your changes.

If that's the case, I think we could instead add a separate example for posting message attachments using a webhook. If we do that, we should probably also update the existing examples to be posting to https://{your-mattermost-site}/api/v4/posts since that's the correct API endpoint for that

@Omar8345
Copy link
Contributor Author

Thanks for the PR, @Omar8345! I think there might be some confusion going on here because the syntax in those examples matches how POST /api/v4/posts expects the payload to look.

When you tried this, were you perhaps using an incoming webhook? I think the payload sent to a webhook uses the attachments field without props like in your changes.

If that's the case, I think we could instead add a separate example for posting message attachments using a webhook. If we do that, we should probably also update the existing examples to be posting to https://{your-mattermost-site}/api/v4/posts since that's the correct API endpoint for that

Thanks for your catch, I have made the corresponding changes requested and added a section for how to post a message attachment using a webhook.

Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Thanks for adding that! I'm sure I'll find this useful since I've always needed to manually construct the curl command for this when I occasionally need to test something involving message attachments. I've got a couple minor tweaks I want to make, but they're more nitpicky so I can apply them myself

@hmhealey hmhealey added the 2: Editor Review Requires review by an editor label Jan 11, 2024
Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Actually, would you be able to apply those? I guess I don't have the required permission to do that here

Omar8345 and others added 3 commits January 12, 2024 11:15
Co-authored-by: Harrison Healey <[email protected]>
Co-authored-by: Harrison Healey <[email protected]>
Co-authored-by: Harrison Healey <[email protected]>
@Omar8345
Copy link
Contributor Author

Omar8345 commented Jan 12, 2024

Actually, would you be able to apply those? I guess I don't have the required permission to do that here

Applied your suggested edits! I was a bit confused 😅

Edit: I just made a commit because I discovered that the curl and HTTP requests in the first FAQ wasn't putting attachments key inside props field

@cwarnermm
Copy link
Member

@Omar8345 - Can you update your branch with latest master, please? Thanks!

@cwarnermm cwarnermm added the preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories label Jan 12, 2024
@Omar8345
Copy link
Contributor Author

Done @cwarnermm

Copy link
Member

@cwarnermm cwarnermm left a comment

Choose a reason for hiding this comment

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

Thank you, @Omar8345!

@cwarnermm
Copy link
Member

@hmhealey - This PR is ready for your review.

Copy link
Member

@hmhealey hmhealey left a comment

Choose a reason for hiding this comment

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

Great catch! I remember mentioning to do that, but I forgot to confirm that it was done in both parts. Thanks for fixing that!

@hmhealey hmhealey added 4: Reviews Complete All reviewers have approved the pull request and removed 1: Dev Review Requires review by a core commiter 2: Editor Review Requires review by an editor labels Jan 15, 2024
@hmhealey hmhealey merged commit 79c76d9 into mattermost:master Jan 15, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Contributor preview-environment Allow the preview environment to be generated for Pull Requests coming from fork repositories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Help Wanted: Correct Attachments documentation
5 participants