Support unwrapped bbox values in changeset history queries

diff --git a/app/assets/javascripts/index/history.js b/app/assets/javascripts/index/history.js
index 1a23ac7b1198188e0c7cfaaa1a10cb09ca9ce1d5..c35aaf2887c13c1390e4b5ddcbeaf07542310628 100644 (file)
--- a/app/assets/javascripts/index/history.js
+++ b/app/assets/javascripts/index/history.js
@@ -220,7 +220,7 @@ OSM.History = function (map) {
     const neClamped = crs.unproject(crs.project(ne));
 
     if (sw.lat >= swClamped.lat || ne.lat <= neClamped.lat || ne.lng - sw.lng < 360) {
-      data.set("bbox", map.getBounds().wrap().toBBoxString());
+      data.set("bbox", map.getBounds().toBBoxString());
     }
   }
 

diff --git a/app/controllers/changesets_controller.rb b/app/controllers/changesets_controller.rb
index 5e79f4da62a70ac42bc4e75203c60a70d073de24..07e7a365f0d1a3884e62a99f3cdb61fc82e64bad 100644 (file)
--- a/app/controllers/changesets_controller.rb
+++ b/app/controllers/changesets_controller.rb
@@ -54,7 +54,10 @@ class ChangesetsController < ApplicationController
                        changesets.where("false")
                      end
       elsif @params[:bbox]
-        changesets = conditions_bbox(changesets, BoundingBox.from_bbox_params(params))
+        bbox_array = @params[:bbox].split(",").map(&:to_f)
+        raise OSM::APIBadUserInput, "The parameter bbox must be of the form min_lon,min_lat,max_lon,max_lat" unless bbox_array.count == 4
+
+        changesets = conditions_bbox(changesets, *bbox_array)
       elsif @params[:friends] && current_user
         changesets = changesets.where(:user => current_user.followings.identifiable)
       elsif @params[:nearby] && current_user
@@ -113,21 +116,37 @@ class ChangesetsController < ApplicationController
   #------------------------------------------------------------
 
   ##
-  # if a bounding box was specified do some sanity checks.
   # restrict changesets to those enclosed by a bounding box
-  def conditions_bbox(changesets, bbox)
-    if bbox
-      bbox.check_boundaries
-      bbox = bbox.to_scaled
-
-      changesets.where("min_lon < ? and max_lon > ? and min_lat < ? and max_lat > ?",
-                       bbox.max_lon.to_i, bbox.min_lon.to_i,
-                       bbox.max_lat.to_i, bbox.min_lat.to_i)
-    else
+  def conditions_bbox(changesets, min_lon, min_lat, max_lon, max_lat)
+    db_min_lat = (min_lat * GeoRecord::SCALE).to_i
+    db_max_lat = (max_lat * GeoRecord::SCALE).to_i
+    db_min_lon = (wrap_lon(min_lon) * GeoRecord::SCALE).to_i
+    db_max_lon = (wrap_lon(max_lon) * GeoRecord::SCALE).to_i
+
+    changesets = changesets.where("min_lat < ? and max_lat > ?", db_max_lat, db_min_lat)
+
+    if max_lon - min_lon >= 360
+      # the query bbox spans the entire world, therefore no lon checks are necessary
       changesets
+    elsif db_min_lon <= db_max_lon
+      # the normal case when the query bbox doesn't include the antimeridian
+      changesets.where("min_lon < ? and max_lon > ?", db_max_lon, db_min_lon)
+    else
+      # the query bbox includes the antimeridian
+      # this case works as if there are two query bboxes:
+      #   [-180*SCALE .. db_max_lon], [db_min_lon .. 180*SCALE]
+      # it would be necessary to check if changeset bboxes intersect with either of the query bboxes:
+      #   (changesets.min_lon < db_max_lon and changesets.max_lon > -180*SCALE) or (changesets.min_lon < 180*SCALE and changesets.max_lon > db_min_lon)
+      # but the comparisons with -180*SCALE and 180*SCALE are unnecessary:
+      #   (changesets.min_lon < db_max_lon) or (changesets.max_lon > db_min_lon)
+      changesets.where("min_lon < ? or max_lon > ?", db_max_lon, db_min_lon)
     end
   end
 
+  def wrap_lon(lon)
+    ((lon + 180) % 360) - 180
+  end
+
   ##
   # eliminate empty changesets (where the bbox has not been set)
   # this should be applied to all changeset list displays

diff --git a/test/controllers/changesets_controller_test.rb b/test/controllers/changesets_controller_test.rb
index 914f7f651187b38ee2a5e579bdcbdc92413f4ebd..007a5d679e35f726d5077ea3afcd9d3c93b356e8 100644 (file)
--- a/test/controllers/changesets_controller_test.rb
+++ b/test/controllers/changesets_controller_test.rb
@@ -117,6 +117,127 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest
     check_index_result(changesets)
   end
 
+  def test_index_bbox_across_antimeridian_with_changesets_close_to_antimeridian
+    west_of_antimeridian_changeset = create(:changeset, :num_changes => 1, :bbox => [176, 0, 178, 1])
+    east_of_antimeridian_changeset = create(:changeset, :num_changes => 1, :bbox => [-178, 0, -176, 1])
+
+    get history_path(:format => "html", :list => "1")
+    assert_response :success
+    check_index_result([east_of_antimeridian_changeset, west_of_antimeridian_changeset])
+
+    # negative longitudes
+    get history_path(:format => "html", :list => "1", :bbox => "-190,-10,-170,10")
+    assert_response :success
+    check_index_result([east_of_antimeridian_changeset, west_of_antimeridian_changeset])
+
+    get history_path(:format => "html", :list => "1", :bbox => "-183,-10,-177,10")
+    assert_response :success
+    check_index_result([east_of_antimeridian_changeset, west_of_antimeridian_changeset])
+
+    get history_path(:format => "html", :list => "1", :bbox => "-181,-10,-177,10")
+    assert_response :success
+    check_index_result([east_of_antimeridian_changeset])
+
+    get history_path(:format => "html", :list => "1", :bbox => "-183,-10,-179,10")
+    assert_response :success
+    check_index_result([west_of_antimeridian_changeset])
+
+    get history_path(:format => "html", :list => "1", :bbox => "-181,-10,-179,10")
+    assert_response :success
+    check_index_result([])
+
+    # positive longitudes
+    get history_path(:format => "html", :list => "1", :bbox => "170,-10,190,10")
+    assert_response :success
+    check_index_result([east_of_antimeridian_changeset, west_of_antimeridian_changeset])
+
+    get history_path(:format => "html", :list => "1", :bbox => "177,-10,183,10")
+    assert_response :success
+    check_index_result([east_of_antimeridian_changeset, west_of_antimeridian_changeset])
+
+    get history_path(:format => "html", :list => "1", :bbox => "177,-10,181,10")
+    assert_response :success
+    check_index_result([west_of_antimeridian_changeset])
+
+    get history_path(:format => "html", :list => "1", :bbox => "179,-10,183,10")
+    assert_response :success
+    check_index_result([east_of_antimeridian_changeset])
+
+    get history_path(:format => "html", :list => "1", :bbox => "179,-10,181,10")
+    assert_response :success
+    check_index_result([])
+  end
+
+  def test_index_bbox_across_antimeridian_with_changesets_around_globe
+    changeset1 = create(:changeset, :num_changes => 1, :bbox => [-150, 40, -140, 50])
+    changeset2 = create(:changeset, :num_changes => 1, :bbox => [-30, -30, -20, -20])
+    changeset3 = create(:changeset, :num_changes => 1, :bbox => [10, 60, 20, 70])
+    changeset4 = create(:changeset, :num_changes => 1, :bbox => [150, -60, 160, -50])
+
+    # no bbox, get all changesets
+    get history_path(:format => "html", :list => "1")
+    assert_response :success
+    check_index_result([changeset4, changeset3, changeset2, changeset1])
+
+    # large enough bbox within normal range
+    get history_path(:format => "html", :list => "1", :bbox => "-170,-80,170,80")
+    assert_response :success
+    check_index_result([changeset4, changeset3, changeset2, changeset1])
+
+    # bbox for [1,2] within normal range
+    get history_path(:format => "html", :list => "1", :bbox => "-160,-80,0,80")
+    assert_response :success
+    check_index_result([changeset2, changeset1])
+
+    # bbox for [1,4] containing antimeridian with negative lon
+    get history_path(:format => "html", :list => "1", :bbox => "-220,-80,-100,80")
+    assert_response :success
+    check_index_result([changeset4, changeset1])
+
+    # bbox for [1,4] containing antimeridian with positive lon
+    get history_path(:format => "html", :list => "1", :bbox => "100,-80,220,80")
+    assert_response :success
+    check_index_result([changeset4, changeset1])
+
+    # large enough bbox outside normal range
+    get history_path(:format => "html", :list => "1", :bbox => "-220,-80,220,80")
+    assert_response :success
+    check_index_result([changeset4, changeset3, changeset2, changeset1])
+  end
+
+  ##
+  # Test that -180..180 longitudes don't result in empty bbox
+  def test_index_bbox_entire_world
+    changeset = create(:changeset, :num_changes => 1, :bbox => [30, 60, 31, 61])
+
+    get history_path(:format => "html", :list => "1", :bbox => "-180,-80,-180,80")
+    assert_response :success
+    check_index_result([])
+
+    get history_path(:format => "html", :list => "1", :bbox => "180,-80,180,80")
+    assert_response :success
+    check_index_result([])
+
+    get history_path(:format => "html", :list => "1", :bbox => "-180,-80,180,80")
+    assert_response :success
+    check_index_result([changeset])
+  end
+
+  ##
+  # Test that -270..270 longitudes don't result in 90..-90 bbox
+  def test_index_bbox_larger_than_entire_world
+    changeset1 = create(:changeset, :num_changes => 1, :bbox => [30, 60, 31, 61])
+    changeset2 = create(:changeset, :num_changes => 1, :bbox => [130, 60, 131, 61])
+
+    get history_path(:format => "html", :list => "1", :bbox => "-90,-80,90,80")
+    assert_response :success
+    check_index_result([changeset1])
+
+    get history_path(:format => "html", :list => "1", :bbox => "-270,-80,270,80")
+    assert_response :success
+    check_index_result([changeset2, changeset1])
+  end
+
   ##
   # Checks the display of the user changesets listing
   def test_index_user

