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-124 Improve "Attach to Issue" function #162

Merged
merged 15 commits into from
Jan 9, 2020

Conversation

hectorgabucio
Copy link
Contributor

@hectorgabucio hectorgabucio commented Nov 30, 2019

Summary

  • Feedback in mattermost channel when an issue comment is created.
  • Permalink in issue comment.
  • Showing org/owner, repo and issue number in the search list.
  • Locked issues are shown but can't be selected.
  • More precise feedback when the usser doesn't have enough permissions (maybe banned or blocked from org) or the repo has turned private.
  • List issues in which the user is not the assignee.

Issue link

Fixes #124

@jespino jespino added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels Nov 30, 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.

@DHaussermann how can I get a spinwick to test out this contribution?

server/api.go Show resolved Hide resolved
@hanzei hanzei added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Dec 2, 2019
@mattermod mattermod removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Dec 2, 2019
@DHaussermann
Copy link

@aaronrothschild I will deploy this to a test server and ping you credentials.

@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

@jasonblais
Copy link
Contributor

@aaronrothschild / @DHaussermann quick reminder to help with reviews after the holidays. Thank you!

server/api.go Outdated Show resolved Hide resolved
@DHaussermann
Copy link

DHaussermann commented Jan 2, 2020

Hi @hector2, Sorry for the delay on this. Thanks for the round of improvements. The feedback that the comment has been added really makes this nicer to use!

I've done some testing on this and have a couple questions:

  1. Was there previously any type of restriction on which repos I could search in for issues or, did it just seem that way because I could only search for issues where I was the assignee? Currently, it seems like you can search across any public repo. Is that correct?

  2. Would you be able to make two small improvements to the post menu. The option reads Github instead of GitHub and the text seems to be top justified instead of middle justified. This is not directly related to your PR so, if not, I'd be happy to create a separate issue.

@hectorgabucio
Copy link
Contributor Author

Hey @DHaussermann !

1 - Yes, before these changes you could only list the issues in which the user it's the asignee. I did that because I though that was desired, but maybe I'm wrong, let me know! :D

-> his 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't have permissions to comment?

2 - What do you mean by "the text seems top justified"? I looked at the post menu but it seemed fine. The options are aligned to the left.

image

And yes I can make those improvements of course!

@DHaussermann
Copy link

  1. @aaronrothschild do you have any thoughts on this? The current design certainly offers the most functionality but, the draw back is that it's a bit confusing since your search can return issues from countless repositories.

  2. @hector2 This is a bit easier to notice when compared to other plugins like Jira. The text is not vertically centered to line up with the middle of the icon. It sits a bit high such that the top of the text lines up with the top of the icon.
    Icon-text

@hectorgabucio
Copy link
Contributor Author

hectorgabucio commented Jan 3, 2020

@DHaussermann hey, I think the alignment problem is related to mattermost/mattermost-plugin-jira#407 . Let me check if I can do the same in this case.

EDIT: I did a change and now it should be aligned like in the jira plugin.

@hanzei hanzei requested a review from crspeller January 7, 2020 12:54
@DHaussermann
Copy link

DHaussermann commented Jan 7, 2020

@hector2 Thanks for fixing the alignment issues! I had a couple follow up questions about error cases...

A. When the user does not have permission to make the comment, it's great that we return allot of info but, the error looks a bit cryptic.
GitHubAttch
Can we trim off the beginning and about id and message and loose the braces? Or is there limited control over what goes up to the UI?
Edit: Actually disregard A. I'm sure there would be cases where there is data to return in those empty fields.

B. When a conversation becomes locked in GitHub I noticed that the issue becomes non-selectable in the UI. The user's pointer changes to a crossed out circle. Just curious if there is a reason this is different rather than have the user select the option and see an error in the modal

@aaronrothschild to clarify my comment from above, the UX of opening the search and seeing issues from countless different projects to search for would only be the case if the plugin is not locked to an organization or a users' work space. When locked it only returns issues from within these repos in the issue search results. Please confirm you're okay with this design.
Also, both of the issues above a very minor, maybe I should simply create a follow-up issue for the Error UX and not hold up this PR.

@aaronrothschild
Copy link
Contributor

aaronrothschild commented Jan 7, 2020

Edit: This comment isn't relevant anymore, ignore it.

@asaadmahmood Any thoughts on this?

I think ideally we can "scope" the search to only include repositories they own or belong to at first. It becomes pretty unwieldy. Perhaps a radio button select: [ ] My/MyOrg's Repositories [ ] All Public Github Repos. OR alternatively should we break the search into two parts: ask for the repo keywords first, then allow search by keywords through issues in that repo.

You start to realize how commonly used some words are in issues and it becomes almost impossible to find the relevant issue unless you add the keyword of the gituhub repo name, then it becomes more manageable. I think the UI should encourage that behavior perhaps - by splitting the search box into two fields, one for the repo name and one for the keywords being searched within that repo. I'm 2/5 though.

cc @DHaussermann ?

I'm not suggesting we make a heavy interface like this, but I like how Gitbook's Github Repo selection allows me to choose based on which repos are related to me:
2020-01-07_10-32-33 (1)

Just an example when you use some words that are unusual it reduces the results set, but I had a hard time finding some of the issues I was expecting to be able to find easily.
2020-01-07_10-30-34

@aaronrothschild
Copy link
Contributor

aaronrothschild commented Jan 7, 2020

@aaronrothschild to clarify my comment from above, the UX of opening the search and seeing issues from countless different projects to search for would only be the case if the plugin is not locked to an organization or a users' work space. When locked it only returns issues from within these repos in the issue search results. Please confirm you're okay with this design.
Also, both of the issues above a very minor, maybe I should simply create a follow-up issue for the Error UX and not hold up this PR.

@DHaussermann @hector2 OK, that's great and ideal then. Ignore my comment above, I had half-written it earlier today and just finished typing it when I saw Dylan's note.

@DHaussermann
Copy link

Okay Thanks @aaronrothschild I'd be curious how common it is for people to use the plugin not locked to an organization. There may be a worthwhile story in the future IF we get feedback that it would be valuable to restrict attachment functionality.

Copy link

@DHaussermann DHaussermann left a comment

Choose a reason for hiding this comment

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

Tested and passed

  • Tested to ensure post is created with link to github issue
  • Tested various error cases for attach (User permission, thread locked, issues disabled)
  • Tested to ensure that when locked the attach search only returns issue from that Organization or Wrokspace
  • Ensured issues are hidden for appropriate users if repo is made private
  • Post menu option has been fixed
    Added necessary steps to release testing.
    LGTM! Thanks @hector2 for the work on this!

Will follow up with Aaron to see if future changes are needed for error UX

@DHaussermann DHaussermann removed the 3: QA Review Requires review by a QA tester label Jan 8, 2020
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 @hector2! Just one request about style

webapp/src/components/github_issue_selector.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, thanks @hector2!!

@mickmister mickmister merged commit d44f441 into mattermost:master Jan 9, 2020
@hanzei hanzei added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jan 9, 2020
@hanzei hanzei added this to the v0.13.0 milestone Jan 9, 2020
@hectorgabucio hectorgabucio deleted the GH-124 branch April 27, 2020 19:56
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.

Improve "Attach to Issue" function
9 participants