Skip to content
This repository has been archived by the owner on Sep 20, 2023. It is now read-only.

Subscribe/Unsubscribe #2482

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

Subscribe/Unsubscribe #2482

wants to merge 7 commits into from

Conversation

Huddie
Copy link
Collaborator

@Huddie Huddie commented Nov 25, 2018

Closes #2149

So I think I have everything in place. Just need to configure the request that subscribes and unsubscribes.

Where I'm a bit stuck:

There is a difference between watching repositories (https://developer.github.com/v3/activity/watching/) vs subscribing a thread https://developer.github.com/v3/activity/notifications/).

I think we're looking for the latter and so I'm trying to figure out how to obtain the thread_id from the issueOrPullRequest query that we use. We have V3SubscribeThreadRequest which takes an id and ignore. So once I get the thread_id I can pass it in.

Anyone know where I can get the thread_id from the issueOrPullRequest query response?

@Sherlouk you seem to know the GitHub API stuff well!

@Sherlouk
Copy link
Member

Not super familiar with the subscriptions side of it all but I believe in this context a thread is just an issue and/or Pull Request so you're looking for the ID of the Issue/PR

    issueOrPullRequest(number: 364) {
      ... on PullRequest {
        id
      }
    }

MDExOlB1bGxSZXF1ZXN0MTYyNzg3MzYw

I'm thinking this because if you look at the protocol Subscribable the only 3 fields are id (the one above), viewerCanSubscribe and viewerSubscription so think that makes sense

@@ -12,6 +12,10 @@ import StyledTextKit

extension IssueOrPullRequestQuery.Data.Repository.IssueOrPullRequest.AsPullRequest: IssueType {

var viewerSubscribed: Bool {
return viewerSubscription?.rawValue != "IGNORED"
Copy link
Member

Choose a reason for hiding this comment

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

nit: We should use a constant for this value here: SubscriptionState.ignored.rawValue

@@ -11,6 +11,10 @@ import IGListKit

extension IssueOrPullRequestQuery.Data.Repository.IssueOrPullRequest.AsIssue: IssueType {

var viewerSubscribed: Bool {
return viewerSubscription?.rawValue != "IGNORED"
Copy link
Member

Choose a reason for hiding this comment

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

nit: We should use a constant for this value here: SubscriptionState.ignored.rawValue

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I shall fix this!

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 25, 2018

@Sherlouk I tried that. I put a print statement to get that ID and I put a print to get the ID of the issue in the inbox and the ids were different. One was: 182635 (or something like that, all numbers) and then the other was the MDEx.... iD 🤔. Thoughts?
I also tried passing the MDEx... one through and got an error

Sent with GitHawk

@Sherlouk
Copy link
Member

Sherlouk commented Nov 25, 2018

If it's all numbers it sounds like it could just be the PR/Issue number (e.g. this one is #2482) I believe it should just be the MDEx one - what error are you seeing?

There don't appear to be any other IDs 🤔

Unless this is one of those stupid APIs where v4 API doesn't surface information you need in the V3 API. We've done some mad hacks in the past for that

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 25, 2018

In the docs you can see it’s called a thread_id rather than a regular ID, not really sure where to get that/what that is. In the NotificationModelController it’s called v3id, so seems to be some V3 API. If you capture the v3id you get something like:

415678928 (this is an actual example for this thread)

Sent with GitHawk

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 25, 2018

Well, this is frustrating. I ended up posting on the Github Dev Forum. So close to finishing this :/

@Sherlouk
Copy link
Member

@Huddie I know it's a bit of a side step from what you were looking into but could we maybe investigate using the V4 API mutation for this? Documented here

It looks like we have all the information necessary for this one?

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 25, 2018

@Sherlouk Oh that looks like it could work. Any idea what the mutation would look like. I'm barely familiar with V3, so if you have an idea that could get me started that would be great.

@Sherlouk
Copy link
Member

@Huddie It's been a while so I don't know how much I can help specifically but if you look at the add comment flow or the add/remove reaction flow then both of these use V4 mutations to work!

Should be able to reuse/take notes from those implementations!

If you have any specific questions though feel free to post them and I'm sure someone here will be able to help if not myself!

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 29, 2018

@Sherlouk great idea with V4 mutation. The mutation I added (UpdateSubscription.graphql) works in graphiQL

mutation UpdateSubscription($subscribable_Id: ID!, $subscription_state: SubscriptionState!) {
  updateSubscription(input:{subscribableId: $subscribable_Id, state: $subscription_state}) {
    subscribable{
      id
      viewerCanSubscribe
      viewerSubscription
    }
  }
}

{
   "subscribable_Id": "MDU6SXNzdWUzODU1MTAyMTU=",
   "subscription_state": "IGNORED"
}

but doesn't seem to work in-app. I'm not getting back an error, just nil and nothing is changing on github. When I used GraphiQL I am able to change the subscription status on github. Any ideas?

@Sherlouk
Copy link
Member

Hmm 😬 Interesting that it works in one but not the other - one would think it has something to do with the inputs at that point?

Maybe @rnystrom can help here, has a lot more knowledge!

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 29, 2018

@Sherlouk The inputs I put in the GraphiQL were taken from the console after running the app and printing the inputs. So they should be exactly the same. I’ll look into it more

Sent with GitHawk

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 30, 2018

@Sherlouk Got it working, dumb mistake on my part. Thanks for the help.
@rnystrom my mutation is working on repos that I owe but when I try subscribing to an issue on GitHawk or a random repo I get

"Although you appear to have the correct authorization credentials, the GitHawkApp organization has enabled OAuth app access restrictions, meaning that data access to third-parties is limited"

why would subscribing to threads require any special access? If anything I feel like sub/unsub should be open to all repos regardless of access. Plus I can sub/unsub on github.com so it must be a mistake i'm making. Any ideas?

@ijm8710
Copy link

ijm8710 commented Nov 30, 2018

Might be stating the obvious but I feel this all relates back to the repo scope issue. If you manually apply a new token with those fields added manually I feel it will always work, no?

(And ultimately those are the permissions everyone should always effectively have anyway)

screen shot 2018-11-04 at 7 09 14 pm

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 30, 2018

@ijm8710 The only necessary permission im pretty sure is 'notifications'

@rnystrom
Copy link
Member

rnystrom commented Nov 30, 2018

@Huddie if you use a PAT to login, does it work then?

Sent with GitHawk

@ijm8710
Copy link

ijm8710 commented Nov 30, 2018

Yeah exactly, I would be shocked if you run into that issue with the pat

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 30, 2018

@rnystrom PAT works.

I removed all scopes but those we ask for in GitHawk and it worked. I don't think this is a scope issue. Seems to be a PAT vs OAuth difference.

@ijm8710
Copy link

ijm8710 commented Nov 30, 2018

Yeah this all goes back to what ryan and I summed up in #2376 per what we investigated in #2165. Imo, unless GitHub changes how the OAuth works (because I think all of us agree the OAuth alone should grant sufficient access) the permissions should be reset for all current githawk users and reapplied going forward for all new githawk users to grant permissions on a token and not OAuth level or make it apply both

Either way since the PAT issue affects in other places as well I don’t see it as a holdup in this ticket though

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 30, 2018

Well think stinks. @rnystrom Any way around this or am I putting this pr on the shelf ?

Sent with GitHawk

@rnystrom
Copy link
Member

I don’t know if it’s a blocker for this, the same error message occurs for other actions, like reactions, right?

Sent with GitHawk

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 30, 2018

@rnystrom right. Ya I forgot that for a second. Okay so I guess I’ll keep going.

Sent with GitHawk

@ijm8710
Copy link

ijm8710 commented Nov 30, 2018

Def don’t put on shelf!

It’s the same error message you get for even more critical items such as posting comments in some repos as well. (PAT issues have existed for a while)

The awesome work on this is not really being newly blocked by anything.

If anything that just increases priority on fixing PAT but again no new issue here.

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 30, 2018

@ijm8710 Thanks!

Well... in that case @rnystrom this is ready for review 😀

General changes:

  1. Manager button will Always show.
    • Reason for this is that you can always subscribe/unsubscribe to an issue
  2. Subscribe/Unsubscribe is the first cell under the divider in the manager menu, not locked/unlocked.
    • Reason: Seemed to make more sense, especially since sometimes sun/unsub is the only option.
  3. Added viewerSubscription to API request for Issues/PR's.
    • Reason: We need this information for determining the current subscription status
  4. Added a new mutation that allows for changing users subscription status

@ijm8710
Copy link

ijm8710 commented Nov 30, 2018

Just quick food for thought-

where the ‘edit title addition’ would order-up relating to your second bullet (believe that PR was close to completed).

Honestly I think the staticness of the button, now, raises even more support for the discussed addition of the mark read button intra-post being through the burger as welll. I think there could be a bunch of viable options perhaps for the burger menu now that it’s always visible👍

please don’t take this as a hijack just hitting on bullet 2 :)

@Huddie
Copy link
Collaborator Author

Huddie commented Nov 30, 2018

@ijm8710 Not sure I fully follow but does this have to do with this PR specifically or is it something that should be addressed later? Just want to make sure this thread isn’t about future idea/features and is strictly about this PR. I’m sure it’s related to this topic but if it isn’t about changing a piece of code in this PR or reason it shouldn’t be merged I think it’s better fit for an issue

Sent with GitHawk

@ijm8710
Copy link

ijm8710 commented Nov 30, 2018

Just talking the burger menu button order in general as some things were already in the works coming to it and you brought it up. It can be revisited further in those tickets but since you mentioned the order of it and also how it’s always static I thought it should be on mind

Awesome work again!

@rnystrom rnystrom mentioned this pull request Dec 1, 2018
@Huddie Huddie changed the title [WIP] Subscribe/Unsubscribe Subscribe/Unsubscribe Dec 2, 2018
@Huddie Huddie added the 💤 awaiting review Pull Request is awaiting code reviews label Dec 2, 2018
@ijm8710 ijm8710 mentioned this pull request Dec 3, 2018
@Huddie
Copy link
Collaborator Author

Huddie commented Dec 26, 2018

@rnystrom

Sent with GitHawk

@rnystrom
Copy link
Member

Haven't had time to catch up. @Huddie what's the current status w/ this?

@Huddie
Copy link
Collaborator Author

Huddie commented Jan 6, 2019

@rnystrom Sorry for the delay. So I tested this after pulling upstream and still works. It still has the PAT issue but that won't be fixed until all PAT issues are fixed. Once/If this is merged the manager button will always be present because you can always sub/unsub from a thread that you have access to. So I guess this is waiting for your review ATM

@Huddie
Copy link
Collaborator Author

Huddie commented Mar 11, 2019

@BasThomas Can I get a review on this. Might have time next week to fix it up if there are requested changes.

Sent with GitHawk

Copy link
Collaborator

@BasThomas BasThomas left a comment

Choose a reason for hiding this comment

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

Haven't tested this, but it's looking good :)

func setSubscription(
previous: IssueResult,
subscribe: Bool,
completion: ((Result<SubscriptionState?>) -> Void)? = nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, what would it mean if the state is nil? Would that be an actual success?

let cache = self.cache
cache.set(value: optimisticResult)

let mutation = UpdateSubscriptionMutation(subscribable_Id: previous.id, subscription_state: state)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we change these parameters to camelCase?

iconName = "unmute"
case .unsubscribe:
title = Constants.Strings.unsubscribe
iconName = "mute"
}

// Lock always has the divider above it assuming you're a collaborator.
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should update this comment.

@@ -114,9 +114,7 @@ class SplitViewTests: XCTestCase {
client: GithubClient(userSession: nil),
repo: RepositoryDetails(
owner: "Foo",
name: "Bar",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this changed? 🤔

Copy link
Collaborator

@BasThomas BasThomas left a comment

Choose a reason for hiding this comment

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

Wait... do we not have this already? You can unsubscribe from the issue overview, isn't it? Am I missing something?

@Huddie
Copy link
Collaborator Author

Huddie commented Mar 17, 2019

@BasThomas can you explain where you think we have this again? Not sure I fully understood

Sent with GitHawk

@BasThomas
Copy link
Collaborator

GitHawk Upload by BasThomas

Sent with GitHawk

@Huddie
Copy link
Collaborator Author

Huddie commented Mar 17, 2019

@BasThomas ya so that’s true for notifications. If you comment on a issue etc etc you get subscribed and then once you get the notification you can unsubscribe. Here you can pick any issue and subscribe to it, and the. Unsubscribe whenever as wel, even if you hasn’t gotten a notification.

Sent with GitHawk

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
💤 awaiting review Pull Request is awaiting code reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants