[GH-618]: Added slack attachment for webhook posts, in order to comment, edit, and close issues. #636

Nityanand13 · 2023-01-31T10:00:05Z

Summary

Whenever a user creates an issue on github then three actions will be shown in the custom post created by the plugin's bot. The three actions are:

Comment
Edit
Close

With the help of the above mentioned actions user can create a comment, edit, close, and reopen a github issue from the mattermost. As you see we haven't made a reopen button so when a user closes an issue then this close button will turn to reopen.

Ticket Link

Issue: #618

…t and close issues (#13) * [MI-2502]:Added custom type for webhook post in order to comment, edit and close issues * [MI-2502]: Added EOF * [MI-2502]: Fixed lint errors * [MI-2502]: Review fixes done 1. Improved code quality 2. Made few contants * [MI-2502]: Review fixes done 1. Changed few files to .tsx 2. Improved code quality * [MI-2502]: Review fixes done 1. Improved code quality 2. Made a constant * [MI-2502]: Review fixes done 1. Improved code readability * [MI-2502]: Review fixes done 1. Made a package dor constants 2. Made a package for struct 3. Improved code redability * [MI-2502]: Review fixes done 1. Improved code quality 2. Fixed the issue that I was facing while creating issue or attaching a comment to the issue. 3. Changed few file names * [MI-2502]: Review fixes done 1. Changed the names of few functions 2. Improved code quality * [MI-2502]: Review fixes done 1.Improved code readability * [MI-2502]: Review fixes done 1. Improved code readability * [MI-2502]: Review done 1. Improved code quality * [MI-2502]: Review fixes done 1. Improved code quality

…ub into MM-618

mattermost-build · 2023-01-31T10:00:14Z

Hello @Nityanand13,

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.

hanzei

That is a very nifty feature 🎉

One broad question before I dive into the code too much: The implementation currently uses a custom post and a webapp model. Could this feature also be implemented using interactive dialogues and post actions? That way it's supported on mobile.

hanzei · 2023-02-03T12:31:17Z

Using the Comment feature posts this
message in the thread, which is misleading as no message from mattermost got attached.

Nityanand13 · 2023-02-07T07:04:08Z

That is a very nifty feature tada

One broad question before I dive into the code too much: The implementation currently uses a custom post and a webapp model. Could this feature also be implemented using interactive dialogues and post actions? That way it's supported on mobile.

@hanzei I think we can not implement this feature using interactive dialogues because in that we do not have any case in which we can select more than one option for a particular field.
For here if we want to edit an issue then there are fields like labels in which there might be a case in which we need to select more than one option from the list which would not be possible using interactive dialogues.

webapp/src/client/client.js

mickmister · 2023-02-07T23:51:14Z

webapp/src/components/modals/attach_comment_to_issue/attach_comment_to_issue.jsx

                <Input
                    label='Message Attached to GitHub Issue'
                    type='textarea'
                    isDisabled={true}
-                    value={this.props.post.message}
+                    value={post?.message}
                    disabled={false}
                    readOnly={true}
                />


What box does this serve if the message is blank?

In that case, we are getting a message something like Please provide a valid non empty comment.

Let's just remove the box if there are no contents and the user isn't going to type anything there. Why is the box blank anyway? What case do we not have a post? Maybe we should let the user type in a message regardless?

If the feature is to attach a comment, shouldn't we allow the user to type in a comment?

In this PR I only fixed issue #618. Where if a user clicks on the comment button as shown in the image attached to the description of this PR. So a modal will open as shown below where a user can type a comment and attach that to the github issue.

Other than this a user can create a github issue or attach a message to the github issue from message actions. For now, I have not changed the logic of this thing and the image of the modal that you are seeing where the user cannot type anything is getting opened when a user wants to attach a message to the github issue from message action. So in this case the message field usually contains the content of that particular post. And in the case of slack attachment posts, this message field remains empty. So should I make this field editable or what do you say?

So should I make this field editable or what do you say?

Yeah it probably should be editable, even though it wasn't editable before. The Jira plugin allows you to edit in this case, and I prefer that UX.

I'm trying to understand something still - Under what circumstance do we want to attach a comment, but have no text to send over to GitHub? It seems like a no-op to me in that case

@mickmister Sure I will make it editable. If we want to attach a comment from message actions of slack attachment type post then the comment field remains empty.

webapp/src/components/modals/create_update_issue/create_update_issue.jsx

…ub into MM-618

* [MI-2736]: Review fixes done 1. Improved code readability * [MI-2736]: Review fixes done 1. Fixed linting errors * [MI-2736]: Review fixes done 1. Fixed linting error

server/plugin/api.go

mickmister · 2023-02-09T16:58:14Z

webapp/src/components/modals/attach_comment_to_issue/attach_comment_to_issue.jsx

                <Input
                    label='Message Attached to GitHub Issue'
                    type='textarea'
                    isDisabled={true}
-                    value={this.props.post.message}
+                    value={post?.message}
                    disabled={false}
                    readOnly={true}
                />


If the feature is to attach a comment, shouldn't we allow the user to type in a comment?

* [MI-2736]: Review fixes done 1. Improved code readability * [MI-2736]: Review fixes done 1. Fixed linting errors * [MI-2736]: Review fixes done 1. Fixed linting error * [MI-2736]: Review fixes done 1. Improved code readability * [MI-2736]: Review fixes done 1. Changed the case of few endpoints to snake case

codecov-commenter · 2023-02-16T05:38:35Z

Codecov Report

Attention: Patch coverage is 1.32450% with 596 lines in your changes missing coverage. Please review.

Project coverage is 14.79%. Comparing base (da4c4df) to head (70ae885).
Report is 10 commits behind head on master.

❗ Current head 70ae885 differs from pull request most recent head 39fe3cf

Please upload reports for the commit 39fe3cf to get more accurate results.

Files	Patch %	Lines
server/plugin/api.go	2.02%	385 Missing and 2 partials ⚠️
server/plugin/utils.go	0.00%	117 Missing ⚠️
server/plugin/webhook.go	0.00%	58 Missing ⚠️
server/plugin/command.go	0.00%	18 Missing ⚠️
server/plugin/plugin.go	0.00%	13 Missing ⚠️
server/plugin/mm_34646_token_refresh.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #636      +/-   ##
==========================================
- Coverage   16.16%   14.79%   -1.37%     
==========================================
  Files          17       15       -2     
  Lines        6021     5563     -458     
==========================================
- Hits          973      823     -150     
+ Misses       5003     4697     -306     
+ Partials       45       43       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mickmister

Just a few more questions from my side. I can approve once conversation is wrapped up

mickmister · 2023-02-16T16:34:20Z

webapp/src/components/github_issue/index.tsx

+    button: {
+        fontFamily: 'Open Sans',
+        fontSize: '12px',
+        fontWeight: 'bold',
+        letterSpacing: '1px',
+        lineHeight: '19px',
+        margin: '12px 12px 8px 0px',
+        borderRadius: '4px',
+        color: theme.buttonColor,
+    },


Instead of these styles, can we make the buttons match the styling of most other buttons in the MM UI? There are class names used for this, in the footer of the modals in this project for instance. The current styling seems inconsistent with the rest of the application

@DHaussermann I'm curious about your thoughts on this. Should these buttons be made to look like the slack attachment buttons as much as possible, or maybe the buttons that are at the bottom of the modals?

mickmister · 2023-02-16T16:39:11Z

server/plugin/webhook.go

+		if action == constants.ActionOpened {
+			post.Type = "custom_git_issue"
+			post.Props = map[string]interface{}{
+				constants.TitleForProps:       *issue.Title,
+				constants.IssueURLForProps:    *issue.HTMLURL,
+				constants.IssueNumberForProps: *issue.Number,
+				constants.DescriptionForProps: description,


So the current conditions on this is to show these controls on a post created by the plugin, specifically for when an issue is opened.

I think it would be useful to have for PRs and when labeling issues. And possibly in the bot DMs as well, though that could get noisy. @hanzei What are your thoughts on this?

1/5 that it's handy to have actions attached to the DM notification as this allows user to answer them directly in MM. Maybe we even add the buttons to all posts?

@asatkinson What do you think?

hanzei · 2023-02-23T10:28:06Z

server/constants/constants.go

@@ -0,0 +1,46 @@
+package constants


From https://go.dev/blog/package-names

Avoid meaningless package names. Packages named util, common, or misc provide clients with no sense of what the package contains.

What value does a separate package provide?

@hanzei Keeping things in separate package helps us in writing unit tests and creating mocks for packages.

4/5 that a constants package is not an idiomatic way to store variables, as it contradicts the above quote.

hanzei · 2023-02-23T10:28:55Z

server/serializer/error.go

@@ -0,0 +1,11 @@
+package serializer


Same question about the package structure here.

server/plugin/api.go

hanzei · 2023-02-23T10:35:22Z

server/plugin/api.go

+	p.API.PublishWebSocketEvent(
+		wsEventAttachCommentToIssue,
+		map[string]interface{}{
+			"postId": post.Id,
+			"owner":  req.RepoOwner,
+			"repo":   req.RepoName,
+			"number": req.IssueNumber,
+		},
+		&model.WebsocketBroadcast{UserId: userID},
+	)


Why is the request to the plugin needed if it simply returns a websocket event?

The same question goes for openCloseOrReopenIssueModal.

Now we are not making any request for these cases

server/plugin/api.go

mattermost-build · 2023-03-06T01:01:50Z

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

matthewbirtch · 2024-07-05T15:57:16Z

@matthewbirtch Updated the post acc to your suggestion. Please re-review.

Thanks @raghavaggarwal2308, the screenshot looks good, but could you update the test server? These changes aren't showing for me there.

raghavaggarwal2308 · 2024-07-06T16:51:57Z

Thanks @raghavaggarwal2308, the screenshot looks good, but could you update the test server? These changes aren't showing for me there.

@matthewbirtch The test server is updated now.

matthewbirtch · 2024-07-07T22:22:56Z

Thanks @raghavaggarwal2308. A few details:

Since this post shows when a new issue is created, I wonder if we need a line at the beginning of these posts that says '{User} created a new issue in {owner/repo}'. Otherwise it doesn't really provide a reason why this is getting posted in the channel.
I'm also thinking that the details of the issue should show within a card UI element.
I'm seeing extra space above the buttons if there are no assignee/label fields showing. See comparison below:
Also, I'm wondering why we show the issue number first in the title, but in the hover popover, we show the issue number after the name and not bolded. I think we should be consistent here and show the issue number after the title.
On the mobile app, I'm seeing extra space below the issue title and the title is not linked like it is on the webapp. The title is also inconsistent with webapp as it doesn't include the issue number as part of the title. In addition, I think there should be a paragraph space here above the labels/assignee fields. Also noticed these fields are appearing in a different order than on the webapp.
When I update an issue from the edit modal, a reply is posted that says Updated GitHub issue #80 from a message. I think we can remove 'from a message' as it seems odd in this context.

Let me know if any of this is out of scope for this PR

raghavaggarwal2308 · 2024-07-08T12:09:34Z

@matthewbirtch All these suggestions will take some time to fix. Also, we will have to handle styling of many things:

Adding the username and org link in the pretext. This will become quire easy in a slack attachment.
Rendering as a card.

. What are your opinions on replacing the custom post with a slack attachment for webapp as well?

matthewbirtch · 2024-07-08T13:18:29Z

@matthewbirtch All these suggestions will take some time to fix. Also, we will have to handle styling of many things:

Adding the username and org link in the pretext. This will become quire easy in a slack attachment.

Rendering as a card.

. What are your opinions on replacing the custom post with a slack attachment for webapp as well?

I think that could work quite well and makes a lot of sense to me. @mickmister any objections to this?

mickmister · 2024-07-10T02:01:53Z

@raghavaggarwal2308 Sounds good to me 👍

null

…ation event on Github (#37) * [MM-617]: converted the custom post to slack attachment for issue creation * [MM-617]: fixed lint and testcases * [MM-617]: removed unused variables * [MM-617]: Fixed the title of the issue slackAttachment * [MM-617]: review fixes * [MM-617]: fixed lint * [MM-617]: Review fixes and code clean up * [MM-617]: removed the custom post file * [MM-617]: review fixes * [MM-617]: review fixes * [MM-617]: Fixed lint * [MM-617]: improved the error logging

…ub into MM-618

raghavaggarwal2308 · 2024-08-07T07:53:43Z

@matthewbirtch Converted the custom post to slack attachment and updated the screenshot in description. The test server is also updated with the latest build. Please re-review.

…ub into MM-618

matthewbirtch · 2024-09-03T15:07:26Z

@raghavaggarwal2308 the webapp looks good to me, but this is not rendering at all on mobile for me on the test server.

Webapp	Mobile

Kshitij-Katiyar · 2024-09-04T11:29:09Z

@raghavaggarwal2308 the webapp looks good to me, but this is not rendering at all on mobile for me on the test server.

@matthewbirtch We tried reproducing the issue but it works fine on my local server and the maintenance testing cloud server.
If you can still reproduce the issue, please let us know the steps.

matthewbirtch · 2024-09-04T12:50:21Z

@raghavaggarwal2308 it seems better now. Although, now I'm seeing a slightly different issue.

I'm testing on your test server in the Town Square channel in the devsecops team and I'm still seeing some inconsistencies with the webapp.

The mobile app is showing two empty posts from the github bot. While the webapp does not show these.

webapp	mobile app

Also, the mobile app is not showing the replies that I'm seeing on the webapp. However, when I tap on the post, it opens the thread and shows the replies. When I go back to the channel from there, it's showing the replies. I'm not sure if this is a bigger problem or if it's related to this PR. Here's a screen recording:

RPReplay_Final1725453848.MP4

raghavaggarwal2308 · 2024-09-04T16:36:59Z

@matthewbirtch Sure, we will look into it.

Kshitij-Katiyar · 2024-09-05T09:53:34Z

@raghavaggarwal2308 it seems better now. Although, now I'm seeing a slightly different issue.

@matthewbirtch
I investigated the issues you reported and would like to share my findings:

Regarding the blank post from the GitHub bot, I was unable to reproduce the issue. Could you try reproducing it in a new channel and team to ensure it's not caused by a custom component from the previous version of the PR?
For the issue with replies to Slack attachments, I was able to reproduce it. After reviewing the PR, I found no missing rootID or related technical issue, suggesting the problem may lie on the Mattermost side.
Let me know your thoughts.

matthewbirtch · 2024-09-05T13:29:21Z

Regarding the blank post from the GitHub bot, I was unable to reproduce the issue. Could you try reproducing it in a new channel and team to ensure it's not caused by a custom component from the previous version of the PR?

I think this is the case. It's probably a leftover from the previous implementation of the custom component. I think we're good with this part.

For the issue with replies to Slack attachments, I was able to reproduce it. After reviewing the PR, I found no missing rootID or related technical issue, suggesting the problem may lie on the Mattermost side.
Let me know your thoughts.

For this one, I'll need support from @wiggin77 or @mickmister to identify if this is a plugin issue or a platform issue.

hanzei

Can you please resolve the conflicts first?

…ub into MM-618

raghavaggarwal2308 · 2024-09-27T07:53:48Z

@hanzei Resolved conflicts

hanzei

@wiggin77 Can you please confirm from a product owner perspective that this is a feature we want to incorporate into the plugin?

Nityanand13 and others added 2 commits January 31, 2023 15:02

Merge branch 'master' of github.com:mattermost/mattermost-plugin-gith…

4378034

…ub into MM-618

Nityanand13 requested a review from hanzei as a code owner January 31, 2023 10:00

mattermost-build added the Contributor label Jan 31, 2023

hanzei requested a review from mickmister January 31, 2023 10:12

hanzei added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Jan 31, 2023

hanzei linked an issue Jan 31, 2023 that may be closed by this pull request

Add custom post type for webhook posts, in order to comment, edit, and close issues/PRs #618

Open

hanzei previously requested changes Feb 3, 2023

View reviewed changes

mickmister previously requested changes Feb 8, 2023

View reviewed changes

Nityanand13 mentioned this pull request Feb 8, 2023

[MI-2736]: Done the review fixes of a github PR #636 Brightscout/mattermost-plugin-github#20

Merged

Nityanand13 and others added 2 commits February 8, 2023 15:34

Merge branch 'master' of github.com:mattermost/mattermost-plugin-gith…

20ab25d

…ub into MM-618

[MI-2736]: Done the review fixes of a github PR mattermost#636 (#20)

70ae885

* [MI-2736]: Review fixes done 1. Improved code readability * [MI-2736]: Review fixes done 1. Fixed linting errors * [MI-2736]: Review fixes done 1. Fixed linting error

Nityanand13 requested a review from mickmister February 9, 2023 12:12

mickmister reviewed Feb 9, 2023

View reviewed changes

Nityanand13 mentioned this pull request Feb 10, 2023

[MI-2736]: Done the review fixes of a github PR #636 Brightscout/mattermost-plugin-github#21

Merged

Nityanand13 requested a review from mickmister February 16, 2023 05:38

mickmister reviewed Feb 16, 2023

View reviewed changes

hanzei self-requested a review February 20, 2023 13:23

hanzei reviewed Feb 23, 2023

View reviewed changes

mattermost-build added the Lifecycle/1:stale label Mar 6, 2023

hanzei added the Awaiting Submitter Action Blocked on the author label Mar 6, 2023

Nityanand13 added a commit to Brightscout/mattermost-plugin-github that referenced this pull request Mar 7, 2023

[MI-2814]: Review fixes done of github PR mattermost#636

53d7770

Nityanand13 mentioned this pull request Mar 7, 2023

[MI-2814] Done the review fixes of github PR #636 Brightscout/mattermost-plugin-github#22

Merged

raghavaggarwal2308 requested a review from matthewbirtch July 5, 2024 09:57

mickmister removed their request for review July 10, 2024 05:31

Kshitij-Katiyar mentioned this pull request Aug 6, 2024

[MM-617]: converted the custom post to slack attachment for issue creation event on Github Brightscout/mattermost-plugin-github#37

Merged

Kshitij-Katiyar and others added 2 commits August 6, 2024 15:15

Merge branch 'master' of github.com:mattermost/mattermost-plugin-gith…

79c266e

…ub into MM-618

Kshitij-Katiyar changed the title ~~[GH-618]: Added custom post type for webhook posts, in order to comment, edit, and close issues.~~ [GH-618]: Added slack attachment for webhook posts, in order to comment, edit, and close issues. Aug 7, 2024

raghavaggarwal2308 modified the milestones: v2.3.0, v2.4.0 Aug 28, 2024

Kshitij-Katiyar added 2 commits August 30, 2024 13:05

Merge branch 'master' of github.com:mattermost/mattermost-plugin-gith…

28300d5

…ub into MM-618

[MM-618]: Fixed the lint

b413828

wiggin77 requested a review from hanzei September 25, 2024 21:23

hanzei requested changes Sep 26, 2024

View reviewed changes

Merge branch 'master' of github.com:mattermost/mattermost-plugin-gith…

64655a0

…ub into MM-618

hanzei self-requested a review September 27, 2024 08:21

hanzei reviewed Oct 11, 2024

View reviewed changes

[GH-618]: Added slack attachment for webhook posts, in order to comment, edit, and close issues. #636

Are you sure you want to change the base?

[GH-618]: Added slack attachment for webhook posts, in order to comment, edit, and close issues. #636

Conversation

Nityanand13 commented Jan 31, 2023 • edited by Kshitij-Katiyar Loading

Summary

Ticket Link

mattermost-build commented Jan 31, 2023

hanzei left a comment

Choose a reason for hiding this comment

hanzei commented Feb 3, 2023

Nityanand13 commented Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 16, 2023 • edited Loading

Codecov Report

mickmister left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mickmister Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattermost-build commented Mar 6, 2023

matthewbirtch commented Jul 5, 2024

raghavaggarwal2308 commented Jul 6, 2024

matthewbirtch commented Jul 7, 2024 • edited Loading

raghavaggarwal2308 commented Jul 8, 2024 • edited Loading

matthewbirtch commented Jul 8, 2024

mickmister commented Jul 10, 2024

raghavaggarwal2308 commented Aug 7, 2024

matthewbirtch commented Sep 3, 2024

Kshitij-Katiyar commented Sep 4, 2024 • edited Loading

matthewbirtch commented Sep 4, 2024

raghavaggarwal2308 commented Sep 4, 2024

Kshitij-Katiyar commented Sep 5, 2024 • edited Loading

matthewbirtch commented Sep 5, 2024

hanzei left a comment

Choose a reason for hiding this comment

raghavaggarwal2308 commented Sep 27, 2024

hanzei left a comment

Choose a reason for hiding this comment

Nityanand13 commented Jan 31, 2023 •

edited by Kshitij-Katiyar

Loading

Nityanand13 commented Feb 7, 2023 •

edited

Loading

codecov-commenter commented Feb 16, 2023 •

edited

Loading

mickmister Feb 16, 2023 •

edited

Loading

matthewbirtch commented Jul 7, 2024 •

edited

Loading

raghavaggarwal2308 commented Jul 8, 2024 •

edited

Loading

Kshitij-Katiyar commented Sep 4, 2024 •

edited

Loading

Kshitij-Katiyar commented Sep 5, 2024 •

edited

Loading