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-618]: Added slack attachment for webhook posts, in order to comment, edit, and close issues. #636

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

Nityanand13
Copy link
Contributor

@Nityanand13 Nityanand13 commented Jan 31, 2023

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.

Screenshot from 2024-07-22 18-05-32

Ticket Link

Issue: #618

Nityanand13 and others added 2 commits January 31, 2023 15:02
…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
@mattermost-build
Copy link
Contributor

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 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
hanzei previously requested changes Feb 3, 2023
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor

hanzei commented Feb 3, 2023

Using the Comment feature posts this
Screenshot from 2023-02-03 13-30-01 message in the thread, which is misleading as no message from mattermost got attached.

@Nityanand13
Copy link
Contributor Author

Nityanand13 commented Feb 7, 2023

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 Outdated Show resolved Hide resolved
Comment on lines 138 to 145
<Input
label='Message Attached to GitHub Issue'
type='textarea'
isDisabled={true}
value={this.props.post.message}
value={post?.message}
disabled={false}
readOnly={true}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

What box does this serve if the message is blank?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Screenshot from 2023-02-08 12-07-59

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Screenshot from 2023-02-10 13-15-54

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Nityanand13 and others added 2 commits February 8, 2023 15:34
* [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 Outdated Show resolved Hide resolved
Comment on lines 138 to 145
<Input
label='Message Attached to GitHub Issue'
type='textarea'
isDisabled={true}
value={this.props.post.message}
value={post?.message}
disabled={false}
readOnly={true}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link

codecov-commenter commented Feb 16, 2023

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.

Copy link
Contributor

@mickmister mickmister left a comment

Choose a reason for hiding this comment

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

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

Comment on lines 95 to 104
button: {
fontFamily: 'Open Sans',
fontSize: '12px',
fontWeight: 'bold',
letterSpacing: '1px',
lineHeight: '19px',
margin: '12px 12px 8px 0px',
borderRadius: '4px',
color: theme.buttonColor,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@mickmister mickmister Feb 16, 2023

Choose a reason for hiding this comment

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

@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?

Comment on lines 556 to 562
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,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 hanzei self-requested a review February 20, 2023 13:23
@@ -0,0 +1,46 @@
package constants
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -0,0 +1,11 @@
package serializer
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question about the package structure here.

server/plugin/api.go Outdated Show resolved Hide resolved
Comment on lines 981 to 990
p.API.PublishWebSocketEvent(
wsEventAttachCommentToIssue,
map[string]interface{}{
"postId": post.Id,
"owner": req.RepoOwner,
"repo": req.RepoName,
"number": req.IssueNumber,
},
&model.WebsocketBroadcast{UserId: userID},
)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The same question goes for openCloseOrReopenIssueModal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we are not making any request for these cases

server/plugin/api.go Outdated Show resolved Hide resolved
@mattermost-build
Copy link
Contributor

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!

@hanzei 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
@matthewbirtch
Copy link

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

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

@raghavaggarwal2308
Copy link
Contributor

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
Copy link

matthewbirtch commented Jul 7, 2024

Thanks @raghavaggarwal2308. A few details:

  1. 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.

  2. I'm also thinking that the details of the issue should show within a card UI element.
    image

  3. I'm seeing extra space above the buttons if there are no assignee/label fields showing. See comparison below:
    Screenshot 2024-07-07 at 5 46 46 PM

  4. 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.
    image

  5. 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.

  6. 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.
    image

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

@raghavaggarwal2308
Copy link
Contributor

raghavaggarwal2308 commented Jul 8, 2024

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

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

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

@matthewbirtch
Copy link

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

  1. Adding the username and org link in the pretext. This will become quire easy in a slack attachment.
  2. 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
Copy link
Contributor

@raghavaggarwal2308 Sounds good to me 👍

Kshitij-Katiyar and others added 2 commits August 6, 2024 15:15
…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
@raghavaggarwal2308
Copy link
Contributor

@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.

@Kshitij-Katiyar 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 raghavaggarwal2308 modified the milestones: v2.3.0, v2.4.0 Aug 28, 2024
@matthewbirtch
Copy link

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

Webapp Mobile
Screenshot 2024-09-03 at 11 04 40 AM

@Kshitij-Katiyar
Copy link
Contributor

Kshitij-Katiyar commented Sep 4, 2024

@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.
1000213680

@matthewbirtch
Copy link

@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.

  1. The mobile app is showing two empty posts from the github bot. While the webapp does not show these.
webapp mobile app
image
  1. 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
Copy link
Contributor

@matthewbirtch Sure, we will look into it.

@Kshitij-Katiyar
Copy link
Contributor

Kshitij-Katiyar commented Sep 5, 2024

@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
Copy link

  • 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.

Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

Can you please resolve the conflicts first?

@raghavaggarwal2308
Copy link
Contributor

@hanzei Resolved conflicts

@hanzei hanzei self-requested a review September 27, 2024 08:21
Copy link
Contributor

@hanzei hanzei left a comment

Choose a reason for hiding this comment

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1: UX Review Requires review by a UX Designer 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester Lifecycle/1:stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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