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

Added getUserPublicOrganizations method #510

Merged
merged 7 commits into from
Oct 25, 2019

Conversation

awittha
Copy link
Contributor

@awittha awittha commented Mar 27, 2019

It may be the case (especially with GitHub Enterprise installations) that a single authenticated user wants to find out about Organization membership for users other than themselves.

This adds a getUserPublicOrganizations(GHUser user) method to enable that lookup.

I wasn't able to complete the test-suite locally due to permission issues that sounded similar to this mailing-list post from August 2018.

@awittha awittha changed the title Added getUserPublicOrganizations Added getUserPublicOrganizations method Mar 27, 2019
import static com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility.NONE;
import static java.net.HttpURLConnection.HTTP_UNAUTHORIZED;
import static java.util.logging.Level.FINE;
import static org.kohsuke.github.Previews.DRAX;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorted & alphabetized import statements.

Copy link
Member

Choose a reason for hiding this comment

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

They were alphabetical before. Please revert the regrouping. There should be a setting in your IDE to not do this (or use the same sorting as the project already uses).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

What was the setting that you used? (This happens pretty often.)

@@ -172,6 +175,7 @@ public static GitHub connect() throws IOException {
* @deprecated
* Use {@link #connectToEnterpriseWithOAuth(String, String, String)}
*/
@Deprecated
public static GitHub connectToEnterprise(String apiUrl, String oauthAccessToken) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added @Deprecated annotation to match @deprecated javadoc; this is best-practice but may start causing IDE or compile warnings for existing users of the methods.

If the method is truly deprecated, this is fine.

@@ -486,6 +492,7 @@ public GHRepository getRepository(String name) throws IOException {
@Preview @Deprecated
public PagedIterable<GHLicense> listLicenses() throws IOException {
return new PagedIterable<GHLicense>() {
@Override
public PagedIterator<GHLicense> _iterator(int pageSize) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added @Override annotation to overridden methods as a matter of lint/best-practice.

* Alias for {@link #getUserPublicOrganizations(String)}.
*/
public Map<String, GHOrganization> getUserPublicOrganizations(GHUser user) throws IOException {
return getUserPublicOrganizations( user.getLogin() );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One method takes a GH* object, here, a GHUser.

*
* @return the public Organization memberships for the user
*/
public Map<String, GHOrganization> getUserPublicOrganizations(String login) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other method takes a plain old String, so that users' code doesn't have to construct a GHUser just to look up organizations.

* @return the public Organization memberships for the user
*/
public Map<String, GHOrganization> getUserPublicOrganizations(String login) throws IOException {
GHOrganization[] orgs = retrieve().to("/users/" + login + "/orgs", GHOrganization[].class);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

This is great, could you all the override and other tweaks in a separate commit for clarity.
Also, revert the import sorting - the order is already as it should be for this project.

@awittha
Copy link
Contributor Author

awittha commented May 23, 2019

Done.

…emberships of any user, not just the currently-authenticated one.
@bitwiseman
Copy link
Member

@awittha
I've added you as collaborator to the the test org. Give the tests a try now. I've also added a mocking and snapshotting framework if you want to give the instructions in CONTRIBUTING.md a try that would be great.

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

All this looks reasonable. We just need tests in order to merge it.

@bitwiseman
Copy link
Member

@awittha Do you think you'll have time to do this in the next week?

@awittha
Copy link
Contributor Author

awittha commented Oct 23, 2019

Sorry, it had dropped off my radar. It's already Wednesday, but I'll keep an eye on it and see what I can get done by the end of the week.

@awittha
Copy link
Contributor Author

awittha commented Oct 24, 2019

@bitwiseman , wrote some tests!

@awittha
Copy link
Contributor Author

awittha commented Oct 24, 2019

I didn't see your note about instructions in CONTRIBUTING.md, until after I'd tried & figured out the mocking framework by reading the new code... It seems to work well and do what I need. Didn't get to test the instructions, though.

@bitwiseman bitwiseman merged commit 1d004a3 into hub4j:master Oct 25, 2019
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.

2 participants