SrBrahma / GitHub-Repository-Manager

VS Code Extension to quickly clone, initialize and open your GitHub repositories
MIT License
31 stars 14 forks source link

feat: add organisation support #10

Closed jonathan-fielding closed 4 years ago

jonathan-fielding commented 4 years ago

This is an initial attempt at getting organisations support working with only readOrgs permission. So far I only got this working with personal access token though but wanted to share my progress so far to see if you have any pointers for OAuth method.

Todo:

SrBrahma commented 4 years ago

There is an easier way to achieve that. I actually found a workaround in GitHub forum and implemented it in branch wip2 (unfortunately it has so many and different changes that it became like a dead lock of confusion), that to get the orgs repos at the time that I tested, it's needed to only add this affiliations in the already existing GraphQL query. (and also, in the OAuth/token, add the 'user' if I am not mistaken).

However, the repositories tree view became a little mess with the orgs repos. I thought of implementing another tree child on the root, for the user and the repos, where the corresponding repos would be.

New query that allows orgs repos: src/octokit/commands/getRepos.ts in branch wip2

query getRepos ($after: String) {
  viewer {
    repositories(
      first: 100,
      affiliations: [OWNER, ORGANIZATION_MEMBER, COLLABORATOR], ownerAffiliations:[OWNER, ORGANIZATION_MEMBER, COLLABORATOR],
      orderBy: {field: NAME, direction: ASC}, after: $after
    ) {

      pageInfo {
        endCursor
        hasNextPage
      }
      nodes {
        name
        description
        owner {
          login
        }
        primaryLanguage {
          name
        }
        url

        isPrivate
        isFork
        isTemplate

        parent {
          name
          owner {
            login
          }
        }

        createdAt
        updatedAt
      }
    }
  }
}
`;
SrBrahma commented 4 years ago

I am working full time at another project right now so I can't study, work and review this issue. However, if you want to, you could see the wip2 branch and extract this part.

I really should restart this wip2. My lack of experience in updating codes with git really made me want to not touch it again as it got so messy.

jonathan-fielding commented 4 years ago

@SrBrahma Have updated to use this alternative request but it is a lot slower, definitely will need to change how the loading works to be more asynchronous

I am loading in 2164 repositories at moment and takes a while

jonathan-fielding commented 4 years ago

Looked through the wip2 branch, I think the feedback I would give is you tried to do to much in one branch, small iterative changes are a lot easier to complete and merge in

SrBrahma commented 4 years ago

That's a good point you raised that I hadn't thought, about it being slow as it is sequential.

But, would it be a good idea to fetch all repos at every startup, including cases like yours that have thousands of repos? It's probable that if you open multiple VsCode windows and/or restart it a couple of times, GitHub API may temporarily ban your requests (ain't sure about their API limits).

Maybe, if the tree becomes separated by the repo owner and also may have another separation if more than 100 repos,

Cloned
----SrBrahma
--------[...myRepos]
----$org1Name
--------[...org1Repos]
----$org2Name(hundreds of repos)
--------[...org2Repos_0~99]
------------[...org2Repos]
--------[...org2Repos_100~199]
------------[...org2Repos]

The fetchs would only happen at the opened trees (and those opened trees could be saved in the local storage to be persistent).

Maybe always fetch the user repos and selectively fetch the orgs repos.

It's your call. I really don't know how it is to have so many repos lol

I also have the idea of marking repos as favorites to make their finding easier in the tree.

SrBrahma commented 4 years ago

https://developer.github.com/v4/guides/resource-limitations/

Edit: Hmmm, looks like it is ok to make multiple requests in this case, as it divides by 100.

jonathan-fielding commented 4 years ago

I don't think we should do requests on every load no, we should cache with a age of the cache, if cache is older than X it's marked as stale but we show the data to user anyway while refreshing in background,

Users could disable showing stale data in settings to allow users to have more up to date info if they so desire

jonathan-fielding commented 4 years ago

To display a list of orgs before fetching we would need to get a users orgs somehow, can only think of way I did before using org read permissions tho, maybe the reorging into nested tree should be a layer pr if it becomes a problem

SrBrahma commented 4 years ago

Indeed, your previous solution solve well this issue.

That idea of caching. Hmmm... Would be good for offline mode (so the user could still see the cloned repos), but maybe the age part wouldn't do good enough. On start it could use the cached, and refresh on data fetched (and on refresh click). Buuut... if the user is browsing data and it refreshes with new data, it could make the user angry as he would lose what he was selecting / looking at.

There is however the loading bar when the tree view is awaiting a promise (the fetch in this case), so the user could be aware to wait for the data fully arrive. I don't recall if the project tree view loading bar is working.

jonathan-fielding commented 4 years ago

The problem with the paralisation of the requests is its cursor based pagination so I don't know the $after until I have got the previous page of results πŸ˜”.

On caching note, this is a dev tool and devs should understand why caching is necessary so I think users will understand πŸ˜‚

jonathan-fielding commented 4 years ago

How easy would it be to add a read:org permission to the permissions? to do this loading in a performant way I keep running into this as limitation?

SrBrahma commented 4 years ago

It's very easy to add it to the OAuth. However, you should add a check or ignore the error if the user hasn't the necessary authenticantion (read:org). Maybe you can check the return of the org query, as in the error it states the insufficient permission.

Maybe I will revoke the access to the ~500 OAuth users to make them OAuth again with the new permission, but I can't do the same for Personal Access token users, so they remain with the old permissions

SrBrahma commented 4 years ago

FYI, maybe someday I will move the auth to the new integrated VsCode GitHub auth (that was released some days after this extension release).

GitHub Pull Request extension do this way: https://github.com/microsoft/vscode-pull-request-github/blob/5d009c2623c6060a7453a53c4ef3c57004c9ef00/src/github/credentials.ts

jonathan-fielding commented 4 years ago

Just to keep you updated, I am currently going through the different auth flows for orgs, I am in fortunate situation in this case where I am in an GitHub org that is SSO authenticated which means I get extra error messages which I am sure other users will experience so I am working on handling them.

jonathan-fielding commented 4 years ago

Another progress update, I now have orgs loading into the not cloned section like seen in screenshot

Screenshot 2020-07-25 at 22 29 50

This pr is getting quite big (haven't pushed yet) to get all this working nicely so when its ready I am happy to do a call with you if thats easier to talk through the change

jonathan-fielding commented 4 years ago

The biggest challenge I am dealing with right now is state, so previously state was managed with 2 separate classes, one in Repos and one in Users, however when you add in Orgs you have a kind of messy relationship

User has many repos User has many orgs Orgs has many repos Repos do not always have org

So having to reconcile that in a way that I can render a UI nicely

SrBrahma commented 4 years ago

Sure, we can schedule a call. You can add me on Discord: Henrique Bruno#0704 . But, maybe it's better to stay via text as this project codes are no longer fresh in my memory hehe

For the relationships, I am not getting your point. As the org repos still appears in the Repos tree view, and orgs can be identified by the 'author' repo field, I don't see why there is a need for a 2-way relationship.

jonathan-fielding commented 4 years ago

Making good progress, just need to do some work on the cloned repo stuff now as I broke that

jonathan-fielding commented 4 years ago

Ok so functionality wise I think this PR is complete, I think it prob needs a bit of a tidy up code wise though and some thorough testing. Also I decided not to implement caching at this stage but will add in a separate pr as don't want to complicate this already large pr to much

jonathan-fielding commented 4 years ago

also I will clean up git history at very end

jonathan-fielding commented 4 years ago

@SrBrahma functionality wise I think everything is working, would be good to get your eyes on this if you have time?

SrBrahma commented 4 years ago

Ok! Today or tomorrow I will check everything and merge and publish if ok

SrBrahma commented 4 years ago

Which additional scopes it requires now? Going to add to OAuth now. Looks like the OAuth (that should still works even without the new scope) went broken. Can you try to use OAuth to login on your machine to see if it is working?

SrBrahma commented 4 years ago

Found out: On octokit.ts, as I lack the orgs read access, the loadUser() throws the 'Error: Insufficient Access, please login to GitHub Repository Manager'.

When you tell me the scope needed, I will update this Error message including the necessary scopes (for token users, OAuth users I will revoke all to make them reOAuth), and add a vscode.window.showErrorMessage in the initOctokit catch block

jonathan-fielding commented 4 years ago

just needs the read:org scope AFAIK,

SrBrahma commented 4 years ago

How do you allow access to the organizations repositories using personal tokens?

Via OAuth, this is done on GitHub Settings -> Applications -> Authorized OAuth Apps -> VS Code GitHub Repository Manager, and allow the access to each organization. Captura de tela de 2020-08-04 04-15-37

Also, can you make the cloned repos stay under a tree for each user/org, like the not cloned ones? Captura de tela de 2020-08-04 03-42-48

jonathan-fielding commented 4 years ago

re: Also, can you make the cloned repos stay under a tree for each user/org, like the not cloned ones?

The reason I didn't do this was the expectation that while you might work for a org with 2000 repos (like me), realistically I only have 20 checked out and the inconvenience of opening up the org for a cloned repo seemed high (vs the one off opening when cloning a repo)

jonathan-fielding commented 4 years ago

re: How do you allow access to the organizations repositories using personal tokens?

Screenshot 2020-08-04 at 21 16 36

Just check this box, altho if the org requires SSO you will need to click the Enable SSO button as well which will take you through the SSO auth flow

Screenshot 2020-08-04 at 21 17 23
SrBrahma commented 4 years ago

re: Also, can you make the cloned repos stay under a tree for each user/org, like the not cloned ones?

The reason I didn't do this was the expectation that while you might work for a org with 2000 repos (like me), realistically I only have 20 checked out and the inconvenience of opening up the org for a cloned repo seemed high (vs the one off opening when cloning a repo)

Well, the cloned trees could initialize opened, there is an option for it somewhere when constructing the tree. The vertical space used will be just (nOrgs+1) (+1 being the user), and the horizontal space for removing the {author}/ will be greatly smaller; will look cleaner. I also have something like ~20 cloned repos, and doesn't looks very good having 20 "SrBrahma/" before each repo hehe

I think you will also see it being better ;) Tomorrow I will code it and send a print here.

There could even be a persistent state to save the "is opened" status for each tree, but maybe in a future update.

SrBrahma commented 4 years ago

re: How do you allow access to the organizations repositories using personal tokens?

Screenshot 2020-08-04 at 21 16 36

Just check this box, altho if the org requires SSO you will need to click the Enable SSO button as well which will take you through the SSO auth flow

Screenshot 2020-08-04 at 21 17 23

Shit, looks like OAuth always needs the Org owner to allow the application. Maybe this partially explains my old problem (https://github.com/SrBrahma/GitHub-Repository-Manager/issues/4#issuecomment-641631847)... And the Personal Access doesn't require it.

https://docs.github.com/en/github/setting-up-and-managing-organizations-and-teams/about-oauth-app-access-restrictions

The OAuth is already using the new scope, as I can change it without updating this extension. Can you reOAuth again and see if all your organizations repos shows up?

jonathan-fielding commented 4 years ago

re: How do you allow access to the organizations repositories using personal tokens?

Screenshot 2020-08-04 at 21 16 36

Just check this box, altho if the org requires SSO you will need to click the Enable SSO button as well which will take you through the SSO auth flow

Screenshot 2020-08-04 at 21 17 23

Shit, looks like OAuth always needs the Org owner to allow the application. Maybe this partially explains my old problem (#4 (comment))... And the Personal Access doesn't require it.

https://docs.github.com/en/github/setting-up-and-managing-organizations-and-teams/about-oauth-app-access-restrictions

The OAuth is already using the new scope, as I can change it without updating this extension. Can you reOAuth again and see if all your organizations repos shows up?

OAuth is working for me thank you,

SrBrahma commented 4 years ago

It is showing to you the orgs that even don't belong to you?

SrBrahma commented 4 years ago

I am changing the code a bit right now. The loading of the user and of the repos trees are a little strange (you can see it by reloading the VsCode in master version and in this fork one). It is starting not logged (instead of logging if key already stored), and the LocalPaths finder could happen simultaneously with some steps.

jonathan-fielding commented 4 years ago

I can only see the repos in my orgs and my list of personal repos which is correct

The local paths finder can't be simultaneous to the requests for repos because the request for repos expects the local paths data to be available once its done, (this is slightly more performant and easier to debug than trying to do simultaneously and reconciling afterwards, I tried the simultaneous approach and it was a pain)

SrBrahma commented 4 years ago

I can only see the repos in my orgs and my list of personal repos

The local paths finder can't be simultaneous to the requests for repos because the request for repos expects the local paths data to be available once its done, (this is slightly more performant and easier to debug than trying to do simultaneously and reconciling afterwards, I tried the simultaneous approach and it was a pain)

When you ReOAuthed again, did you select every org to give the extension access?

The Finder can help simultaneously with the loadUser()

jonathan-fielding commented 4 years ago

I am changing the code a bit right now. The loading of the user and of the repos trees are a little strange (you can see it by reloading the VsCode in master version and in this fork one). It is starting not logged (instead of logging if key already stored), and the LocalPaths finder could happen simultaneously with some steps.

I tried reloading, so yeah it always starts logged out until its done its login, its not that strange however it might be as easy as changing how the initial state is setup by redux.

jonathan-fielding commented 4 years ago

I can only see the repos in my orgs and my list of personal repos The local paths finder can't be simultaneous to the requests for repos because the request for repos expects the local paths data to be available once its done, (this is slightly more performant and easier to debug than trying to do simultaneously and reconciling afterwards, I tried the simultaneous approach and it was a pain)

When you ReOAuthed again, did you select every org to give the extension access?

The Finder can help simultaneously with the loadUser()

It selected them all by default

jonathan-fielding commented 4 years ago

I can only see the repos in my orgs and my list of personal repos The local paths finder can't be simultaneous to the requests for repos because the request for repos expects the local paths data to be available once its done, (this is slightly more performant and easier to debug than trying to do simultaneously and reconciling afterwards, I tried the simultaneous approach and it was a pain)

When you ReOAuthed again, did you select every org to give the extension access?

The Finder can help simultaneously with the loadUser()

Was trying to avoid mixing concerns, so in this case users and repos as can be confusing and the perf benefit is low.

Interesting note, the OAuth api seems to be faster than the personal access token one which is nice.

SrBrahma commented 4 years ago

I can only see the repos in my orgs and my list of personal repos The local paths finder can't be simultaneous to the requests for repos because the request for repos expects the local paths data to be available once its done, (this is slightly more performant and easier to debug than trying to do simultaneously and reconciling afterwards, I tried the simultaneous approach and it was a pain)

When you ReOAuthed again, did you select every org to give the extension access? The Finder can help simultaneously with the loadUser()

Was trying to avoid mixing concerns, so in this case users and repos as can be confusing and the perf benefit is low.

Interesting note, the OAuth api seems to be faster than the personal access token one which is nice.

Ain't sure about that, as the OAuth at the end also returns a token, like the personal access token... only if the token made by the OAuth is processed faster by their server

jonathan-fielding commented 4 years ago

Definitely appears to be faster at loading, still will be looking to add caching in a subsequent pr,

SrBrahma commented 4 years ago

Can you test if all looks good? Added 'Loading...' in user treeview, changed 'Not Loaded' in repos tree view also to 'Loading...' (it didn't make too much sense to the user)

SrBrahma commented 4 years ago

Also, do you know MobX? I am currently using it in my current react native project, and really looks to me better than Redux. It like mixes my previous class style with the usefulness of redux (but prettier hehe!).

Not that I would or ask to you to change the current code to it, but just to make MobX advertisement (:

jonathan-fielding commented 4 years ago

I have heard of MobX but as one of my focuses in my career has been web performance I have avoided if because its far bigger and heavier on the browser than redux and I would rather spend those bytes on a nice animation library that enhances the user experience than on a library that only helps mine ☺️

Like I said before, cloned repos probably shouldn't be nested as devs work for a org with a lot of repos but don't actually work on more than a handful so having to keep expanding and contracting to find stuff is super weird. It makes sense for looking at the remote repos you haven't cloned yet tho as you might be in 20 orgs (I used to be, im only in three at moment tho)

jonathan-fielding commented 4 years ago

Also orgs that I haven't cloned things from also are appearing in cloned section which also feels weird

SrBrahma commented 4 years ago

Tomorrow I will add an option to choose between nested or flat cloned repos. If Flat, have the old 'author / repoName'. Also, if nested, only show the org parent if there is a child.

Will work for all cases.

Then I will publish it

SrBrahma commented 4 years ago

Hey, I started to code the changes right now, and I had the idea about the cloned repos: The Tree could look like this:

Cloned -- userRepoA -- userRepoZ -- orgA / orgRepoA -- orgA / orgRepoZ -- orgZ / orgRepo

So, it would only show the name of the repo author (user or org) if it isn't the user itself. All would be alphabetically ordered. No additional trees, and with less visual pollution.

Looks good to you?

jonathan-fielding commented 4 years ago

sure that works,

SrBrahma commented 4 years ago

Published! In a few minutes will be available. Great work!

jonathan-fielding commented 4 years ago

Thanks for the help :D