Fixed some bugs in changeset query code. Added more test cases.

diff --git a/app/controllers/application.rb b/app/controllers/application.rb
index 579c50e95a725217070beb7532d2880899636c5d..f5ea0063db38b76c7d9cdb74b536b1c8f25cd182 100644 (file)
--- a/app/controllers/application.rb
+++ b/app/controllers/application.rb
@@ -22,7 +22,11 @@ class ApplicationController < ActionController::Base
     redirect_to :controller => 'user', :action => 'login', :referer => request.request_uri unless @user
   end
 
     redirect_to :controller => 'user', :action => 'login', :referer => request.request_uri unless @user
   end
 

-  def authorize(realm='Web Password', errormessage="Couldn't authenticate you") 
+  ##
+  # sets up the @user object for use by other methods. this is mostly called
+  # from the authorize method, but can be called elsewhere if authorisation
+  # is optional.
+  def setup_user_auth

     username, passwd = get_auth_data # parse from headers
     # authenticate per-scheme
     if username.nil?
     username, passwd = get_auth_data # parse from headers
     # authenticate per-scheme
     if username.nil?


@@ -32,6 +36,11 @@ class ApplicationController < ActionController::Base
     else
       @user = User.authenticate(:username => username, :password => passwd) # basic auth
     end
     else
       @user = User.authenticate(:username => username, :password => passwd) # basic auth
     end

+  end
+
+  def authorize(realm='Web Password', errormessage="Couldn't authenticate you") 
+    # make the @user object from any auth sources we have
+    setup_user_auth

 
     # handle authenticate pass/fail
     unless @user
 
     # handle authenticate pass/fail
     unless @user

diff --git a/app/controllers/changeset_controller.rb b/app/controllers/changeset_controller.rb
index 1e6a44189ba1bac5cd0f8508dbdb71419cf86ba5..5a0be35888039347ea877adc9b5488304424cf45 100644 (file)
--- a/app/controllers/changeset_controller.rb
+++ b/app/controllers/changeset_controller.rb
@@ -236,9 +236,9 @@ class ChangesetController < ApplicationController
     # create the conditions that the user asked for. some or all of
     # these may be nil.
     conditions = conditions_bbox(params['bbox'])
     # create the conditions that the user asked for. some or all of
     # these may be nil.
     conditions = conditions_bbox(params['bbox'])

-    cond_merge conditions, conditions_user(params['user'])
-    cond_merge conditions, conditions_time(params['time'])
-    cond_merge conditions, conditions_open(params['open'])
+    conditions = cond_merge conditions, conditions_user(params['user'])
+    conditions = cond_merge conditions, conditions_time(params['time'])
+    conditions = cond_merge conditions, conditions_open(params['open'])

 
     # create the results document
     results = OSM::API.new.get_xml_doc
 
     # create the results document
     results = OSM::API.new.get_xml_doc


@@ -333,7 +333,15 @@ class ChangesetController < ApplicationController
   def conditions_user(user)
     unless user.nil?
       u = User.find(user.to_i)
   def conditions_user(user)
     unless user.nil?
       u = User.find(user.to_i)

-      raise OSM::APINotFoundError unless u.data_public?
+      # should be able to get changesets of public users only, or 
+      # our own changesets regardless of public-ness.
+      unless u.data_public?
+        # get optional user auth stuff so that users can see their own
+        # changesets if they're non-public
+        setup_user_auth
+        
+        raise OSM::APINotFoundError if @user.nil? or @user.id != u.id
+      end

       return ['user_id = ?', u.id]
     else
       return nil
       return ['user_id = ?', u.id]
     else
       return nil


@@ -347,11 +355,11 @@ class ChangesetController < ApplicationController
       # if there is a range, i.e: comma separated, then the first is 
       # low, second is high - same as with bounding boxes.
       if time.count(',') == 1
       # if there is a range, i.e: comma separated, then the first is 
       # low, second is high - same as with bounding boxes.
       if time.count(',') == 1

-        from, to = time.split(/,/).collect { |t| Date.parse(t) }
+        from, to = time.split(/,/).collect { |t| DateTime.parse(t) }

         return ['created_at > ? and created_at < ?', from, to]
       else
         # if there is no comma, assume its a lower limit on time
         return ['created_at > ? and created_at < ?', from, to]
       else
         # if there is no comma, assume its a lower limit on time

-        return ['created_at > ?', Date.parse(time)]
+        return ['created_at > ?', DateTime.parse(time)]

       end
     else
       return nil
       end
     else
       return nil

