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

Add "Attach to Github Issue" similar to Jira plugin #120

Merged
merged 11 commits into from Sep 24, 2019

Conversation

niklabh
Copy link
Contributor

@niklabh niklabh commented Sep 11, 2019

Summary

Add "Attach to Github Issue" similar to Jira plugin
Here is the flow for implementations:

  • Click Attach to Github Issue in context menu (shown when user connects his github account with plugin installed) on non system messages
    Screenshot from 2019-09-11 13-26-14
  • Attach to github modal will open
    Screenshot from 2019-09-11 13-26-30
  • You can search the issue. The query to github is is:open is:issue assignee:{connected_user} archived:false [org:{organisation}] [search term]
    Screenshot from 2019-09-11 13-26-47
  • Comment is attached to github
    Screenshot from 2019-09-11 13-37-28

Issue

Fixes #119

@niklabh niklabh changed the title Comment issue Add "Attach to Github Issue" similar to Jira plugin Sep 11, 2019
@hanzei hanzei requested a review from levb September 11, 2019 11:51
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Sep 11, 2019
@hanzei hanzei added the 3: QA Review Requires review by a QA tester label Sep 11, 2019
@levb levb added the 1: PM Review Requires review by a product manager label Sep 11, 2019
@levb levb requested review from mickmister and removed request for levb September 11, 2019 14:34
@levb
Copy link
Contributor

levb commented Sep 11, 2019

This is an awesome addition, @niklabh! Thank you for the contribution!

@hanzei hanzei added this to the v0.11.0 milestone Sep 11, 2019
Copy link
Contributor

@aaronrothschild aaronrothschild left a comment

Choose a reason for hiding this comment

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

AHHHH-mazing! Looks great. Thank you @niklabh !

Copy link
Member

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

Great work @niklabh! Just a few requested changes. Thank you for this great improvement!!

server/api.go Show resolved Hide resolved
webapp/src/components/github_issue_selector/index.js Outdated Show resolved Hide resolved
webapp/src/components/setting.jsx Show resolved Hide resolved
webapp/src/components/setting.jsx Outdated Show resolved Hide resolved
Copy link
Member

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

LGTM, great work @niklabh!! 🎉

Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple requests

webapp/package.json Outdated Show resolved Hide resolved
webapp/src/actions/index.js Outdated Show resolved Hide resolved
Copy link
Member

@jwilander jwilander left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@mickmister mickmister removed 1: PM Review Requires review by a product manager 2: Dev Review Requires review by a core committer labels Sep 12, 2019
@mattermod
Copy link
Contributor

This issue 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!

/cc @jasonblais @hanzei

@hanzei
Copy link
Contributor

hanzei commented Sep 23, 2019

@DHaussermann gentle reminder to review this

@DHaussermann
Copy link

Thanks for the contribution @niklabh. This addition to the functionality is great!
No issues found with the work in this PR. It is behaving as expected.

@aaronrothschild I do see some opportunities to add to this further that would improve the experience. Let me know of any of this would be worth adding and I can create separate issues.

  1. When I attach a comment, I should get feedback that this was done successfully (Similar integrations post a reply with such a message and a link but, a simpler way to provide feedback wold work as well)
  2. In the search results, ideally we would return the issue number in addition to the title to cover cases where there titles are similar. Though the issue numbers could overlap across multiple repos within the same list.
  3. This design is limited to adding comments where the user is the assignee. There would also be value in adding comments to other issues as well. Would we need a more complex design to handle cases where you select an issue and don'y have permissions to comment?

@jwilander jwilander merged commit 7fdc4dc into mattermost:master Sep 24, 2019
@jwilander jwilander removed 3: QA Review Requires review by a QA tester Lifecycle/1:stale labels Sep 24, 2019
@hanzei hanzei added the 4: Reviews Complete All reviewers have approved the pull request label Sep 24, 2019
@aaronrothschild
Copy link
Contributor

@niklabh see my comments inline

@aaronrothschild I do see some opportunities to add to this further that would improve the experience. Let me know of any of this would be worth adding and I can create separate issues.

  1. When I attach a comment, I should get feedback that this was done successfully (Similar integrations post a reply with such a message and a link but, a simpler way to provide feedback wold work as well)

Yes, I've had a few instances where the confirmation is re-assuring - certainly during the first experience. I agree with @DHaussermann , the utility of having the link referenced within the conversation is pretty critical to the long-term user experience. Over time, without the reference to the attached issue, a critical piece of development conversations can get lost. it also serves the dual purpose of confirming that it was successful in attaching to the issue. I'm 4/5 we should add this before merge.

  1. In the search results, ideally we would return the issue number in addition to the title to cover cases where there titles are similar. Though the issue numbers could overlap across multiple repos within the same list.

This request has come up from a few different customers that use the equivalent function in the Jira plugin. Displaying the issue numbers clears ambiquity and lets the user be confident they chose the right option/value. Not sure how much effort is involved.

  1. This design is limited to adding comments where the user is the assignee. There would also be value in adding comments to other issues as well. Would we need a more complex design to handle cases where you select an issue and don'y have permissions to comment?

Hmmm. Not sure about this one, could we even accomodate a scenario where we couldn't post as the user (b/c permissions)?

@mickmister
Copy link
Member

@aaronrothschild This PR is already merged. Adding the link in a post will have to be a separate PR.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Attach to Github Issue" similar to Jira plugin
8 participants