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

Creating a LocalImageView, so that front ends have the Person struct. #4631

Merged
merged 5 commits into from
Apr 16, 2024

Conversation

dessalines
Copy link
Member

@dessalines dessalines commented Apr 16, 2024

The local_user_id wasn't enough for lemmy-ui to show the person listing, and link to the uploader.

The linked lemmy-ui PR below has a video of how this looks in action.

@Nutomic
Copy link
Member

Nutomic commented Apr 16, 2024

This should only be necessary for the admin endpoint list_all_media. For normal users with list_media, everything is uploaded by the own user which is already returned by /api/v3/site.

@dessalines
Copy link
Member Author

I'd rather not add two separate media types returned, especially since the front end uses a component that works for both, and uses the same type.

@SleeplessOne1917 suggested that I could remove the local_user: LocalUser field tho, as it probably isn't necessary.

@dessalines dessalines marked this pull request as draft April 16, 2024 12:49
@dessalines dessalines marked this pull request as ready for review April 16, 2024 12:52
@@ -71,7 +71,7 @@ test("Upload image and delete it", async () => {

// The deleteUrl is a combination of the endpoint, delete token, and alias
let firstImage = listMediaRes.images[0];
let deleteUrl = `${alphaUrl}/pictrs/image/delete/${firstImage.pictrs_delete_token}/${firstImage.pictrs_alias}`;
let deleteUrl = `${alphaUrl}/pictrs/image/delete/${firstImage.local_image.pictrs_delete_token}/${firstImage.local_image.pictrs_alias}`;
Copy link
Member

Choose a reason for hiding this comment

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

Add some asserts here to ensure the correct person is returned. Otherwise looks fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

K done.

@dessalines dessalines enabled auto-merge (squash) April 16, 2024 13:14
@dessalines
Copy link
Member Author

pnpm just had a release a few hours ago that's causing the CI error. I'll fix it by upgrading pnpm soon.

@dessalines dessalines merged commit 6efab9a into main Apr 16, 2024
2 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