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

Fix public thumbnails #22026

Merged
merged 2 commits into from Apr 24, 2018
Merged

Fix public thumbnails #22026

merged 2 commits into from Apr 24, 2018

Conversation

islemaster
Copy link
Contributor

I broke it.

screen shot 2018-04-24 at 10 56 52 am

When we enabled image moderation yesterday we started getting empty thumbnails in the public gallery.

Root cause: We pass an IO instance to the ImageModeration module, and we later use that IO as the response body. When ImageModeration reads the IO object its read cursor gets advanced to the end of the file, and subsequent reads give an empty result. The solution is to rewind the read cursor to the beginning of the file before we return the IO as the response body.

My unit tests didn't catch this because they stub ImageModeration.rate_image and therefore in tests the read cursor wasn't advanced the way it is in production code. I've expanded one of the test cases to simulate this read behavior.

Ensures IO stream pointer is at the beginning of the file before using it in the response.

Thumbnails for App Lab and Game Lab are coming back blank, because we pass an IO object to the image moderation component (it seemed preferable to operate on streams whenever possible) and it doesn't rewind the stream pointer so the stream we return for the response is already at end-of-file.  This is solved by rewinding the stream pointer before returning it because we always want to return the whole file in the response.
Copy link
Contributor

@aoby aoby left a comment

Choose a reason for hiding this comment

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

LGTM, but it seems odd that we do this on one arbitrary test case. Can we either move this into its own test case, or apply it to all test cases, e.g. with a shared stub_rate_image(return_value) method?

file.read
# Return true so this expectation matches any arguments.
true
end
Copy link
Contributor

Choose a reason for hiding this comment

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

clever

@islemaster
Copy link
Contributor Author

In the :racy and :adult cases, the expected behavior is a response with an empty body so there's no difference in the test outcome when we do/don't simulate this behavior. That's why I only implemented this for the :everyone case where we can expect the image body in the response - it's the only test that asserts (or needs to assert) a matching response body.

@islemaster islemaster merged commit 18516d4 into staging Apr 24, 2018
@islemaster islemaster deleted the fix-public-thumbnails branch April 24, 2018 19:14
@islemaster
Copy link
Contributor Author

Merging to ensure this gets shipped today, happy to make adjustments in a follow-up PR if there's any other feedback.

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

Apologies, I thought I had approved this when I first looked at it. LGTM!

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.

None yet

3 participants