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 refresh token support and improved users.recentlyPlayed() (fixes #30) #31

Closed
wants to merge 1 commit into from

Conversation

nrubin29
Copy link
Contributor

This commit adds the ability for the user to specify a refresh token to SpotifyApiCredentials. If a refresh token is specified, it is used by _refreshToken().

This commit also improves the users.recentlyPlayed() function to allow for specifying the query parameters limit, after, and before, and it now returns a Future<Iterable<PlayHistory>> which captures the played_at timestamp in addition to the track.

@nrubin29
Copy link
Contributor Author

Thinking about it now, perhaps it doesn't make sense to store the refresh token in SpotifyApiCredentials. For my particular use case, I'm going to pull a user's refresh token out of a database and then make some API calls on their behalf. With the way I implemented this PR, I'd need to instantiate a new SpotifyAPI for each user. I'm not sure of how else to do it, because I feel like adding a variable to SpotifyAPI would be hacky. Thoughts?

@nrubin29
Copy link
Contributor Author

Looks like #28 should probably be merged, and then this PR can just be about users.recentlyPlayed().

@@ -99,5 +99,27 @@ main() async {

var relatedArtists = await spotify.artists.relatedArtists('0OdUWJ0sBjDrqHygGUXeCF');
print('related Artists: ${relatedArtists.length}');

Copy link
Owner

Choose a reason for hiding this comment

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

Please remove that change from the PR, since it's coming in with #28 😸

@@ -52,7 +52,15 @@ abstract class SpotifyApiBase {
Future<Null> _refreshToken() async {
if (_apiToken == null || _apiToken.isExpired) {
var headers = {'Authorization': 'Basic ${_credentials.basicAuth}'};
var body = {'grant_type': 'client_credentials'};
var body;
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove that change from the PR, since it's coming in with #28 😸


SpotifyApiCredentials(this.clientId, this.clientSecret);
SpotifyApiCredentials(this.clientId, this.clientSecret, [this.refreshToken = null]);
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove that change from the PR, since it's coming in with #28 😸

throw new Exception('Cannot specify both after and before.');
}

var queryParams = [];
Copy link
Owner

Choose a reason for hiding this comment

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

Please use our existing "tools" for generating the query. We have a _buildQuery(...) method in the base class, which takes a Map and produces the query string. It also filters out keys where the values are null. For usage you can check here.
Thanks!

@rinukkusu
Copy link
Owner

Thinking about it now, perhaps it doesn't make sense to store the refresh token in SpotifyApiCredentials. For my particular use case, I'm going to pull a user's refresh token out of a database and then make some API calls on their behalf. With the way I implemented this PR, I'd need to instantiate a new SpotifyAPI for each user. I'm not sure of how else to do it, because I feel like adding a variable to SpotifyAPI would be hacky. Thoughts?

Right, at the moment an instance always belongs to a specific set of credentials. What's your use case? Do you run through many users and have to instantiate a lot of SpotifyApi instances?

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