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

🧑‍💻 Replace user details by LightUserResource in LeaderboardUserResource #2635

Merged
merged 17 commits into from
Jul 3, 2024

Conversation

marhei
Copy link
Contributor

@marhei marhei commented Jun 1, 2024

To provide the same basic informations across all user resources

@marhei marhei changed the title Added user id and display name to LeaderboardUserResource Added user id and display name to LeaderboardUserResource Jun 1, 2024
@marhei
Copy link
Contributor Author

marhei commented Jun 1, 2024

Maybe some also needs to recreate the API docs, I don't know how this works…

storage/api-docs/api-docs.json Outdated Show resolved Hide resolved
Comment on lines 24 to 27
'id' => (int) $this->user->id,
'displayName' => (string) $this->user->name,
'username' => (string) $this->user->username,
'profilePicture' => ProfilePictureController::getUrl($this->user),
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can include the LightUserResource here. This reduced the amount of different resources with similar attributes.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer my proposed implementation because it fits better into my existing implementation of the user model.

@MrKrisKrisu MrKrisKrisu changed the title Added user id and display name to LeaderboardUserResource 🧑‍💻 add user id and display name to LeaderboardUserResource Jun 2, 2024
@marhei marhei requested a review from MrKrisKrisu June 6, 2024 10:17
@marhei
Copy link
Contributor Author

marhei commented Jun 28, 2024

@MrKrisKrisu As you wanted: LeaderboardUserResource returns now a LightUserResource

@marhei marhei changed the title 🧑‍💻 add user id and display name to LeaderboardUserResource 🧑‍💻 Replace user details by LightUserResource in LeaderboardUserResource Jun 28, 2024
@marhei marhei changed the title 🧑‍💻 Replace user details by LightUserResource in LeaderboardUserResource 🧑‍💻 Replace user details by LightUserResource in LeaderboardUserResource Jun 28, 2024
Copy link
Member

@MrKrisKrisu MrKrisKrisu left a comment

Choose a reason for hiding this comment

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

Thanks for the change. But maybe we should get a third opinion before we just blindly change something just because I like it more?

(@HerrLevin?)

@marhei
Copy link
Contributor Author

marhei commented Jul 2, 2024

@HerrLevin May I be merged? 🥺👉👈

config/l5-swagger.php Outdated Show resolved Hide resolved
storage/api-docs/api-docs.json Outdated Show resolved Hide resolved
@HerrLevin
Copy link
Member

@marhei I know where you're coming from. From a client pov I would prefer the original user pov. But I know our code and our database... This would most likely impact our performance (which we don't have :D) severely 🥴

I'm fine with the PR as it is currently, if you revert the two code occurences I've mentioned in my review. /cc @MrKrisKrisu

@marhei
Copy link
Contributor Author

marhei commented Jul 2, 2024

I removed all unwanted changes, thanks for the review!

@HerrLevin HerrLevin merged commit 81e2708 into Traewelling:develop Jul 3, 2024
4 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