Introduce /api/profile_images/:username endpoint #10547

diogoosorio · 2020-10-04T08:53:58Z

What type of PR is this? (check all applicable)

Description

This PR aims to introduce the operation GET /api/profile_images/:username to the API.

I struggled a bit with what the contract for the operation should be (as this isn't a "resource" per se), I ended up keeping it simple and re-using the structure I saw throughout the rest of the API, when dealing with profile images:

{
  "type_of": "profile_image",
  "profile_image": "/uploads/organization/profile_image/be5e87e9-5b77-40ea-81e8-dd10c6d01146.png",
  "profile_image_90": "/uploads/organization/profile_image/be5e87e9-5b77-40ea-81e8-dd10c6d01146.png"
}

Having said that, I'm happy to refactor the contract to whatever the maintainers believe it to be the correct way to represent a profile image in this endpoint - I just ask for concrete example of what the response's payload should look like 😄 .

Related Tickets & Documents

#10132

QA Instructions, Screenshots, Recordings

curl "http://localhost:3000/api/profile_images/:username"

Added tests?

yes
no, because they aren't needed
no, because I need help

Added to documentation?

docs.forem.com
readme
no documentation needed

This endpoint receives either an user or organization username as input via the URL path and returns the profile image information for that user/orgnization.

rhymes

Looks great, left a few notes! :-)

rhymes · 2020-10-04T09:18:29Z

app/controllers/api/v0/profile_images_controller.rb

+module Api
+  module V0
+    class ProfileImagesController < ApiController
+      before_action :set_cache_control_headers, only: %i[show]


If you set the caching headers, you also need a way to destroy that cache, so you need the surrogate keys for the resource, which in this case it could be a user or an org.

I'm not sure it's worth to cache this endpoint though and in general I think we should lean towards caching endpoints that have either complex fetching queries or complex serialization structures.

This have neither, so I'd lean towards removing the cache altogether

Yup, didn't looked into this with much attention - giving the method a quick glance I got the idea that it was just setting a 1 day cache control header (I didn't even realize that there was a cache invalidation mechanism also put in place).

Thanks for noticing this, I've removed the action. 👏

rhymes · 2020-10-04T09:19:56Z

app/controllers/api/v0/profile_images_controller.rb

+        @user ||= User.select(:id, :profile_image)
+          .find_by(username: params[:username], registered: true)


love that you thought about the registered users. Can I suggest making it even more explicit by using the .registered scope?

Something like

User.registered.select().find_by()

rhymes · 2020-10-04T09:26:14Z

app/views/api/v0/profile_images/show.json.jbuilder

+json.type_of "profile_image"
+
+json.profile_image Images::Profile.call(@profile_image_url, length: 640)
+json.profile_image_90 Images::Profile.call(@profile_image_url, length: 90)


I read your doubts in the description but I think the serialization is simple enough to be effective, if we need more versions of the image we can add them but this closely resembles what we send back with the user or organization representations in their own partials.

I would add a key that should help the client: is this the image of a user or an org?

Maybe:

{ "type_of": "profile_image", "image_of: "user" ... }

what do you think?

Yup I tinkered a bit with the idea of embedding the owner somehow on the response:

{ ... "owner": { "type_of": "user" | "organization", ... } }

But given the use case (simply retrieving the profile image), it seemed an overkill tbh. I also tinkered with the possibility of trying to model this using OpenAPI Links, but I couldn't find a way to do so. 😄

Anyway I agree and I ended up implementing your suggestion. 👍

Adapts the response according to the review comments: 1. Removes the cache directives 1. Tweaks how the user query is being done 1. Adds a `image_of` property to the response

rhymes

Works well, thanks @diogoosorio!

mstruve

LGTM!

Looks like a merge conflict needs to be fixed but then this should be ready to go

rhymes · 2020-10-12T13:00:13Z

I fixed the conflict by upping the file version value to 0.8.2

* Documents the new /profile-images/:username API endpoint * Introduces the /api/profile_images/:username endpoint This endpoint receives either an user or organization username as input via the URL path and returns the profile image information for that user/orgnization. * Updates the /api/profile_images/:username response Adapts the response according to the review comments: 1. Removes the cache directives 1. Tweaks how the user query is being done 1. Adds a `image_of` property to the response * Bump API version Co-authored-by: rhymes <rhymes@hey.com> Co-authored-by: Molly Struve <mollylbs@gmail.com>

diogoosorio added 2 commits October 2, 2020 19:21

Documents the new /profile-images/:username API endpoint

af8aff7

Introduces the /api/profile_images/:username endpoint

aaaa082

This endpoint receives either an user or organization username as input via the URL path and returns the profile image information for that user/orgnization.

pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Oct 4, 2020

diogoosorio changed the title ~~Diogoosorio/profile images api endpoint 10132~~ Introduce /api/profile_images/:username endpoint Oct 4, 2020

diogoosorio marked this pull request as ready for review October 4, 2020 09:09

diogoosorio requested a review from jacobherrington as a code owner October 4, 2020 09:09

diogoosorio requested a review from a team October 4, 2020 09:09

diogoosorio requested a review from a team as a code owner October 4, 2020 09:09

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Oct 4, 2020

rhymes reviewed Oct 4, 2020

View reviewed changes

Updates the /api/profile_images/:username response

c873558

Adapts the response according to the review comments: 1. Removes the cache directives 1. Tweaks how the user query is being done 1. Adds a `image_of` property to the response

rhymes approved these changes Oct 4, 2020

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 4, 2020

mstruve approved these changes Oct 12, 2020

View reviewed changes

Merge branch 'master' into diogoosorio/profile-images-api-endpoint-10132

6a1522e

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 12, 2020

mstruve added 2 commits October 14, 2020 13:22

Merge branch 'master' into diogoosorio/profile-images-api-endpoint-10132

6a6e26a

Bump API version

3339f80

mstruve merged commit 2b72a35 into forem:master Oct 14, 2020

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: unreviewed bot applied label for PR's with no review labels Oct 14, 2020

Introduce /api/profile_images/:username endpoint #10547

Introduce /api/profile_images/:username endpoint #10547

diogoosorio commented Oct 4, 2020

rhymes left a comment •

edited

rhymes Oct 4, 2020

diogoosorio Oct 4, 2020

rhymes Oct 4, 2020

rhymes Oct 4, 2020

diogoosorio Oct 4, 2020

rhymes left a comment

mstruve left a comment •

edited

rhymes commented Oct 12, 2020

		@user \|\|= User.select(:id, :profile_image)
		.find_by(username: params[:username], registered: true)

Introduce /api/profile_images/:username endpoint #10547

Introduce /api/profile_images/:username endpoint #10547

Conversation

diogoosorio commented Oct 4, 2020

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Added tests?

Added to documentation?

rhymes left a comment • edited

Choose a reason for hiding this comment

rhymes Oct 4, 2020

Choose a reason for hiding this comment

diogoosorio Oct 4, 2020

Choose a reason for hiding this comment

rhymes Oct 4, 2020

Choose a reason for hiding this comment

rhymes Oct 4, 2020

Choose a reason for hiding this comment

diogoosorio Oct 4, 2020

Choose a reason for hiding this comment

rhymes left a comment

Choose a reason for hiding this comment

mstruve left a comment • edited

Choose a reason for hiding this comment

rhymes commented Oct 12, 2020

rhymes left a comment •

edited

mstruve left a comment •

edited