From 7c34795a1abc385ac2039417b130ca48cf9dd434 Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Wed, 15 Apr 2020 17:41:05 +0200 Subject: [PATCH 1/9] Bring back font-awesome-rails we need it untill we remove all the old admin pages --- Gemfile | 1 + Gemfile.lock | 3 +++ app/assets/stylesheets/application.scss | 1 + 3 files changed, 5 insertions(+) diff --git a/Gemfile b/Gemfile index 14407ddc5..be4cfbaea 100644 --- a/Gemfile +++ b/Gemfile @@ -28,6 +28,7 @@ gem 'dotenv-rails', require: 'dotenv/rails-now' # dotenv should always be loaded gem 'flipper' gem 'flipper-active_record' gem 'flipper-ui' +gem 'font-awesome-rails' gem 'fugit' gem 'geocoder' gem 'gon' diff --git a/Gemfile.lock b/Gemfile.lock index 7e80e2d2b..5544e433f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -236,6 +236,8 @@ GEM fog-core (~> 2.1) fog-json (>= 1.0) ipaddress (>= 0.8) + font-awesome-rails (4.7.0.5) + railties (>= 3.2, < 6.1) formatador (0.2.5) fugit (1.3.3) et-orbi (~> 1.1, >= 1.1.8) @@ -751,6 +753,7 @@ DEPENDENCIES flipper flipper-active_record flipper-ui + font-awesome-rails fugit geocoder gon diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 121de171b..ee4b980da 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -33,6 +33,7 @@ // = require attestation_template_edit // = require_self +// = require font-awesome // = require leaflet // = require franceconnect // = require bootstrap-wysihtml5 From 22604013d0f612904317a1c4e8e7acd1d4d218aa Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Thu, 9 Apr 2020 17:32:20 +0200 Subject: [PATCH 2/9] Expose multiple user selections on champ carte --- app/controllers/champs/carte_controller.rb | 20 ++- app/helpers/champ_helper.rb | 14 ++ app/javascript/components/MapReader.js | 83 ++++------- app/models/champs/carte_champ.rb | 135 ++++++++++++------ app/models/geo_area.rb | 13 +- app/serializers/dossier_serializer.rb | 9 +- app/services/api_carto_service.rb | 6 - app/services/geojson_service.rb | 17 --- app/views/dossiers/show.pdf.prawn | 2 +- .../shared/champs/carte/_geo_areas.html.haml | 24 ++-- app/views/shared/champs/carte/_show.html.haml | 4 +- .../champs/carte_controller_spec.rb | 8 +- spec/models/champs/carte_champ_spec.rb | 98 +++++-------- spec/serializers/champ_serializer_spec.rb | 42 ++---- 14 files changed, 230 insertions(+), 245 deletions(-) diff --git a/app/controllers/champs/carte_controller.rb b/app/controllers/champs/carte_controller.rb index 4459ab781..d1be197af 100644 --- a/app/controllers/champs/carte_controller.rb +++ b/app/controllers/champs/carte_controller.rb @@ -39,9 +39,10 @@ class Champs::CarteController < ApplicationController end end - selection_utilisateur = ApiCartoService.generate_selection_utilisateur(coordinates) - selection_utilisateur[:source] = GeoArea.sources.fetch(:selection_utilisateur) - geo_areas << selection_utilisateur + selections_utilisateur = legacy_selections_utilisateur_to_polygons(coordinates) + geo_areas += selections_utilisateur.map do |selection_utilisateur| + selection_utilisateur.merge(source: GeoArea.sources.fetch(:selection_utilisateur)) + end @champ.geo_areas = geo_areas.map do |geo_area| GeoArea.new(geo_area) @@ -58,4 +59,17 @@ class Champs::CarteController < ApplicationController flash.alert = 'Les données cartographiques sont temporairement indisponibles. Réessayez dans un instant.' response.status = 503 end + + private + + def legacy_selections_utilisateur_to_polygons(coordinates) + coordinates.map do |lat_longs| + { + geometry: { + type: 'Polygon', + coordinates: [lat_longs.map { |lat_long| [lat_long['lng'], lat_long['lat']] }] + } + } + end + end end diff --git a/app/helpers/champ_helper.rb b/app/helpers/champ_helper.rb index 1e51c662d..b41d81fa2 100644 --- a/app/helpers/champ_helper.rb +++ b/app/helpers/champ_helper.rb @@ -37,4 +37,18 @@ module ChampHelper champs_piece_justificative_url(object.id) end end + + def geo_area_label(geo_area) + case geo_area.source + when GeoArea.sources.fetch(:cadastre) + capture do + concat "Parcelle n° #{geo_area.numero} - Feuille #{geo_area.code_arr} #{geo_area.section} #{geo_area.feuille} - #{geo_area.surface_parcelle.round} m" + concat content_tag(:sup, "2") + end + when GeoArea.sources.fetch(:quartier_prioritaire) + "#{geo_area.commune} : #{geo_area.nom}" + when GeoArea.sources.fetch(:parcelle_agricole) + "Culture : #{geo_area.culture} - Surface : #{geo_area.surface} ha" + end + end end diff --git a/app/javascript/components/MapReader.js b/app/javascript/components/MapReader.js index aaa2380ab..2e3c16fa5 100644 --- a/app/javascript/components/MapReader.js +++ b/app/javascript/components/MapReader.js @@ -1,48 +1,36 @@ import React from 'react'; import ReactMapboxGl, { ZoomControl, GeoJSONLayer } from 'react-mapbox-gl'; -import mapboxgl, { LngLatBounds } from 'mapbox-gl'; +import mapboxgl from 'mapbox-gl'; import PropTypes from 'prop-types'; const Map = ReactMapboxGl({}); -const MapReader = ({ geoData }) => { - let [selectionCollection, cadastresCollection] = [[], []]; +const MapReader = ({ featureCollection }) => { + const [a1, a2, b1, b2] = featureCollection.bbox; + const boundData = [ + [a1, a2], + [b1, b2] + ]; - for (let selection of geoData.selection.coordinates) { - selectionCollection.push({ - type: 'Feature', - geometry: { - type: 'Polygon', - coordinates: selection - } - }); - } - - for (let cadastre of geoData.cadastres) { - cadastresCollection.push({ - type: 'Feature', - geometry: { - type: 'Polygon', - coordinates: cadastre.geometry.coordinates[0] - } - }); - } - - const selectionData = { - type: 'geojson', - data: { - type: 'FeatureCollection', - features: selectionCollection - } + const selectionsFeatureCollection = { + type: 'FeatureCollection', + features: [] + }; + const cadastresFeatureCollection = { + type: 'FeatureCollection', + features: [] }; - const cadastresData = { - type: 'geojson', - data: { - type: 'FeatureCollection', - features: cadastresCollection + for (let feature of featureCollection.features) { + switch (feature.properties.source) { + case 'selection_utilisateur': + selectionsFeatureCollection.features.push(feature); + break; + case 'cadastre': + cadastresFeatureCollection.features.push(feature); + break; } - }; + } const polygonSelectionFill = { 'fill-color': '#EC3323', @@ -65,19 +53,6 @@ const MapReader = ({ geoData }) => { 'line-dasharray': [1, 1] }; - let bounds = new LngLatBounds(); - - for (let selection of selectionCollection) { - for (let coordinate of selection.geometry.coordinates[0]) { - bounds.extend(coordinate); - } - } - let [swCoords, neCoords] = [ - Object.values(bounds._sw), - Object.values(bounds._ne) - ]; - const boundData = [swCoords, neCoords]; - if (!mapboxgl.supported()) { return (

