From 5dbe2cbf24e06c57d33e6590d84480156ea6a180 Mon Sep 17 00:00:00 2001 From: CoslaJohn Date: Fri, 19 Jul 2024 12:13:43 +0100 Subject: [PATCH 1/7] Support FeatureCollection and MultiPolygon in geozones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We're reworking the format validation to correctly interpret feature collection, feature, and geometry, according to RFC 7946 [1]. Since Leaflet interprets GeoJSON format, we're rendering the GeoJSON as a layer instead of as a set of points. For that, we're normalizing the GeoJSON to make sure it contains either a Feature or a FeatureCollection. We're also adding the Leaflet images to the assets path so the markers used for point geometries are rendered correctly. Note we no longer allow a GeoJSON containing a geometry but not a defined type. Since there might be invalid GeoJSON in existing Consul Democracy databases, we're normalizing these existing geometry objects to be part of a feature object. We're also wrapping the outline points in a FeatureCollection object because most of the large GIS systems eg ArcGIS, QGIS export geojson as a complete FeatureCollection. [1] https://datatracker.ietf.org/doc/html/rfc7946 Co-authored-by: Javi Martín --- app/assets/config/manifest.js | 1 + app/assets/javascripts/map.js | 23 +- .../concerns/geojson_format_validator.rb | 92 ++++- app/models/geozone.rb | 36 +- config/application.rb | 1 + config/locales/en/activerecord.yml | 3 +- config/locales/es/activerecord.yml | 3 +- spec/factories/administration.rb | 10 +- spec/models/geojson_format_validator_spec.rb | 329 ++++++++++++++++++ spec/models/geozone_spec.rb | 222 +++++++----- spec/system/admin/geozones_spec.rb | 21 +- spec/system/budgets/investments_spec.rb | 6 +- 12 files changed, 635 insertions(+), 112 deletions(-) create mode 100644 spec/models/geojson_format_validator_spec.rb diff --git a/app/assets/config/manifest.js b/app/assets/config/manifest.js index c4cf761a4..3bc6e3f23 100644 --- a/app/assets/config/manifest.js +++ b/app/assets/config/manifest.js @@ -14,3 +14,4 @@ //= link print.css //= link pdf_fonts.css //= link_tree ../../../vendor/assets/images +//= link_tree ../../../node_modules/leaflet/dist/images diff --git a/app/assets/javascripts/map.js b/app/assets/javascripts/map.js index 3793e2858..cb5b9835f 100644 --- a/app/assets/javascripts/map.js +++ b/app/assets/javascripts/map.js @@ -220,17 +220,22 @@ } }, addGeozone: function(geozone, map) { - var polygon = L.polygon(geozone.outline_points, { - color: geozone.color, - fillOpacity: 0.3, - className: "map-polygon" + var geojsonData = JSON.parse(geozone.outline_points); + + var geoJsonLayer = L.geoJSON(geojsonData, { + style: { + color: geozone.color, + fillOpacity: 0.3, + className: "map-polygon" + }, + onEachFeature: function(feature, layer) { + if (geozone.headings) { + layer.bindPopup(geozone.headings.join("
")); + } + } }); - if (geozone.headings !== undefined) { - polygon.bindPopup(geozone.headings.join("
")); - } - - polygon.addTo(map); + geoJsonLayer.addTo(map); }, getPopupContent: function(data) { return "" + data.title + ""; diff --git a/app/models/concerns/geojson_format_validator.rb b/app/models/concerns/geojson_format_validator.rb index e2a88f0e8..1614cc794 100644 --- a/app/models/concerns/geojson_format_validator.rb +++ b/app/models/concerns/geojson_format_validator.rb @@ -3,8 +3,13 @@ class GeojsonFormatValidator < ActiveModel::EachValidator if value.present? geojson = parse_json(value) - unless geojson?(geojson) + unless valid_geojson?(geojson) record.errors.add(attribute, :invalid) + return + end + + unless valid_coordinates?(geojson) + record.errors.add(attribute, :invalid_coordinates) end end end @@ -12,12 +17,91 @@ class GeojsonFormatValidator < ActiveModel::EachValidator private def parse_json(geojson_data) - JSON.parse(geojson_data) rescue nil + JSON.parse(geojson_data) + rescue JSON::ParserError + nil end - def geojson?(geojson) + def valid_geojson?(geojson) return false unless geojson.is_a?(Hash) - geojson.dig("geometry", "coordinates").is_a?(Array) + if geojson["type"] == "FeatureCollection" + valid_feature_collection?(geojson) + elsif geojson["type"] == "Feature" + valid_feature?(geojson) + else + valid_geometry?(geojson) + end + end + + def valid_feature_collection?(geojson) + return false unless geojson["features"].is_a?(Array) + + geojson["features"].all? { |feature| valid_feature?(feature) } + end + + def valid_feature?(feature) + feature["type"] == "Feature" && valid_geometry?(feature["geometry"]) + end + + def valid_geometry?(geometry) + geometry.is_a?(Hash) && valid_geometry_types.include?(geometry["type"]) + end + + def valid_geometry_types + [ + "Point", "LineString", "Polygon", "MultiPoint", "MultiLineString", "MultiPolygon", + "GeometryCollection" + ] + end + + def valid_coordinates?(geojson) + if geojson["type"] == "FeatureCollection" + geojson["features"].all? { |feature| valid_coordinates?(feature) } + elsif geojson["type"] == "Feature" + valid_geometry_coordinates?(geojson["geometry"]) + else + valid_geometry_coordinates?(geojson) + end + end + + def valid_geometry_coordinates?(geometry) + if geometry["type"] == "GeometryCollection" + geometries = geometry["geometries"] + + return geometries.is_a?(Array) && geometries.all? { |geom| valid_geometry_coordinates?(geom) } + end + + coordinates = geometry["coordinates"] + + return false unless coordinates.is_a?(Array) + + case geometry["type"] + when "Point" + valid_wgs84_coordinates?(coordinates) + when "LineString", "MultiPoint" + coordinates.all? { |coordinates| valid_wgs84_coordinates?(coordinates) } + when "Polygon", "MultiLineString" + valid_polygon_coordinates?(coordinates) + when "MultiPolygon" + coordinates.all? do |polygon_coordinates| + valid_polygon_coordinates?(polygon_coordinates) + end + else + false + end + end + + def valid_wgs84_coordinates?(coordinates) + return false unless coordinates.is_a?(Array) && coordinates.size == 2 + + longitude, latitude = coordinates + (-180.0..180.0).include?(longitude) && (-90.0..90.0).include?(latitude) + end + + def valid_polygon_coordinates?(polygon_coordinates) + polygon_coordinates.all? do |ring| + ring.all? { |coordinates| valid_wgs84_coordinates?(coordinates) } + end end end diff --git a/app/models/geozone.rb b/app/models/geozone.rb index d3fc484d5..7c72cdded 100644 --- a/app/models/geozone.rb +++ b/app/models/geozone.rb @@ -21,26 +21,40 @@ class Geozone < ApplicationRecord end def outline_points - normalized_coordinates.map { |longlat| [longlat.last, longlat.first] } + normalized_geojson&.to_json end private - def normalized_coordinates + def normalized_geojson if geojson.present? - if geojson.match(/"coordinates"\s*:\s*\[\s*\[\s*\[\s*\[/) - coordinates.reduce([], :concat).reduce([], :concat) - elsif geojson.match(/"coordinates"\s*:\s*\[\s*\[\s*\[/) - coordinates.reduce([], :concat) + parsed_geojson = JSON.parse(geojson) + + if parsed_geojson["type"] == "FeatureCollection" + parsed_geojson + elsif parsed_geojson["type"] == "Feature" + wrap_in_feature_collection(parsed_geojson) + elsif parsed_geojson["geometry"] + wrap_in_feature_collection(wrap_in_feature(parsed_geojson["geometry"])) + elsif parsed_geojson["type"] && parsed_geojson["coordinates"] + wrap_in_feature_collection(wrap_in_feature(parsed_geojson)) else - coordinates + raise ArgumentError, "Invalid GeoJSON fragment" end - else - [] end end - def coordinates - JSON.parse(geojson)["geometry"]["coordinates"] + def wrap_in_feature(geometry) + { + type: "Feature", + geometry: geometry + } + end + + def wrap_in_feature_collection(feature) + { + type: "FeatureCollection", + features: [feature] + } end end diff --git a/config/application.rb b/config/application.rb index 593c41866..ed72b992a 100644 --- a/config/application.rb +++ b/config/application.rb @@ -125,6 +125,7 @@ module Consul config.assets.paths << Rails.root.join("app", "assets", "fonts") config.assets.paths << Rails.root.join("vendor", "assets", "fonts") config.assets.paths << Rails.root.join("node_modules", "jquery-ui", "themes", "base") + config.assets.paths << Rails.root.join("node_modules", "leaflet", "dist") config.assets.paths << Rails.root.join("node_modules") config.active_job.queue_adapter = :delayed_job diff --git a/config/locales/en/activerecord.yml b/config/locales/en/activerecord.yml index 43397129f..068a4dde0 100644 --- a/config/locales/en/activerecord.yml +++ b/config/locales/en/activerecord.yml @@ -551,7 +551,8 @@ en: geozone: attributes: geojson: - invalid: "The GeoJSON provided does not follow the correct format. It must follow the \"Polygon\" or \"MultiPolygon\" type format." + invalid: "The GeoJSON provided does not follow the correct format. It must follow the RFC 7946 standard format" + invalid_coordinates: "The GeoJSON provided contains invalid coordinates; the coordinates must be in the required \"Longitude, Latitude\" format and follow the RFC 7946 standard format" image: attributes: attachment: diff --git a/config/locales/es/activerecord.yml b/config/locales/es/activerecord.yml index 4aa224ae7..9bab05b5a 100644 --- a/config/locales/es/activerecord.yml +++ b/config/locales/es/activerecord.yml @@ -551,7 +551,8 @@ es: geozone: attributes: geojson: - invalid: "Los datos GeoJSON proporcionados no tienen el formato correcto. Deben tener un tipo del formato \"Polygon\" o \"MultiPolygon\"." + invalid: "Los datos GeoJSON proporcionados no tienen el formato correcto. Deben seguir el formato estándar RFC 7946" + invalid_coordinates: "Los datos GeoJSON proporcionados contienen coordenadas inválidas; las coordenadas deben utilizar el formato \"Longitud, Latitud\" y seguir el formato estándar RFC 7946" image: attributes: attachment: diff --git a/spec/factories/administration.rb b/spec/factories/administration.rb index 1855b1069..b02cfcb20 100644 --- a/spec/factories/administration.rb +++ b/spec/factories/administration.rb @@ -20,7 +20,15 @@ FactoryBot.define do trait :with_geojson do geojson do - '{ "geometry": { "type": "Polygon", "coordinates": [[0.117,51.513],[0.118,51.512],[0.119,51.514]] } }' + <<~JSON + { + "type": "Feature", + "geometry": { + "type": "Polygon", + "coordinates": [[[0.117, 51.513], [0.118, 51.512], [0.119, 51.514]]] + } + } + JSON end end end diff --git a/spec/models/geojson_format_validator_spec.rb b/spec/models/geojson_format_validator_spec.rb new file mode 100644 index 000000000..1a3ebbf54 --- /dev/null +++ b/spec/models/geojson_format_validator_spec.rb @@ -0,0 +1,329 @@ +require "rails_helper" + +describe GeojsonFormatValidator do + before do + dummy_model = Class.new do + include ActiveModel::Model + attr_accessor :geojson + validates :geojson, geojson_format: true + end + + stub_const("DummyModel", dummy_model) + end + + let(:record) { DummyModel.new } + + it "is not valid with an empty hash" do + record.geojson = "{}" + + expect(record).not_to be_valid + end + + it "is not valid with arbitrary keys" do + record.geojson = '{ "invalid": "yes" }' + + expect(record).not_to be_valid + end + + it "is not valid without a type" do + record.geojson = '{ "coordinates": [1.23, 4.56] }' + + expect(record).not_to be_valid + end + + it "is not valid without a type but a geometry" do + record.geojson = '{ "geometry": { "type": "Point", "coordinates": [1.23, 4.56] } }' + + expect(record).not_to be_valid + end + + context "Point geometry" do + it "is not valid without coordinates" do + record.geojson = '{ "type": "Point" }' + + expect(record).not_to be_valid + end + + it "is not valid with only one the longitude" do + record.geojson = '{ "type": "Point", "coordinates": 1.23 }' + + expect(record).not_to be_valid + end + + it "is not valid with non-numerical coordinates" do + record.geojson = '{ "type": "Point", "coordinates": ["1.23", "4.56"] }' + + expect(record).not_to be_valid + end + + it "is not valid with 3-dimensional coordinates" do + record.geojson = '{ "type": "Point", "coordinates": [1.23, 4.56, 7.89] }' + + expect(record).not_to be_valid + end + + it "is not valid with multiple coordinates" do + record.geojson = '{ "type": "Point", "coordinates": [[1.23, 4.56], [7.89, 10.11]] }' + + expect(record).not_to be_valid + end + + it "is not valid with a longitude above 180" do + record.geojson = '{ "type": "Point", "coordinates": [180.01, 4.56] }' + + expect(record).not_to be_valid + end + + it "is not valid with a longitude below -180" do + record.geojson = '{ "type": "Point", "coordinates": [-180.01, 4.56] }' + + expect(record).not_to be_valid + end + + it "is not valid with a latitude above 90" do + record.geojson = '{ "type": "Point", "coordinates": [1.23, 90.01] }' + + expect(record).not_to be_valid + end + + it "is not valid with a latitude below -90" do + record.geojson = '{ "type": "Point", "coordinates": [1.23, -90.01] }' + + expect(record).not_to be_valid + end + + it "is valid with coordinates in the valid range" do + record.geojson = '{ "type": "Point", "coordinates": [1.23, 4.56] }' + + expect(record).to be_valid + end + + it "is valid with coordinates at the positive end of the range" do + record.geojson = '{ "type": "Point", "coordinates": [180.0, 90.0] }' + + expect(record).to be_valid + end + + it "is valid with coordinates at the negative end of the range" do + record.geojson = '{ "type": "Point", "coordinates": [-180.0, -90.0] }' + + expect(record).to be_valid + end + end + + context "LineString or MultiPoint geometry" do + it "is not valid with a one-dimensional array of coordinates" do + record.geojson = '{ "type": "LineString", "coordinates": [1.23, 4.56] }' + + expect(record).not_to be_valid + + record.geojson = '{ "type": "MultiPoint", "coordinates": [1.23, 4.56] }' + + expect(record).not_to be_valid + end + + it "is valid with a two-dimensional array including only one point" do + record.geojson = '{ "type": "LineString", "coordinates": [[1.23, 4.56]] }' + + expect(record).to be_valid + + record.geojson = '{ "type": "MultiPoint", "coordinates": [[1.23, 4.56]] }' + + expect(record).to be_valid + end + + it "is not valid when some coordinates are invalid" do + record.geojson = '{ "type": "LineString", "coordinates": [[1.23, 4.56], [180.01, 4.56]] }' + + expect(record).not_to be_valid + + record.geojson = '{ "type": "MultiPoint", "coordinates": [[1.23, 4.56], [180.01, 4.56]] }' + + expect(record).not_to be_valid + end + + it "is valid when all the coordinates are valid" do + record.geojson = '{ "type": "LineString", "coordinates": [[1.23, 4.56], [7.89, 4.56]] }' + + expect(record).to be_valid + + record.geojson = '{ "type": "MultiPoint", "coordinates": [[1.23, 4.56], [7.89, 4.56]] }' + + expect(record).to be_valid + end + end + + context "GeometryCollection" do + it "is not valid if it doesn't contain geometries" do + record.geojson = '{ "type": "GeometryCollection" }' + + expect(record).not_to be_valid + end + + it "is not valid if geometries is not an array" do + record.geojson = <<~JSON + { + "type": "GeometryCollection", + "geometries": { "type": "Point", "coordinates": [1.23, 4.56] } + } + JSON + + expect(record).not_to be_valid + end + + it "is valid if the array of geometries is empty" do + record.geojson = '{ "type": "GeometryCollection", "geometries": [] }' + + expect(record).to be_valid + end + + it "is valid if all geometries are valid" do + record.geojson = <<~JSON + { + "type": "GeometryCollection", + "geometries": [ + { + "type": "Point", + "coordinates": [100.0, 0.0] + }, + { + "type": "LineString", + "coordinates": [ + [101.0, 0.0], + [102.0, 1.0] + ] + } + ] + } + JSON + + expect(record).to be_valid + end + + it "is not valid if some geometries are invalid" do + record.geojson = <<~JSON + { + "type": "GeometryCollection", + "geometries": [ + { + "type": "Point", + "coordinates": [100.0, 0.0] + }, + { + "type": "LineString", + "coordinates": [101.0, 0.0] + } + ] + } + JSON + + expect(record).not_to be_valid + end + end + + context "Feature" do + it "is valid with a valid geometry" do + record.geojson = <<~JSON + { + "type": "Feature", + "geometry": { + "type": "Point", + "coordinates": [1.23, 4.56] + } + } + JSON + + expect(record).to be_valid + end + + it "is not valid with a valid geometry" do + record.geojson = <<~JSON + { + "type": "Feature", + "geometry": { + "type": "Point", + "coordinates": [1.23] + } + } + JSON + + expect(record).not_to be_valid + end + end + + context "FeatureCollection" do + it "is not valid without features" do + record.geojson = '{ "type": "FeatureCollection" }' + end + + it "is not valid if features is not an array" do + record.geojson = <<~JSON + { + "type": "FeatureCollection", + "features": { + "type": "Feature", + "geometry": { + "type": "Point", + "coordinates": [1.23, 4.56] + } + } + } + JSON + end + + it "is valid if the array of features is empty" do + record.geojson = '{ "type": "FeatureCollection", "features": [] }' + + expect(record).to be_valid + end + + it "is valid if all features are valid" do + record.geojson = <<~JSON + { + "type": "FeatureCollection", + "features": [ + { + "type": "Feature", + "geometry": { + "type": "Point", + "coordinates": [1.23, 4.56] + } + }, + { + "type": "Feature", + "geometry": { + "type": "LineString", + "coordinates": [[101.0, 0.0], [102.0, 1.0]] + } + } + ] + } + JSON + + expect(record).to be_valid + end + + it "is not valid if some features are invalid" do + record.geojson = <<~JSON + { + "type": "FeatureCollection", + "features": [ + { + "type": "Feature", + "geometry": { + "type": "Point", + "coordinates": [1.23, 4.56] + } + }, + { + "type": "LineString", + "coordinates": [[101.0, 0.0], [102.0, 1.0]] + } + ] + } + JSON + + expect(record).not_to be_valid + end + end +end diff --git a/spec/models/geozone_spec.rb b/spec/models/geozone_spec.rb index 973e61139..dcae19120 100644 --- a/spec/models/geozone_spec.rb +++ b/spec/models/geozone_spec.rb @@ -18,10 +18,20 @@ describe Geozone do end it "is not valid with invalid geojson file format" do - geozone.geojson = '{"geo\":{"type":"Incorrect key","coordinates": [ - [40.8792937308316, -3.9259027239257], - [40.8788966596619, -3.9249047078766], - [40.8789131852224, -3.9247799675785]]}}' + geozone.geojson = <<~JSON + { + "type": "Feature", + "geometry": { + "type": "Incorrect", + "coordinates": [ + [40.8792937308316, -3.9259027239257], + [40.8788966596619, -3.9249047078766], + [40.8789131852224, -3.9247799675785] + ] + } + } + JSON + expect(geozone).not_to be_valid end @@ -54,99 +64,149 @@ describe Geozone do end describe "#outline_points" do - it "returns empty array when geojson is nil" do - expect(geozone.outline_points).to eq([]) + it "returns nil when geojson is nil" do + geozone.geojson = nil + + expect(geozone.outline_points).to be nil end - it "returns coordinates array when geojson is not nil" do - geozone = build(:geozone, geojson: '{ - "geometry": { - "type": "Polygon", - "coordinates": [ - [40.8792937308316, -3.9259027239257], - [40.8788966596619, -3.9249047078766], - [40.8789131852224, -3.9247799675785] - ] + it "returns normalized feature collection when geojson is a valid FeatureCollection" do + geozone.geojson = <<~JSON + { + "type": "FeatureCollection", + "features": [{ + "type": "Feature", + "geometry": { + "type": "Polygon", + "coordinates": [[ + [-3.9259027239257, 40.8792937308316], + [-3.9249047078766, 40.8788966596619], + [-3.9247799675785, 40.8789131852224], + [-3.9259027239257, 40.8792937308316] + ]] + } + }] } - }') + JSON - expect(geozone.outline_points).to eq( - [[-3.9259027239257, 40.8792937308316], - [-3.9249047078766, 40.8788966596619], - [-3.9247799675785, 40.8789131852224]] - ) + expected = { + type: "FeatureCollection", + features: [{ + type: "Feature", + geometry: { + type: "Polygon", + coordinates: [[ + [-3.9259027239257, 40.8792937308316], + [-3.9249047078766, 40.8788966596619], + [-3.9247799675785, 40.8789131852224], + [-3.9259027239257, 40.8792937308316] + ]] + } + }] + } + + expect(geozone.outline_points).to eq expected.to_json end - it "handles coordinates with three-dimensional arrays" do - geozone = build(:geozone, geojson: '{ - "geometry": { - "type": "Polygon", - "coordinates": [[[40.8792937308316, -3.9259027239257], - [40.8788966596619, -3.9249047078766], - [40.8789131852224, -3.9247799675785]]] + it "returns normalized feature collection when geojson is a valid Feature" do + geozone.geojson = <<~JSON + { + "type": "Feature", + "geometry": { + "type": "Polygon", + "coordinates": [[ + [-3.9259027239257, 40.8792937308316], + [-3.9249047078766, 40.8788966596619], + [-3.9247799675785, 40.8789131852224], + [-3.9259027239257, 40.8792937308316] + ]] + } } - }') + JSON - expect(geozone.outline_points).to eq( - [[-3.9259027239257, 40.8792937308316], - [-3.9249047078766, 40.8788966596619], - [-3.9247799675785, 40.8789131852224]] - ) + expected = { + type: "FeatureCollection", + features: [{ + type: "Feature", + geometry: { + type: "Polygon", + coordinates: [[ + [-3.9259027239257, 40.8792937308316], + [-3.9249047078766, 40.8788966596619], + [-3.9247799675785, 40.8789131852224], + [-3.9259027239257, 40.8792937308316] + ]] + } + }] + } + + expect(geozone.outline_points).to eq expected.to_json end - it "handles coordinates with three-dimensional arrays with spaces between brackets" do - geozone = build(:geozone, geojson: '{ - "geometry": { + it "returns normalized feature collection when geojson is a valid Geometry object" do + geozone.geojson = <<~JSON + { + "geometry": { + "type": "Polygon", + "coordinates": [[ + [-3.9259027239257, 40.8792937308316], + [-3.9249047078766, 40.8788966596619], + [-3.9247799675785, 40.8789131852224], + [-3.9259027239257, 40.8792937308316] + ]] + } + } + JSON + + expected = { + type: "FeatureCollection", + features: [{ + type: "Feature", + geometry: { + type: "Polygon", + coordinates: [[ + [-3.9259027239257, 40.8792937308316], + [-3.9249047078766, 40.8788966596619], + [-3.9247799675785, 40.8789131852224], + [-3.9259027239257, 40.8792937308316] + ]] + } + }] + } + + expect(geozone.outline_points).to eq expected.to_json + end + + it "returns normalized feature collection when geojson is a valid top-level Geometry object" do + geozone.geojson = <<~JSON + { "type": "Polygon", "coordinates": [[ - [40.8792937308316, -3.9259027239257], - [40.8788966596619, -3.9249047078766], - [40.8789131852224, -3.9247799675785] + [-3.9259027239257, 40.8792937308316], + [-3.9249047078766, 40.8788966596619], + [-3.9247799675785, 40.8789131852224], + [-3.9259027239257, 40.8792937308316] ]] } - }') + JSON - expect(geozone.outline_points).to eq( - [[-3.9259027239257, 40.8792937308316], - [-3.9249047078766, 40.8788966596619], - [-3.9247799675785, 40.8789131852224]] - ) - end + expected = { + type: "FeatureCollection", + features: [{ + type: "Feature", + geometry: { + type: "Polygon", + coordinates: [[ + [-3.9259027239257, 40.8792937308316], + [-3.9249047078766, 40.8788966596619], + [-3.9247799675785, 40.8789131852224], + [-3.9259027239257, 40.8792937308316] + ]] + } + }] + } - it "handles coordinates with four-dimensional arrays" do - geozone = build(:geozone, geojson: '{ - "geometry": { - "type": "Polygon", - "coordinates": [[[[40.8792937308316, -3.9259027239257], - [40.8788966596619, -3.9249047078766], - [40.8789131852224, -3.9247799675785]]]] - } - }') - - expect(geozone.outline_points).to eq( - [[-3.9259027239257, 40.8792937308316], - [-3.9249047078766, 40.8788966596619], - [-3.9247799675785, 40.8789131852224]] - ) - end - - it "handles coordinates with four-dimensional arrays with spaces between brackets" do - geozone = build(:geozone, geojson: '{ - "geometry": { - "type": "Polygon", - "coordinates": [[[ - [40.8792937308316, -3.9259027239257], - [40.8788966596619, -3.9249047078766], - [40.8789131852224, -3.9247799675785] - ]]] - } - }') - - expect(geozone.outline_points).to eq( - [[-3.9259027239257, 40.8792937308316], - [-3.9249047078766, 40.8788966596619], - [-3.9247799675785, 40.8789131852224]] - ) + expect(geozone.outline_points).to eq expected.to_json end end end diff --git a/spec/system/admin/geozones_spec.rb b/spec/system/admin/geozones_spec.rb index 3ece9f6f4..2adec9b3e 100644 --- a/spec/system/admin/geozones_spec.rb +++ b/spec/system/admin/geozones_spec.rb @@ -110,8 +110,16 @@ describe "Admin geozones", :admin do scenario "Show polygons when a heading is associated with a geozone" do Setting["feature.map"] = true + geojson = <<~JSON + { + "type": "Feature", + "geometry": { + "type": "Polygon", + "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6]]] + } + } + JSON - geojson = '{ "geometry": { "type": "Polygon", "coordinates": [[-0.1,51.5],[-0.2,51.4],[-0.3,51.6]] } }' geozone = create(:geozone, name: "Polygon me!") budget = create(:budget) group = create(:budget_group, budget: budget) @@ -145,7 +153,16 @@ describe "Admin geozones", :admin do scenario "Show polygons on geozone admin view" do Setting["feature.map"] = true - geojson = '{ "geometry": { "type": "Polygon", "coordinates": [[-0.1,51.5],[-0.2,51.4],[-0.3,51.6]] } }' + geojson = <<~JSON + { + "type": "Feature", + "geometry": { + "type": "Polygon", + "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6]]] + } + } + JSON + geozone = create(:geozone, name: "Polygon me!", geojson: geojson) visit admin_geozones_path diff --git a/spec/system/budgets/investments_spec.rb b/spec/system/budgets/investments_spec.rb index b18ff6e81..45930fe45 100644 --- a/spec/system/budgets/investments_spec.rb +++ b/spec/system/budgets/investments_spec.rb @@ -1658,18 +1658,20 @@ describe "Budget Investments" do scenario "Shows the polygon associated to the current heading" do triangle = <<~JSON { + "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[-0.1,51.5],[-0.2,51.4],[-0.3,51.6]] + "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6]]] } } JSON rectangle = <<~JSON { + "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[-0.1,51.5],[-0.2,51.5],[-0.2,51.6],[-0.1,51.6]] + "coordinates": [[[-0.1, 51.5], [-0.2, 51.5], [-0.2, 51.6], [-0.1, 51.6]]] } } JSON From cb8b0ad6ff498a6e925aefde8232d77cabbfe495 Mon Sep 17 00:00:00 2001 From: CoslaJohn Date: Fri, 19 Jul 2024 12:15:00 +0100 Subject: [PATCH 2/7] Support different colors and headings on each feature We're making sure each feature contains properties in order to avoid possible JavaScript errors. We're also adding a default color to a geozone. --- app/assets/javascripts/map.js | 16 ++++++++++------ app/models/geozone.rb | 13 ++++++++++++- spec/models/geozone_spec.rb | 12 ++++++++---- spec/system/admin/geozones_spec.rb | 29 +++++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 11 deletions(-) diff --git a/app/assets/javascripts/map.js b/app/assets/javascripts/map.js index cb5b9835f..daae3393b 100644 --- a/app/assets/javascripts/map.js +++ b/app/assets/javascripts/map.js @@ -223,14 +223,18 @@ var geojsonData = JSON.parse(geozone.outline_points); var geoJsonLayer = L.geoJSON(geojsonData, { - style: { - color: geozone.color, - fillOpacity: 0.3, - className: "map-polygon" + style: function(feature) { + return { + color: feature.properties.color || geozone.color, + fillOpacity: 0.3, + className: "map-polygon" + }; }, onEachFeature: function(feature, layer) { - if (geozone.headings) { - layer.bindPopup(geozone.headings.join("
")); + var headings = feature.properties.headings || geozone.headings; + + if (headings) { + layer.bindPopup(headings.join("
")); } } }); diff --git a/app/models/geozone.rb b/app/models/geozone.rb index 7c72cdded..b37a4e7ef 100644 --- a/app/models/geozone.rb +++ b/app/models/geozone.rb @@ -1,6 +1,8 @@ class Geozone < ApplicationRecord include Graphqlable + attribute :color, default: "#0000ff" + has_many :proposals has_many :debates has_many :users @@ -31,10 +33,18 @@ class Geozone < ApplicationRecord parsed_geojson = JSON.parse(geojson) if parsed_geojson["type"] == "FeatureCollection" + parsed_geojson["features"].each do |feature| + feature["properties"] ||= {} + end + parsed_geojson elsif parsed_geojson["type"] == "Feature" + parsed_geojson["properties"] ||= {} + wrap_in_feature_collection(parsed_geojson) elsif parsed_geojson["geometry"] + parsed_geojson["properties"] ||= {} + wrap_in_feature_collection(wrap_in_feature(parsed_geojson["geometry"])) elsif parsed_geojson["type"] && parsed_geojson["coordinates"] wrap_in_feature_collection(wrap_in_feature(parsed_geojson)) @@ -47,7 +57,8 @@ class Geozone < ApplicationRecord def wrap_in_feature(geometry) { type: "Feature", - geometry: geometry + geometry: geometry, + properties: {} } end diff --git a/spec/models/geozone_spec.rb b/spec/models/geozone_spec.rb index dcae19120..ec4735d86 100644 --- a/spec/models/geozone_spec.rb +++ b/spec/models/geozone_spec.rb @@ -101,7 +101,8 @@ describe Geozone do [-3.9247799675785, 40.8789131852224], [-3.9259027239257, 40.8792937308316] ]] - } + }, + properties: {} }] } @@ -136,7 +137,8 @@ describe Geozone do [-3.9247799675785, 40.8789131852224], [-3.9259027239257, 40.8792937308316] ]] - } + }, + properties: {} }] } @@ -170,7 +172,8 @@ describe Geozone do [-3.9247799675785, 40.8789131852224], [-3.9259027239257, 40.8792937308316] ]] - } + }, + properties: {} }] } @@ -202,7 +205,8 @@ describe Geozone do [-3.9247799675785, 40.8789131852224], [-3.9259027239257, 40.8792937308316] ]] - } + }, + properties: {} }] } diff --git a/spec/system/admin/geozones_spec.rb b/spec/system/admin/geozones_spec.rb index 2adec9b3e..922472574 100644 --- a/spec/system/admin/geozones_spec.rb +++ b/spec/system/admin/geozones_spec.rb @@ -173,4 +173,33 @@ describe "Admin geozones", :admin do expect(page).to have_link "Polygon me!", href: edit_admin_geozone_path(geozone) end end + + scenario "overwrites geozone data with features data" do + geojson = <<~JSON + { + "type": "Feature", + "geometry": { + "type": "Polygon", + "coordinates": [[[-0.1, 51.5], [-0.2, 51.5], [-0.2, 51.6], [-0.1, 51.6], [-0.1, 51.5]]] + }, + "properties": { + "color": "#ff5733", + "headings": ["Zone 1", "Test zone"] + } + } + JSON + + create(:geozone, color: "#001122", geojson: geojson) + + visit admin_geozones_path + + expect(page).to have_css ".map-polygon[fill='#ff5733']" + expect(page).not_to have_css ".map-polygon[fill='#001122']" + expect(page).not_to have_content "Zone 1" + expect(page).not_to have_content "Test zone" + + find(".map-polygon").click + + expect(page).to have_content "Zone 1\nTest zone" + end end From 624e60eab9696bbed1db066911d2dbc6d2945856 Mon Sep 17 00:00:00 2001 From: CoslaJohn Date: Wed, 7 Aug 2024 14:14:34 +0100 Subject: [PATCH 3/7] Added layer control to map to allow each geozone display to be toggled on/off Note we're adding a `name` property to the geozones investments sidebar map even if we don't render the geozones in the map, in order to simplify the JavaScript function `geozoneLayers`. --- app/assets/javascripts/map.js | 35 +++++++++++---- .../admin/geozones/index_component.rb | 3 +- .../budgets/investments/map_component.rb | 3 +- app/components/budgets/map_component.rb | 9 +++- spec/system/admin/geozones_spec.rb | 43 +++++++++++++++++++ 5 files changed, 81 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/map.js b/app/assets/javascripts/map.js index daae3393b..9cbbb7c8a 100644 --- a/app/assets/javascripts/map.js +++ b/app/assets/javascripts/map.js @@ -15,7 +15,7 @@ App.Map.maps = []; }, initializeMap: function(element) { - var createMarker, editable, investmentsMarkers, map, marker, markerClustering, + var createMarker, editable, geozoneLayers, investmentsMarkers, map, marker, markerClustering, markerData, markerIcon, markers, moveOrPlaceMarker, removeMarker, removeMarkerSelector; App.Map.cleanInvestmentCoordinates(element); removeMarkerSelector = $(element).data("marker-remove-selector"); @@ -84,7 +84,10 @@ } App.Map.addInvestmentsMarkers(investmentsMarkers, createMarker); - App.Map.addGeozones(map); + geozoneLayers = App.Map.geozoneLayers(map); + App.Map.addGeozones(map, geozoneLayers); + App.Map.addLayerControl(map, geozoneLayers); + map.addLayer(markers); }, leafletMap: function(element) { @@ -210,19 +213,34 @@ map.attributionControl.setPrefix(App.Map.attributionPrefix()); L.tileLayer(mapTilesProvider, { attribution: mapAttribution }).addTo(map); }, - addGeozones: function(map) { + addGeozones: function(map, geozoneLayers) { + $.each(geozoneLayers, function(_, geozoneLayer) { + App.Map.addGeozone(map, geozoneLayer); + }); + }, + addLayerControl: function(map, geozoneLayers) { + if (Object.keys(geozoneLayers).length > 1) { + L.control.layers(null, geozoneLayers).addTo(map); + } + }, + geozoneLayers: function(map) { var geozones = $(map._container).data("geozones"); + var layers = {}; if (geozones) { geozones.forEach(function(geozone) { - App.Map.addGeozone(geozone, map); + if (geozone.outline_points) { + layers[geozone.name] = App.Map.geozoneLayer(geozone); + } }); } + + return layers; }, - addGeozone: function(geozone, map) { + geozoneLayer: function(geozone) { var geojsonData = JSON.parse(geozone.outline_points); - var geoJsonLayer = L.geoJSON(geojsonData, { + return L.geoJSON(geojsonData, { style: function(feature) { return { color: feature.properties.color || geozone.color, @@ -238,8 +256,9 @@ } } }); - - geoJsonLayer.addTo(map); + }, + addGeozone: function(map, geozoneLayer) { + geozoneLayer.addTo(map); }, getPopupContent: function(data) { return "" + data.title + ""; diff --git a/app/components/admin/geozones/index_component.rb b/app/components/admin/geozones/index_component.rb index bdade404e..5d25a7188 100644 --- a/app/components/admin/geozones/index_component.rb +++ b/app/components/admin/geozones/index_component.rb @@ -25,7 +25,8 @@ class Admin::Geozones::IndexComponent < ApplicationComponent { outline_points: geozone.outline_points, color: geozone.color, - headings: [link_to(geozone.name, edit_admin_geozone_path(geozone))] + headings: [link_to(geozone.name, edit_admin_geozone_path(geozone))], + name: geozone.name } end end diff --git a/app/components/budgets/investments/map_component.rb b/app/components/budgets/investments/map_component.rb index 9730cf184..fe887ae28 100644 --- a/app/components/budgets/investments/map_component.rb +++ b/app/components/budgets/investments/map_component.rb @@ -27,7 +27,8 @@ class Budgets::Investments::MapComponent < ApplicationComponent [ { outline_points: heading.geozone.outline_points, - color: heading.geozone.color + color: heading.geozone.color, + name: heading.name } ] end diff --git a/app/components/budgets/map_component.rb b/app/components/budgets/map_component.rb index 3124f8cfa..97ae0ef20 100644 --- a/app/components/budgets/map_component.rb +++ b/app/components/budgets/map_component.rb @@ -27,10 +27,15 @@ class Budgets::MapComponent < ApplicationComponent { outline_points: geozone.outline_points, color: geozone.color, - headings: budget.headings.where(geozone: geozone).map do |heading| + headings: geozone_headings(geozone).map do |heading| link_to heading.name, budget_investments_path(budget, heading_id: heading.id) - end + end, + name: geozone_headings(geozone).map(&:name).join(", ") } end end + + def geozone_headings(geozone) + budget.headings.where(geozone: geozone) + end end diff --git a/spec/system/admin/geozones_spec.rb b/spec/system/admin/geozones_spec.rb index 922472574..37b448d2a 100644 --- a/spec/system/admin/geozones_spec.rb +++ b/spec/system/admin/geozones_spec.rb @@ -202,4 +202,47 @@ describe "Admin geozones", :admin do expect(page).to have_content "Zone 1\nTest zone" end + + scenario "includes a control to select which geozones to display" do + north = <<~JSON + { + "type": "Feature", + "geometry": { + "type": "Polygon", + "coordinates": [[[-0.1, 51.5], [-0.2, 51.5], [-0.2, 51.6], [-0.1, 51.6], [-0.1, 51.5]]] + }, + "properties": {} + } + JSON + + south = <<~JSON + { + "type": "Feature", + "geometry": { + "type": "Polygon", + "coordinates": [[[-0.1, 51.45], [-0.2, 51.45], [-0.2, 51.35], [-0.1, 51.35], [-0.1, 51.45]]] + }, + "properties": {} + } + JSON + + create(:geozone, name: "North", geojson: north) + create(:geozone, name: "South", geojson: south) + + visit admin_geozones_path + + within(".map-location") do + expect(page).to have_css ".map-polygon", count: 2 + + find(".leaflet-control-layers").click + uncheck "South" + + expect(page).to have_css ".map-polygon", count: 1 + + find(".map-polygon").click + + expect(page).to have_content "North" + expect(page).not_to have_content "South" + end + end end From 9ef68f863a086f09045bc953fee4775f96eac236 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 29 Nov 2024 17:06:35 +0100 Subject: [PATCH 4/7] Make sure a LineString has at least two points According to the GeoJSON specification [1]: > For type "LineString", the "coordinates" member is an array of two or > more positions. Note that the same doesn't seem to apply to a MultiPoint [2]: > For type "MultiPoint", the "coordinates" member is an array of > positions. [1] https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.4 [2] https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.3 --- .../concerns/geojson_format_validator.rb | 14 +++++++--- spec/models/geojson_format_validator_spec.rb | 26 ++++++++++++------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/app/models/concerns/geojson_format_validator.rb b/app/models/concerns/geojson_format_validator.rb index 1614cc794..d3a7dc58d 100644 --- a/app/models/concerns/geojson_format_validator.rb +++ b/app/models/concerns/geojson_format_validator.rb @@ -79,8 +79,10 @@ class GeojsonFormatValidator < ActiveModel::EachValidator case geometry["type"] when "Point" valid_wgs84_coordinates?(coordinates) - when "LineString", "MultiPoint" - coordinates.all? { |coordinates| valid_wgs84_coordinates?(coordinates) } + when "LineString" + coordinates.many? && valid_coordinates_array?(coordinates) + when "MultiPoint" + valid_coordinates_array?(coordinates) when "Polygon", "MultiLineString" valid_polygon_coordinates?(coordinates) when "MultiPolygon" @@ -99,9 +101,13 @@ class GeojsonFormatValidator < ActiveModel::EachValidator (-180.0..180.0).include?(longitude) && (-90.0..90.0).include?(latitude) end + def valid_coordinates_array?(coordinates_array) + coordinates_array.all? { |coordinates| valid_wgs84_coordinates?(coordinates) } + end + def valid_polygon_coordinates?(polygon_coordinates) - polygon_coordinates.all? do |ring| - ring.all? { |coordinates| valid_wgs84_coordinates?(coordinates) } + polygon_coordinates.all? do |ring_coordinates| + valid_coordinates_array?(ring_coordinates) end end end diff --git a/spec/models/geojson_format_validator_spec.rb b/spec/models/geojson_format_validator_spec.rb index 1a3ebbf54..f05aa5b74 100644 --- a/spec/models/geojson_format_validator_spec.rb +++ b/spec/models/geojson_format_validator_spec.rb @@ -122,16 +122,6 @@ describe GeojsonFormatValidator do expect(record).not_to be_valid end - it "is valid with a two-dimensional array including only one point" do - record.geojson = '{ "type": "LineString", "coordinates": [[1.23, 4.56]] }' - - expect(record).to be_valid - - record.geojson = '{ "type": "MultiPoint", "coordinates": [[1.23, 4.56]] }' - - expect(record).to be_valid - end - it "is not valid when some coordinates are invalid" do record.geojson = '{ "type": "LineString", "coordinates": [[1.23, 4.56], [180.01, 4.56]] }' @@ -153,6 +143,22 @@ describe GeojsonFormatValidator do end end + context "LineString geometry" do + it "is not valid with only one point" do + record.geojson = '{ "type": "LineString", "coordinates": [[1.23, 4.56]] }' + + expect(record).not_to be_valid + end + end + + context "MultiPoint geometry" do + it "is valid with only one point" do + record.geojson = '{ "type": "MultiPoint", "coordinates": [[1.23, 4.56]] }' + + expect(record).to be_valid + end + end + context "GeometryCollection" do it "is not valid if it doesn't contain geometries" do record.geojson = '{ "type": "GeometryCollection" }' From c3bda443a6b9ac844d9a049e44199d0c0feb236a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 2 Dec 2024 12:38:28 +0100 Subject: [PATCH 5/7] Make sure all lines in a MultiLineString are valid Note we're starting to use hashes in tests because the objects here are complex and using hashes makes the tests easier to read. --- .../concerns/geojson_format_validator.rb | 15 ++++- spec/models/geojson_format_validator_spec.rb | 58 +++++++++++++++++++ 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/geojson_format_validator.rb b/app/models/concerns/geojson_format_validator.rb index d3a7dc58d..42950c1e2 100644 --- a/app/models/concerns/geojson_format_validator.rb +++ b/app/models/concerns/geojson_format_validator.rb @@ -80,10 +80,14 @@ class GeojsonFormatValidator < ActiveModel::EachValidator when "Point" valid_wgs84_coordinates?(coordinates) when "LineString" - coordinates.many? && valid_coordinates_array?(coordinates) + valid_linestring_coordinates?(coordinates) when "MultiPoint" valid_coordinates_array?(coordinates) - when "Polygon", "MultiLineString" + when "MultiLineString" + coordinates.all? do |linestring_coordinates| + valid_linestring_coordinates?(linestring_coordinates) + end + when "Polygon" valid_polygon_coordinates?(coordinates) when "MultiPolygon" coordinates.all? do |polygon_coordinates| @@ -102,7 +106,12 @@ class GeojsonFormatValidator < ActiveModel::EachValidator end def valid_coordinates_array?(coordinates_array) - coordinates_array.all? { |coordinates| valid_wgs84_coordinates?(coordinates) } + coordinates_array.is_a?(Array) && + coordinates_array.all? { |coordinates| valid_wgs84_coordinates?(coordinates) } + end + + def valid_linestring_coordinates?(coordinates) + valid_coordinates_array?(coordinates) && coordinates.many? end def valid_polygon_coordinates?(polygon_coordinates) diff --git a/spec/models/geojson_format_validator_spec.rb b/spec/models/geojson_format_validator_spec.rb index f05aa5b74..c4b4780f9 100644 --- a/spec/models/geojson_format_validator_spec.rb +++ b/spec/models/geojson_format_validator_spec.rb @@ -159,6 +159,64 @@ describe GeojsonFormatValidator do end end + context "Polygon or MultiLineString geometry" do + it "is not valid with a one-dimensional array of coordinates" do + record.geojson = '{ "type": "MultiLineString", "coordinates": [1.23, 4.56] }' + + expect(record).not_to be_valid + + record.geojson = '{ "type": "Polygon", "coordinates": [1.23, 4.56] }' + + expect(record).not_to be_valid + end + + it "is not valid with a two-dimensional array of coordinates" do + record.geojson = '{ "type": "MultiLineString", "coordinates": [[1.23, 4.56], [7.89, 4.56]] }' + + expect(record).not_to be_valid + + record.geojson = '{ "type": "Polygon", "coordinates": [[1.23, 4.56], [7.89, 4.56]] }' + + expect(record).not_to be_valid + end + end + + context "MultiLineString geometry" do + it "is valid with just one line" do + record.geojson = '{ "type": "MultiLineString", "coordinates": [[[1.23, 4.56], [7.89, 4.56]]] }' + + expect(record).to be_valid + end + + it "is valid with multiple valid lines" do + record.geojson = <<~JSON + { + "type": "MultiLineString", + "coordinates": [ + [[1.23, 4.56], [7.89, 4.56]], + [[10.11, 12.13], [14.15, 16.17]] + ] + } + JSON + + expect(record).to be_valid + end + + it "is not valid if some lines are invalid" do + record.geojson = <<~JSON + { + "type": "MultiLineString", + "coordinates": [ + [[1.23, 4.56], [7.89, 4.56]], + [[10.11, 12.13]] + ] + } + JSON + + expect(record).not_to be_valid + end + end + context "GeometryCollection" do it "is not valid if it doesn't contain geometries" do record.geojson = '{ "type": "GeometryCollection" }' From 1f627d34f1b028f079807a32ea65f01b8e5e8e06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 2 Dec 2024 13:21:49 +0100 Subject: [PATCH 6/7] Make sure polygons contain valid rings According to the GeoJSON specification [1]: > * A linear ring is a closed LineString with four or more positions. > * The first and last positions are equivalent, and they MUST contain > identical values; their representation SHOULD also be identical. > (...) > * For type "Polygon", the "coordinates" member MUST be an array of > linear ring coordinate arrays. Note that, for simplicity, right now we aren't checking whether the coordinates are defined counterclockwise for exterior rings and clockwise for interior rings, which is what the specification expects. [1] https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.6 --- .../concerns/geojson_format_validator.rb | 11 +- spec/factories/administration.rb | 2 +- spec/models/geojson_format_validator_spec.rb | 215 ++++++++++++++++++ spec/system/admin/geozones_spec.rb | 4 +- spec/system/budgets/investments_spec.rb | 4 +- 5 files changed, 228 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/geojson_format_validator.rb b/app/models/concerns/geojson_format_validator.rb index 42950c1e2..2182aa3b3 100644 --- a/app/models/concerns/geojson_format_validator.rb +++ b/app/models/concerns/geojson_format_validator.rb @@ -115,8 +115,13 @@ class GeojsonFormatValidator < ActiveModel::EachValidator end def valid_polygon_coordinates?(polygon_coordinates) - polygon_coordinates.all? do |ring_coordinates| - valid_coordinates_array?(ring_coordinates) - end + polygon_coordinates.is_a?(Array) && + polygon_coordinates.all? { |ring_coordinates| valid_ring_coordinates?(ring_coordinates) } + end + + def valid_ring_coordinates?(ring_coordinates) + valid_coordinates_array?(ring_coordinates) && + ring_coordinates.size >= 4 && + ring_coordinates.first == ring_coordinates.last end end diff --git a/spec/factories/administration.rb b/spec/factories/administration.rb index b02cfcb20..21d1e9b60 100644 --- a/spec/factories/administration.rb +++ b/spec/factories/administration.rb @@ -25,7 +25,7 @@ FactoryBot.define do "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[[0.117, 51.513], [0.118, 51.512], [0.119, 51.514]]] + "coordinates": [[[0.117, 51.513], [0.118, 51.512], [0.119, 51.514], [0.117, 51.513]]] } } JSON diff --git a/spec/models/geojson_format_validator_spec.rb b/spec/models/geojson_format_validator_spec.rb index c4b4780f9..a0a5f4eef 100644 --- a/spec/models/geojson_format_validator_spec.rb +++ b/spec/models/geojson_format_validator_spec.rb @@ -217,6 +217,221 @@ describe GeojsonFormatValidator do end end + context "Polygon geometry" do + it "is not valid with a ring having less than four elements" do + record.geojson = <<~JSON + { + "type": "Polygon", + "coordinates": [[ + [1.23, 4.56], + [7.89, 10.11], + [1.23, 4.56] + ]] + } + JSON + + expect(record).not_to be_valid + end + + it "is not valid with a ring which with different starting and end points" do + record.geojson = <<~JSON + { + "type": "Polygon", + "coordinates": [[ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [16.17, 18.19] + ]] + } + JSON + + expect(record).not_to be_valid + end + + it "is valid with one valid ring" do + record.geojson = <<~JSON + { + "type": "Polygon", + "coordinates": [[ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [1.23, 4.56] + ]] + } + JSON + + expect(record).to be_valid + end + + it "is valid with multiple valid rings" do + record.geojson = <<~JSON + { + "type": "Polygon", + "coordinates": [ + [ + [100.0, 0.0], + [101.0, 0.0], + [101.0, 1.0], + [100.0, 1.0], + [100.0, 0.0] + ], + [ + [100.8, 0.8], + [100.8, 0.2], + [100.2, 0.2], + [100.2, 0.8], + [100.8, 0.8] + ] + ] + } + JSON + + expect(record).to be_valid + end + + it "is not valid with multiple rings if some rings are invalid" do + record.geojson = <<~JSON + { + "type": "Polygon", + "coordinates": [ + [ + [100.0, 0.0], + [101.0, 0.0], + [101.0, 1.0], + [100.0, 1.0], + [100.0, 0.0] + ], + [ + [100.8, 0.8], + [100.8, 0.2], + [100.2, 0.2] + ] + ] + } + JSON + + expect(record).not_to be_valid + end + end + + context "MultiPolygon geometry" do + it "is not valid with a one-dimensional array of coordinates" do + record.geojson = '{ "type": "MultiPolygon", "coordinates": [1.23, 4.56] }' + + expect(record).not_to be_valid + end + + it "is not valid with a two-dimensional array of coordinates" do + record.geojson = '{ "type": "MultiPolygon", "coordinates": [[1.23, 4.56], [7.89, 4.56]] }' + + expect(record).not_to be_valid + end + + it "is not valid with a three-dimensional polygon coordinates array" do + record.geojson = <<~JSON + { + "type": "MultiPolygon", + "coordinates": [[ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [1.23, 4.56] + ]] + } + JSON + + expect(record).not_to be_valid + end + + it "is valid with a valid polygon" do + record.geojson = <<~JSON + { + "type": "MultiPolygon", + "coordinates": [[[ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [1.23, 4.56] + ]]] + } + JSON + + expect(record).to be_valid + end + + it "is valid with multiple valid polygons" do + record.geojson = <<~JSON + { + "type": "MultiPolygon", + "coordinates": [ + [ + [ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [1.23, 4.56] + ] + ], + [ + [ + [100.0, 0.0], + [101.0, 0.0], + [101.0, 1.0], + [100.0, 1.0], + [100.0, 0.0] + ], + [ + [100.8, 0.8], + [100.8, 0.2], + [100.2, 0.2], + [100.2, 0.8], + [100.8, 0.8] + ] + ] + ] + } + JSON + + expect(record).to be_valid + end + + it "is not valid with multiple polygons if some polygons are invalid" do + record.geojson = <<~JSON + { + "type": "MultiPolygon", + "coordinates": [ + [ + [ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [1.23, 4.56] + ] + ], + [ + [ + [100.0, 0.0], + [101.0, 0.0], + [101.0, 1.0], + [100.0, 1.0], + [100.0, 0.0] + ], + [ + [100.8, 0.8], + [100.8, 0.2], + [100.2, 0.2] + ] + ] + ] + } + JSON + + expect(record).not_to be_valid + end + end + context "GeometryCollection" do it "is not valid if it doesn't contain geometries" do record.geojson = '{ "type": "GeometryCollection" }' diff --git a/spec/system/admin/geozones_spec.rb b/spec/system/admin/geozones_spec.rb index 37b448d2a..27b9f48f0 100644 --- a/spec/system/admin/geozones_spec.rb +++ b/spec/system/admin/geozones_spec.rb @@ -115,7 +115,7 @@ describe "Admin geozones", :admin do "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6]]] + "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6], [-0.1, 51.5]]] } } JSON @@ -158,7 +158,7 @@ describe "Admin geozones", :admin do "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6]]] + "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6], [-0.1, 51.5]]] } } JSON diff --git a/spec/system/budgets/investments_spec.rb b/spec/system/budgets/investments_spec.rb index 45930fe45..87a9ed0b1 100644 --- a/spec/system/budgets/investments_spec.rb +++ b/spec/system/budgets/investments_spec.rb @@ -1661,7 +1661,7 @@ describe "Budget Investments" do "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6]]] + "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6], [-0.1, 51.5]]] } } JSON @@ -1671,7 +1671,7 @@ describe "Budget Investments" do "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[[-0.1, 51.5], [-0.2, 51.5], [-0.2, 51.6], [-0.1, 51.6]]] + "coordinates": [[[-0.1, 51.5], [-0.2, 51.5], [-0.2, 51.6], [-0.1, 51.6], [-0.1, 51.5]]] } } JSON From 887d0d24198caea4513ae19b064e11f0d9e7bb8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 20 Dec 2024 14:17:31 +0100 Subject: [PATCH 7/7] Use render_map to render the map component This is done for consistency. We always use `render_map`, but forgot to do the same in commit 529357c98. --- app/components/admin/geozones/index_component.html.erb | 2 +- app/components/admin/geozones/index_component.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/components/admin/geozones/index_component.html.erb b/app/components/admin/geozones/index_component.html.erb index c423a6f87..18d8eda72 100644 --- a/app/components/admin/geozones/index_component.html.erb +++ b/app/components/admin/geozones/index_component.html.erb @@ -34,5 +34,5 @@

<%= t("admin.geozones.index.geojson_map") %>

<%= t("admin.geozones.index.geojson_map_help") %>

- <%= render Shared::MapLocationComponent.new(nil, geozones_data: geozones_data) %> + <%= render_map(nil, geozones_data: geozones_data) %> diff --git a/app/components/admin/geozones/index_component.rb b/app/components/admin/geozones/index_component.rb index 5d25a7188..0d3c0f6ff 100644 --- a/app/components/admin/geozones/index_component.rb +++ b/app/components/admin/geozones/index_component.rb @@ -1,6 +1,7 @@ class Admin::Geozones::IndexComponent < ApplicationComponent include Header attr_reader :geozones + use_helpers :render_map def initialize(geozones) @geozones = geozones