diff --git a/test/system/history_test.rb b/test/system/history_test.rb
index 50215abc5515b4e80c474e4687d47a27eab6b0ef..cb36473541757a33a86bd5c0898ac1da6a4d1d0d 100644 (file)
--- a/test/system/history_test.rb
+++ b/test/system/history_test.rb
@@ -152,6 +152,30 @@ class HistoryTest < ApplicationSystemTestCase
     end
   end
 
+  test "changesets at both sides of antimeridian are listed" do
+    user = create(:user)
+    PAGE_SIZE.times do
+      create(:changeset, :user => user, :num_changes => 1, :bbox => [176, 0, 178, 1]) do |changeset|
+        create(:changeset_tag, :changeset => changeset, :k => "comment", :v => "West-of-antimeridian-changeset")
+      end
+      create(:changeset, :user => user, :num_changes => 1, :bbox => [-178, 0, -176, 1]) do |changeset|
+        create(:changeset_tag, :changeset => changeset, :k => "comment", :v => "East-of-antimeridian-changeset")
+      end
+    end
+
+    visit history_path(:anchor => "map=6/0/179")
+
+    within_sidebar do
+      assert_link "West-of-antimeridian-changeset", :count => PAGE_SIZE / 2
+      assert_link "East-of-antimeridian-changeset", :count => PAGE_SIZE / 2
+
+      click_on "Older Changesets"
+
+      assert_link "West-of-antimeridian-changeset", :count => PAGE_SIZE
+      assert_link "East-of-antimeridian-changeset", :count => PAGE_SIZE
+    end
+  end
+
   private
 
   def create_visible_changeset(user, comment)