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

feat(api): add endpoint for user's Want to Play Games list #2549

Conversation

ioslife
Copy link
Contributor

@ioslife ioslife commented Jul 8, 2024

Trying my hand at adding an endpoint for getting a user's backlog/want to play list.

This is a draft, would like to get Wes' opinion on if I am going down the right road before I get any further into this.

@ioslife
Copy link
Contributor Author

ioslife commented Jul 9, 2024

Helpers are being deprecated, so going to go a different route.

@wescopeland
Copy link
Member

My summarized feedback, just for public visibility:

  • We're in the process of deprecating all helpers, so let's not add a new one.
  • We should either reuse (or add reusable) model attributes, or keep the logic strictly in-line within the API endpoint file itself.
  • The endpoint needs to be comprehensively tested and documented, both here in RAWeb and in api-docs.
  • There is a user in prod who apparently has over 8000 games on their want to play list -- we should likely paginate this endpoint.
  • We can't review PRs that are untested - please do not rely on CI alone to verify the code. Verify locally before the code hits CI.

@ioslife
Copy link
Contributor Author

ioslife commented Jul 9, 2024

api-docs PR: RetroAchievements/api-docs#52

@ioslife ioslife changed the title DRAFT: feat(api): Add endpoint for User's Want to Play Games List feat(api): Add endpoint for User's Want to Play Games List Jul 9, 2024
@ioslife ioslife marked this pull request as ready for review July 9, 2024 20:37
@ioslife
Copy link
Contributor Author

ioslife commented Jul 9, 2024

the phpunit step failed for something I haven't touched and haven't seen fail before. Is this something I need to fix for this PR?

@wescopeland
Copy link
Member

Would this be achieved by checking username vs supplied API key?

This sounds like authorization to me, which leads me to believe it should be achieved via an ability in a UserGameListEntryPolicy. There may be some nuance here as the model supports three different types of lists (see UserGameListType -- this should probably be renamed to UserGameListEntryType).

The only thing I'd like to challenge with this is maybe allowing users that follow each other the ability to see each other's list.
i.e. if I follow wes on the site and wes follows me, we could both see each other's list.

I think I'd be fine with this as long as the authorization is enforced solely by the policy mentioned above.

@ioslife
Copy link
Contributor Author

ioslife commented Jul 22, 2024

Would this be achieved by checking username vs supplied API key?

This sounds like authorization to me, which leads me to believe it should be achieved via an ability in a UserGameListEntryPolicy. There may be some nuance here as the model supports three different types of lists (see UserGameListType -- this should probably be renamed to UserGameListEntryType).

The only thing I'd like to challenge with this is maybe allowing users that follow each other the ability to see each other's list.
i.e. if I follow wes on the site and wes follows me, we could both see each other's list.

I think I'd be fine with this as long as the authorization is enforced solely by the policy mentioned above.

I'll have to look more into policies as that is definitely out of my realm of knowledge.

This puts this PR on hold along with my PR for GetComments #2552

If anyone has some recommended reading for how to go about this, or would be interested in pairing with me to walk through how these work, let me know.

@wescopeland wescopeland dismissed their stale review July 28, 2024 12:50

Dismissing as latest feedback is worked through

@ioslife ioslife marked this pull request as draft July 29, 2024 14:48
@ioslife
Copy link
Contributor Author

ioslife commented Jul 29, 2024

I got the policy and friend relationship working as expected. There is probably some optimization that can be done, but will get to that later.

I am trying to get the tests to work as expected, but using this->actingAs($user) is not respecting using the call as $user.

@ioslife
Copy link
Contributor Author

ioslife commented Jul 29, 2024

Resolved the issue with actingAs($user) by no longer using BootstrapApiV1 for setup. This was automatically setting a user as the one to use the webAPIKey which caused issues with how we were setting the user.

@ioslife ioslife marked this pull request as ready for review July 29, 2024 15:59
@ioslife
Copy link
Contributor Author

ioslife commented Jul 29, 2024

@Jamiras based on your feedback I have added a policy for only allowing users to see their own want to play list, with one exception. If two users are have a mutual following relationship (are friends), they are allowed to see the list.

app/Models/User.php Outdated Show resolved Hide resolved
app/Policies/UserGameListEntryPolicy.php Outdated Show resolved Hide resolved
app/Policies/UserGameListEntryPolicy.php Outdated Show resolved Hide resolved
app/Policies/UserGameListEntryPolicy.php Outdated Show resolved Hide resolved
app/Models/User.php Outdated Show resolved Hide resolved
Copy link
Member

@wescopeland wescopeland left a comment

Choose a reason for hiding this comment

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

I only see one other thing in the current implementation that is worth commenting on. I'll do a final round of functional testing later tonight or tomorrow evening.

app/Community/Concerns/ActsAsCommunityMember.php Outdated Show resolved Hide resolved
Co-authored-by: Wes Copeland <wlcopeland1@gmail.com>
@wescopeland wescopeland changed the title feat(api): Add endpoint for User's Want to Play Games List feat(api): add endpoint for user's Want to Play Games list Aug 2, 2024
@wescopeland wescopeland merged commit ab930cc into RetroAchievements:master Aug 2, 2024
5 checks passed
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.

3 participants