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

Implement photo size parameter #147

Open
wants to merge 45 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
45 commits
Select commit Hold shift + click to select a range
f6b7338
Add documentation for size
Apr 1, 2021
558bb88
Add size documentation for all rovers
Apr 1, 2021
819ecfa
Rename photos() to search_photos()
Apr 1, 2021
c709393
Permit size
Apr 1, 2021
aeea8e3
Add photo resizing
Apr 1, 2021
4285d09
Fix parameter size example
Apr 1, 2021
ce4c7a2
Move photo resize methods to helper
Apr 1, 2021
3c3a67e
Refactor and downcase rover_name
Apr 1, 2021
b5d22f7
Rename private methods for clarity
Apr 1, 2021
31f70ae
Add error handling to photo resize
Apr 2, 2021
953da53
Fix nested index method
Apr 2, 2021
d8e4633
Add photo resizing to latest_photos_controller
Apr 2, 2021
7d03c01
Move search_photos() to PhotoHelper
Apr 2, 2021
778211a
Permit :size in latest_photos_controller
Apr 2, 2021
23fc0ac
Make helper methods private and pass params
Apr 2, 2021
0c8cd39
Replace include with new helpers.<helper> syntax
Apr 3, 2021
18c882f
Refactor replace_photo_suffix
Apr 6, 2021
c471c42
Add and implement lookup_suffix()
Apr 7, 2021
d4eb91c
Isolate sizes to avoid edge case
Apr 7, 2021
ecd8402
Update error message
Apr 7, 2021
814fae6
Remove put
Apr 7, 2021
69bd885
Refactor resize photo methods to return altered copy of photos
Apr 7, 2021
13e6e85
Improve error handling
Apr 8, 2021
57117ff
Move search_photos method logic to Photo.search model method
Apr 8, 2021
32d105d
Improve error handling
Apr 8, 2021
45a6e00
Fix photo suffix capitalization
Apr 8, 2021
e9d16c8
Merge branch 'master' into photo-size-parameter
Apr 9, 2021
0d76781
Fix params name conflict
Apr 9, 2021
1508b2e
Change sequencing of photos to make suffix compatible
Apr 9, 2021
32d9080
Add test for photos index with size
Apr 9, 2021
8f196db
Simplify test for photos index with size
Apr 9, 2021
66c5b3b
Add test for photos show with size
Apr 9, 2021
3de4ae7
Improve test for photos with size
Apr 9, 2021
80afd8d
Create test for latest_photos_controller
Apr 9, 2021
2543edd
Minor change
Apr 9, 2021
ac1cdce
Update size parameter documentation
Apr 9, 2021
c7cff19
Add test for invalid size parameter
Apr 10, 2021
5db1913
Remove repeated information
Apr 10, 2021
8b8a50b
Change instance variable rover to local variable
Apr 10, 2021
c2cf788
Fix code style
Apr 20, 2021
d94eaae
Raise error for invalid size parameter
Apr 20, 2021
d044624
Fix test for invalid size parameter
Apr 20, 2021
0fca483
Improve error output
Apr 22, 2021
cf164ec
Move search_photos back to PhotoHelper
Apr 22, 2021
fda4b56
Remove unused code
Apr 22, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,21 @@ If you just want to receive photo data for the most recent Sol for which photos

https://api.nasa.gov/mars-photos/api/v1/rovers/curiosity/latest_photos?api_key=DEMO_KEY

#### Size parameter:

Specify the size of the photo you would like to receive. Default value is `large`.

https://api.nasa.gov/mars-photos/api/v1/rovers/curiosity/photos?api_key=DEMO_KEY&size=small

| Size Parameter Value | Format | Curiosity | Opportunity | Spirit | Perseverance |
|----------------------|--------|:-----------:|:-----------:|:------:|:------------:|
| `small` | JPEG | ✔ | ✔ | ✔ | ✔ |
| `medium` | JPEG | ✔ | ✔ | ✔ | ✔ |
| `large` | JPEG | ✔ | ✔ | ✔ | ✔ |
| `full` | PNG | N/A | N/A | N/A | ✔ |

*Note: Only **Perseverance** `small`, `medium`, and `large` size images have guaranteed widths of `300px`, `800px`, and `1200px` respectively. All other dimensions are variable.*

### Mission Manifest Endpoint

A mission manifest is available for each Rover at the `/manifests/<rover_name>`. This manifest will list details of the Rover's mission to help narrow down photo queries to the API. The information in the manifest includes:
Expand Down
33 changes: 14 additions & 19 deletions app/controllers/api/v1/latest_photos_controller.rb
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
Copy link
Owner

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 remove helpers. and also the PhotoHelper:: namespace of the InvalidSizeParameter.

Copy link
Author

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?

Copy link
Owner

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.


def photo_params
params.permit(:camera, :earth_date, :rover_id).merge(sol: @rover.photos.maximum(:sol))
Copy link
Owner

Choose a reason for hiding this comment

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

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.

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
34 changes: 21 additions & 13 deletions app/controllers/api/v1/photos_controller.rb
Original file line number Diff line number Diff line change
@@ -1,13 +1,29 @@
class Api::V1::PhotosController < ApplicationController
before_action :photo_params
Copy link
Owner

Choose a reason for hiding this comment

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

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.


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
Expand All @@ -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
108 changes: 108 additions & 0 deletions app/helpers/photo_helper.rb
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
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

The 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_id to exist in params in the show action. But I can just params.merge that in before calling resize_photos [photo], params.

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
Copy link
Owner

Choose a reason for hiding this comment

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

This certainly works, but an alternative would be to use photo.image_src.gsub(old_suffix, new_suffix).

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
6 changes: 2 additions & 4 deletions app/models/photo.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,8 @@ class Photo < ActiveRecord::Base

def self.search(params, rover)
photos = search_by_date params
if params[:camera]
if photos.any?
photos = photos.search_by_camera params, rover
end
if params[:camera] and photos.any?
photos = photos.search_by_camera params, rover
end
photos
end
Expand Down
145 changes: 145 additions & 0 deletions spec/controllers/api/v1/latest_photos_controller_spec.rb
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
Loading