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

Update webapp example #324

Closed
marioortizmanero opened this issue Jun 11, 2022 · 10 comments
Closed

Update webapp example #324

marioortizmanero opened this issue Jun 11, 2022 · 10 comments
Labels
enhancement New feature or request good first issue Good for newcomers Stale

Comments

@marioortizmanero
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

The webapp example could definitely be improved.

Describe the solution you'd like

  • Add token refreshing
  • Use the new API properly (at some point we do spotify.token = ..., we can surely avoid that).
  • Proper error handling? It has too many unwrap in my opinion.
  • Add string interpolation where possible
  • The Rocket framework isn't that active, unfortunately. It would surely be more helpful if it used Actix instead.
  • Include it in the CI somehow so that this doesn't happen anymore.

Describe alternatives you've considered

I've fixed a couple things in #305, but it was out of scope.

@marioortizmanero marioortizmanero added enhancement New feature or request good first issue Good for newcomers labels Jun 11, 2022
@ramsayleung
Copy link
Owner

ramsayleung commented Jun 13, 2022

I totally agree with your point, the webapp example is a little stale, we should refactor this example with the new API, to make it polished and robust.

Except for this point:

  • The Rocket framework isn't that active, unfortunately. It would surely be more helpful if it used Actix instead.

I am not sure if we could be beneficial from switching to Actix. Comparing the recent PR history of Rocket with Actix, I think there are roughly active.

And we should start to refactor webapp example after #305 merged, and honestly speaking, #305 goes a little far so that I need times to get involved in.

@marioortizmanero
Copy link
Collaborator Author

The Rocket framework isn't that active, unfortunately. It would surely be more helpful if it used Actix instead.

Fair enough. I was worried because v0.5.0 hasn't been released yet, but it does seem like the repo is active.

And we should start to refactor webapp example after #305 merged, and honestly speaking, #305 goes a little far so that I need times to get involved in.

Sorry, I didn't quite understand that. What do you mean?

@ramsayleung
Copy link
Owner

Sorry, I didn't quite understand that. What do you mean?

Never mind, I just think the work of refactoring the webapp example should wait until #305 get merged.

@graves501
Copy link

Since #305 already got merged, I guess the webapp is ready for some polish?

I tried out the webapp example and I noticed that I am not able to fetch the top artists...

#[get("/topartists")]
fn top_artists(cookies: Cookies) -> AppResponse {
    let mut spotify = init_spotify(&cookies);
    if !spotify.config.cache_path.exists() {
        return AppResponse::Redirect(Redirect::to("/"));
    }

    let token = spotify.read_token_cache(false).unwrap();
    spotify.token = Arc::new(Mutex::new(token));

    let top_artists = spotify
        .current_user_top_artists(Some(&TimeRange::LongTerm))
        .take(10)
        .filter_map(Result::ok)
        .collect::<Vec<_>>();

    if top_artists.is_empty() {
        return AppResponse::Redirect(Redirect::to("/"));
    }

    AppResponse::Json(json!(top_artists))
}

top_artists is always empty. I'm still new to Rust and especially new to this library. So I guess I'm missing something? Or is that a general issue?

@ramsayleung
Copy link
Owner

Probably you could reproduce this problem with Spotify's Web Console, I guess it's something wrong with your data, e.g. your top artists.

https://developer.spotify.com/console/get-current-user-top-artists-and-tracks/

@graves501
Copy link

Thanks for the hint! I successfully tried out the developer console, so my data seems to be fine.

I also tried spotify.current_user_top_artists(Some(&TimeRange::MediumTerm)) but the top_artists would still be empty. Another thing I tried is to use release version 0.11.0 of rspotify, but that also didn't salvage the problem.

@ramsayleung
Copy link
Owner

I can reproduce this issue, the problem is that the default scopes in example are "user-read-currently-playing", "playlist-modify-private", the required scopes to get user's top artists are user-top-read.

https://github.com/ramsayleung/rspotify/blob/master/examples/webapp/src/main.rs#L90

You should add user-top-read to the scopes:

    // Please notice that protocol of redirect_uri, make sure it's http
    // (or https). It will fail if you mix them up.
    let oauth = OAuth {
        scopes: scopes!(
            "user-read-currently-playing",
            "playlist-modify-private",
            "user-top-read"
        ),
        redirect_uri: "http://localhost:8000/callback".to_owned(),
        ..Default::default()
    };

it works for me now.

@ramsayleung ramsayleung mentioned this issue Sep 20, 2022
6 tasks
@graves501
Copy link

Thanks for pointing out the scope issue! It works for me now!

@marioortizmanero
Copy link
Collaborator Author

Yeah, I guess that @graves501 ran into that issue because we:

  1. Turn the Result into Option with Result::ok
  2. Filter out the None entries with filter_map

This is something we could improve as well; otherwise we are ignoring possibly important errors.

@github-actions
Copy link

Message to comment on stale issues. If none provided, will not mark issues stale

@github-actions github-actions bot added the Stale label Jun 26, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers Stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants