From be800d1e1f8512b0417d59492182e93a06753743 Mon Sep 17 00:00:00 2001 From: Brad Humphrey Date: Mon, 6 May 2013 12:56:22 -0600 Subject: [PATCH] adding additional api support for the app center Reworked how the calls were passed through to take advantage of the app center pagination. Also added an endpoint for retrieving app reviews. The following canvas api calls should work, after configuring the app center plugin - /api/v1/accounts/:account_id/app_center/apps - /api/v1/accounts/:account_id/app_center/apps/:app_id/reviews - /api/v1/courses/:course_id/app_center/apps - /api/v1/courses/:course_id/app_center/apps/:app_id/reviews These should also accept a page and per_page params Change-Id: I15fe1225c01d04bc2ad6efa1736013b88e541c51 Reviewed-on: https://gerrit.instructure.com/20455 Tested-by: Jenkins Reviewed-by: Jeremy Stanley Product-Review: Brian Whitmer QA-Review: Clare Strong --- app/controllers/app_center_controller.rb | 29 ++- .../plugins/_app_center_settings.html.erb | 10 +- config/routes.rb | 7 +- lib/app_center/app_api.rb | 54 ++-- lib/canvas/http.rb | 2 +- lib/canvas/plugins/default_plugins.rb | 3 +- .../validators/app_center_validator.rb | 2 +- .../controllers/app_center_controller_spec.rb | 60 +++++ spec/lib/app_center/app_api_spec.rb | 230 ++++++++++++++---- 9 files changed, 319 insertions(+), 78 deletions(-) create mode 100644 spec/controllers/app_center_controller_spec.rb diff --git a/app/controllers/app_center_controller.rb b/app/controllers/app_center_controller.rb index 89a4e329510..bda2c1f1f4c 100644 --- a/app/controllers/app_center_controller.rb +++ b/app/controllers/app_center_controller.rb @@ -19,17 +19,30 @@ class AppCenterController < ApplicationController before_filter :require_context - def index - collection = PaginatedCollection.build do |pager| - current_page = pager.current_page ? pager.current_page.to_i - 1 : 0 - apps = AppCenter::AppApi.new.get_apps(current_page * pager.per_page, pager.per_page) || [] - pager.replace(apps) - pager.next_page = current_page + 2 if apps.size > 0 + def generate_app_api_collection(base_url) + PaginatedCollection.build do |pager| + page = (params['page'] || 1).to_i + response = yield AppCenter::AppApi.new, page + response ||= {} + pager.replace(response['objects'] || []) + pager.next_page = response['meta']['next_page'] if response['meta'] pager end + end + def index + per_page = params['per_page'] || 72 endpoint_scope = (@context.is_a?(Account) ? 'account' : 'course') - render :json => Api.paginate(collection, self, - send("api_v1_#{endpoint_scope}_app_center_apps_url")) + base_url = send("api_v1_#{endpoint_scope}_app_center_apps_url") + collection = generate_app_api_collection(base_url) {|app_api, page| app_api.get_apps(page, per_page)} + render :json => Api.paginate(collection, self, base_url, :per_page => per_page.to_i) + end + + def reviews + per_page = params['per_page'] || 15 + endpoint_scope = (@context.is_a?(Account) ? 'account' : 'course') + base_url = send("api_v1_#{endpoint_scope}_app_center_app_reviews_url") + collection = generate_app_api_collection(base_url) {|app_api, page| app_api.get_app_reviews(params[:app_id], page, per_page)} + render :json => Api.paginate(collection, self, base_url, :per_page => per_page.to_i) end end \ No newline at end of file diff --git a/app/views/plugins/_app_center_settings.html.erb b/app/views/plugins/_app_center_settings.html.erb index 8cf61fa99f4..3c881dcb832 100644 --- a/app/views/plugins/_app_center_settings.html.erb +++ b/app/views/plugins/_app_center_settings.html.erb @@ -25,9 +25,15 @@ - <%= f.blabel :apps_endpoint, :en => "Apps Endpoint" %> + <%= f.blabel :apps_index_endpoint, :en => "Apps Index Endpoint" %> - <%= f.text_field :apps_endpoint %> + <%= f.text_field :apps_index_endpoint %> + + + + <%= f.blabel :app_reviews_endpoint, :en => "App Show Endpoint" %> + + <%= f.text_field :app_reviews_endpoint %> diff --git a/config/routes.rb b/config/routes.rb index 70fe6cdba7d..93a5b9c9d5d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1137,8 +1137,11 @@ ActionController::Routing::Routes.draw do |map| end api.with_options(:controller => :app_center) do |app_center| - app_center.get 'accounts/:account_id/app_center/apps', :action => :index, :path_name => 'account_app_center_apps' - app_center.get 'courses/:course_id/app_center/apps', :action => :index, :path_name => 'course_app_center_apps' + ['course', 'account'].each do |context| + prefix = "#{context}s/:#{context}_id/app_center" + app_center.get "#{prefix}/apps", :action => :index, :path_name => "#{context}_app_center_apps" + app_center.get "#{prefix}/apps/:app_id/reviews", :action => :reviews, :path_name => "#{context}_app_center_app_reviews" + end end end diff --git a/lib/app_center/app_api.rb b/lib/app_center/app_api.rb index 713a14db914..d49726828dd 100644 --- a/lib/app_center/app_api.rb +++ b/lib/app_center/app_api.rb @@ -6,27 +6,47 @@ module AppCenter @app_center = Canvas::Plugin.find(:app_center) end - def get_apps(offset = 0, per_page = 10) - if @app_center && @app_center.enabled? && !@app_center.settings['base_url'].empty? + def valid_app_center? + @app_center && @app_center.enabled? && !@app_center.settings['base_url'].empty? + end - base_url = @app_center.settings['base_url'] - endpoint = @app_center.settings['apps_endpoint'] - response = Rails.cache.fetch(['app_center', base_url, endpoint, offset, per_page].cache_key, :expires_in => 5.minutes) do - path = "#{@app_center.settings['apps_endpoint']}?page=#{offset}&per_page=#{per_page}" - response = Canvas::HTTP.get("#{@app_center.settings['base_url']}/#{path}").body - end + def fetch_app_center_response(endpoint, expires, page, per_page) + return {} unless valid_app_center? - apps = JSON.parse(response)['objects'] + base_url = @app_center.settings['base_url'] + page = page.to_i + per_page = per_page.to_i + offset = ( page - 1 ) * per_page - #Temporary hack until edu-apps pagination works correctly - if apps.size > per_page - apps = apps[offset, per_page] - end - - return apps - else - return [] + cache_key = ['app_center', base_url, endpoint, offset].cache_key + response = Rails.cache.fetch(cache_key, :expires_in => expires) do + uri = URI.parse("#{base_url}#{endpoint}") + uri.query = [uri.query, "offset=#{offset}"].compact.join('&') + Canvas::HTTP.get(uri.to_s).body end + + begin + json = JSON.parse(response) + json['meta']['next_page'] = page + 1 if (json['meta'] && json['meta']['next']) || (json['objects'] && json['objects'].size > per_page) + json['objects'] = json['objects'][0, per_page] if json['objects'] + rescue + json = {} + Rails.cache.delete cache_key + end + + return json + end + + def get_apps(page = 1, per_page = 72) + return {} unless valid_app_center? + + fetch_app_center_response(@app_center.settings['apps_index_endpoint'], 5.minutes, page, per_page) + end + + def get_app_reviews(id, page = 1, per_page = 15) + return {} unless valid_app_center? + + fetch_app_center_response(@app_center.settings['app_reviews_endpoint'].gsub(':id', id.to_s), 60.minutes, page, per_page) end end end \ No newline at end of file diff --git a/lib/canvas/http.rb b/lib/canvas/http.rb index 58b820c7a01..2fac4c72fe5 100644 --- a/lib/canvas/http.rb +++ b/lib/canvas/http.rb @@ -29,7 +29,7 @@ module Canvas::HTTP raise(TooManyRedirectsError) if redirect_limit <= 0 url, uri = CustomValidations.validate_url(url_str) - request = Net::HTTP::Get.new(uri.path, other_headers) + request = Net::HTTP::Get.new(uri.request_uri, other_headers) http = Net::HTTP.new(uri.host, uri.port) http.use_ssl = true if uri.scheme == 'https' http.verify_mode = OpenSSL::SSL::VERIFY_NONE diff --git a/lib/canvas/plugins/default_plugins.rb b/lib/canvas/plugins/default_plugins.rb index fa074f9ff77..ade759c58ac 100644 --- a/lib/canvas/plugins/default_plugins.rb +++ b/lib/canvas/plugins/default_plugins.rb @@ -240,7 +240,8 @@ Canvas::Plugin.register('app_center', nil, { :settings => { :base_url => 'https://www.edu-apps.org', :token => nil, - :apps_endpoint => '/api/v1/apps' + :apps_index_endpoint => '/api/v1/apps', + :app_reviews_endpoint => '/api/v1/apps/:id/reviews' }, :validator => 'AppCenterValidator' }) diff --git a/lib/canvas/plugins/validators/app_center_validator.rb b/lib/canvas/plugins/validators/app_center_validator.rb index 82cc34d195f..03768a02b29 100644 --- a/lib/canvas/plugins/validators/app_center_validator.rb +++ b/lib/canvas/plugins/validators/app_center_validator.rb @@ -30,7 +30,7 @@ module Canvas::Plugins::Validators::AppCenterValidator plugin_setting.errors.add_to_base(I18n.t('canvas.plugins.errors.invalid_url', 'Invalid URL')) false else - settings.slice(:base_url, :token) + settings.slice(:base_url, :token, :apps_index_endpoint, :app_reviews_endpoint) end end end diff --git a/spec/controllers/app_center_controller_spec.rb b/spec/controllers/app_center_controller_spec.rb new file mode 100644 index 00000000000..cdbe0aae7ee --- /dev/null +++ b/spec/controllers/app_center_controller_spec.rb @@ -0,0 +1,60 @@ +# +# Copyright (C) 2013 Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . +# + +require File.expand_path(File.dirname(__FILE__) + '/../spec_helper') + +describe AppCenterController do + describe "#generate_app_api_collection" do + let(:controller) do + controller = AppCenterController.new + controller.params = {} + controller + end + + it "generates a valid paginated collection" do + objects = ['object1', 'object2'] + next_page = 10 + will_paginate = controller.generate_app_api_collection('www.example.com/endpoint') do |app_api, page| + app_api.should be_a AppCenter::AppApi + page.should == 1 + { + 'objects' => ['object1', 'object2'], + 'meta' => {"next_page" => next_page} + } + end + will_paginate.should be_a PaginatedCollection::Proxy + collection = will_paginate.paginate(:per_page => 72) + collection.should == objects + collection.next_page.should == next_page + end + + it "handles an empty response" do + will_paginate = controller.generate_app_api_collection('') {} + will_paginate.paginate(:per_page => 50).should == [] + end + + it "passes the page param as the offset" do + controller.params['page'] = 32 + controller.params['per_page'] = 12 + will_paginate = controller.generate_app_api_collection('') do |app_api, page, per_page| + page.should == controller.params['page'] + per_page.should == controller.params['per_pages'] + end + end + end +end diff --git a/spec/lib/app_center/app_api_spec.rb b/spec/lib/app_center/app_api_spec.rb index c7d338c6b5e..732acd4196e 100644 --- a/spec/lib/app_center/app_api_spec.rb +++ b/spec/lib/app_center/app_api_spec.rb @@ -21,31 +21,12 @@ require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper.rb') # Manually stubbing the actual API request describe AppCenter::AppApi do let(:api){ AppCenter::AppApi.new } - let(:response) do - response = mock - response.stubs(:body).returns( - { - 'meta' => { "next" => "https://www.example.com/api/v1/apps?offset=72"}, - 'current_offset' => 0, - 'limit' => 72, - 'objects' => [ - { - 'name' => 'First Tool', - 'id' => 'first_tool', - }, - { - 'name' => 'Second Tool', - 'id' => 'second_tool', - } - ] - }.to_json - ) - response - end before(:each) do default_settings = api.app_center.default_settings default_settings['base_url'] = 'www.example.com' + default_settings['apps_index_endpoint'] = '/apps' + default_settings['app_reviews_endpoint'] = '/apps/:id' PluginSetting.create(:name => api.app_center.id, :settings => default_settings) end @@ -56,41 +37,130 @@ describe AppCenter::AppApi do api.app_center.settings['base_url'].should_not be_empty end + describe '#fetch_app_center_response' do + let(:response) do + response = mock + response.stubs(:body).returns( + { + 'meta' => { "next" => "https://www.example.com/api/v1/apps?offset=60"}, + 'current_offset' => 0, + 'limit' => 50, + 'objects' => %w(object1 object2 object3 object4) + }.to_json + ) + response + end + + it "can handle query params in the endpoint" do + endpoint = '/?myparam=value' + per_page = 11 + page = 3 + Canvas::HTTP.expects(:get).with("#{api.app_center.settings['base_url']}#{endpoint}&offset=#{page * per_page - per_page}").returns(response) + api.fetch_app_center_response(endpoint, 11.minutes, page, per_page) + end + + it "can handle an invalid response" do + response.stubs(:body).returns('') + Canvas::HTTP.expects(:get).returns(response) + api.fetch_app_center_response('', 12.minutes, 8, 3).should == {} + end + + it "resets the cache when getting an invalid response" do + enable_cache do + response.stubs(:body).returns('') + Canvas::HTTP.expects(:get).returns(response).twice() + api.fetch_app_center_response('', 13.minutes, 7,4).should == {} + api.fetch_app_center_response('', 13.minutes, 7,4).should == {} + end + end + + it "can handle an error response" do + message = {"message" => "Tool not found","type" =>"error"} + response.stubs(:body).returns(message.to_json) + Canvas::HTTP.expects(:get).returns(response) + api.fetch_app_center_response('', 13.minutes, 6,9).should == message + end + + it "respects per_page param" do + endpoint = '/?myparam=value' + per_page = 1 + page = 1 + offset = page * per_page - per_page + Canvas::HTTP.expects(:get).with("#{api.app_center.settings['base_url']}#{endpoint}&offset=#{offset}").returns(response) + response = api.fetch_app_center_response(endpoint, 11.minutes, page, per_page) + results = response['objects'] + results.size.should == 1 + results.first.should == 'object1' + response['meta']['next_page'].should == 2 + end + + it "can omit next page" do + message = {"objects" => %w(object1 object2 object3 object4), "meta" => {}} + response.stubs(:body).returns(message.to_json) + endpoint = '/?myparam=value' + per_page = 5 + page = 1 + offset = page * per_page - per_page + Canvas::HTTP.expects(:get).with("#{api.app_center.settings['base_url']}#{endpoint}&offset=#{offset}").returns(response) + response = api.fetch_app_center_response(endpoint, 11.minutes, page, per_page) + results = response['objects'] + results.size.should == 4 + response['meta']['next_page'].should be_nil + end + end + describe '#get_apps' do - before(:each) { Canvas::HTTP.stubs(:get).returns(response)} + let(:response) do + response = mock + response.stubs(:body).returns( + { + 'meta' => { "next" => "https://www.example.com/api/v1/apps?offset=72"}, + 'current_offset' => 0, + 'limit' => 72, + 'objects' => [ + { + 'name' => 'First Tool', + 'id' => 'first_tool', + }, + { + 'name' => 'Second Tool', + 'id' => 'second_tool', + } + ] + }.to_json + ) + response + end it "gets a list of apps" do - apps = api.get_apps + Canvas::HTTP.stubs(:get).returns(response) + apps = api.get_apps()['objects'] apps.should be_a Array apps.size.should == 2 end - it "returns an empty array if the app center is disabled" do + it "returns an empty hash if the app center is disabled" do + Canvas::HTTP.stubs(:get).returns(response) setting = PluginSetting.find_by_name(api.app_center.id) setting.destroy api.app_center.should_not be_enabled - apps = api.get_apps - apps.should be_a Array - apps.size.should == 0 + response = api.get_apps() + response.should be_a Hash + response.size.should == 0 end - it "respects per_page" do - apps = api.get_apps(0, 1) - apps.size.should == 1 - apps.first['id'].should == 'first_tool' - end - - it "respects offset" do - apps = api.get_apps(1,1) - apps.size.should == 1 - apps.first['id'].should == 'second_tool' - end - - it "caches results" do + it "gets the next page" do + enable_cache do + Canvas::HTTP.stubs(:get).returns(response) + response = api.get_apps + response['meta']['next_page'].should == 2 + end + end + + it "caches apps results" do enable_cache do - Canvas::HTTP.unstub(:get) Canvas::HTTP.expects(:get).returns(response) api.get_apps() api.get_apps() @@ -99,12 +169,80 @@ describe AppCenter::AppApi do it "caches multiple calls" do enable_cache do - Canvas::HTTP.unstub(:get) Canvas::HTTP.expects(:get).returns(response).times(2) - api.get_apps(0,1) - api.get_apps(1,1) - api.get_apps(0,1) - api.get_apps(1,1) + api.get_apps(0) + api.get_apps(1) + api.get_apps(0) + api.get_apps(1) + end + end + end + + describe '#get_app_reviews' do + let(:response) do + response = mock + response.stubs(:body).returns( + { + 'meta' => { "next" => "https://www.example.com/api/v1/apps/first_tool/reviews?offset=15"}, + 'current_offset' => 0, + 'limit' => 15, + 'objects' => [ + { + 'user_name' => 'Iron Man', + 'user_avatar_url' => 'http://www.example.com/rich.ico', + 'comments' => 'This tool is so great', + }, + { + 'user_name' => 'The Hulk', + 'user_avatar_url' => 'http://www.example.com/beefy.ico', + 'comments' => 'SMASH!', + } + ] + }.to_json + ) + response + end + + it "gets an apps reviews" do + Canvas::HTTP.stubs(:get).returns(response) + reviews = api.get_app_reviews('first_tool')['objects'] + reviews.should be_a Array + reviews.size.should == 2 + end + + it "returns an empty hash if the app center is disabled" do + Canvas::HTTP.stubs(:get).returns(response) + setting = PluginSetting.find_by_name(api.app_center.id) + setting.destroy + + api.app_center.should_not be_enabled + + response = api.get_app_reviews('first_tool') + response.should be_a Hash + response.size.should == 0 + end + + it "gets the next page" do + Canvas::HTTP.stubs(:get).returns(response) + response = api.get_app_reviews('first_tool') + response['meta']['next_page'].should == 2 + end + + it "caches apps results" do + enable_cache do + Canvas::HTTP.expects(:get).returns(response) + api.get_app_reviews('first_tool') + api.get_app_reviews('first_tool') + end + end + + it "caches multiple calls" do + enable_cache do + Canvas::HTTP.expects(:get).returns(response).times(2) + api.get_app_reviews('first_tool',0) + api.get_app_reviews('first_tool',1) + api.get_app_reviews('first_tool',0) + api.get_app_reviews('first_tool',1) end end end