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

fix(profile): fix the wrong follows you badge #311

Merged
merged 3 commits into from Sep 11, 2017

Conversation

chinesedfan
Copy link
Member

Closed #308

Copy link
Member

@andrewda andrewda left a comment

Choose a reason for hiding this comment

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

👌

@machour
Copy link
Member

machour commented Sep 8, 2017

This partially fixes the problem. Now, a user following me isn't shown as so :/

For example, the user tunecino is one of my followers, but its profile only shows that I'm following him.

@machour
Copy link
Member

machour commented Sep 8, 2017

Guys, after some analysis, here's what I've found:

We should either take the list of users X is following and compare them towards the authenticated user, or maybe use another lighter API call:

GH Api allows the following call:
https://api.github.com/users/tunecino/following/machour

If tunecino is following machour, the request will just respond with a 204 status code and no contents.
If tunecino is not following machour, the request will respond with a 404 status code

🍺  ~/gitpoint-clean (master)*$ wget https://api.github.com/users/tunecino/following/machour
--2017-09-08 14:50:05--  https://api.github.com/users/tunecino/following/machour
Resolving api.github.com... 192.30.253.117, 192.30.253.116
Connecting to api.github.com|192.30.253.117|:443... connected.
HTTP request sent, awaiting response... **204 No Content**
2017-09-08 14:50:07 (0.00 B/s) - ‘machour’ saved [0]

🍺  ~/gitpoint-clean (master)*$ wget https://api.github.com/users/tunecino/following/chinesedfan
--2017-09-08 14:50:13--  https://api.github.com/users/tunecino/following/chinesedfan
Resolving api.github.com... 192.30.253.117, 192.30.253.116
Connecting to api.github.com|192.30.253.117|:443... connected.
HTTP request sent, awaiting response... **404 Not Found**
2017-09-08 14:50:14 ERROR 404: Not Found.

Could you please review my assertions? My brain may have been scrambled by following/followers/isFollowing :) (maybe we do need to rename variables to something like isFollowingMe, .. )

@chinesedfan
Copy link
Member Author

@machour I agree with you.

The current method is not 100% right even if we fixed those bugs. Because obtained followers are just the first page, whose size is 100. I will try the specified API to check whether A is following B.

@chinesedfan
Copy link
Member Author

@machour Hope I really solved the problem this time. Renaming variables needs to modify too many codes, so I give up. But I add 2 comments in src/user/user.reducer.js to clarify differences of them: https://github.com/chinesedfan/git-point/blob/issue308/src/user/user.reducer.js#L18.

@lex111 @andrewda Please review again.

@machour
Copy link
Member

machour commented Sep 10, 2017

Nice work @chinesedfan, everything looks fine with my tests 👌

Copy link
Member

@housseindjirdeh housseindjirdeh left a comment

Choose a reason for hiding this comment

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

Maan I can't thank you folks enough. Thank you @chinesedfan for diving into this and thank you @machour for spotting this bug and testing this and thank you @lex111 and @andrewda for reviewing this 🙌

Looks solid all around in my opinion!

@housseindjirdeh housseindjirdeh merged commit 5c1cc6a into gitpoint:master Sep 11, 2017
@chinesedfan chinesedfan deleted the issue308 branch September 11, 2017 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants