Add specs / Fix faulty shapes on map

app/forms/decidim/locations/location_form.rb

@@ -1,8 +1,5 @@
 # frozen_string_literal: true
 
-require "rgeo"
-require "rgeo/geo_json"
-
 module Decidim
   module Locations
     # A form object to be represent a location.
@@ -18,7 +15,7 @@ class LocationForm < Decidim::Form
 
       validates :address, presence: true, if: ->(form) { !form.deleted && (form.latitude.blank? || form.longitude.blank?) }
       validates :latitude, :longitude, presence: true, if: ->(form) { !form.deleted && form.address.blank? }
-      validate :json_validation, if: ->(form) { form.geojson.present? && !form.deleted }
+      validate :json_validation, if: ->(form) { !form.deleted }
 
       def to_param
         return id if id.present?
@@ -39,29 +36,29 @@ def json_validation
           valid_polygon?(geo_parse)
         end
       rescue JSON::ParserError
-        errors.add(:geojson, "Coordinates not valid")
+        errors.add(:geojson, "Invalid value")
       end
 
       def valid_marker?(geo_parse)
-        errors.add(:geojson, "Coordinates not found") unless valid_coord?(geo_parse)
+        valid_coord?(geo_parse)
       end
 
       def valid_line?(geo_parse)
         geo_parse.map do |coord|
-          errors.add(:geojson, "Coordinates not found") unless valid_coord?(coord)
+          valid_coord?(coord)
         end
       end
 
       def valid_polygon?(geo_parse)
         geo_parse.each do |coords|
           coords.each do |coord|
-            errors.add(:geojson, "Coordinates not found") unless valid_coord?(coord)
+            valid_coord?(coord)
           end
         end
       end
 
       def valid_coord?(geo_parse)
-        geo_parse["lat"].between?(-90, 90) && geo_parse["lng"].between?(-180, 180)
+        errors.add(:geojson, "Invalid coordinates") unless geo_parse["lat"].between?(-90, 90) && geo_parse["lng"].between?(-180, 180)
       end
     end
   end

app/packs/src/decidim/locations/map/integration/forms.js

@@ -2,6 +2,7 @@ import addShapeField from "./add_shape_field.js";
 import addInputGroup from "./add_input_group.js";
 import coordAverage from "./coord_average.js";
 import centerShape from "./center_shape.js";
