From c809f79912a3b998ddf3a21973582254db564183 Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Fri, 10 May 2013 16:03:37 +0100 Subject: [PATCH] Tidy up user preferences controller --- app/controllers/user_preference_controller.rb | 104 +++++++++--------- lib/osm.rb | 19 +++- .../user_preference_controller_test.rb | 4 +- 3 files changed, 70 insertions(+), 57 deletions(-) diff --git a/app/controllers/user_preference_controller.rb b/app/controllers/user_preference_controller.rb index 88173d70f..df5f279b8 100644 --- a/app/controllers/user_preference_controller.rb +++ b/app/controllers/user_preference_controller.rb @@ -6,39 +6,8 @@ class UserPreferenceController < ApplicationController before_filter :require_allow_write_prefs, :except => [:read_one, :read] around_filter :api_call_handle_error - def read_one - pref = UserPreference.find(@user.id, params[:preference_key]) - - render :text => pref.v.to_s, :content_type => "text/plain" - rescue ActiveRecord::RecordNotFound => ex - render :text => 'OH NOES! PREF NOT FOUND!', :status => :not_found - end - - def update_one - begin - pref = UserPreference.find(@user.id, params[:preference_key]) - pref.v = request.raw_post.chomp - pref.save - rescue ActiveRecord::RecordNotFound - pref = UserPreference.new - pref.user = @user - pref.k = params[:preference_key] - pref.v = request.raw_post.chomp - pref.save - end - - render :nothing => true, :content_type => "text/plain" - end - - def delete_one - UserPreference.find(@user.id, params[:preference_key]).delete - - render :nothing => true, :content_type => "text/plain" - rescue ActiveRecord::RecordNotFound => ex - render :text => "param: #{params[:preference_key]} not found", :status => :not_found - end - - # print out all the preferences as a big xml block + ## + # return all the preferences as an XML document def read doc = OSM::API.new.get_xml_doc @@ -54,43 +23,70 @@ class UserPreferenceController < ApplicationController render :text => doc.to_s, :content_type => "text/xml" end + ## + # return the value for a single preference + def read_one + pref = UserPreference.find(@user.id, params[:preference_key]) + + render :text => pref.v.to_s, :content_type => "text/plain" + end + # update the entire set of preferences def update - doc = XML::Parser.string(request.raw_post).parse + old_preferences = @user.preferences.reduce({}) do |preferences,preference| + preferences[preference.k] = preference + preferences + end - prefs = [] + new_preferences = {} - keyhash = {} + doc = XML::Parser.string(request.raw_post).parse doc.find('//preferences/preference').each do |pt| - pref = UserPreference.new - - unless keyhash[pt['k']].nil? # already have that key - render :text => 'OH NOES! CAN HAS UNIQUE KEYS?', :status => :not_acceptable, :content_type => "text/plain" - return + if preference = old_preferences.delete(pt["k"]) + preference.v = pt["v"] + elsif new_preferences.include?(pt["k"]) + raise OSM::APIDuplicatePreferenceError.new(pt["k"]) + else + preference = @user.preferences.build(:k => pt["k"], :v => pt["v"]) end - keyhash[pt['k']] = 1 + new_preferences[preference.k] = preference + end - pref.k = pt['k'] - pref.v = pt['v'] - pref.user_id = @user.id - prefs << pref + old_preferences.each_value do |preference| + preference.delete end - if prefs.size > 150 - render :text => 'Too many preferences', :status => :request_entity_too_large, :content_type => "text/plain" - return + new_preferences.each_value do |preference| + preference.save! end - # kill the existing ones - UserPreference.delete_all(['user_id = ?', @user.id]) + render :nothing => true, :content_type => "text/plain" + end - # save the new ones - prefs.each do |pref| - pref.save! + ## + # update the value of a single preference + def update_one + begin + pref = UserPreference.find(@user.id, params[:preference_key]) + rescue ActiveRecord::RecordNotFound + pref = UserPreference.new + pref.user = @user + pref.k = params[:preference_key] end + pref.v = request.raw_post.chomp + pref.save! + + render :nothing => true, :content_type => "text/plain" + end + + ## + # delete a single preference + def delete_one + UserPreference.find(@user.id, params[:preference_key]).delete + render :nothing => true, :content_type => "text/plain" end end diff --git a/lib/osm.rb b/lib/osm.rb index 393011dac..ba28378f3 100644 --- a/lib/osm.rb +++ b/lib/osm.rb @@ -298,6 +298,23 @@ module OSM end end + # raised when a two preferences have a duplicate key string. + class APIDuplicatePreferenceError < APIError + def initialize(key) + @key = key + end + + attr_reader :key + + def status + :bad_request + end + + def to_s + "Duplicate preferences with key #{@key}" + end + end + # Helper methods for going to/from mercator and lat/lng. class Mercator include Math @@ -497,7 +514,7 @@ module OSM country = "GB" if country == "UK" end end - + return country.upcase end diff --git a/test/functional/user_preference_controller_test.rb b/test/functional/user_preference_controller_test.rb index 871f9000b..f5369387b 100644 --- a/test/functional/user_preference_controller_test.rb +++ b/test/functional/user_preference_controller_test.rb @@ -120,9 +120,9 @@ class UserPreferenceControllerTest < ActionController::TestCase content "" put :update end - assert_response :not_acceptable + assert_response :bad_request assert_equal "text/plain", @response.content_type - assert_equal "OH NOES! CAN HAS UNIQUE KEYS?", @response.body + assert_equal "Duplicate preferences with key key", @response.body assert_equal "new_value", UserPreference.find(1, "key").v # try a put with invalid content -- 2.43.2