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

Return UserRelation instead of UserCompact for friends API #7768

Closed
wants to merge 3 commits into from

Conversation

cl8n
Copy link
Member

@cl8n cl8n commented Jun 21, 2021

resolves #7637

@nanaya
Copy link
Collaborator

nanaya commented Jun 21, 2021

It looks like this will need to be coordinated with lazer.

@cl8n
Copy link
Member Author

cl8n commented Jun 21, 2021

yep. I'll wait for review on this first to get the API decided

I also removed the statistics include from the users in this response because I don't think it made sense to use the auth'd user's default mode (friends.read doesn't imply knowledge of that)... but lazer does use that info currently. should I add it back and require identify scope too or something?

@peppy peppy added the type:api label Jun 23, 2021
@peppy
Copy link
Member

peppy commented Jun 23, 2021

Fine to go ahead with this at your discretion (assuming this seems like a sane change - I think it does but haven't looked into it with too much thought).

Will coordinate the lazer side changes if/when this is merged.

if (is_api_request()) {
return $usersJson;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

About statistics; identify scope is always implied. If anything, the check should be at default_mode function instead.

Comment on lines +117 to +121
'target',
'target.country',
'target.cover',
'target.groups',
'target.support_level',
Copy link
Collaborator

Choose a reason for hiding this comment

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

static $userIncludes = [ ... ];

if (is_api_request()) {
  return json_collection(..., ..., [
    "target:mode({$currentMode})",
    ...array_map(..., $userIncludes),
  ]);
}

Comment on lines +110 to +112
->relations()
->friends()
->withMutual()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make its own relation and load that instead.

// in User model
public function relationFriends()
{
  return $this->relations()->friends()->withMutual();
}

// in this controller action
// the loaded reloaded data will be used regardless (in json for api, in current user default json for html)
$relations = $currentUser->relationFriends;
$relations->load(['target' => fn ($q) => $q->...]);

if (is_api...) {
    ...
}

$user = $relations->pluck('target');

// in Transformers etc
$user->relationFriends

@peppy
Copy link
Member

peppy commented Jul 20, 2021

@cl8n still willing to apply the review to this one?

@cl8n cl8n marked this pull request as draft May 21, 2024 04:12
@nanaya
Copy link
Collaborator

nanaya commented Oct 24, 2024

The main thing has been merged through a different PR. Feel free to open another PR for the documentation (or maybe wait for the documentation system update PR (#11433) first?).

@nanaya nanaya closed this Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change /api/v2/friends to return collection of UserRelation
3 participants