Skip to content

Conversation

@xwsg
Copy link

@xwsg xwsg commented Feb 10, 2017

parameters.add(WeChatConstants.STATE, state);
}

return parameters.appendTo(getAuthorizationBaseUrl());
Copy link
Member

Choose a reason for hiding this comment

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

pity. for single param name...
Maybe let it be like this:

final ParameterList parameters = new ParameterList(additionalParams);
parameters.add(WeChatConstants.CLIENT_ID, config.getApiKey());
return parameters.appendTo(super.getAuthorizationUrl(config, additionalParams));

or even

if (additionalParams==null) {
  additionalParams = Collections.singletonMap(WeChatConstants.CLIENT_ID, config.getApiKey());
} else {
  additionalParams.put(WeChatConstants.CLIENT_ID, config.getApiKey());
}
return super.getAuthorizationUrl(config, additionalParams);

request.addParameter(WeChatConstants.CODE, code);
request.addParameter(WeChatConstants.GRANT_TYPE, WeChatConstants.AUTHORIZATION_CODE);

return request;
Copy link
Member

Choose a reason for hiding this comment

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

the same here.
final OAuthRequest request = super.createAccessTokenRequest(code);
request.addParameter...
return request;


public WeChatToken(String accessToken, String openId, String unionId, String rawResponse) {
this(accessToken, null, null, null, null, openId, unionId, rawResponse);
}
Copy link
Member

Choose a reason for hiding this comment

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

Seems we don't need this constructor. Constructor in GoogleToken as well.

@kullfar
Copy link
Member

kullfar commented Feb 10, 2017

Hi,
your pull request looks good. The only things I can't like are createAccessTokenRequest and getAuthorizationUrl methods overriden. But seems there is no better way to do this for the moment. So it's up to you. We can leave them as it is now. Not used Constructor in Token as well. We can leave it.
Add the line to the README.md with new API added please.

And I need test user and app credentials to run the example before merging to the master.

ps.Thanks!

@xwsg
Copy link
Author

xwsg commented Feb 10, 2017

@kullfar
Thanks. WeChat has some differences from other APIs.
Test user and app credentials has been sent to your email ([email protected]).

@kullfar
Copy link
Member

kullfar commented Mar 9, 2017

Provided credentials aren't working (maybe they are outdated already?)
I'm waiting for the new working test credentials to run the example and merge it to the master.

@kullfar
Copy link
Member

kullfar commented Apr 10, 2017

There are problems with test user credentials. I have tried some times to register. No success. WeChat didn't send me the sms.
I still need test user credentials to run the example to merge pull request to the master.

@xwsg
Copy link
Author

xwsg commented Apr 11, 2017

Another test user account has been sent to you email, please try again.

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