Skip to content

refactor(orgs): get organizations via GraphQL and display name instead login #347

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.

Sign up for GitHub

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

Closed
lex111 wants to merge 5 commits into from
Closed

refactor(orgs): get organizations via GraphQL and display name instead login #347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
78ef77e
refactor(orgs): get organizations via GraphQL
lex111 Sep 20, 2017
f3a5ee5
feat(oauth): add read org scope to GitHub OAuth
lex111 Sep 20, 2017
16c27ee
feat(orgs): made name display instead of login
lex111 Sep 20, 2017
fe77ed0
refactor(fetchUserOrgs): using map instead reduce
lex111 Sep 21, 2017
70a8496
Merge branch 'master' into refactor-get-orgs
machour Sep 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
"dependencies": {
"entities": "^1.1.1",
"fuzzy-search": "^1.4.0",
"lodash.uniqby": "^4.7.0",
"lowlight": "^1.5.0",
"marked": "^0.3.6",
"md5": "^2.2.1",
Expand Down
50 changes: 50 additions & 0 deletions src/api/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import { abbreviateNumber } from 'utils';
export const CLIENT_ID = '87c7f05700c052937cfb';
export const CLIENT_SECRET = '3a70aee4d5e26c457720a31c3efe2f9062a4997a';

export const GRAPHQL_ENDPOINT = `${root}/graphql`;

const ACCEPT = {
JSON: 'application/vnd.github.v3+json',
HTML: 'application/vnd.github.v3.html+json',
Expand Down Expand Up @@ -336,3 +338,51 @@ export async function fetchAccessToken(code, state) {

return response.json();
}

// GraphQL

const accessTokenParametersGraphQL = (accessToken, query, variables) => ({
method: 'POST',
headers: {
Accept: 'application/vnd.github.v3+json',
Authorization: `token ${accessToken}`,
},
body: JSON.stringify({
query: query.replace(/\n/g, ''),
variables,
}),
});

export async function fetchUserOrgs(user, accessToken) {
const getOrganizationsQuery = `
query($login: String!) {
user(login: $login) {
organizations(first: 100) {
edges {
node {
id
login
name
avatarUrl
}
}
}
}
}
`;

const response = await fetch(
GRAPHQL_ENDPOINT,
accessTokenParametersGraphQL(accessToken, getOrganizationsQuery, {
login: user,
})
);

return response.json().then(data => {
return data.data.user.organizations.edges
.map(item => item.node)
.sort((org1, org2) =>
org1.name.toLowerCase().localeCompare(org2.name.toLowerCase())
);
});
}
13 changes: 2 additions & 11 deletions src/auth/auth.action.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
import { AsyncStorage } from 'react-native';

import uniqby from 'lodash.uniqby';
import { delay, resetNavigationTo, configureLocale } from 'utils';
import { saveLanguage } from 'locale';

import {
fetchAccessToken,
fetchAuthUser,
fetchAuthUserOrgs,
fetchUserOrgs,
fetchUserEvents,
fetchStarCount,
Expand Down Expand Up @@ -115,18 +113,11 @@ export const getOrgs = () => {

dispatch({ type: GET_AUTH_ORGS.PENDING });

Promise.all([
fetchAuthUserOrgs(accessToken),
fetchUserOrgs(login, accessToken),
])
fetchUserOrgs(login, accessToken)
.then(data => {
const orgs = data[0].concat(data[1]);

dispatch({
type: GET_AUTH_ORGS.SUCCESS,
payload: uniqby(orgs, 'login').sort(
(org1, org2) => org1.login > org2.login
),
payload: data,
});
})
.catch(error => {
Expand Down
4 changes: 3 additions & 1 deletion src/auth/screens/auth-profile.screen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,8 +171,10 @@ class AuthProfile extends Component {
>
{orgs.map(item =>
<UserListItem
key={item.id}
user={item}
key={item.id}
imageUrl={item.avatarUrl}
title={item.name}
navigation={navigation}
/>
)}
Expand Down
2 changes: 1 addition & 1 deletion src/auth/screens/login.screen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ class Login extends Component {
<View style={styles.browserSection}>
<WebView
source={{
uri: `https://github.com/login/oauth/authorize?response_type=token&client_id=${CLIENT_ID}&redirect_uri=gitpoint://welcome&scope=user%20repo&state=${stateRandom}`,
uri: `https://github.com/login/oauth/authorize?response_type=token&client_id=${CLIENT_ID}&redirect_uri=gitpoint://welcome&scope=user%20repo%20read:org&state=${stateRandom}`,
Copy link
Copy Markdown
Member

@alejandronanez alejandronanez Sep 28, 2017

Choose a reason for hiding this comment

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

Maybe for later but I think we can move this url into a constants files.

}}
onLoadStart={e => this.toggleCancelButton(e, true)}
onLoadEnd={e => this.toggleCancelButton(e, false)}
Expand Down
4 changes: 3 additions & 1 deletion src/components/user-list-item.component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ class UserListItemComponent extends Component {
subtitle: string,
onlyImageNavigate: boolean,
titleStyle: Object,
imageUrl: string,
navigation: Object,
icon: string,
iconAction: Function,
Expand All @@ -85,6 +86,7 @@ class UserListItemComponent extends Component {
title,
subtitle,
titleStyle,
imageUrl,
onlyImageNavigate,
navigation,
icon,
Expand Down Expand Up @@ -131,7 +133,7 @@ class UserListItemComponent extends Component {
<Image
style={styles.avatar}
source={{
uri: user.avatar_url,
uri: imageUrl || user.avatar_url,
Copy link
Copy Markdown
Member

@machour machour Sep 23, 2017

Choose a reason for hiding this comment

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

I think it's best to make the imageUrl prop mandatory and pass it as { user.avatar_url } where needed

Copy link
Copy Markdown
Member Author

@lex111 lex111 Sep 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I do not think that it will turn out, i.e. when querying GraphQL, another notation is used (lowerCase instead of snake_case). We have already done this here.

Copy link
Copy Markdown
Member

@machour machour Sep 23, 2017

Choose a reason for hiding this comment

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

I still think that having an optional prop, magically provided by another prop in some cases, is a bit confusing. I'm in favor of "real dumb" presentation components.

Also in the spirit of #350 (did you check it?), I think GraphQL responses should be stored using the same schemas as REST responses in the entities array. That would enable us to access the data in the same way, regardless of where it came from (rest or graphql). Am I making sense? :/

Copy link
Copy Markdown
Member Author

@lex111 lex111 Sep 24, 2017

Choose a reason for hiding this comment

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

I have not watched, but probably this will solve the problem, it seems to me...

}}
/>
</ImageContainerComponent>
Expand Down
Loading