+import validJson from "./valid_json.js";
 
 export default () => {
   document.querySelectorAll("[data-location-picker]").forEach((wrapperEl) => {
@@ -77,7 +78,13 @@ export default () => {
     typeLocButton.addEventListener("click", (event) => {
       event.preventDefault();
       const shapeId = ctrl.addMarker(typeLocCoords, "typeEv");
-      const addressData = { address: typeLocInput.value, position: {lat: typeLocCoords[0], lng: typeLocCoords[1]}, shapeId };
+      const addressData = {
+        address: typeLocInput.value,
+        position: { lat: typeLocCoords[0], lng: typeLocCoords[1] },
+        shapeId: shapeId,
+        objectShape: "Marker",
+        coordinates: { lat: typeLocCoords[0], lng: typeLocCoords[1] }
+      };
       typeLocInput.value = "";
       typeLocWrap.querySelector(".hint").classList.add("hide");
       displayList = true;
@@ -196,7 +203,6 @@ export default () => {
           // is not completed yet, so the user cannot edit the address yet.
           return;
         } else if (inputDiv && inputDiv.hasChildNodes()) {
-          console.log(inputDiv.querySelector(".location-address").value)
           editModalEl.querySelector(".location-fields input[name=address]").value = inputDiv.querySelector(".location-address").value;
         } else {
           locFields.querySelector("[data-modal-address]").setAttribute("disabled", true);
@@ -226,26 +232,60 @@ export default () => {
         (locContainer) => {
           const objectShape = locContainer.querySelector(".location-shape").value;
           const geojson = locContainer.querySelector(".location-geojson").value;
-          const lat = parseFloat(locContainer.querySelector(".location-latitude").value);
-          const lng = parseFloat(locContainer.querySelector(".location-longitude").value);
-          if (objectShape === "Marker") {
-            ctrl.addMarker([lat, lng], "editEv", locContainer.dataset.shapeId);
-            bounds.push([lat, lng]);
-          } else if (objectShape === "Line") {
-            const lineGeoJson = JSON.parse(geojson).map((coords) => [coords.lat, coords.lng]);
-            ctrl.addLine(lineGeoJson, "editEv", locContainer.dataset.shapeId);
-            bounds.push(lineGeoJson);
-          } else if (objectShape === "Polygon") {
-            const polygonGeoJson = JSON.parse(geojson).map(
-              (coord) => coord.map(
-                (coords) => [coords.lat, coords.lng]));
-            ctrl.addPolygon(polygonGeoJson, "editEv", locContainer.dataset.shapeId);
-            bounds.push(polygonGeoJson);
-          };
+          if (validJson(geojson)) {
+            let valid = true;
+
+            if (objectShape === "Marker") {
+              const markerGeoJson = JSON.parse(geojson)
+              if (markerGeoJson.lat < -90 || markerGeoJson.lat > 90 || markerGeoJson.lng < -180 || markerGeoJson.lng > 180) {
+                ctrl.deleteShape(locContainer.dataset.shapeId);
+                shapeFieldContainer.querySelector(`[data-shape-id="${locContainer.dataset.shapeId}"]`).remove();
+                valid = false;
+              }
+              if (valid) {
+                ctrl.addMarker(markerGeoJson, "editEv", locContainer.dataset.shapeId);
+                bounds.push(markerGeoJson);
+              }
+            } else if (objectShape === "Line") {
+              const lineGeoJson = JSON.parse(geojson).map((coords) => {
+                if (coords.lat < -90 || coords.lat > 90 || coords.lng < -180 || coords.lng > 180) {
+                  ctrl.deleteShape(locContainer.dataset.shapeId);
+                  shapeFieldContainer.querySelector(`[data-shape-id="${locContainer.dataset.shapeId}"]`).remove();
+                  valid = false;
+                }
+                return [coords.lat, coords.lng];
+              })
+              if (valid) {
+                ctrl.addLine(lineGeoJson, "editEv", locContainer.dataset.shapeId);
+                bounds.push(lineGeoJson);
+              }
+            } else if (objectShape === "Polygon") {
+              const polygonGeoJson = JSON.parse(geojson).map(
+                (coord) => coord.map(
+                  (coords) => {
+                    if (coords.lat < -90 || coords.lat > 90 || coords.lng < -180 || coords.lng > 180) {
+                      ctrl.deleteShape(locContainer.dataset.shapeId);
+                      shapeFieldContainer.querySelector(`[data-shape-id="${locContainer.dataset.shapeId}"]`).remove();
+                      valid = false;
+                    }
+                    return [coords.lat, coords.lng];
+                  })
+              )
+              if (valid) {
+                ctrl.addPolygon(polygonGeoJson, "editEv", locContainer.dataset.shapeId);
+                bounds.push(polygonGeoJson);
+              }
+            };
+          } else {
+            shapeFieldContainer.querySelector(`[data-shape-id="${locContainer.dataset.shapeId}"]`).remove();
+          }
         }
       );
-      const area = new L.LatLngBounds(bounds)
-      ctrl.map.fitBounds(area);
+
+      if (bounds.length > 0) {
+        const area = new L.LatLngBounds(bounds);
+        ctrl.map.fitBounds(area);
+      }
     }
   });
 };

app/packs/src/decidim/locations/map/integration/valid_json.js

@@ -0,0 +1,10 @@
+const validJson = function (value) {
+  try {
+    JSON.parse(value);
+    return true;
+  } catch (error) {
+    return false;
+  }
+};
+
+export default validJson;

spec/forms/decidim/locations/location_form_spec.rb

@@ -8,13 +8,27 @@
   let(:form) { described_class.from_params(attributes) }
 
   context "when all details are provided" do
-    let(:attributes) { { address: "Veneentekijäntie 4 A, 00210 Helsinki", latitude: 60.149792, longitude: 24.887430 } }
+    let(:attributes) do
+      {
+        address: "Veneentekijäntie 4 A, 00210 Helsinki",
+        latitude: 60.149792,
+        longitude: 24.887430,
+        shape: "Marker",
+        geojson: "{ \"lat\":60.149792,\"lng\":24.887430 }"
+      }
+    end
 
     it { is_expected.to be_valid }
   end
 
   context "when all details are blank" do
-    let(:attributes) { { address: "" } }
+    let(:attributes) do
+      {
+        address: "Veneentekijäntie 4 A, 00210 Helsinki",
+        shape: "",
+        geojson: ""
+      }
+    end
 
     it { is_expected.not_to be_valid }
 
@@ -25,21 +39,61 @@
     end
   end
 
-  context "when address is blank but latitude and longitude are provided" do
-    let(:attributes) { { address: "", latitude: 60.149792, longitude: 24.887430 } }
+  context "when address is blank but latitude, longitude, shape and geojson are provided" do
+    let(:attributes) do
+      {
+        address: "",
+        latitude: 60.149792,
+        longitude: 24.887430,
+        shape: "Marker",
+        geojson: "{ \"lat\":60.149792,\"lng\":24.887430 }"
+      }
+    end
 
     it { is_expected.to be_valid }
   end
 
-  context "when address is blank but only latitude is provided" do
-    let(:attributes) { { address: "", latitude: 60.149792 } }
+  context "when geojson not provided" do
+    let(:attributes) do
+      {
+        address: "",
+        latitude: 60.149792,
+        longitude: 24.887430,
+        shape: "Marker",
+        geojson: ""
+      }
+    end
 
     it { is_expected.not_to be_valid }
   end
 
-  context "when address is blank but only longitude is provided" do
-    let(:attributes) { { address: "", longitude: 60.149792 } }
+  context "when geojson is not correct" do
+    context "when cannot be parsed to JSON" do
+      let(:attributes) do
+        {
+          address: "Veneentekijäntie 4 A, 00210 Helsinki",
+          latitude: 60.149792,
+          longitude: 24.887430,
+          shape: "Marker",
+          geojson: "{ \"lat\":Example,\"lng\":24.887430 }"
+        }
+      end
 
-    it { is_expected.not_to be_valid }
+      it { is_expected.not_to be_valid }
+    end
+
+    context "when coordinates don't exist" do
+      let(:attributes) do
+        {
+          address: "Veneentekijäntie 4 A, 00210 Helsinki",
+          latitude: 90.149792,
+          longitude: 24.887430,
+          shape: "Marker",
+          geojson: "{ \"lat\":90.149792,\"lng\":24.887430 }"
+        }
+      end
+
+      it { is_expected.not_to be_valid }
+    end
   end
 end

spec/system/map_spec.rb

@@ -291,14 +291,14 @@ def add_line(latitude: [60.24240524264389, 60.24116980538663], longitude: [25.10
       find("div.leaflet-pm-actions-container a.leaflet-pm-action.action-finish").click
     end
 
-    def add_polygon(latitude: [8.231, 7.123, 10.341, 8.231], longitude: [5.343, 4.674, 7.231, 5.343])
+    def add_polygon(latitude: [85.05109772344713, 85.05106347807192, 85.05106162696384], longitude: [24.741251170635227, 24.7408863902092, 24.741551578044895])
       find('div[title="Draw Polygons"] a').click
       polygon_add = <<~JS
         var map = $(".picker-wrapper [data-decidim-map]").data("map");
         var first = L.latLng(#{latitude}[0], #{longitude}[0]);
         var second = L.latLng(#{latitude}[1], #{longitude}[1]);
         var third = L.latLng(#{latitude}[2], #{longitude}[2]);
-        var fourth = L.latLng(#{latitude}[3], #{longitude}[3]);
+        var fourth = L.latLng(#{latitude}[0], #{longitude}[0]);
         map.fire("click", { latlng: first });
         map.fire("click", { latlng: second });
         map.fire("click", { latlng: third });
@@ -821,10 +821,10 @@ def drag_polygon
             page.execute_script(revgeo)
             add_marker
             add_marker(latitude: 11.523, longitude: 5.523)
-            expect(page).to have_css(".leaflet-marker-icon", count: 2, visible: :all)
+            expect(page).to have_css(".leaflet-marker-icon", count: 2)
             find(".leaflet-marker-icon", match: :first).click
             click_button "Delete shape"
-            expect(page).to have_css(".leaflet-marker-icon", count: 1, visible: :all)
+            expect(page).to have_css(".leaflet-marker-icon", count: 1)
           end
         end
 
@@ -833,10 +833,22 @@ def drag_polygon
             page.execute_script(revgeo)
             add_line
             add_line(latitude: [60.25240524264372, 60.25116980538645], longitude: [25.10409421539333, 25.104342109680122])
-            expect(page).to have_css(".leaflet-interactive", count: 2, visible: :all)
+            expect(page).to have_css(".leaflet-interactive", count: 2)
             find(".leaflet-interactive", match: :first).click
             click_button "Delete shape"
-            expect(page).to have_css(".leaflet-interactive", count: 1, visible: :all)
+            expect(page).to have_css(".leaflet-interactive", count: 1)
+          end
+        end
+
+        context "when polygons" do
+          it "deletes only one" do
+            page.execute_script(revgeo)
+            add_polygon
+            add_polygon(latitude: [85.05109772344434, 85.05101347805167, 85.05106165693364], longitude: [24.741211170645243, 24.7438833903077, 24.74355157804477])
+            expect(page).to have_css(".leaflet-interactive", count: 2)
+            find(".leaflet-interactive", match: :first).click
+            click_button "Delete shape"
+            expect(page).to have_css(".leaflet-interactive", count: 1)
           end
         end
       end