Subscribe/Unsubscribe #2482

Huddie · 2018-11-25T03:05:29Z

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 · 2018-11-25T11:28:06Z

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

Sherlouk · 2018-11-25T11:28:53Z

Classes/Issues/PullRequest+IssueType.swift

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

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

+    var viewerSubscribed: Bool {
+        return viewerSubscription?.rawValue != "IGNORED"


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

Sherlouk · 2018-11-25T11:29:21Z

Classes/Issues/Issue+IssueType.swift

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

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

+    var viewerSubscribed: Bool {
+        return viewerSubscription?.rawValue != "IGNORED"


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

I shall fix this!

Huddie · 2018-11-25T12:12:41Z

@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 · 2018-11-25T12:29:45Z

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 · 2018-11-25T12:35:54Z

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 · 2018-11-25T20:08:59Z

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

Sherlouk · 2018-11-25T22:00:15Z

@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 · 2018-11-25T22:20:56Z

@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 · 2018-11-25T23:20:56Z

@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 · 2018-11-29T01:09:59Z

@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 · 2018-11-29T08:31:18Z

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 · 2018-11-29T13:29:39Z

@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 · 2018-11-30T06:02:53Z

@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 · 2018-11-30T13:27:45Z

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)

Huddie · 2018-11-30T15:16:30Z

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

rnystrom · 2018-11-30T15:22:27Z

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

_{Sent with GitHawk}

ijm8710 · 2018-11-30T15:48:16Z

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

Huddie · 2018-11-30T17:26:25Z

@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 · 2018-11-30T18:11:08Z

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 · 2018-11-30T18:44:59Z

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

_{Sent with GitHawk}

rnystrom · 2018-11-30T19:01:29Z

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 · 2018-11-30T19:12:26Z

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

_{Sent with GitHawk}

ijm8710 · 2018-11-30T19:13:44Z

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 · 2018-11-30T19:31:52Z

@ijm8710 Thanks!

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

General changes:

Manager button will Always show.
- Reason for this is that you can always subscribe/unsubscribe to an issue
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.
Added viewerSubscription to API request for Issues/PR's.
- Reason: We need this information for determining the current subscription status
Added a new mutation that allows for changing users subscription status

ijm8710 · 2018-11-30T20:10:09Z

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 · 2018-11-30T20:17:52Z

@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 · 2018-11-30T20:24:12Z

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!

Huddie · 2018-12-26T23:57:16Z

@rnystrom

_{Sent with GitHawk}

rnystrom · 2018-12-29T20:44:41Z

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

Huddie · 2019-01-06T02:16:29Z

@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 · 2019-03-11T05:09:53Z

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

BasThomas

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

BasThomas · 2019-03-16T14:22:03Z

Classes/Issues/GithubClient+Issues.swift

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


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

BasThomas · 2019-03-16T14:22:21Z

Classes/Issues/GithubClient+Issues.swift

+        let cache = self.cache
+        cache.set(value: optimisticResult)
+
+        let mutation = UpdateSubscriptionMutation(subscribable_Id: previous.id, subscription_state: state)


Can we change these parameters to camelCase?

BasThomas · 2019-03-16T14:24:10Z

Classes/Issues/IssueManagingContextController.swift

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

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


We should update this comment.

BasThomas · 2019-03-16T14:25:53Z

FreetimeTests/SplitViewTests.swift

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


Why was this changed? 🤔

BasThomas

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

Huddie · 2019-03-17T00:38:24Z

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

_{Sent with GitHawk}

BasThomas · 2019-03-17T08:46:54Z

_{Sent with GitHawk}

Huddie · 2019-03-17T11:55:09Z

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

Subscribe/Unsubscribe

ab5e531

ijm8710 mentioned this pull request Nov 25, 2018

Missing Subscribe/Unsubscribe #2149

Open

Sherlouk reviewed Nov 25, 2018

View reviewed changes

This was referenced Nov 27, 2018

Try again for “Cannot load file type” does not work #2493

Closed

Inbox Redesign #2497

Open

Working on mutation

aa61bca

Huddie added 2 commits November 29, 2018 23:58

Fixed mutation

bbda2d3

Fixed code logic

5f1f379

Fixing merge

35758ba

Huddie added 2 commits November 30, 2018 14:25

Fixed test

9124e6d

Switched ignored to unsubscribed

7cbe77a

rnystrom mentioned this pull request Dec 1, 2018

Subscribe to issue/PR #2511

Closed

Huddie changed the title ~~[WIP] Subscribe/Unsubscribe~~ Subscribe/Unsubscribe Dec 2, 2018

Huddie added the 💤 awaiting review Pull Request is awaiting code reviews label Dec 2, 2018

ijm8710 mentioned this pull request Dec 3, 2018

Issue Dashboard #2413

Closed

Huddie mentioned this pull request Mar 8, 2019

Toggling the watch status on a repository #2656

Open

BasThomas reviewed Mar 16, 2019

View reviewed changes

BasThomas suggested changes Mar 16, 2019

View reviewed changes

ijm8710 mentioned this pull request Jul 23, 2019

General question of the github app roadmap! #2806

Open

Subscribe/Unsubscribe #2482

Are you sure you want to change the base?

Subscribe/Unsubscribe #2482

Conversation

Huddie commented Nov 25, 2018 • edited

Sherlouk commented Nov 25, 2018

Sherlouk Nov 25, 2018

Choose a reason for hiding this comment

Sherlouk Nov 25, 2018

Choose a reason for hiding this comment

Huddie Nov 25, 2018

Choose a reason for hiding this comment

Huddie commented Nov 25, 2018 • edited

Sherlouk commented Nov 25, 2018 • edited

Huddie commented Nov 25, 2018

Huddie commented Nov 25, 2018

Sherlouk commented Nov 25, 2018

Huddie commented Nov 25, 2018

Sherlouk commented Nov 25, 2018

Huddie commented Nov 29, 2018

Sherlouk commented Nov 29, 2018

Huddie commented Nov 29, 2018

Huddie commented Nov 30, 2018 • edited

ijm8710 commented Nov 30, 2018 • edited

Huddie commented Nov 30, 2018

rnystrom commented Nov 30, 2018 • edited

ijm8710 commented Nov 30, 2018

Huddie commented Nov 30, 2018 • edited

ijm8710 commented Nov 30, 2018 • edited

Huddie commented Nov 30, 2018

rnystrom commented Nov 30, 2018

Huddie commented Nov 30, 2018

ijm8710 commented Nov 30, 2018 • edited

Huddie commented Nov 30, 2018

ijm8710 commented Nov 30, 2018 • edited

Huddie commented Nov 30, 2018 • edited

ijm8710 commented Nov 30, 2018 • edited

Huddie commented Dec 26, 2018

rnystrom commented Dec 29, 2018

Huddie commented Jan 6, 2019

Huddie commented Mar 11, 2019

BasThomas left a comment

Choose a reason for hiding this comment

BasThomas Mar 16, 2019

Choose a reason for hiding this comment

BasThomas Mar 16, 2019

Choose a reason for hiding this comment

BasThomas Mar 16, 2019

Choose a reason for hiding this comment

BasThomas Mar 16, 2019

Choose a reason for hiding this comment

BasThomas left a comment • edited

Choose a reason for hiding this comment

Huddie commented Mar 17, 2019

BasThomas commented Mar 17, 2019

Huddie commented Mar 17, 2019

Huddie commented Nov 25, 2018 •

edited

Huddie commented Nov 25, 2018 •

edited

Sherlouk commented Nov 25, 2018 •

edited

Huddie commented Nov 30, 2018 •

edited

ijm8710 commented Nov 30, 2018 •

edited

rnystrom commented Nov 30, 2018 •

edited

Huddie commented Nov 30, 2018 •

edited

ijm8710 commented Nov 30, 2018 •

edited

ijm8710 commented Nov 30, 2018 •

edited

ijm8710 commented Nov 30, 2018 •

edited

Huddie commented Nov 30, 2018 •

edited

ijm8710 commented Nov 30, 2018 •

edited

BasThomas left a comment •

edited