Skip to content

Conversation

noisecapella
Copy link
Contributor

@noisecapella noisecapella commented Dec 28, 2016

What are the relevant tickets?

Fixes #2213

What's this PR do?

Makes image_medium for the Profile model and the /api/v0/profiles/.../ REST API.

How should this be manually tested?

Run the migrations then verify that you see a new image_medium URL in /api/v0/profiles/<username_here>/ and that it is 128x128 pixels.

Any background context you want to provide?

@roberthouse54 What pixel dimensions should this image have?

@odlbot odlbot temporarily deployed to micromasters-ci-pr-2218 December 28, 2016 17:11 Inactive
@odlbot odlbot temporarily deployed to micromasters-ci-pr-2218 December 28, 2016 17:11 Inactive
@codecov-io
Copy link

codecov-io commented Dec 28, 2016

Current coverage is 85.59% (diff: 100%)

Merging #2218 into master will increase coverage by <.01%

@@             master      #2218   diff @@
==========================================
  Files           218        218          
  Lines         11471      11478     +7   
  Methods         975        975          
  Messages          0          0          
  Branches       1565       1565          
==========================================
+ Hits           9818       9825     +7   
+ Misses         1624       1620     -4   
- Partials         29         33     +4   

Powered by Codecov. Last update 64ec089...a50f4f9

@giocalitri giocalitri self-assigned this Dec 28, 2016
@giocalitri
Copy link
Contributor

the code looks good and works as expected.

Just a question: what happens if the image is smaller that 128px? If this is not a problem, feel free to merge. 👍

@noisecapella
Copy link
Contributor Author

If both dimensions of an image are smaller than 128, the image size will not change. The image will not be resized bigger than it is

I'm going to wait for @roberthouse54's opinion on if 128 is a good value

@noisecapella noisecapella merged commit 99608b9 into master Dec 29, 2016
@noisecapella noisecapella deleted the gs/image_medium branch December 29, 2016 15:08
@noisecapella noisecapella mentioned this pull request Jan 3, 2017
54 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants