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

Repository invitations changes #1410

Merged

Conversation

martinscholz83
Copy link
Contributor

@martinscholz83 martinscholz83 commented Jul 1, 2016

fixed #1399

add a new RepositoryInvitationsClient to invite a user to collaborate. I have to create some new overloads and classes.

Todo

  • add unit and integration tests

@martinscholz83 martinscholz83 force-pushed the repository-invitations-changes branch from 407e30b to cd95700 Compare July 1, 2016 08:04
@martinscholz83 martinscholz83 force-pushed the repository-invitations-changes branch from b47c537 to aba8653 Compare July 1, 2016 08:33
@ryangribble
Copy link
Contributor

Build is failing convention tests due to missing unit test class for TheCtor in RepositoryInvitationsClientTests.cs

/// See the <a href="https://developer.github.com/v3/repos/invitations/#list-invitations-for-a-repository">API documentation</a> for more information.
/// </remarks>
/// <param name="id">The id of the repository</param>
IObservable<RepositoryInvitation> GetAllForRepository(int id);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for consistency, this variable should be named repositoryId

@martinscholz83
Copy link
Contributor Author

I reduce the integration tests to only receive invitations. Tests for accept and decline not possible yet. Anything more to do?

@ryangribble
Copy link
Contributor

Anything more to do?

Integration tests for Delete() and Edit() ?

@ryangribble
Copy link
Contributor

Also it seems with the existing integration tests you are inviting YOURSELF to a test repository you created. And this actually seems to work, with the invite showing up in the system.

So given it seems to work to invite yourself, then you probably CAN have tests for Accept() and Decline() ?

@martinscholz83
Copy link
Contributor Author

martinscholz83 commented Jul 16, 2016

Unfortunately not. I always catch an NotFoundexception if try to decline and accept with the same account. I have tested it with different accounts. There i can accept and decline the invitiations.

@ryangribble
Copy link
Contributor

ryangribble commented Jul 16, 2016

Unfortunately not. I always catch an NotFoundexception if try to decline and accept with the same account. I have tested it with different accounts. There i can accept and decline the invitiations.

It seems to work for me? Eg this passes:

        [IntegrationTest]
        public async Task CanDeclineInvitation()
        {
            var github = Helper.GetAuthenticatedClient();
            var repoName = Helper.MakeNameWithTimestamp("public-repo");

            using (var context = await github.CreateRepositoryContext(new NewRepository(repoName)))
            {
                var fixture = github.Repository.Collaborator;
                var permission = new CollaboratorRequest(Permission.Push);

                // invite a collaborator
                var response = await fixture.Invite(context.RepositoryOwner, context.RepositoryName, context.RepositoryOwner, permission);

                Assert.Equal(context.RepositoryOwner, response.Invitee.Login);
                Assert.Equal(InvitationPermissionType.Write, response.Permissions);

                // Decline the invitation
                var declined = await github.Repository.Invitation.Decline(response.Id);

                Assert.True(declined);
            }
        }

@martinscholz83
Copy link
Contributor Author

I still don't get this test working 😕

[IntegrationTest]
        public async Task CanDeclineInvitation()
        {
            var github = Helper.GetAuthenticatedClient();
            var repoName = Helper.MakeNameWithTimestamp("public-repo");

            using (var context = await github.CreateRepositoryContext(new NewRepository(repoName)))
            {
                var fixture = github.Repository.Collaborator;
                var permission = new CollaboratorRequest(Permission.Push);

                // invite a collaborator
                var response = await fixture.Invite(context.RepositoryOwner, context.RepositoryName, context.RepositoryOwner, permission);

                Assert.Equal(context.RepositoryOwner, response.Invitee.Login);
                Assert.Equal(InvitationPermissionType.Write, response.Permissions);

                // Decline the invitation
                var declined = await github.Repository.Invitation.Decline(response.Id);

                Assert.True(declined);
            }
        }

Should i though write these tests although they don't work with my account?

@ryangribble
Copy link
Contributor

👍 write them and I'll check them my side. What error do you get?

@martinscholz83
Copy link
Contributor Author

In that try catch block I get an 404 NotFoundException. Later I can send you detailed informations.

On Tue, Jul 19, 2016 at 1:15 PM +0200, "Ryan Gribble" [email protected] wrote:

👍 write them and I'll check them my side. What error do you get?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1410 (comment)

@ryangribble
Copy link
Contributor

ryangribble commented Jul 20, 2016

Your test works fine on my side 👍

image

So if you can also add an "Accept" invitation integration test I will double check that too and then I think we are good!

Not sure what might be the issue with your 404? I guess it means the invitation doesnt exist but its strange, in my case I can even login to github with my test account and see the invitation etc. And there's nothing special about my test account, its just a free account. 😕

@martinscholz83
Copy link
Contributor Author

If i login i can also see that invitation. But i can't accept or decline it with octokit. But if it works with your account then maybe its not an issue. Maybe someone third should test it.

@ryangribble
Copy link
Contributor

Yep. that Accept test works for me too.

Maybe someone third should test it.

Yep probably a good idea

@shiftkey shiftkey self-assigned this Jul 21, 2016
@shiftkey
Copy link
Member

shiftkey commented Jul 21, 2016

Maybe someone third should test it.

Yep probably a good idea

👀

@ryangribble
Copy link
Contributor

ryangribble commented Jul 21, 2016

Hey @maddin2016 I was pondering why accepting/declining a self invite might work for me and not you... one thing that came to mind is that I currently have my integration test configuration setup to use username and password only (i.e I have not specified an apikey/personal access token in the environment vars). How have you got yours setup? If you have an access token defined, try not specifying one, just to see if that makes any difference.

@martinscholz83
Copy link
Contributor Author

winner
100 points 🎉 That's it.

@ryangribble
Copy link
Contributor

ryangribble commented Jul 22, 2016

Nice one!

OK so I reckon this is good to 🚢 but let's get a 👍 from @shiftkey first

@shiftkey shiftkey changed the title [WIP] Repository invitations changes Repository invitations changes Jul 22, 2016
@shiftkey
Copy link
Member

Lets do this

@ryangribble
Copy link
Contributor

LGTM

@ryangribble ryangribble merged commit 89500f4 into octokit:master Jul 23, 2016
@martinscholz83
Copy link
Contributor Author

😂 That last gif

@ryangribble
Copy link
Contributor

ryangribble commented Jul 25, 2016

Just a quick update on

On the topic of the email/notifications not occuring, I contacted github support and they confirm they can reproduce it, they are logging an internal issue, so we shouldn't need to worry about that aspect at least

GitHub support notified me recently that they had pushed a fix for this, and I can confirm that emails are now being received when creating invites with octokit 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repository Invitations Changes
3 participants