Add GetAll method to OrganizationsClient #1469

malamour-work · 2016-09-21T01:09:55Z

Hi,

I noticed that the list all organizations GET /organizations listed here https://developer.github.com/v3/orgs/ was missing from the OrganizationClient. I did not find any issue related to this in the backlog.

I added the implementation in the two client that i found (Base and Observable) with the documentation and added some unit tests.

Let me know if i forgot something or if something need to be changed.

Renamed the existing organizations uri to userOrganizations and created a new organizations uri.

ryangribble · 2016-09-21T03:02:24Z

Hi @malamour-work thanks heaps for your contribution! 😀

A couple of things for you:

Breaking changes to public interfaces/methods

Being a public API we do need to be careful about not breaking existing users and the renamed methods in ApiUrls class would currently be a "breaking" change for people. Whilst they are just helper methods, given they are public we could have users that are using them and as such I believe they would fall under our obligation to not make breaking changes without going through the normal process (see below).

It makes sense to rename the existing ApiUrls.Organizations() to ApiUrls.UserOrganizations() as you've done but technically that means the old name can't be used for the new "all organizations" URL since it needs to be obsoleted. You'll need to use a different name for that one such as ApiUrls.AllOrganizations().

Maintaining naming consistency

We try to follow a naming convention where GetAll gets the given resource, GetAllForCurrent() is for the current user, GetAllForUser() is for a nominated user etc. So in this case, GetAllOrganizations() should really just be named GetAll() since they are in the organizations client already. I realize you went with this option because the other methods were already just called GetAll but infact they SHOULD have been called GetAllForUser(string login) all this time... So it would be cool if you could rename GetAll(string user) => GetAllForUser(string login) as part of this change, and implement GetAll() as your new method rather than calling it GetAllOrganizations(). Any renames would be done following the "normal process" outlined below.

Process for breaking changes:

Create the new method/implementation/classes and mark any old ones as
[Obsolete("Please use OrganizationsClient.GetAllForUser() instead. This method will be removed in a future version")]

We wait at least 2 releases before removing the obsolete items from the code base. This gives users some notice (in the form of compiler warnings) before things change on them 👍

Let me know if you need any help with this and thanks for stepping up to correct this missing functionality!

ryangribble · 2016-09-21T03:05:26Z

Octokit/Helpers/ApiUrls.cs

@@ -20,6 +20,7 @@ public static partial class ApiUrls
        static readonly Uri _currentUserOwnedAndMemberIssues = new Uri("user/issues", UriKind.Relative);
        static readonly Uri _oauthAuthorize = new Uri("login/oauth/authorize", UriKind.Relative);
        static readonly Uri _oauthAccessToken = new Uri("login/oauth/access_token", UriKind.Relative);
+        static readonly Uri _organizationUrl = new Uri("organizations", UriKind.Relative);


Id like to steer away from these class level variables (that are only used in one place anyway) in favour of just returning the Urls in the helper method below

ryangribble · 2016-09-21T03:06:18Z

Octokit/Helpers/ApiUrls.cs

        {
            return "users/{0}/orgs".FormatUri(login);
        }
+
+        /// <summary>
+        /// Returns the <see cref="Uri"/> that returns all of the organizations for the currently logged in user.


Extra text here from cut+paste 😀

Returns the that returns all of the organizations ~~for the currently logged in user~~

ryangribble · 2016-09-21T03:09:52Z

Another thing I just realized is that the API docs mentions this "List organizations" API method is not paginated like normal methods (pagesize, count etc), and instead just uses the since parameter. So you wont be able to take ApiOptions in your method signature, instead you'll need a custom request class with the Since parameter. You can have a look at the Repositories.GetAllPublic(PublicRepositoryRequest request) method for some ideas on how to tackle that

…ible. Created new client method and marked the old one [Obsolete] to be removed in a futur release. Created a new request class to support the since attribute. Updated the Unit Tests Updated all the csproj to have the proper references.

Applied the modification from the review

malamour-work · 2016-09-26T15:12:33Z

Hi @ryangribble

Thanks for your time and for the review. I did the modification you have requested.

ryangribble

Hey @malamour-work thanks for making those changes, I think we are almost done!

Given the "old" GetAll method has been obsoleted/renamed to GetAllForUser() I think it makes sense to also obsolete/rename the ApiUrls.Organizations() to ApiUrls.UserOrganizations() while we're going through the obsoleting process. This will aid in future consistency and remove confusion over the previous naming of these methods
Could you please add integration tests for your new methods so we can ensure they do work in the real world? At a minimum you'd want to make the new call and assert a non null/empty list is returned. Bonus points for a 2nd test which makes 2 calls using different "since" parameters and asserting that the lists returned are different! This is similar to the integration tests we have for methods that paginate using ApiOptions where we call the method with differing page counts and assert the returned lists are different

ryangribble · 2016-09-28T14:10:26Z

Octokit.Tests/Clients/OrganizationsClientTests.cs

+            }
+
+            [Fact]
+            public async Task RequestsTheCorrectUrlWithApiOptions()


Since this isn't actually using ApiOptions it should be named something else, perhaps RequestsTheCorrectUrlWithRequestParameter

…anizations method.

Renamed the existing one to make it clear they test the user organization.

malamour-work · 2016-10-03T18:48:41Z

Hi @ryangribble

I have obsoleted the old ApiUrls.Organizations() and created the integration tests like you requested.

If anything else let me know.

Thanks

ryangribble · 2016-10-03T23:58:04Z

Thanks @malamour-work sorry I hadnt had a chance over the weekend to look at this. I will try and do it tonight.

Have you actually run the integration tests? I notice they are flagged as GitHubEnterprise (because they create organizations to then run the tests against, and create organizations is not avaialble on github.com only github enterprise). For something like a GET call, we could potentially run the test against github.com but I guess that becomes hard since we dont know what organization ID to use as the since parameter (and as time marches on, any ID used in the test will start to become further and further back in time, leading to the test retrieving larger amounts of orgs until it eventually times out!). So probably leaving the tests as GitHubEnterprise tests is fine, but we do just need to make sure they actually run succesfully of course 😀

I wll pull this down and run the tests etc and get back to you

ryangribble · 2016-11-21T12:57:24Z

Sorry for the delay on this one 😭

Ive run the integration tests and made a couple of tweaks and pushed them back up.

This LGTM now - many thanks for your PR and particularly working through all the review comments 😀

malamour-work added 7 commits September 20, 2016 13:02

Update IOrganizationsClient.cs

dc8957c

Added the Uri for the organizations

902b84d

Renamed the existing organizations uri to userOrganizations and created a new organizations uri.

Implementation of the GetAllOrganizations

9e138cd

Interface modification for the GetAllOrganizations

9ab62c4

Implementation of the GetAllOrganizations

2bdf642

Created the tests for the two new methods

ec3f75b

remove new line

50fdfb3

ryangribble reviewed Sep 21, 2016

View reviewed changes

malamour-work added 2 commits September 25, 2016 21:18

Merge pull request #2 from malamour-work/ReviewRequestModification

009168b

Applied the modification from the review

ryangribble suggested changes Sep 28, 2016

View reviewed changes

malamour-work added 3 commits September 28, 2016 19:09

Renamed test

bb94ae8

Deprecated the Organizations method to be replaced by the new UserOrg…

cab385f

…anizations method.

Created 2 new test for the getAll method.

56b83bd

Renamed the existing one to make it clear they test the user organization.

ryangribble approved these changes Oct 4, 2016

View reviewed changes

tweaks to integration tests

5561a65

ryangribble merged commit 195de68 into octokit:master Nov 21, 2016

ryangribble changed the title ~~Added a get all organizations method~~ Add a GetAll method to OrganizationsClient Jan 15, 2017

ryangribble changed the title ~~Add a GetAll method to OrganizationsClient~~ Add a GetAll method to OrganizationsClient Jan 15, 2017

ryangribble changed the title ~~Add a GetAll method to OrganizationsClient~~ Add GetAll method to OrganizationsClient Jan 15, 2017

ryangribble added the feature label Jan 15, 2017

nickfloyd added Type: Feature New feature or request and removed category: feature labels Oct 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add GetAll method to OrganizationsClient #1469

Add GetAll method to OrganizationsClient #1469

malamour-work commented Sep 21, 2016

ryangribble commented Sep 21, 2016

ryangribble Sep 21, 2016

ryangribble Sep 21, 2016

ryangribble commented Sep 21, 2016 •

edited

Loading

malamour-work commented Sep 26, 2016

ryangribble left a comment

ryangribble Sep 28, 2016

malamour-work commented Oct 3, 2016

ryangribble commented Oct 3, 2016

ryangribble commented Nov 21, 2016

Add GetAll method to OrganizationsClient #1469

Add GetAll method to OrganizationsClient #1469

Conversation

malamour-work commented Sep 21, 2016

ryangribble commented Sep 21, 2016

Breaking changes to public interfaces/methods

Maintaining naming consistency

Process for breaking changes:

ryangribble Sep 21, 2016

Choose a reason for hiding this comment

ryangribble Sep 21, 2016

Choose a reason for hiding this comment

ryangribble commented Sep 21, 2016 • edited Loading

malamour-work commented Sep 26, 2016

ryangribble left a comment

Choose a reason for hiding this comment

ryangribble Sep 28, 2016

Choose a reason for hiding this comment

malamour-work commented Oct 3, 2016

ryangribble commented Oct 3, 2016

ryangribble commented Nov 21, 2016

ryangribble commented Sep 21, 2016 •

edited

Loading