diff --git a/lib/osm.rb b/lib/osm.rb
index c132ff25cfc4ef2bcffc40e84a3a714da36bea1b..128e65aac41a73d12d28a2f86a3aa396cf27eb9c 100644 (file)
--- a/lib/osm.rb
+++ b/lib/osm.rb
@@ -17,6 +17,9 @@ module OSM
 
   # Raised when an API object is not found.
   class APINotFoundError < APIError
 
   # Raised when an API object is not found.
   class APINotFoundError < APIError

+    def render_opts
+      { :nothing => true, :status => :not_found }
+    end

   end
 
   # Raised when a precondition to an API action fails sanity check.
   end
 
   # Raised when a precondition to an API action fails sanity check.

diff --git a/test/fixtures/changesets.yml b/test/fixtures/changesets.yml
index 2047af8d5e2f0fb45807337765050679f6b73c10..ed0df02288843f4fc8c5347ebd9d91661da5cc3c 100644 (file)
--- a/test/fixtures/changesets.yml
+++ b/test/fixtures/changesets.yml
@@ -30,12 +30,17 @@ normal_user_version_change:
   user_id: 1
   created_at: "2008-01-01 00:00:00"
   open: true
   user_id: 1
   created_at: "2008-01-01 00:00:00"
   open: true

+  min_lon: <%= 1 * SCALE %>
+  min_lat: <%= 1 * SCALE %>
+  max_lon: <%= 4 * SCALE %>
+  max_lat: <%= 4 * SCALE %>

 
 # changeset to contain all the invalid stuff that is in the
 
 # changeset to contain all the invalid stuff that is in the

-# fixtures (nodes outside the world, etc...)
+# fixtures (nodes outside the world, etc...), but needs to have
+# a valid user...

 invalid_changeset:
   id: 5
 invalid_changeset:
   id: 5

-  user_id: 0
+  user_id: 3

   created_at: "2008-01-01 00:00:00"
   open: false
   
\ No newline at end of file
   created_at: "2008-01-01 00:00:00"
   open: false
   
\ No newline at end of file

diff --git a/test/functional/changeset_controller_test.rb b/test/functional/changeset_controller_test.rb
index 6cbee1eb886b6f777c8f02df361f70426ce7cfa5..2b8fa360a8b3cba9f7ee2d24f639b4eac5e92ed0 100644 (file)
--- a/test/functional/changeset_controller_test.rb
+++ b/test/functional/changeset_controller_test.rb
@@ -595,7 +595,37 @@ EOF
   def test_changeset_by_bbox
     get :query, :bbox => "-10,-10, 10, 10"
     assert_response :success, "can't get changesets in bbox"
   def test_changeset_by_bbox
     get :query, :bbox => "-10,-10, 10, 10"
     assert_response :success, "can't get changesets in bbox"

-    # FIXME: write the actual test bit after fixing the fixtures!
+    assert_changesets [1,4]
+
+    get :query, :bbox => "4.5,4.5,4.6,4.6"
+    assert_response :success, "can't get changesets in bbox"
+    assert_changesets [1]
+
+    # can't get changesets of user 1 without authenticating
+    get :query, :user => users(:normal_user).id
+    assert_response :not_found, "shouldn't be able to get changesets by non-public user"
+
+    # but this should work
+    basic_authorization "test@openstreetmap.org", "test"
+    get :query, :user => users(:normal_user).id
+    assert_response :success, "can't get changesets by user"
+    assert_changesets [1,3,4]
+
+    get :query, :user => users(:normal_user).id, :open => true
+    assert_response :success, "can't get changesets by user and open"
+    assert_changesets [1,4]
+
+    get :query, :time => '2007-12-31'
+    assert_response :success, "can't get changesets by time-since"
+    assert_changesets [2,4,5]
+
+    get :query, :time => '2008-01-01T12:34Z'
+    assert_response :success, "can't get changesets by time-since with hour"
+    assert_changesets [2]
+
+    get :query, :time => '2007-12-31T23:59Z,2008-01-01T00:01Z'
+    assert_response :success, "can't get changesets by time-range"
+    assert_changesets [4,5]

   end
 
   ##
   end
 
   ##


@@ -641,6 +671,16 @@ EOF
   # utility functions
   #------------------------------------------------------------
 
   # utility functions
   #------------------------------------------------------------
 

+  ##
+  # boilerplate for checking that certain changesets exist in the
+  # output.
+  def assert_changesets(ids)
+    assert_select "osm>changeset", ids.size
+    ids.each do |id|
+      assert_select "osm>changeset[id=#{id}]", 1
+    end
+  end
+

   ##
   # call the include method and assert properties of the bbox
   def check_after_include(changeset_id, lon, lat, bbox)
   ##
   # call the include method and assert properties of the bbox
   def check_after_include(changeset_id, lon, lat, bbox)