Implement photo size parameter #147
Conversation
|
|
||
| def photo_params | ||
| params.permit(:camera, :earth_date, :rover_id).merge(sol: @rover.photos.maximum(:sol)) |
There was a problem hiding this comment.
I would leave these parameters in a photo_params method in order to keep the complexity of the index method down and to separate the responsibilities.
| validated_params = params | ||
| .permit(:rover_id, :camera, :earth_date, :size, :page, :per_page) | ||
| .merge(sol: rover.photos.maximum(:sol)) | ||
| photos = helpers.search_photos rover, validated_params |
There was a problem hiding this comment.
If you add include PhotoHelper to the top of the controller, you can remove helpers. and also the PhotoHelper:: namespace of the InvalidSizeParameter.
There was a problem hiding this comment.
I read this warning about using the include keyword. Is this something we are concerned with?
There was a problem hiding this comment.
Hm, I don't think it's relevant, because if the config/routes.rb file is configured correctly there won't be URLs that will reach any potential extra controller actions.
| @@ -1,13 +1,29 @@ | |||
| class Api::V1::PhotosController < ApplicationController | |||
| before_action :photo_params | |||
There was a problem hiding this comment.
This doesn't need to be done as a before_action. You can instead call the photo_params method where it's needed, instead of setting the @params instance variable.
| end | ||
| end | ||
|
|
||
| def resize_photo photo, params |
There was a problem hiding this comment.
I think what I would do here is delete the resize_photo method, and instead call resize_photos([photo]) in the case of the show action. What are your thoughts on that?
There was a problem hiding this comment.
I think I can make that change pretty easily. The only reason I have two different methods is because I don't expect :rover_id to exist in params in the show action. But I can just params.merge that in before calling resize_photos [photo], params.
|
|
||
| def replace_photo_suffix photo, old_suffix_length, new_suffix | ||
| new_photo = photo.clone; | ||
| new_photo.img_src = photo.img_src[0, photo.img_src.length - old_suffix_length] + new_suffix |
There was a problem hiding this comment.
This certainly works, but an alternative would be to use photo.image_src.gsub(old_suffix, new_suffix).
This pull request addresses multiple issues around the consistency and control of photo sizing (#117 and #130).
Going forward, users may pass a
sizeparameter that will specify the size of the returned photos. This is achieved by altering the suffix of eachimg_srcin thephotos_controller.rbandlatest_photos_controller.rbto correspond with the requestedsizeandrover_id. I stored the logic for resizing inphoto_helper.rbso it can be shared between both controllers.Original suffixes (what is stored in the database)
.jpgor.JPG-BR.JPG-BR.JPG_1200.jpgNew suffix outputs
small-thm.jpg-THM.JPG-THM.JPG_320.jpgmedium-br.jpg-BR.JPG-BR.JPG_800.jpglargeornil.JPG.JPG.JPG_1200.jpgfull.pngNote: Some of these suffixes are case sensitive
Other changes
paramsandroverfromphotos_controller.rbandlatest_photos_controller.rbtoPhoto.searchmethod inmodels/photo.rblatest_photos_controller.rb(Non-breaking syntax error in latest_photos_controller.rb #145)latest_photos_controller.rbphotos_controller.rbandlatest_photos_controller.rbwithsizeparameterimg_srcis sequenced inspec/factories.rb, so it does not conflict with tests involvingsizeparameterPotential problems for users
sizeparameter, photos from Opportunity and Spirit rovers will now be served in theirlargeformat; whereas they used to be served asmedium.Notes before merge
I have not tested these changes extensively with Opportunity and Spirit photos, because their scrapers are deprecated. Please test on a full development version of the API with Opportunity and Spirit photos before merging.