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

Distinguish pending self and team review requests #2028

Draft
wants to merge 5 commits into
base: master
from

Conversation

@chrisarcand
Copy link
Member

chrisarcand commented Sep 21, 2019

Supersedes #1940

Description

Creating a superset of our own logic (even just a single thing) to further distinguish GitHub's vague reasons in the Notifications API - aka #1940 - didn't prove to be a good solution for seeing open review requests. There were many times things didn't line up. There are too many different moving parts to the state machine that is 'reasons' for a notification and ultimately the information contained there isn't as important as the actual current state of the subject....which leads us to here.

These changes sync requested reviewers (as #1940 did, but with team review requests as well) and focus on the state of the subject and not at all about the notification reason. This aligns much better with 1) How Octobox currently works for basically everything else and 2) GitHub, because we aren't interpreting any differing state fighting with the semantics of the Notifications API.

It's currently being tested at HashiCorp and we've had much, much better results this time around due to the simplicity of it.

UX

  • A new state filter is added to the sidebar for individual review requests on your user; something that's applicable to anyone:
    image
  • Team review requests do not have their own sidebar states. See considerations, below, for more. A URL filter is available to use manually and for future considerations (?team_review_requested=octobox%2Fsecurity)
  • Both individual and team review states are fully integrated in to the search syntax: review_requested:chrisarcand would be all notifications with a PR awaiting review from me; team_review_requested:octobox/security would be all notifications with a PR awaiting review from the Octobox security team. Both filters can have multiple comma-delimited values and both can be negated. Both of these filters match the existing syntax on GitHub's end (but are underscored! see considerations)
  • Instead of their own sidebar states, with the search syntax above you can create your own pinned searches for the GitHub teams you'd like to monitor. This pairs very nicely with #1985, since pinned searches are just as good as the out-of-box set you're given.

Considerations

  • On not having dedicated sidebar states per team review request: Simply put, this is to avoid having to add the logic of syncing a user's GitHub teams with Octobox. This can be done, but I don't have the time at the moment and there's plenty of consideration to be had for how/why we'd want to do this. With pinned searches, the functionality I want is there without taking on this syncing. This work can be easily built on top of if team syncing is a thing anyone wants to do later.
  • On search syntax: Technically the search syntax isn't exactly GitHub's. GitHub does something weird in that it mixes both underscores and hyphens. Example: review:changes_requested reviewed-by:chrisarcand (note keys are hyphenated and values underscored). The syntax for this feature in Octobox is the first where we have a multi-word key. The search syntax logic we have doesn't currently support hyphens (it thinks its a negation and mangles it) and it wasn't something I wanted to bother digging in to, especially since it's sort of nice that it's just all underscores now. Hence: review_requested:chrisarcand type:pull_request.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.