-
Notifications
You must be signed in to change notification settings - Fork 49
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
Implement photo size parameter #147
base: master
Are you sure you want to change the base?
Changes from all commits
f6b7338
558bb88
819ecfa
c709393
aeea8e3
4285d09
ce4c7a2
3c3a67e
b5d22f7
31f70ae
953da53
d8e4633
7d03c01
778211a
23fc0ac
0c8cd39
18c882f
c471c42
d4eb91c
ecd8402
814fae6
69bd885
13e6e85
57117ff
32d105d
45a6e00
e9d16c8
0d76781
1508b2e
32d9080
8f196db
66c5b3b
3de4ae7
80afd8d
2543edd
ac1cdce
c7cff19
5db1913
8b8a50b
c2cf788
d94eaae
d044624
0fca483
cf164ec
fda4b56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,26 +1,21 @@ | ||
class Api::V1::LatestPhotosController < ApplicationController | ||
def index | ||
def index | ||
@rover = Rover.find_by name: params[:rover_id].titleize | ||
if @rover | ||
render json: search_photos, each_serializer: PhotoSerializer, root: :latest_photos | ||
else | ||
render json: { errors: "Invalid Rover Name" }, status: :bad_request | ||
end | ||
end | ||
end | ||
rover = Rover.find_by name: params[:rover_id].titleize | ||
|
||
private | ||
if rover | ||
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 | ||
|
||
def photo_params | ||
params.permit(:camera, :earth_date, :rover_id).merge(sol: @rover.photos.maximum(:sol)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would leave these parameters in a |
||
end | ||
|
||
def search_photos | ||
photos = @rover.photos.order(:camera_id, :id).search photo_params, @rover | ||
if params[:page] | ||
photos = photos.page(params[:page]).per params[:per_page] | ||
begin | ||
photos = helpers.resize_photos photos, validated_params | ||
render json: photos, each_serializer: PhotoSerializer, root: :latest_photos | ||
rescue PhotoHelper::InvalidSizeParameter => e | ||
render json: { errors: e.message }, status: :bad_request | ||
end | ||
else | ||
render json: { errors: "Invalid Rover Name" }, status: :bad_request | ||
end | ||
photos | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,29 @@ | ||
class Api::V1::PhotosController < ApplicationController | ||
before_action :photo_params | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't need to be done as a |
||
|
||
def show | ||
photo = Photo.find params[:id] | ||
render json: photo, serializer: PhotoSerializer, root: :photo | ||
photo = Photo.find @params[:id] | ||
|
||
begin | ||
resized_photo = helpers.resize_photo photo, @params | ||
render json: resized_photo, serializer: PhotoSerializer, root: :photo | ||
rescue StandardError => e | ||
render json: { errors: e.message }, status: :bad_request | ||
end | ||
end | ||
|
||
def index | ||
rover = Rover.find_by name: params[:rover_id].titleize | ||
rover = Rover.find_by name: @params[:rover_id].titleize | ||
|
||
if rover | ||
render json: photos(rover), each_serializer: PhotoSerializer, root: :photos | ||
photos = helpers.search_photos rover, @params | ||
|
||
begin | ||
photos = helpers.resize_photos photos, @params | ||
render json: photos, each_serializer: PhotoSerializer, root: :photos | ||
rescue PhotoHelper::InvalidSizeParameter => e | ||
render json: { errors: e.message }, status: :bad_request | ||
end | ||
else | ||
render json: { errors: "Invalid Rover Name" }, status: :bad_request | ||
end | ||
|
@@ -16,14 +32,6 @@ def index | |
private | ||
|
||
def photo_params | ||
params.permit :sol, :camera, :earth_date, :rover_id | ||
end | ||
|
||
def photos(rover) | ||
photos = rover.photos.order(:camera_id, :id).search photo_params, rover | ||
if params[:page] | ||
photos = photos.page(params[:page]).per params[:per_page] | ||
end | ||
photos | ||
@params = params.permit :id, :rover_id, :sol, :camera, :earth_date, :size, :page, :per_page | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,108 @@ | ||
module PhotoHelper | ||
|
||
def search_photos rover, params | ||
photos = rover.photos.order(:camera_id, :id).search params, rover | ||
|
||
if params[:page] | ||
photos = photos.page(params[:page]).per params[:per_page] | ||
end | ||
|
||
photos | ||
end | ||
|
||
class InvalidSizeParameter < StandardError | ||
def initialize( | ||
size, | ||
rover_name, | ||
exception_type="custom" | ||
) | ||
super("Invalid size parameter '#{size}' for #{rover_name.titleize}") | ||
end | ||
end | ||
|
||
def resize_photo photo, params | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think what I would do here is delete the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I can make that change pretty easily. The only reason I have two different methods is because I don't expect |
||
rover_name = photo.rover.name.downcase | ||
suffix_data = lookup_suffix rover_name, params[:size] | ||
|
||
if !suffix_data.nil? | ||
mtilda marked this conversation as resolved.
Show resolved
Hide resolved
|
||
replace_photo_suffix photo, suffix_data[:old_length], suffix_data[:new] | ||
else | ||
raise InvalidSizeParameter.new params[:size], rover_name | ||
end | ||
end | ||
|
||
def resize_photos photos, params | ||
rover_name = params[:rover_id].downcase | ||
suffix_data = lookup_suffix rover_name, params[:size] | ||
|
||
if suffix_data.present? | ||
replace_each_photo_suffix photos, suffix_data[:old_length], suffix_data[:new] | ||
else | ||
raise InvalidSizeParameter.new params[:size], rover_name | ||
end | ||
end | ||
|
||
private | ||
|
||
def replace_each_photo_suffix photos, old_suffix_length, new_suffix | ||
photos.map do |photo| | ||
replace_photo_suffix photo, old_suffix_length, new_suffix | ||
end | ||
end | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This certainly works, but an alternative would be to use |
||
new_photo | ||
end | ||
|
||
def lookup_suffix rover_name, size | ||
suffix_hash = { | ||
:curiosity => { | ||
:original_length => 4, # .jpg or .JPG | ||
:sizes => { | ||
:small => '-thm.jpg', | ||
:medium => '-br.jpg', | ||
:large => '.JPG' | ||
} | ||
}, | ||
:spirit => { | ||
:original_length => 7, # -BR.JPG | ||
:sizes => { | ||
:small => '-THM.JPG', | ||
:medium => '-BR.JPG', | ||
:large => '.JPG' | ||
} | ||
}, | ||
:opportunity => { | ||
:original_length => 7, # -BR.JPG | ||
:sizes => { | ||
:small => '-THM.JPG', | ||
:medium => '-BR.JPG', | ||
:large => '.JPG' | ||
} | ||
}, | ||
:perseverance => { | ||
:original_length => 9, # _1200.jpg | ||
:sizes => { | ||
:small => '_320.jpg', | ||
:medium => '_800.jpg', | ||
:large => '_1200.jpg', | ||
:full => '.png' | ||
} | ||
} | ||
} | ||
|
||
rover = rover_name.downcase.to_sym | ||
size = size.nil? ? :large : size.to_sym | ||
|
||
if suffix_hash.key?(rover) && suffix_hash[rover][:sizes].key?(size) then | ||
{ | ||
:old_length => suffix_hash[rover][:original_length], | ||
:new => suffix_hash[rover][:sizes][size] | ||
} | ||
else | ||
nil | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,145 @@ | ||
require 'rails_helper' | ||
|
||
describe Api::V1::LatestPhotosController do | ||
let(:rover) { create(:rover) } | ||
let(:camera) { create(:camera, rover: rover) } | ||
let!(:photo) { create(:photo, rover: rover, camera: camera) } | ||
|
||
suffix_hash = { | ||
'Curiosity' => { | ||
'small' => '-thm.jpg', | ||
'medium' => '-br.jpg', | ||
'large' => '.JPG' | ||
}, | ||
'Spirit' => { | ||
'small' => '-THM.JPG', | ||
'medium' => '-BR.JPG', | ||
'large' => '.JPG' | ||
}, | ||
'Opportunity' => { | ||
'small' => '-THM.JPG', | ||
'medium' => '-BR.JPG', | ||
'large' => '.JPG' | ||
}, | ||
'Perseverance' => { | ||
'small' => '_320.jpg', | ||
'medium' => '_800.jpg', | ||
'large' => '_1200.jpg', | ||
'full' => '.png' | ||
} | ||
} | ||
|
||
describe "GET 'index'" do | ||
let!(:latest_photo) { create(:photo, rover: rover, camera: camera) } | ||
before(:each) do | ||
latest_photo.update(sol: 999) | ||
end | ||
|
||
context "with rover_id" do | ||
before(:each) do | ||
get :index, params: { rover_id: rover.name.downcase } | ||
end | ||
|
||
it "returns http 200 success" do | ||
expect(response.status).to eq 200 | ||
end | ||
|
||
it "renders photo data from most recent sol" do | ||
expect(json["latest_photos"].first["sol"]).to eq 999 | ||
end | ||
end | ||
|
||
context "with bad Rover name" do | ||
before(:each) do | ||
get :index, params: { rover_id: "this doesn't exist"} | ||
end | ||
|
||
it "returns http 400 bad request" do | ||
expect(response.status).to eq 400 | ||
end | ||
|
||
it "returns an error message" do | ||
expect(json["errors"]).to eq "Invalid Rover Name" | ||
end | ||
end | ||
|
||
context "with camera query" do | ||
before(:each) do | ||
create_list(:photo, 3, rover: rover, camera: create(:camera, {name: 'TEST', rover: rover}), sol: 9999) | ||
get :index, params: { rover_id: rover.name, camera: 'TEST' } | ||
end | ||
|
||
it "returns http 200 success" do | ||
expect(response.status).to eq 200 | ||
end | ||
|
||
it "renders photo data matching sol and camera" do | ||
expect(json["latest_photos"].length).to eq 3 | ||
expect(json["latest_photos"].first["camera"]["name"]).to eq 'TEST' | ||
end | ||
end | ||
|
||
context "with pagination" do | ||
let(:params) { {rover_id: rover.name.downcase} } | ||
|
||
before(:each) do | ||
create_list(:photo, 35, rover: rover, camera: camera, sol: 9999) | ||
end | ||
|
||
it "returns 25 entries per page when a page param is provided" do | ||
get :index, params: params.merge(page: 1) | ||
|
||
expect(json["latest_photos"].length).to eq 25 | ||
end | ||
|
||
it "returns all entries when no page param is provided" do | ||
get :index, params: params | ||
|
||
expect(json["latest_photos"].length).to eq 35 | ||
end | ||
|
||
it "returns n entries per page when a per_page param is provided" do | ||
get :index, params: params.merge(page: 1, per_page: 30) | ||
|
||
expect(json["latest_photos"].length).to eq 30 | ||
end | ||
|
||
it "returns the remaining entries on the last page of results" do | ||
get :index, params: params.merge(page: 2, per_page: 30) | ||
|
||
expect(json["latest_photos"].length).to eq 5 | ||
end | ||
end | ||
|
||
suffix_hash.each do |rover_id, sizes| | ||
context "with rover_id '#{rover_id}'" do | ||
before(:each) do | ||
rover.update(name: rover_id) | ||
end | ||
|
||
sizes.each do |size, suffix| | ||
context "and size '#{size}'" do | ||
before(:each) do | ||
get :index, params: { rover_id: rover_id, size: size } | ||
end | ||
|
||
it "modifies img_src" do | ||
photo = json['latest_photos'].first | ||
expect(photo['img_src']).to end_with suffix | ||
end | ||
end | ||
end | ||
|
||
context "with invalid size parameter" do | ||
before(:each) do | ||
get :index, params: { rover_id: rover_id, size: "not a size" } | ||
end | ||
|
||
it "returns http 400 bad request" do | ||
expect(response.status).to eq 400 | ||
end | ||
end | ||
end | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you add
include PhotoHelper
to the top of the controller, you can removehelpers.
and also thePhotoHelper::
namespace of the InvalidSizeParameter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read this warning about using the
include
keyword. Is this something we are concerned with?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.