RetroAchievements
feat(api): Add endpoint for User's Want to Play Games List
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.
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.
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?
The only thing I'm waiting on before doing a final review pass is outstanding feedback to be addressed in https://github.com/RetroAchievements/api-docs/pull/52.
The only thing I'm waiting on before doing a final review pass is outstanding feedback to be addressed in RetroAchievements/api-docs#52.
Fixed, still not sure why it was showing any diff between what appeared to be the same
Approved - I have no reservations with this implementation.
The only thing I think we should consider is whether or not there are any privacy concerns with this, as currently on the website the data is not publicly exposed. I don't think this data is particularly sensitive so it isn't something I'm worried about, but it feels worth noting.
Before merging, I'll give enough time so that any other maintainers can weigh in if they have an opinion regarding this.
The only thing I think we should consider is whether or not there are any privacy concerns with this, as currently on the website the data is not publicly exposed.
Set requests are public, so I don't see any reason why want to play lists couldn't also be public. From an API perspective, I totally understand the desire to have that information in a client, but should it be limited to the user making the call? In my opinion, it's unlikely that a user would ever ask for the want to play list of another user. What would they do with it?
I know there have been several discussions around the want to dev list. Some people want to share their list so others can potentially collaborate, whereas others specifically don't want the list public so people don't pester them about actually starting development. The compromise was to make the list for a game visible to the claimant so they can seek collaborators after they claim it. People from the first group still want their list public as a user might not claim a game unless they had already arranged the collaboration.
should it be limited to the user making the call? In my opinion, it's unlikely that a user would ever ask for the want to play list of another user. What would they do with it?
I think this is a great point. I'm in favor of this endpoint returning a 401 if the caller is requesting a list that isn't their own.
** But this isn't a terribly strong opinion, and not one I'd hold the PR up for, for the same reason you've stated:
it's unlikely that a user would ever ask for the want to play list of another user. What would they do with it?
The only thing I can imagine where this sort of situation would come up is an app/integration reusing the same API key across multiple users, or an app that is using some form of connection pooling to bypass our rate limit.
Would this be achieved by checking username vs supplied API key?
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 don't feel super strongly about that idea and not sure what the implementation would look like on the API side, but I could see a use case for it as an end user.
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.
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 (seeUserGameListType-- this should probably be renamed toUserGameListEntryType).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 https://github.com/RetroAchievements/RAWeb/pull/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.
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.
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.
@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.