@@ -99,12 +74,12 @@ const MapReader = ({ geoData }) => { }} > @@ -114,10 +89,10 @@ const MapReader = ({ geoData }) => { }; MapReader.propTypes = { - geoData: PropTypes.shape({ - position: PropTypes.object, - selection: PropTypes.object, - cadastres: PropTypes.array + featureCollection: PropTypes.shape({ + type: PropTypes.string, + bbox: PropTypes.array, + features: PropTypes.array }) }; diff --git a/app/models/champs/carte_champ.rb b/app/models/champs/carte_champ.rb index a12792cc6..a2fd97737 100644 --- a/app/models/champs/carte_champ.rb +++ b/app/models/champs/carte_champ.rb @@ -19,8 +19,10 @@ class Champs::CarteChamp < Champ end end - def selection_utilisateur - geo_areas.find(&:selection_utilisateur?) + def selections_utilisateur + geo_areas.filter do |area| + area.source == GeoArea.sources.fetch(:selection_utilisateur) + end end def cadastres? @@ -47,68 +49,73 @@ class Champs::CarteChamp < Champ end end - def geo_json - @geo_json ||= begin - geo_area = selection_utilisateur + def bounding_box + factory = RGeo::Geographic.simple_mercator_factory + bounding_box = RGeo::Cartesian::BoundingBox.new(factory) - if geo_area - geo_area.geometry - else - geo_json_from_value + if geo_areas.present? + geo_areas.each do |area| + bounding_box.add(area.rgeo_geometry) end + elsif dossier.present? + point = dossier.geo_position + bounding_box.add(factory.point(point[:lon], point[:lat])) + else + bounding_box.add(factory.point(2.428462, 46.538192)) + end + + [bounding_box.max_point, bounding_box.min_point].compact.flat_map(&:coordinates) + end + + def to_feature_collection + { + type: 'FeatureCollection', + bbox: bounding_box, + features: (legacy_selections_utilisateur + except_selections_utilisateur).map(&:to_feature) + } + end + + def geometry? + geo_areas.present? + end + + def selection_utilisateur_legacy_geometry + if selection_utilisateur_legacy? + selections_utilisateur.first.geometry + elsif selections_utilisateur.present? + { + type: 'MultiPolygon', + coordinates: selections_utilisateur.filter do |selection_utilisateur| + selection_utilisateur.geometry['type'] == 'Polygon' + end.map do |selection_utilisateur| + selection_utilisateur.geometry['coordinates'] + end + } + else + nil end end - def selection_utilisateur_size - if geo_json.present? - geo_json['coordinates'].size - else - 0 + def selection_utilisateur_legacy_geo_area + geometry = selection_utilisateur_legacy_geometry + if geometry.present? + GeoArea.new( + source: GeoArea.sources.fetch(:selection_utilisateur), + geometry: geometry + ) end end def to_render_data { position: position, - selection: user_geo_area&.geometry, + selection: selection_utilisateur_legacy_geometry, quartiersPrioritaires: quartiers_prioritaires? ? quartiers_prioritaires.as_json(except: :properties) : [], cadastres: cadastres? ? cadastres.as_json(except: :properties) : [], parcellesAgricoles: parcelles_agricoles? ? parcelles_agricoles.as_json(except: :properties) : [] } end - def user_geo_area - geo_area = selection_utilisateur - - if geo_area.present? - geo_area - elsif geo_json_from_value.present? - GeoArea.new( - geometry: geo_json_from_value, - source: GeoArea.sources.fetch(:selection_utilisateur) - ) - end - end - - def geo_json_from_value - @geo_json_from_value ||= begin - parsed_value = value.blank? ? nil : JSON.parse(value) - # We used to store in the value column a json array with coordinates. - if parsed_value.is_a?(Array) - # Empty array is sent instead of blank to distinguish between empty and error - if parsed_value.empty? - nil - else - # If it is a coordinates array, format it as a GEO-JSON - JSON.parse(GeojsonService.to_json_polygon_for_selection_utilisateur(parsed_value)) - end - else - # It is already a GEO-JSON - parsed_value - end - end - end - def for_api nil end @@ -116,4 +123,38 @@ class Champs::CarteChamp < Champ def for_export nil end + + private + + def selection_utilisateur_legacy? + if selections_utilisateur.size == 1 + geometry = selections_utilisateur.first.geometry + return geometry && geometry['type'] == 'MultiPolygon' + end + + false + end + + def legacy_selections_utilisateur + if selection_utilisateur_legacy? + selections_utilisateur.first.geometry['coordinates'].map do |coordinates| + GeoArea.new( + geometry: { + type: 'Polygon', + coordinates: coordinates + }, + properties: {}, + source: GeoArea.sources.fetch(:selection_utilisateur) + ) + end + else + selections_utilisateur + end + end + + def except_selections_utilisateur + geo_areas.filter do |area| + area.source != GeoArea.sources.fetch(:selection_utilisateur) + end + end end diff --git a/app/models/geo_area.rb b/app/models/geo_area.rb index 7109d0313..6ff08e68a 100644 --- a/app/models/geo_area.rb +++ b/app/models/geo_area.rb @@ -27,11 +27,20 @@ class GeoArea < ApplicationRecord selection_utilisateur: 'selection_utilisateur' } + scope :selections_utilisateur, -> { where(source: sources.fetch(:selection_utilisateur)) } scope :quartiers_prioritaires, -> { where(source: sources.fetch(:quartier_prioritaire)) } scope :cadastres, -> { where(source: sources.fetch(:cadastre)) } scope :parcelles_agricoles, -> { where(source: sources.fetch(:parcelle_agricole)) } - def selection_utilisateur? - source == self.class.sources.fetch(:selection_utilisateur) + def to_feature + { + type: 'Feature', + geometry: geometry, + properties: properties.merge(source: source) + } + end + + def rgeo_geometry + RGeo::GeoJSON.decode(geometry.to_json, geo_factory: RGeo::Geographic.simple_mercator_factory) end end diff --git a/app/serializers/dossier_serializer.rb b/app/serializers/dossier_serializer.rb index ba829d3e6..b5a4beeb8 100644 --- a/app/serializers/dossier_serializer.rb +++ b/app/serializers/dossier_serializer.rb @@ -38,11 +38,12 @@ class DossierSerializer < ActiveModel::Serializer end if champ_carte.present? - carto_champs = champ_carte.geo_areas.to_a - if !carto_champs.find(&:selection_utilisateur?) - carto_champs << champ_carte.user_geo_area + champs_geo_areas = geo_areas.filter do |geo_area| + geo_area.source != GeoArea.sources.fetch(:selection_utilisateur) end - champs += carto_champs.compact + champs_geo_areas << champ_carte.selection_utilisateur_legacy_geo_area + + champs += champs_geo_areas.compact end end diff --git a/app/services/api_carto_service.rb b/app/services/api_carto_service.rb index 9bca82a6c..ec0f64dfe 100644 --- a/app/services/api_carto_service.rb +++ b/app/services/api_carto_service.rb @@ -22,10 +22,4 @@ class ApiCartoService ).results end end - - def self.generate_selection_utilisateur(coordinates) - { - geometry: JSON.parse(GeojsonService.to_json_polygon_for_selection_utilisateur(coordinates)) - } - end end diff --git a/app/services/geojson_service.rb b/app/services/geojson_service.rb index a5c98c9ed..0e4c04914 100644 --- a/app/services/geojson_service.rb +++ b/app/services/geojson_service.rb @@ -14,21 +14,4 @@ class GeojsonService polygon.to_json end - - def self.to_json_polygon_for_selection_utilisateur(coordinates) - coordinates = coordinates.map do |lat_longs| - outbounds = lat_longs.map do |lat_long| - [lat_long['lng'], lat_long['lat']] - end - - [outbounds] - end - - polygon = { - type: 'MultiPolygon', - coordinates: coordinates - } - - polygon.to_json - end end diff --git a/app/views/dossiers/show.pdf.prawn b/app/views/dossiers/show.pdf.prawn index 5bc0c9215..29343bb26 100644 --- a/app/views/dossiers/show.pdf.prawn +++ b/app/views/dossiers/show.pdf.prawn @@ -86,7 +86,7 @@ def render_single_champ(pdf, champ) when 'Champs::ExplicationChamp' format_in_2_lines(pdf, champ.libelle, champ.description) when 'Champs::CarteChamp' - format_in_2_lines(pdf, champ.libelle, champ.geo_json.to_s) + format_in_2_lines(pdf, champ.libelle, champ.to_feature_collection.to_json) when 'Champs::SiretChamp' pdf.font 'liberation serif', style: :bold, size: 12 do pdf.text champ.libelle diff --git a/app/views/shared/champs/carte/_geo_areas.html.haml b/app/views/shared/champs/carte/_geo_areas.html.haml index 09f326d1b..6f3565e3d 100644 --- a/app/views/shared/champs/carte/_geo_areas.html.haml +++ b/app/views/shared/champs/carte/_geo_areas.html.haml @@ -3,39 +3,39 @@ .areas - if error.present? .error Merci de dessiner une surface plus petite afin de récupérer les quartiers prioritaires. - - elsif champ.value.blank? + - elsif !champ.geometry? Aucune zone tracée - elsif champ.quartiers_prioritaires.blank? - = t('errors.messages.quartiers_prioritaires_empty', count: champ.selection_utilisateur_size) + = t('errors.messages.quartiers_prioritaires_empty', count: champ.selections_utilisateur.size) - else %ul - - champ.quartiers_prioritaires.each do |qp| - %li #{qp.commune} : #{qp.nom} + - champ.quartiers_prioritaires.each do |geo_area| + %li= geo_area_label(geo_area) - if champ.cadastres? .areas-title Parcelles cadastrales .areas - if error.present? .error Merci de dessiner une surface plus petite afin de récupérer les parcelles cadastrales. - - elsif champ.value.blank? + - elsif !champ.geometry? Aucune zone tracée - elsif champ.cadastres.blank? - = t('errors.messages.cadastres_empty', count: champ.selection_utilisateur_size) + = t('errors.messages.cadastres_empty', count: champ.selections_utilisateur.size) - else %ul - - champ.cadastres.each do |pc| - %li Parcelle n° #{pc.numero} - Feuille #{pc.code_arr} #{pc.section} #{pc.feuille} - #{pc.surface_parcelle.round} m2 + - champ.cadastres.each do |geo_area| + %li= geo_area_label(geo_area) - if champ.parcelles_agricoles? .areas-title Parcelles agricoles (RPG) .areas - if error.present? .error Merci de dessiner une surface plus petite afin de récupérer les parcelles agricoles. - - elsif champ.value.blank? + - elsif !champ.geometry? Aucune zone tracée - elsif champ.parcelles_agricoles.blank? - = t('errors.messages.parcelles_agricoles_empty', count: champ.selection_utilisateur_size) + = t('errors.messages.parcelles_agricoles_empty', count: champ.selections_utilisateur.size) - else %ul - - champ.parcelles_agricoles.each do |pa| - %li Culture : #{pa.culture} - Surface : #{pa.surface} ha + - champ.parcelles_agricoles.each do |geo_area| + %li= geo_area_label(geo_area) diff --git a/app/views/shared/champs/carte/_show.html.haml b/app/views/shared/champs/carte/_show.html.haml index 32d5b4f05..ddf078f83 100644 --- a/app/views/shared/champs/carte/_show.html.haml +++ b/app/views/shared/champs/carte/_show.html.haml @@ -1,4 +1,4 @@ -- if champ.to_s.present? - = react_component("MapReader", { geoData: champ.to_render_data } ) +- if champ.geometry? + = react_component("MapReader", { featureCollection: champ.to_feature_collection } ) .geo-areas = render partial: 'shared/champs/carte/geo_areas', locals: { champ: champ, error: false } diff --git a/spec/controllers/champs/carte_controller_spec.rb b/spec/controllers/champs/carte_controller_spec.rb index 43861373a..dc483d39b 100644 --- a/spec/controllers/champs/carte_controller_spec.rb +++ b/spec/controllers/champs/carte_controller_spec.rb @@ -13,11 +13,10 @@ describe Champs::CarteController, type: :controller do champ_id: champ.id } end - let(:geojson) { [] } let(:champ) do create(:type_de_champ_carte, options: { cadastres: true - }).champ.create(dossier: dossier, value: geojson.to_json) + }).champ.create(dossier: dossier) end describe 'POST #show' do @@ -31,7 +30,7 @@ describe Champs::CarteController, type: :controller do allow_any_instance_of(ApiCarto::CadastreAdapter) .to receive(:results) - .and_return([{ code: "QPCODE1234", surface_parcelle: 4, geometry: { type: "MultiPolygon", coordinates: [[[[2.38715792094576, 48.8723062632126], [2.38724851642619, 48.8721392348061]]]] } }]) + .and_return([{ code: "QPCODE1234", surface_parcelle: 4, geometry: { type: "MultiPolygon", coordinates: [[[[2.38715792094576, 48.8723062632126], [2.38724851642619, 48.8721392348061], [2.38724851642620, 48.8721392348064], [2.38715792094576, 48.8723062632126]]]] } }]) post :show, params: params, format: 'js' end @@ -43,7 +42,7 @@ describe Champs::CarteController, type: :controller do expect(assigns(:error)).to eq(nil) expect(champ.reload.value).to eq(nil) expect(champ.reload.geo_areas).to eq([]) - expect(response.body).to include("DS.fire('carte:update', {\"selector\":\".carte-1\",\"data\":{\"position\":{\"lon\":\"2.428462\",\"lat\":\"46.538192\",\"zoom\":\"13\"},\"selection\":null,\"quartiersPrioritaires\":[],\"cadastres\":[],\"parcellesAgricoles\":[]}});") + expect(response.body).to include("DS.fire('carte:update'") } end @@ -56,7 +55,6 @@ describe Champs::CarteController, type: :controller do end context 'when error' do - let(:geojson) { [[{ "lat": 48.87442541960633, "lng": 2.3859214782714844 }, { "lat": 48.87273183590832, "lng": 2.3850631713867183 }, { "lat": 48.87081237174292, "lng": 2.3809432983398438 }, { "lat": 48.8712640169951, "lng": 2.377510070800781 }, { "lat": 48.87510283703279, "lng": 2.3778533935546875 }, { "lat": 48.87544154230615, "lng": 2.382831573486328 }, { "lat": 48.87442541960633, "lng": 2.3859214782714844 }]] } let(:value) { '' } it { diff --git a/spec/models/champs/carte_champ_spec.rb b/spec/models/champs/carte_champ_spec.rb index 4de97465c..5f286e6f7 100644 --- a/spec/models/champs/carte_champ_spec.rb +++ b/spec/models/champs/carte_champ_spec.rb @@ -1,9 +1,19 @@ describe Champs::CarteChamp do - let(:champ) { Champs::CarteChamp.new(value: value) } + let(:champ) { Champs::CarteChamp.new(geo_areas: geo_areas) } let(:value) { '' } - let(:coordinates) { [[{ "lat" => 48.87442541960633, "lng" => 2.3859214782714844 }, { "lat" => 48.87273183590832, "lng" => 2.3850631713867183 }, { "lat" => 48.87081237174292, "lng" => 2.3809432983398438 }, { "lat" => 48.8712640169951, "lng" => 2.377510070800781 }, { "lat" => 48.87510283703279, "lng" => 2.3778533935546875 }, { "lat" => 48.87544154230615, "lng" => 2.382831573486328 }, { "lat" => 48.87442541960633, "lng" => 2.3859214782714844 }]] } - let(:geo_json_as_string) { GeojsonService.to_json_polygon_for_selection_utilisateur(coordinates) } - let(:geo_json) { JSON.parse(geo_json_as_string) } + let(:coordinates) { [[2.3859214782714844, 48.87442541960633], [2.3850631713867183, 48.87273183590832], [2.3809432983398438, 48.87081237174292], [2.3859214782714844, 48.87442541960633]] } + let(:geo_json) do + { + "type" => 'Polygon', + "coordinates" => coordinates + } + end + let(:legacy_geo_json) do + { + type: 'MultiPolygon', + coordinates: [coordinates] + } + end describe '#to_render_data' do subject { champ.to_render_data } @@ -18,78 +28,44 @@ describe Champs::CarteChamp do } } - context 'when the value is nil' do - let(:value) { nil } - + context 'when has no geo_areas' do + let(:geo_areas) { [] } let(:selection) { nil } it { is_expected.to eq(render_data) } end - context 'when the value is blank' do - let(:value) { '' } - - let(:selection) { nil } - - it { is_expected.to eq(render_data) } - end - - context 'when the value is empty array' do - let(:value) { '[]' } - - let(:selection) { nil } - - it { is_expected.to eq(render_data) } - end - - context 'when the value is coordinates' do - let(:value) { coordinates.to_json } - - let(:selection) { geo_json } - - it { is_expected.to eq(render_data) } - end - - context 'when the value is geojson' do - let(:value) { geo_json.to_json } - - let(:selection) { geo_json } + context 'when has one geo_area' do + let(:geo_areas) { [build(:geo_area, :selection_utilisateur, geometry: geo_json)] } + let(:selection) { legacy_geo_json } it { is_expected.to eq(render_data) } end end - describe '#selection_utilisateur_size' do - subject { champ.selection_utilisateur_size } + describe '#to_feature_collection' do + subject { champ.to_feature_collection } - context 'when the value is nil' do - let(:value) { nil } + let(:feature_collection) { + { + type: 'FeatureCollection', + bbox: champ.bounding_box, + features: features + } + } - it { is_expected.to eq(0) } + context 'when has no geo_areas' do + let(:geo_areas) { [] } + let(:features) { [] } + + it { is_expected.to eq(feature_collection) } end - context 'when the value is blank' do - let(:value) { '' } + context 'when has one geo_area' do + let(:geo_areas) { [build(:geo_area, :selection_utilisateur, geometry: geo_json)] } + let(:features) { geo_areas.map(&:to_feature) } - it { is_expected.to eq(0) } - end - - context 'when the value is empty array' do - let(:value) { '[]' } - - it { is_expected.to eq(0) } - end - - context 'when the value is coordinates' do - let(:value) { coordinates.to_json } - - it { is_expected.to eq(1) } - end - - context 'when the value is geojson' do - let(:value) { geo_json.to_json } - - it { is_expected.to eq(1) } + it { is_expected.to eq(feature_collection) } end end end diff --git a/spec/serializers/champ_serializer_spec.rb b/spec/serializers/champ_serializer_spec.rb index 602356312..97a6712f4 100644 --- a/spec/serializers/champ_serializer_spec.rb +++ b/spec/serializers/champ_serializer_spec.rb @@ -27,9 +27,13 @@ describe ChampSerializer do let(:champ) { create(:champ_carte, value: value, geo_areas: [geo_area].compact) } let(:value) { nil } let(:geo_area) { create(:geo_area, geometry: geo_json) } - let(:geo_json_as_string) { GeojsonService.to_json_polygon_for_selection_utilisateur(coordinates) } - let(:geo_json) { JSON.parse(geo_json_as_string) } - let(:coordinates) { [[{ "lat" => 48.87442541960633, "lng" => 2.3859214782714844 }, { "lat" => 48.87273183590832, "lng" => 2.3850631713867183 }, { "lat" => 48.87081237174292, "lng" => 2.3809432983398438 }, { "lat" => 48.8712640169951, "lng" => 2.377510070800781 }, { "lat" => 48.87510283703279, "lng" => 2.3778533935546875 }, { "lat" => 48.87544154230615, "lng" => 2.382831573486328 }, { "lat" => 48.87442541960633, "lng" => 2.3859214782714844 }]] } + let(:geo_json) do + { + "type" => 'MultiPolygon', + "coordinates" => coordinates + } + end + let(:coordinates) { [[[2.3859214782714844, 48.87442541960633], [2.3850631713867183, 48.87273183590832], [2.3809432983398438, 48.87081237174292], [2.3859214782714844, 48.87442541960633]]] } let(:serialized_champ) { { @@ -60,19 +64,19 @@ describe ChampSerializer do context 'when value is nil' do let(:value) { nil } - it { expect(champ.user_geo_area).to be_nil } + it { expect(champ.selection_utilisateur_legacy_geo_area).to be_nil } end context 'when value is empty array' do let(:value) { '[]' } - it { expect(champ.user_geo_area).to be_nil } + it { expect(champ.selection_utilisateur_legacy_geo_area).to be_nil } end context 'when value is blank' do let(:value) { '' } - it { expect(champ.user_geo_area).to be_nil } + it { expect(champ.selection_utilisateur_legacy_geo_area).to be_nil } end end @@ -80,7 +84,7 @@ describe ChampSerializer do let(:serialized_libelle) { "user geometry" } let(:serialized_type_champ) { "user_geometry" } - let(:serializable_object) { champ.user_geo_area } + let(:serializable_object) { champ.selection_utilisateur_legacy_geo_area } context 'when value is coordinates' do let(:value) { coordinates.to_json } @@ -167,30 +171,6 @@ describe ChampSerializer do it { expect(subject).to eq(serialized_champ) } end end - - context 'and geo_area is quartier_prioritaire' do - let(:geo_area) { create(:geo_area, :quartier_prioritaire, geometry: geo_json) } - - context 'new_api' do - it { - expect(subject[:geo_areas].first).to include( - source: GeoArea.sources.fetch(:quartier_prioritaire), - geometry: geo_json, - nom: 'XYZ', - commune: 'Paris' - ) - expect(subject[:geo_areas].first.key?(:numero)).to be_falsey - } - end - - context 'old_api' do - let(:serializable_object) { champ.geo_areas.first } - let(:serialized_libelle) { "quartier prioritaire" } - let(:serialized_type_champ) { "quartier_prioritaire" } - - it { expect(subject).to eq(serialized_champ) } - end - end end context 'when type champ is siret' do From 0d628bb96b2e0f374cefe7854e46129907f5eaba Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Tue, 14 Apr 2020 13:10:47 +0200 Subject: [PATCH 3/9] Task to split GeoArea with selection utilisateur in multiple polygons --- ...split_geo_area_selection_multipolygons.rake | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 lib/tasks/deployment/20200414104712_split_geo_area_selection_multipolygons.rake diff --git a/lib/tasks/deployment/20200414104712_split_geo_area_selection_multipolygons.rake b/lib/tasks/deployment/20200414104712_split_geo_area_selection_multipolygons.rake new file mode 100644 index 000000000..930a650aa --- /dev/null +++ b/lib/tasks/deployment/20200414104712_split_geo_area_selection_multipolygons.rake @@ -0,0 +1,18 @@ +namespace :after_party do + desc 'Deployment task: split_geo_area_selection_multipolygons' + task split_geo_area_selection_multipolygons: :environment do + puts "Running deploy task 'split_geo_area_selection_multipolygons'" + + Champs::CarteChamp.where.not(value: ['', '[]']).includes(:geo_areas).find_each do |champ| + if champ.send(:selection_utilisateur_legacy?) + legacy_selection_utilisateur = champ.selections_utilisateur.first + champ.send(:legacy_selections_utilisateur).each do |area| + champ.geo_areas << area + end + legacy_selection_utilisateur.destroy + end + end + + AfterParty::TaskRecord.create version: '20200414104712' + end +end From 442a6a3cdded6f6fe490ea6cc5e4a32e74d8efdc Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Wed, 15 Apr 2020 17:55:37 +0200 Subject: [PATCH 4/9] Remove deprecated map reader --- app/javascript/new_design/champs/carte.js | 4 - app/javascript/shared/carte.js | 104 ---------------------- 2 files changed, 108 deletions(-) delete mode 100644 app/javascript/shared/carte.js diff --git a/app/javascript/new_design/champs/carte.js b/app/javascript/new_design/champs/carte.js index 2ae19ac8d..a5b6c2ebe 100644 --- a/app/javascript/new_design/champs/carte.js +++ b/app/javascript/new_design/champs/carte.js @@ -16,10 +16,6 @@ async function loadAndDrawMap(element) { const { drawEditableMap } = await import('../../shared/carte-editor'); drawEditableMap(element, data); - } else { - const { drawMap } = await import('../../shared/carte'); - - drawMap(element, data); } } diff --git a/app/javascript/shared/carte.js b/app/javascript/shared/carte.js deleted file mode 100644 index cd0434076..000000000 --- a/app/javascript/shared/carte.js +++ /dev/null @@ -1,104 +0,0 @@ -import L from 'leaflet'; - -const MAPS = new WeakMap(); - -export function drawMap(element, data) { - const map = initMap(element, data); - - drawCadastre(map, data); - drawQuartiersPrioritaires(map, data); - drawParcellesAgricoles(map, data); - drawUserSelection(map, data); -} - -function initMap(element, { position }) { - if (MAPS.has(element)) { - return MAPS.get(element); - } else { - const map = L.map(element, { - scrollWheelZoom: false - }).setView([position.lat, position.lon], position.zoom); - - const loadTilesLayer = process.env.RAILS_ENV != 'test'; - if (loadTilesLayer) { - L.tileLayer('https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png', { - attribution: - '© OpenStreetMap contributors' - }).addTo(map); - } - - MAPS.set(element, map); - return map; - } -} - -function drawUserSelection(map, { selection }) { - if (selection) { - const layer = L.geoJSON(selection, { - style: USER_SELECTION_POLYGON_STYLE - }); - - layer.addTo(map); - - map.fitBounds(layer.getBounds()); - } -} - -function drawCadastre(map, { cadastres }) { - drawLayer(map, cadastres, noEditStyle(CADASTRE_POLYGON_STYLE)); -} - -function drawQuartiersPrioritaires(map, { quartiersPrioritaires }) { - drawLayer(map, quartiersPrioritaires, noEditStyle(QP_POLYGON_STYLE)); -} - -function drawParcellesAgricoles(map, { parcellesAgricoles }) { - drawLayer(map, parcellesAgricoles, noEditStyle(RPG_POLYGON_STYLE)); -} - -function drawLayer(map, data, style) { - if (Array.isArray(data) && data.length > 0) { - const layer = new L.GeoJSON(undefined, { - interactive: false, - style - }); - - for (let { geometry } of data) { - layer.addData(geometry); - } - - layer.addTo(map); - } -} - -function noEditStyle(style) { - return Object.assign({}, style, { - opacity: 0.7, - fillOpacity: 0.5, - color: style.fillColor - }); -} - -const POLYGON_STYLE = { - weight: 2, - opacity: 0.3, - color: 'white', - dashArray: '3', - fillOpacity: 0.7 -}; - -const CADASTRE_POLYGON_STYLE = Object.assign({}, POLYGON_STYLE, { - fillColor: '#8a6d3b' -}); - -const QP_POLYGON_STYLE = Object.assign({}, POLYGON_STYLE, { - fillColor: '#31708f' -}); - -const RPG_POLYGON_STYLE = Object.assign({}, POLYGON_STYLE, { - fillColor: '#31708f' -}); - -const USER_SELECTION_POLYGON_STYLE = { - color: 'red' -}; From ee3ff78b1285a930fa9ed8b0f53dcb5cdbdef72a Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Thu, 16 Apr 2020 10:22:07 +0200 Subject: [PATCH 5/9] Create constants for default map location --- app/models/champs/carte_champ.rb | 10 +++++++--- app/models/dossier.rb | 4 ++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/models/champs/carte_champ.rb b/app/models/champs/carte_champ.rb index a2fd97737..23269ac02 100644 --- a/app/models/champs/carte_champ.rb +++ b/app/models/champs/carte_champ.rb @@ -1,4 +1,8 @@ class Champs::CarteChamp < Champ + # Default map location. Center of the World, ahm, France... + DEFAULT_LON = 2.428462 + DEFAULT_LAT = 46.538192 + # We are not using scopes here as we want to access # the following collections on unsaved records. def cadastres @@ -41,8 +45,8 @@ class Champs::CarteChamp < Champ if dossier.present? dossier.geo_position else - lon = "2.428462" - lat = "46.538192" + lon = DEFAULT_LON.to_s + lat = DEFAULT_LAT.to_s zoom = "13" { lon: lon, lat: lat, zoom: zoom } @@ -61,7 +65,7 @@ class Champs::CarteChamp < Champ point = dossier.geo_position bounding_box.add(factory.point(point[:lon], point[:lat])) else - bounding_box.add(factory.point(2.428462, 46.538192)) + bounding_box.add(factory.point(DEFAULT_LON, DEFAULT_LAT)) end [bounding_box.max_point, bounding_box.min_point].compact.flat_map(&:coordinates) diff --git a/app/models/dossier.rb b/app/models/dossier.rb index f051c4b88..0fdba3a50 100644 --- a/app/models/dossier.rb +++ b/app/models/dossier.rb @@ -425,8 +425,8 @@ class Dossier < ApplicationRecord point = Geocoder.search(etablissement.geo_adresse).first end - lon = "2.428462" - lat = "46.538192" + lon = Champs::CarteChamp::DEFAULT_LON.to_s + lat = Champs::CarteChamp::DEFAULT_LAT.to_s zoom = "13" if point.present? From d8f3b86b0e79691a836ff3f1beac001847b9f272 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 15 Apr 2020 14:20:31 +0000 Subject: [PATCH 6/9] javascript: move auto-upload attachment to the Uploader class Rationale: - It makes more sense to handle the progress bar updates in a single class; - This will allow us to unify the error handling. --- .../dossiers/auto-upload-controller.js | 69 ++++--------------- .../shared/activestorage/uploader.js | 47 +++++++++++-- 2 files changed, 57 insertions(+), 59 deletions(-) diff --git a/app/javascript/new_design/dossiers/auto-upload-controller.js b/app/javascript/new_design/dossiers/auto-upload-controller.js index ad4eec39c..c33ff4e8e 100644 --- a/app/javascript/new_design/dossiers/auto-upload-controller.js +++ b/app/javascript/new_design/dossiers/auto-upload-controller.js @@ -1,6 +1,5 @@ import Uploader from '../../shared/activestorage/uploader'; -import ProgressBar from '../../shared/activestorage/progress-bar'; -import { ajax, show, hide, toggle } from '@utils'; +import { show, hide, toggle } from '@utils'; // Given a file input in a champ with a selected file, upload a file, // then attach it to the dossier. @@ -11,27 +10,19 @@ export default class AutoUploadController { constructor(input, file) { this.input = input; this.file = file; + this.uploader = new Uploader( + input, + file, + input.dataset.directUploadUrl, + input.dataset.autoAttachUrl + ); } async start() { try { this._begin(); - - // Sanity checks - const autoAttachUrl = this.input.dataset.autoAttachUrl; - if (!autoAttachUrl) { - throw new Error('L’attribut "data-auto-attach-url" est manquant'); - } - - // Upload the file (using Direct Upload) - let blobSignedId = await this._upload(); - - // Attach the blob to the champ - // (The request responds with Javascript, which displays the attachment HTML fragment). - await this._attach(blobSignedId, autoAttachUrl); - - // Everything good: clear the original file input value - this.input.value = null; + await this.uploader.start(); + this._succeeded(); } catch (error) { this._failed(error); throw error; @@ -45,35 +36,8 @@ export default class AutoUploadController { this._hideErrorMessage(); } - async _upload() { - const uploader = new Uploader( - this.input, - this.file, - this.input.dataset.directUploadUrl - ); - return await uploader.start(); - } - - async _attach(blobSignedId, autoAttachUrl) { - // Now that the upload is done, display a new progress bar - // to show that the attachment request is still pending. - const progressBar = new ProgressBar( - this.input, - `${this.input.id}-progress-bar`, - this.file - ); - progressBar.progress(100); - progressBar.end(); - - const attachmentRequest = { - url: autoAttachUrl, - type: 'PUT', - data: `blob_signed_id=${blobSignedId}` - }; - await ajax(attachmentRequest); - - // The progress bar has been destroyed by the attachment HTML fragment that replaced the input, - // so no further cleanup is needed. + _succeeded() { + this.input.value = null; } _failed(error) { @@ -81,12 +45,10 @@ export default class AutoUploadController { return; } - let progressBar = this.input.parentElement.querySelector('.direct-upload'); - if (progressBar) { - progressBar.remove(); - } + this.uploader.progressBar.destroy(); - this._displayErrorMessage(error); + let message = this._messageFromError(error); + this._displayErrorMessage(message); } _done() { @@ -126,11 +88,10 @@ export default class AutoUploadController { } } - _displayErrorMessage(error) { + _displayErrorMessage(message) { let errorNode = this.input.parentElement.querySelector('.attachment-error'); if (errorNode) { show(errorNode); - let message = this._messageFromError(error); errorNode.querySelector('.attachment-error-title').textContent = message.title || ''; errorNode.querySelector('.attachment-error-description').textContent = diff --git a/app/javascript/shared/activestorage/uploader.js b/app/javascript/shared/activestorage/uploader.js index b303a441e..04dd21be5 100644 --- a/app/javascript/shared/activestorage/uploader.js +++ b/app/javascript/shared/activestorage/uploader.js @@ -1,4 +1,5 @@ import { DirectUpload } from '@rails/activestorage'; +import { ajax } from '@utils'; import ProgressBar from './progress-bar'; import errorFromDirectUploadMessage from './errors'; @@ -7,29 +8,65 @@ import errorFromDirectUploadMessage from './errors'; used to track lifecycle and progress of an upload. */ export default class Uploader { - constructor(input, file, directUploadUrl) { + constructor(input, file, directUploadUrl, autoAttachUrl) { this.directUpload = new DirectUpload(file, directUploadUrl, this); this.progressBar = new ProgressBar(input, this.directUpload.id, file); + this.autoAttachUrl = autoAttachUrl; } - start() { + /** + Upload (and optionally attach) the file. + Returns the blob signed id on success. + */ + async start() { this.progressBar.start(); + try { + let blobSignedId = await this._upload(); + + if (this.autoAttachUrl) { + await this._attach(blobSignedId); + } + + this.progressBar.end(); + this.progressBar.destroy(); + + return blobSignedId; + } catch (error) { + this.progressBar.error(error.message); + throw error; + } + } + + /** + Upload the file using the DirectUpload instance, and return the blob signed_id. + */ + async _upload() { return new Promise((resolve, reject) => { this.directUpload.create((errorMsg, attributes) => { if (errorMsg) { - this.progressBar.error(errorMsg); let error = errorFromDirectUploadMessage(errorMsg); reject(error); } else { resolve(attributes.signed_id); } - this.progressBar.end(); - this.progressBar.destroy(); }); }); } + /** + Attach the file by sending a POST request to the autoAttachUrl. + */ + async _attach(blobSignedId) { + const attachmentRequest = { + url: this.autoAttachUrl, + type: 'PUT', + data: `blob_signed_id=${blobSignedId}` + }; + + await ajax(attachmentRequest); + } + uploadRequestDidProgress(event) { const progress = (event.loaded / event.total) * 100; if (progress) { From 432967bd769f36a1d8fbdfa2cebaeb839bbbdb9a Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 15 Apr 2020 16:29:15 +0200 Subject: [PATCH 7/9] javascript: make Uploader always throw the same kind of errors A DirectUpload may fail for several reasons, and return many types of errors (string, xhr response, Error objects, etc). For convenience, wrap all these errors in a FileUploadError object. - It makes easier for clients of the Uploader class to handle errors; - It allows to propagate the error code and failure responsability. --- .../dossiers/auto-upload-controller.js | 37 +++------- app/javascript/shared/activestorage/errors.js | 22 ------ .../shared/activestorage/file-upload-error.js | 67 +++++++++++++++++++ app/javascript/shared/activestorage/ujs.js | 2 +- .../shared/activestorage/uploader.js | 17 ++++- 5 files changed, 92 insertions(+), 53 deletions(-) delete mode 100644 app/javascript/shared/activestorage/errors.js create mode 100644 app/javascript/shared/activestorage/file-upload-error.js diff --git a/app/javascript/new_design/dossiers/auto-upload-controller.js b/app/javascript/new_design/dossiers/auto-upload-controller.js index c33ff4e8e..5ac04e7ad 100644 --- a/app/javascript/new_design/dossiers/auto-upload-controller.js +++ b/app/javascript/new_design/dossiers/auto-upload-controller.js @@ -18,6 +18,8 @@ export default class AutoUploadController { ); } + // Create, upload and attach the file. + // On failure, display an error message and throw a FileUploadError. async start() { try { this._begin(); @@ -55,36 +57,15 @@ export default class AutoUploadController { this.input.disabled = false; } - _isError422(error) { - // Ajax errors have an xhr attribute - if (error && error.xhr && error.xhr.status == 422) return true; - // Rails DirectUpload errors are returned as a String, e.g. 'Error creating Blob for "Demain.txt". Status: 422' - if (error && error.toString().includes('422')) return true; - - return false; - } - _messageFromError(error) { - let allowRetry = !this._isError422(error); + let message = error.message || error.toString(); + let canRetry = error.status && error.status != 422; - if ( - error.xhr && - error.xhr.status == 422 && - error.response && - error.response.errors && - error.response.errors[0] - ) { - return { - title: error.response.errors[0], - description: '', - retry: allowRetry - }; - } else { - return { - title: 'Une erreur s’est produite pendant l’envoi du fichier.', - description: error.message || error.toString(), - retry: allowRetry - }; + + return { + title: 'Le fichier n’a pas pu être envoyé.', + description: message, + retry: canRetry } } diff --git a/app/javascript/shared/activestorage/errors.js b/app/javascript/shared/activestorage/errors.js deleted file mode 100644 index 072e99b5c..000000000 --- a/app/javascript/shared/activestorage/errors.js +++ /dev/null @@ -1,22 +0,0 @@ -// Convert an error message returned by DirectUpload to a proper error object. -// -// This function has two goals: -// 1. Remove the file name from the DirectUpload error message -// (because the filename confuses Sentry error grouping) -// 2. Create each kind of error on a different line -// (so that Sentry knows they are different kind of errors, from -// the line they were created.) -export default function errorFromDirectUploadMessage(message) { - let matches = message.match(/ Status: [0-9]{1,3}/); - let status = (matches && matches[0]) || ''; - - if (message.includes('Error creating')) { - return new Error('Error creating file.' + status); - } else if (message.includes('Error storing')) { - return new Error('Error storing file.' + status); - } else if (message.includes('Error reading')) { - return new Error('Error reading file.' + status); - } else { - return new Error(message); - } -} diff --git a/app/javascript/shared/activestorage/file-upload-error.js b/app/javascript/shared/activestorage/file-upload-error.js new file mode 100644 index 000000000..360557956 --- /dev/null +++ b/app/javascript/shared/activestorage/file-upload-error.js @@ -0,0 +1,67 @@ +// Error while reading the file client-side +export const ERROR_CODE_READ = 'file-upload-read-error'; +// Error while creating the empty blob on the server +export const ERROR_CODE_CREATE = 'file-upload-create-error'; +// Error while uploading the blob content +export const ERROR_CODE_STORE = 'file-upload-store-error'; +// Error while attaching the blob to the record +export const ERROR_CODE_ATTACH = 'file-upload-attach-error'; + +// Failure on the client side (syntax error, error reading file, etc.) +export const FAILURE_CLIENT = 'file-upload-failure-client'; +// Failure on the server side (typically non-200 response) +export const FAILURE_SERVER = 'file-upload-failure-server'; +// Failure during the transfert (request aborted, connection lost, etc) +export const FAILURE_CONNECTIVITY = 'file-upload-failure-connectivity'; + +/** + Represent an error during a file upload. + */ +export default class FileUploadError extends Error { + constructor(message, status, code) { + super(message); + this.name = 'FileUploadError'; + this.status = status; + this.code = code; + } + + /** + Return the component responsible of the error (client, server or connectivity). + See FAILURE_* constants for values. + */ + get failureReason() { + let isNetworkError = this.code != ERROR_CODE_READ; + + if (isNetworkError && this.status != 0) { + return FAILURE_SERVER; + } else if (isNetworkError && this.status == 0) { + return FAILURE_CONNECTIVITY; + } else { + return FAILURE_CLIENT; + } + } +} + +// Convert an error message returned by DirectUpload to a proper error object. +// +// This function has two goals: +// 1. Remove the file name from the DirectUpload error message +// (because the filename confuses Sentry error grouping) +// 2. Create each kind of error on a different line +// (so that Sentry knows they are different kind of errors, from +// the line they were created.) +export function errorFromDirectUploadMessage(message) { + let matches = message.match(/ Status: [0-9]{1,3}/); + let status = (matches && parseInt(matches[0], 10)) || undefined; + + // prettier-ignore + if (message.includes('Error reading')) { + return new FileUploadError('Error reading file.', status, ERROR_CODE_READ); + } else if (message.includes('Error creating')) { + return new FileUploadError('Error creating file.', status, ERROR_CODE_CREATE); + } else if (message.includes('Error storing')) { + return new FileUploadError('Error storing file.', status, ERROR_CODE_STORE); + } else { + return new FileUploadError(message, status, undefined); + } +} diff --git a/app/javascript/shared/activestorage/ujs.js b/app/javascript/shared/activestorage/ujs.js index aa3530ec2..d990d2d87 100644 --- a/app/javascript/shared/activestorage/ujs.js +++ b/app/javascript/shared/activestorage/ujs.js @@ -1,5 +1,5 @@ import ProgressBar from './progress-bar'; -import errorFromDirectUploadMessage from './errors'; +import { errorFromDirectUploadMessage } from './file-upload-error'; import { fire } from '@utils'; const INITIALIZE_EVENT = 'direct-upload:initialize'; diff --git a/app/javascript/shared/activestorage/uploader.js b/app/javascript/shared/activestorage/uploader.js index 04dd21be5..ad9a2ffc1 100644 --- a/app/javascript/shared/activestorage/uploader.js +++ b/app/javascript/shared/activestorage/uploader.js @@ -1,7 +1,7 @@ import { DirectUpload } from '@rails/activestorage'; import { ajax } from '@utils'; import ProgressBar from './progress-bar'; -import errorFromDirectUploadMessage from './errors'; +import FileUploadError, { errorFromDirectUploadMessage, ERROR_CODE_ATTACH } from './file-upload-error'; /** Uploader class is a delegate for DirectUpload instance @@ -17,6 +17,7 @@ export default class Uploader { /** Upload (and optionally attach) the file. Returns the blob signed id on success. + Throws a FileUploadError on failure. */ async start() { this.progressBar.start(); @@ -40,6 +41,7 @@ export default class Uploader { /** Upload the file using the DirectUpload instance, and return the blob signed_id. + Throws a FileUploadError on failure. */ async _upload() { return new Promise((resolve, reject) => { @@ -56,6 +58,8 @@ export default class Uploader { /** Attach the file by sending a POST request to the autoAttachUrl. + Throws a FileUploadError on failure (containing the first validation + error message, if any). */ async _attach(blobSignedId) { const attachmentRequest = { @@ -64,7 +68,16 @@ export default class Uploader { data: `blob_signed_id=${blobSignedId}` }; - await ajax(attachmentRequest); + try { + await ajax(attachmentRequest); + } catch (e) { + let message = e.response && e.response.errors && e.response.errors[0]; + throw new FileUploadError( + message || 'Error attaching file.', + e.xhr.status, + ERROR_CODE_ATTACH + ); + } } uploadRequestDidProgress(event) { From 55788990da1c3be5390c2f2d4bc1d1cab2d9eb5e Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 15 Apr 2020 16:34:25 +0200 Subject: [PATCH 8/9] javascript: add a helpful message on connectivity error --- .../dossiers/auto-upload-controller.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/app/javascript/new_design/dossiers/auto-upload-controller.js b/app/javascript/new_design/dossiers/auto-upload-controller.js index 5ac04e7ad..a2780a65e 100644 --- a/app/javascript/new_design/dossiers/auto-upload-controller.js +++ b/app/javascript/new_design/dossiers/auto-upload-controller.js @@ -1,5 +1,6 @@ import Uploader from '../../shared/activestorage/uploader'; import { show, hide, toggle } from '@utils'; +import { FAILURE_CONNECTIVITY } from '../../shared/activestorage/file-upload-error'; // Given a file input in a champ with a selected file, upload a file, // then attach it to the dossier. @@ -61,11 +62,18 @@ export default class AutoUploadController { let message = error.message || error.toString(); let canRetry = error.status && error.status != 422; - - return { - title: 'Le fichier n’a pas pu être envoyé.', - description: message, - retry: canRetry + if (error.failureReason == FAILURE_CONNECTIVITY) { + return { + title: 'Le fichier n’a pas pu être envoyé.', + description: 'Vérifiez votre connexion à Internet, puis ré-essayez.', + retry: true + }; + } else { + return { + title: 'Le fichier n’a pas pu être envoyé.', + description: message, + retry: canRetry + }; } } From b2231e98d55381168eb978d98b9c785fa3f1c145 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 15 Apr 2020 14:35:39 +0000 Subject: [PATCH 9/9] javascript: don't report connectivity issues to Sentry --- .../new_design/dossiers/auto-uploads-controllers.js | 6 ++++++ app/javascript/shared/activestorage/ujs.js | 9 +++++++-- app/javascript/shared/activestorage/uploader.js | 5 ++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/app/javascript/new_design/dossiers/auto-uploads-controllers.js b/app/javascript/new_design/dossiers/auto-uploads-controllers.js index a50b083ac..ad68b081d 100644 --- a/app/javascript/new_design/dossiers/auto-uploads-controllers.js +++ b/app/javascript/new_design/dossiers/auto-uploads-controllers.js @@ -1,5 +1,6 @@ import Rails from '@rails/ujs'; import AutoUploadController from './auto-upload-controller.js'; +import { FAILURE_CONNECTIVITY } from '../../shared/activestorage/file-upload-error'; // Manage multiple concurrent uploads. // @@ -17,6 +18,11 @@ export default class AutoUploadsControllers { try { let controller = new AutoUploadController(input, file); await controller.start(); + } catch (error) { + // Report errors to Sentry (except connectivity issues) + if (error.failureReason != FAILURE_CONNECTIVITY) { + throw error; + } } finally { this._decrementInFlightUploads(form); } diff --git a/app/javascript/shared/activestorage/ujs.js b/app/javascript/shared/activestorage/ujs.js index d990d2d87..bdc3b5031 100644 --- a/app/javascript/shared/activestorage/ujs.js +++ b/app/javascript/shared/activestorage/ujs.js @@ -1,5 +1,8 @@ import ProgressBar from './progress-bar'; -import { errorFromDirectUploadMessage } from './file-upload-error'; +import { + errorFromDirectUploadMessage, + FAILURE_CONNECTIVITY +} from './file-upload-error'; import { fire } from '@utils'; const INITIALIZE_EVENT = 'direct-upload:initialize'; @@ -56,7 +59,9 @@ addUploadEventListener(ERROR_EVENT, event => { ProgressBar.error(id, errorMsg); let error = errorFromDirectUploadMessage(errorMsg); - fire(document, 'sentry:capture-exception', error); + if (error.failureReason != FAILURE_CONNECTIVITY) { + fire(document, 'sentry:capture-exception', error); + } }); addUploadEventListener(END_EVENT, ({ detail: { id } }) => { diff --git a/app/javascript/shared/activestorage/uploader.js b/app/javascript/shared/activestorage/uploader.js index ad9a2ffc1..1d1dcce8b 100644 --- a/app/javascript/shared/activestorage/uploader.js +++ b/app/javascript/shared/activestorage/uploader.js @@ -1,7 +1,10 @@ import { DirectUpload } from '@rails/activestorage'; import { ajax } from '@utils'; import ProgressBar from './progress-bar'; -import FileUploadError, { errorFromDirectUploadMessage, ERROR_CODE_ATTACH } from './file-upload-error'; +import FileUploadError, { + errorFromDirectUploadMessage, + ERROR_CODE_ATTACH +} from './file-upload-error'; /** Uploader class is a delegate for DirectUpload instance