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

Allow commenting on Gistlog without leaving UI #78

Merged

Conversation

JacobBennett
Copy link
Contributor

fixes Issue #7
requires PR #77

This adds user creation and login through socialite with the purpose of allowing users to comment on a Gistlog post without having to leave the post itself.

@@ -53,4 +55,11 @@ public function show($username, $gistId)
->with('gistlog', $gistlog)
->with('pageTitle', $gistlog->title . ' | ' . $gistlog->author);
}

public function postComment(Request $request, GistClient $client, $gistId)

Choose a reason for hiding this comment

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

Expected 1 space between comma and argument $gistId; 2 found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@JacobBennett
Copy link
Contributor Author

@mattstauffer is the convention on these actually going to be spaces vs tabs? I can correct this if needed, only problem is that the entire code base is currently using tabs for indentation. In fact, I had my code using spaces and changed it to tabs to match the indentations.

@mattstauffer
Copy link
Member

@JacobBennett We'll switch to spaces once all the PRs are merged. :) No worries, there's just no way at the moment to teach nitpick to chill out on the space sniffing. :)

use Illuminate\Contracts\Auth\Guard;
use Illuminate\Contracts\Auth\Registrar;
use Gistlog\User;
use Exception;
Copy link
Member

Choose a reason for hiding this comment

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

alphabetize use block plz

text-align: right;
transition: margin 0.25s;

&.active{
Copy link
Member

Choose a reason for hiding this comment

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

space after active

@mattstauffer
Copy link
Member

@JacobBennett Tried it out; looks good! Could you make these changes and do a merge resolution and then ping me? I need to run it one more time, but I'm out of time now, so I'm going to use asking you to make a few changes as the tool to give me more time until I can actually check it out one more time when you push the updated code. :) Thanks man!

@JacobBennett
Copy link
Contributor Author

@mattstauffer this is ready to merge in with master 👍

@mattstauffer
Copy link
Member

Hey @JacobBennett, the UI looks great, as does the code.

I just tried to run this and got an authentication issue; have you run into this? I regenerated my token and key and secret for the .env file, and still am getting the same thing. I checked out the scope and I see you have gist there, which I assume is enough?

I'm getting this:

Whoops, looks like something went wrong.

RuntimeException in HttpClient.php line 145:
Bad credentials
in HttpClient.php line 145
at HttpClient->request('gists/54b61b7a14c8761131df/comments', '{"body":"This is great!"}', 'POST', array(), array()) in CachedHttpClient.php line 62
at CachedHttpClient->request('gists/54b61b7a14c8761131df/comments', '{"body":"This is great!"}', 'POST', array()) in HttpClient.php line 104
at HttpClient->post('gists/54b61b7a14c8761131df/comments', '{"body":"This is great!"}') in GistClient.php line 55
at GistClient->postGistComment('54b61b7a14c8761131df', 'This is great!') in GistsController.php line 62
at GistsController->postComment(object(Request), object(GistClient), '54b61b7a14c8761131df')
at call_user_func_array(array(object(GistsController), 'postComment'), array(object(Request), object(GistClient), 'gistId' => '54b61b7a14c8761131df')) in Controller.php line 256

In meetings and giving this only 40% of my brain right now, so I'll try to look at it more tonight, but would love to hear if you have run into this before.

@JacobBennett
Copy link
Contributor Author

@mattstauffer I do remember having this issue at one point but don't remember how I solved it. I'll dig into it a bit and get back at you when I have a good solution and explanation for you.

@michaeldyrynda
Copy link

@mattstauffer it looks like this is due to trying to authenticate with GitHub when the application asks for a GistClient instance.

When the service provider (GistClientServiceProvider, AuthorClientServiceProvider) returns the GistClient singleton, if services.github.client_id and services.github.client_secret are set - which they now are in order to hook in to Socialite - it attempts to authenticate with the services.github.token, which is likely unset.

I assume that the work around @JacobBennett use was to either create a personal access token, or manually execute the auth flow to get a token. This token is then used for any requests by gistlog to GitHub's API.

Socialite needs the services.github.client_id and services.github.client_secret of our app (gistlog) in order for users to authorise it to post comments. That said, I reckon if we just replace the current checks in each service provider:

if (config('services.github.client_id') && config('services.github.client_secret')) {

with

if (config('services.github.token')) {

Gistlog can continue to use it's token (i.e. host my own instance of Gistlog and create a token for it) for the 'public' requests, but use the authorised user's token when posting comments.

@JacobBennett
Copy link
Contributor Author

@deringer I can only hope to someday be as wise as you! Thank you for taking the time to diagnose this! You are correct that I in fact generated a personal token to use in my local repo. I can make the changes you described above in the service providers and commit those to this branch. It makes a whole bunch of sense now that you have laid it out.

@mattstauffer agreed?

@michaeldyrynda
Copy link

Thanks man! It was only because I made some changes to the codebase over the weekend that I had noticed the issue - it's only a minor detail, but from my brief testing here, appears to be the cause of the problems :)

@JacobBennett
Copy link
Contributor Author

@mattstauffer think this guy is ready to pull in. Did some local testing after the most recent changes and think we should be all set. Thanks!

@mattstauffer
Copy link
Member

@JacobBennett Great, will pull down and try again. Thanks!

@mattstauffer
Copy link
Member

Here goes nothing!

mattstauffer added a commit that referenced this pull request Sep 27, 2015
…om-gistlog

Allow commenting on Gistlog without leaving UI
@mattstauffer mattstauffer merged commit c6f378a into tighten:master Sep 27, 2015
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.

None yet

4 participants