From 30a7fb25a7141dbda02ea8e150478b7c634225eb Mon Sep 17 00:00:00 2001 From: simon lehericey Date: Tue, 1 Mar 2022 10:01:33 +0100 Subject: [PATCH 01/12] update nokogiri --- Gemfile.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 50bdda94b..c249c873c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -420,7 +420,7 @@ GEM rake mini_magick (4.11.0) mini_mime (1.1.2) - mini_portile2 (2.7.1) + mini_portile2 (2.8.0) minitest (5.15.0) momentjs-rails (2.20.1) railties (>= 3.1) @@ -431,8 +431,8 @@ GEM ruby2_keywords (~> 0.0.1) netrc (0.11.0) nio4r (2.5.8) - nokogiri (1.13.1) - mini_portile2 (~> 2.7.0) + nokogiri (1.13.3) + mini_portile2 (~> 2.8.0) racc (~> 1.4) open4 (1.3.4) openid_connect (1.3.0) From 2a426903889f939aee0fcd67a12a99774f4f9b7f Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Thu, 24 Feb 2022 16:20:17 +0000 Subject: [PATCH 02/12] fix(carto): show map UI before loading map styles --- .eslintrc.js | 3 ++- .../MapEditor/components/AddressInput.tsx | 10 +++------- .../MapEditor/components/DrawLayer.tsx | 15 ++++++++++++++- .../MapEditor/components/PointInput.tsx | 18 ++++++------------ app/javascript/components/MapEditor/index.tsx | 19 ++++++------------- .../MapReader/components/GeoJSONLayer.tsx | 14 +++++++++++++- .../components/shared/maplibre/MapLibre.tsx | 6 +----- 7 files changed, 45 insertions(+), 40 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index cc5432980..1dee4619e 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -57,7 +57,8 @@ module.exports = { 'prettier/prettier': 'error', 'react-hooks/rules-of-hooks': 'error', 'react-hooks/exhaustive-deps': 'error', - '@typescript-eslint/no-explicit-any': 'error' + '@typescript-eslint/no-explicit-any': 'error', + '@typescript-eslint/no-unused-vars': 'error' } } ] diff --git a/app/javascript/components/MapEditor/components/AddressInput.tsx b/app/javascript/components/MapEditor/components/AddressInput.tsx index 9becf7034..f1745d97a 100644 --- a/app/javascript/components/MapEditor/components/AddressInput.tsx +++ b/app/javascript/components/MapEditor/components/AddressInput.tsx @@ -1,12 +1,9 @@ import React from 'react'; -import type { Point } from 'geojson'; +import { fire } from '@utils'; import ComboAdresseSearch from '../../ComboAdresseSearch'; -import { useFlyTo } from '../../shared/maplibre/hooks'; export function AddressInput() { - const flyTo = useFlyTo(); - return (
{ - const geometry = result?.geometry as Point; - flyTo(17, geometry.coordinates as [number, number]); + onChange={(_, feature) => { + fire(document, 'map:zoom', { feature }); }} />
diff --git a/app/javascript/components/MapEditor/components/DrawLayer.tsx b/app/javascript/components/MapEditor/components/DrawLayer.tsx index 9bdeb3a2a..ca0f24e53 100644 --- a/app/javascript/components/MapEditor/components/DrawLayer.tsx +++ b/app/javascript/components/MapEditor/components/DrawLayer.tsx @@ -7,7 +7,8 @@ import { useMapLibre } from '../../shared/maplibre/MapLibre'; import { useFitBounds, useEvent, - useMapEvent + useMapEvent, + useFlyTo } from '../../shared/maplibre/hooks'; import { filterFeatureCollection, @@ -116,6 +117,7 @@ function useExternalEvents( } ) { const fitBounds = useFitBounds(); + const flyTo = useFlyTo(); const onFeatureFocus = useCallback( ({ detail }) => { @@ -132,6 +134,16 @@ function useExternalEvents( [featureCollection, fitBounds] ); + const onZoomFocus = useCallback( + ({ detail }) => { + const { feature } = detail; + if (feature) { + flyTo(17, feature.geometry.coordinates); + } + }, + [flyTo] + ); + const onFeatureCreate = useCallback( ({ detail }) => { const { geometry, properties } = detail; @@ -181,6 +193,7 @@ function useExternalEvents( useEvent('map:feature:create', onFeatureCreate); useEvent('map:feature:update', onFeatureUpdate); useEvent('map:feature:delete', onFeatureDelete); + useEvent('map:zoom', onZoomFocus); } const translations = [ diff --git a/app/javascript/components/MapEditor/components/PointInput.tsx b/app/javascript/components/MapEditor/components/PointInput.tsx index 22a4f9ea0..878235f36 100644 --- a/app/javascript/components/MapEditor/components/PointInput.tsx +++ b/app/javascript/components/MapEditor/components/PointInput.tsx @@ -5,11 +5,7 @@ import { PlusIcon, LocationMarkerIcon } from '@heroicons/react/outline'; import { useId } from '@reach/auto-id'; import CoordinateInput from 'react-coordinate-input'; -import { useFlyTo } from '../../shared/maplibre/hooks'; - export function PointInput() { - const flyTo = useFlyTo(); - const inputId = useId(); const [value, setValue] = useState(''); const [feature, setFeature] = useState(null); @@ -60,15 +56,13 @@ export function PointInput() { setValue(value); if (dd.length) { const coordinates: [number, number] = [dd[1], dd[0]]; - setFeature({ - type: 'Feature', - geometry: { - type: 'Point', - coordinates - }, + const feature = { + type: 'Feature' as const, + geometry: { type: 'Point' as const, coordinates }, properties: {} - }); - flyTo(17, coordinates); + }; + setFeature(feature); + fire(document, 'map:zoom', { feature }); } else { setFeature(null); } diff --git a/app/javascript/components/MapEditor/index.tsx b/app/javascript/components/MapEditor/index.tsx index a84094835..a5d81741e 100644 --- a/app/javascript/components/MapEditor/index.tsx +++ b/app/javascript/components/MapEditor/index.tsx @@ -46,19 +46,11 @@ export default function MapEditor({

{error && } - - - - - } - footer={} - > + + + + + ) : null} + ); } diff --git a/app/javascript/components/MapReader/components/GeoJSONLayer.tsx b/app/javascript/components/MapReader/components/GeoJSONLayer.tsx index f7161d00d..821f709ff 100644 --- a/app/javascript/components/MapReader/components/GeoJSONLayer.tsx +++ b/app/javascript/components/MapReader/components/GeoJSONLayer.tsx @@ -7,7 +7,8 @@ import { useFitBounds, useEvent, EventHandler, - useMapEvent + useMapEvent, + useFlyTo } from '../../shared/maplibre/hooks'; import { filterFeatureCollection, @@ -99,6 +100,7 @@ export function GeoJSONLayer({ function useExternalEvents(featureCollection: FeatureCollection) { const fitBounds = useFitBounds(); + const flyTo = useFlyTo(); const onFeatureFocus = useCallback( ({ detail }) => { const { id } = detail; @@ -109,6 +111,15 @@ function useExternalEvents(featureCollection: FeatureCollection) { }, [featureCollection, fitBounds] ); + const onZoomFocus = useCallback( + ({ detail }) => { + const { feature } = detail; + if (feature) { + flyTo(17, feature.geometry.coordinates); + } + }, + [flyTo] + ); useEffect(() => { fitBounds(featureCollection.bbox as LngLatBoundsLike); @@ -117,6 +128,7 @@ function useExternalEvents(featureCollection: FeatureCollection) { }, [fitBounds]); useEvent('map:feature:focus', onFeatureFocus); + useEvent('map:zoom', onZoomFocus); } function LineStringLayer({ diff --git a/app/javascript/components/shared/maplibre/MapLibre.tsx b/app/javascript/components/shared/maplibre/MapLibre.tsx index 2b439abfa..1c29341c3 100644 --- a/app/javascript/components/shared/maplibre/MapLibre.tsx +++ b/app/javascript/components/shared/maplibre/MapLibre.tsx @@ -19,8 +19,6 @@ const Context = createContext<{ map?: Map | null }>({}); type MapLibreProps = { layers: string[]; - header?: ReactNode; - footer?: ReactNode; children: ReactNode; }; @@ -30,7 +28,7 @@ export function useMapLibre() { return context.map; } -export function MapLibre({ children, header, footer, layers }: MapLibreProps) { +export function MapLibre({ children, layers }: MapLibreProps) { const isSupported = useMemo( () => maplibre.supported({ failIfMajorPerformanceCaveat: true }) && !isIE(), [] @@ -90,12 +88,10 @@ export function MapLibre({ children, header, footer, layers }: MapLibreProps) { return ( - {map ? header : null}
{map ? children : null}
- {map ? footer : null} ); } From 71e1b6c97380f56834d26fbc7f47cd6ccf851c5c Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Wed, 23 Feb 2022 17:04:24 +0100 Subject: [PATCH 03/12] models: delete AdministrateursProcedure when destroying Administrateur By default, `has_and_belongs_to_many` properly deletes the record in the join table. However, as the association is declared manually with a `has_many / through`, it doesn't delete the joined record automatically. As we also lack a foreign-key contraint on the join table, that means a dangling record remains in the join table. To fix this, let's declare it a proper `has_and_belongs_to_many` association, which will let the join record be deleted automatically on destroy. --- app/models/administrateur.rb | 3 +-- spec/models/administrateur_spec.rb | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/app/models/administrateur.rb b/app/models/administrateur.rb index ebd1e9470..7bdaa90fa 100644 --- a/app/models/administrateur.rb +++ b/app/models/administrateur.rb @@ -12,8 +12,7 @@ class Administrateur < ApplicationRecord include ActiveRecord::SecureToken has_and_belongs_to_many :instructeurs - has_many :administrateurs_procedures - has_many :procedures, through: :administrateurs_procedures + has_and_belongs_to_many :procedures has_many :services has_one :user, dependent: :nullify diff --git a/spec/models/administrateur_spec.rb b/spec/models/administrateur_spec.rb index 55e6c4d32..ce21fb67a 100644 --- a/spec/models/administrateur_spec.rb +++ b/spec/models/administrateur_spec.rb @@ -3,7 +3,7 @@ describe Administrateur, type: :model do describe 'associations' do it { is_expected.to have_and_belong_to_many(:instructeurs) } - it { is_expected.to have_many(:procedures) } + it { is_expected.to have_and_belong_to_many(:procedures) } end describe "#renew_api_token" do From 3fde7021e71f1d471e85d64b8efdfc2ca5aefd37 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 1 Mar 2022 07:47:39 +0000 Subject: [PATCH 04/12] db: add foreign key contraint to AdministrateursProcedure --- ...ur_foreign_key_to_administrateurs_procedure.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 db/migrate/20220301160753_add_administrateur_foreign_key_to_administrateurs_procedure.rb diff --git a/db/migrate/20220301160753_add_administrateur_foreign_key_to_administrateurs_procedure.rb b/db/migrate/20220301160753_add_administrateur_foreign_key_to_administrateurs_procedure.rb new file mode 100644 index 000000000..d8d0bbb0a --- /dev/null +++ b/db/migrate/20220301160753_add_administrateur_foreign_key_to_administrateurs_procedure.rb @@ -0,0 +1,15 @@ +class AddAdministrateurForeignKeyToAdministrateursProcedure < ActiveRecord::Migration[6.1] + def up + # Sanity check + say_with_time 'Removing AdministrateursProcedures where the associated Administrateur no longer exists ' do + deleted_administrateur_ids = AdministrateursProcedure.where.missing(:administrateur).pluck(:administrateur_id) + AdministrateursProcedure.where(administrateur_id: deleted_administrateur_ids).delete_all + end + + add_foreign_key :administrateurs_procedures, :administrateurs + end + + def down + remove_foreign_key :administrateurs_procedures, :administrateurs + end +end From 4fd9312fc9cbe7bb50560cfc371f2fa577532f5e Mon Sep 17 00:00:00 2001 From: Matthieu FAURE Date: Thu, 17 Feb 2022 10:39:45 +0100 Subject: [PATCH 05/12] =?UTF-8?q?DOC=20Ajout=20doc=20d=C3=A9ploiement,=20a?= =?UTF-8?q?vec=20mont=C3=A9e=20de=20version=20et=20migration=20de=20donn?= =?UTF-8?q?=C3=A9es=20(afterparty)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- doc/Deploiment.md | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 doc/Deploiment.md diff --git a/doc/Deploiment.md b/doc/Deploiment.md new file mode 100644 index 000000000..4fae56c26 --- /dev/null +++ b/doc/Deploiment.md @@ -0,0 +1,11 @@ +# Déploiement d'une instance du logiciel Démarches-Simplifiées + +## Mises à jour + +* Une montée de version N vers N+1 fonctionne. +* Une montée de version N vers N+X (X>1) n'est pas (encore) gérée +* Depuis 2022, si des tâches de déploiement figurent dans une version, elles sont mentionnées + dans les [releases Github](https://github.com/betagouv/demarches-simplifiees.fr/releases) (mot clé : *AfterParty*) +* Courant 2021, certaines tâches AfterPArty ont malheureusement été supprimées. +* Pour les montées de versions de 2021, les tâches AfterParty sont incomplètes et nécessitent une intervention manuelle + pour migrer les données. From bbc45f25ef35412c54cf59534fbb1129a1fac248 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 22 Feb 2022 09:16:03 +0100 Subject: [PATCH 06/12] doc: add deployment notes --- README.md | 9 +------ doc/DEPLOYMENT.md | 61 +++++++++++++++++++++++++++++++++++++++++++++++ doc/Deploiment.md | 11 --------- 3 files changed, 62 insertions(+), 19 deletions(-) create mode 100644 doc/DEPLOYMENT.md delete mode 100644 doc/Deploiment.md diff --git a/README.md b/README.md index 226ab9857..099f9d371 100644 --- a/README.md +++ b/README.md @@ -132,14 +132,7 @@ Le projet utilise plusieurs linters pour vérifier la lisibilité et la qualité ## Déploiement -Dans le cas d’un déploiement sur plusieurs serveurs, l’application peut être déployée avec la tâche : - -``` -DOMAINS="web1 web2" BRANCH="main" bin/rake deploy -``` - -En interne, cette tâche utilise [mina](https://github.com/mina-deploy/mina) pour lancer les commandes -de déploiement sur tous les serveurs spécifiés. +Voir les notes de déploiement dans [DEPLOYMENT.md](doc/DEPLOYMENT.md) ## Tâches courantes diff --git a/doc/DEPLOYMENT.md b/doc/DEPLOYMENT.md new file mode 100644 index 000000000..1b975066d --- /dev/null +++ b/doc/DEPLOYMENT.md @@ -0,0 +1,61 @@ +# Deployment documentation + +demarches-simplifiees.fr is a standard Rails app, and can be deployed using standard methods (PaaS, Docker, bare-metal, etc.) Deployments are engineered not to require any downtime. + +## 1. Deploying demarches-simplifiees.fr + +Usually, a deployment goes like this (in pseudo-code): + +``` +# Run database schema migrations (e.g. `bin/rails db:migrate`) +# For each server: + # Stop the server + # Get the new code (e.g. `git clone git@github.com:betagouv/demarches-simplifiees.fr.git`) + # Install new dependencies (e.g. `bundle install && yarn install`) + # Restart the app server +# Run data migrations (e.g. `rake after_party:run`) +``` + +On the main instance, this deployment flow is implemented using [`mina`](https://github.com/mina-deploy/mina), which automatically sshs to the application servers, run the appropriate commands (see `lib/tasks/deploy.rake` and `config/deploy.rb`), and restarts the puma webserver in a way that ensures zero-downtime deployments. +A deploy on multiple application servers is typically done using: +```shell +DOMAINS="web1 web2" BRANCH="main" bin/rake deploy +``` + +But of course other methods can be used. + +## 2. Upgrading demarches-simplifiees.fr + +### 2.1 Standard upgrade path + +Theoretically, only deploying each version sequentially is fully supported. This means that to deploy the version N+3, the upgrade plan should be to deploy the version N+1, N+2 and then only N+3, in that order. + +Release notes for each version are available on [GitHub's Releases page](https://github.com/betagouv/demarches-simplifiees.fr/releases). Since 2022, when a release includes a database schema or data migration is present, this is mentionned in the release notes. + +### 2.2 Upgrading several releases at once + +Upgrading from several releases at once (like migrating directly from a version N to a version N+3) is theoretically unsupported. This is because database schema migrations and data migrations have to run in the exact order they were created, along the application code as it was when the migration was written. +That said, it is possible to batch the upgrade of several releases at once, _provided that the data migrations run in the correct order_. + +The rule of thumb is that _an intermediary upgrade should be done before every database schema migration that follows a data migration_. + +_NB: There are some plans to improve this, and contributions are welcome. See https://github.com/betagouv/demarches-simplifiees.fr/issues/6970_ + +# Historical notes + +- During 2021, some older data migration tasks were deleted from the repository. This has to be checked manually when upgrading from an older version. + ``` + lib/tasks/deployment/20200326133630_cleanup_deleted_dossiers.rake + lib/tasks/deployment/20200401123317_process_expired_dossiers_en_construction.rake + lib/tasks/deployment/20200527124112_fix_champ_etablissement.rake + lib/tasks/deployment/20200528124044_fix_dossier_etablissement.rake + lib/tasks/deployment/20200618121241_drop_down_list_options_to_json.rake + lib/tasks/deployment/20200625113026_migrate_revisions.rake + lib/tasks/deployment/20200630154829_add_traitements_from_dossiers.rake + lib/tasks/deployment/20200708101123_add_default_skip_validation_to_piece_justificative.rake + lib/tasks/deployment/20200728150458_fix_cloned_revisions.rake + lib/tasks/deployment/20200813111957_fix_geo_areas_geometry.rake + lib/tasks/deployment/20201001161931_migrate_filters_to_use_stable_id.rake + lib/tasks/deployment/20201006123842_setup_first_stats.rake + lib/tasks/deployment/20201218163035_fix_types_de_champ_revisions.rake + ``` diff --git a/doc/Deploiment.md b/doc/Deploiment.md deleted file mode 100644 index 4fae56c26..000000000 --- a/doc/Deploiment.md +++ /dev/null @@ -1,11 +0,0 @@ -# Déploiement d'une instance du logiciel Démarches-Simplifiées - -## Mises à jour - -* Une montée de version N vers N+1 fonctionne. -* Une montée de version N vers N+X (X>1) n'est pas (encore) gérée -* Depuis 2022, si des tâches de déploiement figurent dans une version, elles sont mentionnées - dans les [releases Github](https://github.com/betagouv/demarches-simplifiees.fr/releases) (mot clé : *AfterParty*) -* Courant 2021, certaines tâches AfterPArty ont malheureusement été supprimées. -* Pour les montées de versions de 2021, les tâches AfterParty sont incomplètes et nécessitent une intervention manuelle - pour migrer les données. From 64a28cc8bdf3d79520a2b3204202d7a418c8e0ff Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 22 Feb 2022 09:16:12 +0100 Subject: [PATCH 07/12] doc: move the ADR to their own directory --- doc/{ => architecture-decision-records}/adr-csrf-forgery.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename doc/{ => architecture-decision-records}/adr-csrf-forgery.md (100%) diff --git a/doc/adr-csrf-forgery.md b/doc/architecture-decision-records/adr-csrf-forgery.md similarity index 100% rename from doc/adr-csrf-forgery.md rename to doc/architecture-decision-records/adr-csrf-forgery.md From 19cde3140e2b8a13089942e9ebac8e2f7fd8b03f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 1 Mar 2022 23:35:43 +0000 Subject: [PATCH 08/12] chore(deps): bump image_processing from 1.12.1 to 1.12.2 Bumps [image_processing](https://github.com/janko/image_processing) from 1.12.1 to 1.12.2. - [Release notes](https://github.com/janko/image_processing/releases) - [Changelog](https://github.com/janko/image_processing/blob/master/CHANGELOG.md) - [Commits](https://github.com/janko/image_processing/compare/v1.12.1...v1.12.2) --- updated-dependencies: - dependency-name: image_processing dependency-type: direct:production ... Signed-off-by: dependabot[bot] --- Gemfile.lock | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index c249c873c..08472eebd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -251,7 +251,7 @@ GEM faraday-net_http_persistent (1.2.0) faraday-patron (1.0.0) faraday-rack (1.0.0) - ffi (1.15.4) + ffi (1.15.5) flipper (0.20.3) flipper-active_record (0.20.3) activerecord (>= 5.0, < 7) @@ -350,7 +350,7 @@ GEM i18n_data (0.13.0) iban-tools (1.1.0) ice_nine (0.11.2) - image_processing (1.12.1) + image_processing (1.12.2) mini_magick (>= 4.9.5, < 5) ruby-vips (>= 2.0.17, < 3) invisible_captcha (2.0.0) @@ -621,8 +621,8 @@ GEM rexml ruby-progressbar (1.11.0) ruby-saml-idp (0.3.5) - ruby-vips (2.0.17) - ffi (~> 1.9) + ruby-vips (2.1.4) + ffi (~> 1.12) ruby2_keywords (0.0.5) ruby_parser (3.15.1) sexp_processor (~> 4.9) From b8a22ae8b20c2525d8e190c430504100e4d4b9dc Mon Sep 17 00:00:00 2001 From: Paul Chavard Date: Wed, 2 Mar 2022 09:48:42 +0000 Subject: [PATCH 09/12] fix(multi-select): fix labels on multi select component --- .../{ComboMultiple.jsx => ComboMultiple.tsx} | 163 ++++++++++-------- package.json | 1 + yarn.lock | 5 + 3 files changed, 96 insertions(+), 73 deletions(-) rename app/javascript/components/{ComboMultiple.jsx => ComboMultiple.tsx} (73%) diff --git a/app/javascript/components/ComboMultiple.jsx b/app/javascript/components/ComboMultiple.tsx similarity index 73% rename from app/javascript/components/ComboMultiple.jsx rename to app/javascript/components/ComboMultiple.tsx index 7938518a0..a0741ad09 100644 --- a/app/javascript/components/ComboMultiple.jsx +++ b/app/javascript/components/ComboMultiple.tsx @@ -5,9 +5,12 @@ import React, { useContext, createContext, useEffect, - useLayoutEffect + useLayoutEffect, + MutableRefObject, + ReactNode, + ChangeEventHandler, + KeyboardEventHandler } from 'react'; -import PropTypes from 'prop-types'; import { Combobox, ComboboxInput, @@ -24,22 +27,51 @@ import invariant from 'tiny-invariant'; import { useDeferredSubmit, useHiddenField } from './shared/hooks'; -const Context = createContext(); +const Context = createContext<{ + selectionsRef: MutableRefObject; + onRemove: (value: string) => void; +} | null>(null); -const optionValueByLabel = (values, options, label) => { - const maybeOption = values.includes(label) +type Option = [label: string, value: string]; + +function isOptions(options: string[] | Option[]): options is Option[] { + return Array.isArray(options[0]); +} + +const optionValueByLabel = ( + values: string[], + options: Option[], + label: string +): string => { + const maybeOption: Option | undefined = values.includes(label) ? [label, label] : options.find(([optionLabel]) => optionLabel == label); - return maybeOption ? maybeOption[1] : undefined; + return maybeOption ? maybeOption[1] : ''; }; -const optionLabelByValue = (values, options, value) => { - const maybeOption = values.includes(value) +const optionLabelByValue = ( + values: string[], + options: Option[], + value: string +): string => { + const maybeOption: Option | undefined = values.includes(value) ? [value, value] : options.find(([, optionValue]) => optionValue == value); - return maybeOption ? maybeOption[0] : undefined; + return maybeOption ? maybeOption[0] : ''; }; -function ComboMultiple({ +export type ComboMultipleProps = { + options: string[] | Option[]; + id: string; + labelledby: string; + describedby: string; + label: string; + group: string; + name?: string; + selected: string[]; + acceptNewValues?: boolean; +}; + +export default function ComboMultiple({ options, id, labelledby, @@ -49,21 +81,21 @@ function ComboMultiple({ name = 'value', selected, acceptNewValues = false -}) { +}: ComboMultipleProps) { invariant(id || label, 'ComboMultiple: `id` or a `label` are required'); invariant(group, 'ComboMultiple: `group` is required'); - const inputRef = useRef(); + const inputRef = useRef(null); const [term, setTerm] = useState(''); const [selections, setSelections] = useState(selected); - const [newValues, setNewValues] = useState([]); + const [newValues, setNewValues] = useState([]); const inputId = useId(id); const removedLabelledby = `${inputId}-remove`; const selectedLabelledby = `${inputId}-selected`; - const optionsWithLabels = useMemo( + const optionsWithLabels = useMemo( () => - Array.isArray(options[0]) + isOptions(options) ? options : options.filter((o) => o).map((o) => [o, o]), [options] @@ -94,11 +126,11 @@ function ComboMultiple({ const [, setHiddenFieldValue, hiddenField] = useHiddenField(group, name); const awaitFormSubmit = useDeferredSubmit(hiddenField); - const handleChange = (event) => { + const handleChange: ChangeEventHandler = (event) => { setTerm(event.target.value); }; - const saveSelection = (fn) => { + const saveSelection = (fn: (selections: string[]) => string[]) => { setSelections((selections) => { selections = fn(selections); setHiddenFieldValue(JSON.stringify(selections)); @@ -106,7 +138,7 @@ function ComboMultiple({ }); }; - const onSelect = (value) => { + const onSelect = (value: string) => { const maybeValue = [...extraOptions, ...optionsWithLabels].find( ([val]) => val == value ); @@ -134,8 +166,8 @@ function ComboMultiple({ hidePopover(); }; - const onRemove = (label) => { - const optionValue = optionValueByLabel(newValues, options, label); + const onRemove = (label: string) => { + const optionValue = optionValueByLabel(newValues, optionsWithLabels, label); if (optionValue) { saveSelection((selections) => selections.filter((value) => value != optionValue) @@ -144,10 +176,10 @@ function ComboMultiple({ newValues.filter((value) => value != optionValue) ); } - inputRef.current.focus(); + inputRef.current?.focus(); }; - const onKeyDown = (event) => { + const onKeyDown: KeyboardEventHandler = (event) => { if ( isHotkey('enter', event) || isHotkey(' ', event) || @@ -210,7 +242,11 @@ function ComboMultiple({ ))} @@ -263,31 +299,35 @@ function ComboMultiple({ ); } -function ComboboxTokenLabel({ onRemove, ...props }) { - const selectionsRef = useRef([]); +function ComboboxTokenLabel({ + onRemove, + children +}: { + onRemove: (value: string) => void; + children: ReactNode; +}) { + const selectionsRef = useRef([]); useLayoutEffect(() => { selectionsRef.current = []; - return () => (selectionsRef.current = []); - }); - - const context = { - onRemove, - selectionsRef - }; + return () => { + selectionsRef.current = []; + }; + }, []); return ( - -
+ +
{children}
 ); } -ComboboxTokenLabel.propTypes = { - onRemove: PropTypes.func -}; - -function ComboboxSeparator({ value }) { +function ComboboxSeparator({ value }: { value: string }) { return (
  • {value.slice(2, -2)} @@ -295,12 +335,17 @@ function ComboboxSeparator({ value }) { ); } -ComboboxSeparator.propTypes = { - value: PropTypes.string -}; - -function ComboboxToken({ value, describedby, ...props }) { - const { selectionsRef, onRemove } = useContext(Context); +function ComboboxToken({ + value, + describedby, + ...props +}: { + value: string; + describedby: string; +}) { + const context = useContext(Context); + invariant(context, 'invalid context'); + const { selectionsRef, onRemove } = context; useEffect(() => { selectionsRef.current.push(value); }); @@ -325,31 +370,3 @@ function ComboboxToken({ value, describedby, ...props }) {
    • ); } - -ComboboxToken.propTypes = { - value: PropTypes.string, - describedby: PropTypes.string -}; - -ComboMultiple.propTypes = { - options: PropTypes.oneOfType([ - PropTypes.arrayOf(PropTypes.string), - PropTypes.arrayOf( - PropTypes.arrayOf( - PropTypes.oneOfType([PropTypes.string, PropTypes.number]) - ) - ) - ]), - selected: PropTypes.arrayOf(PropTypes.string), - arraySelected: PropTypes.arrayOf(PropTypes.array), - acceptNewValues: PropTypes.bool, - mandatory: PropTypes.bool, - id: PropTypes.string, - group: PropTypes.string, - name: PropTypes.string, - labelledby: PropTypes.string, - describedby: PropTypes.string, - label: PropTypes.string -}; - -export default ComboMultiple; diff --git a/package.json b/package.json index 293825ee3..85524e4e0 100644 --- a/package.json +++ b/package.json @@ -47,6 +47,7 @@ "@2fd/graphdoc": "^2.4.0", "@types/debounce": "^1.2.1", "@types/geojson": "^7946.0.8", + "@types/is-hotkey": "^0.1.7", "@types/mapbox__mapbox-gl-draw": "^1.2.3", "@types/rails__ujs": "^6.0.1", "@types/react": "^17.0.38", diff --git a/yarn.lock b/yarn.lock index cf3335137..92b14afda 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2383,6 +2383,11 @@ dependencies: "@types/node" "*" +"@types/is-hotkey@^0.1.7": + version "0.1.7" + resolved "https://registry.yarnpkg.com/@types/is-hotkey/-/is-hotkey-0.1.7.tgz#30ec6d4234895230b576728ef77e70a52962f3b3" + integrity sha512-yB5C7zcOM7idwYZZ1wKQ3pTfjA9BbvFqRWvKB46GFddxnJtHwi/b9y84ykQtxQPg5qhdpg4Q/kWU3EGoCTmLzQ== + "@types/istanbul-lib-coverage@*", "@types/istanbul-lib-coverage@^2.0.0": version "2.0.3" resolved "https://registry.yarnpkg.com/@types/istanbul-lib-coverage/-/istanbul-lib-coverage-2.0.3.tgz#4ba8ddb720221f432e443bd5f9117fd22cfd4762" From 5150e33212e66bab290d87c31af40b9521c3b591 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 1 Mar 2022 16:42:46 +0100 Subject: [PATCH 10/12] models: ensure DROM phone numbers are valid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They were accepted before, because they were 'possible' – but now they are explicitely considered as valid. --- app/models/champs/phone_champ.rb | 23 ++++++++++++++++++++++- lib/core_ext/phonelib.rb | 14 ++++++++++++++ spec/models/champs/phone_champ_spec.rb | 4 ++++ 3 files changed, 40 insertions(+), 1 deletion(-) create mode 100644 lib/core_ext/phonelib.rb diff --git a/app/models/champs/phone_champ.rb b/app/models/champs/phone_champ.rb index baa032d5b..3e4fe39ee 100644 --- a/app/models/champs/phone_champ.rb +++ b/app/models/champs/phone_champ.rb @@ -20,12 +20,33 @@ # type_de_champ_id :integer # class Champs::PhoneChamp < Champs::TextChamp + # We want to allow: + # * international (e164) phone numbers + # * “french format” (ten digits with a leading 0) + # * DROM numbers + # + # However, we need to special-case some ten-digit numbers, + # because the ARCEP assigns some blocks of "O6 XX XX XX XX" numbers to DROM operators. + # Guadeloupe | GP | +590 | 0690XXXXXX, 0691XXXXXX + # Guyane | GF | +594 | 0694XXXXXX + # Martinique | MQ | +596 | 0696XXXXXX, 0697XXXXXX + # Réunion | RE | +262 | 0692XXXXXX, 0693XXXXXX + # Mayotte | YT | +262 | 0692XXXXXX, 0693XXXXXX + # Nouvelle-Calédonie | NC | +687 | + # Polynésie française | PF | +689 | 40XXXXXX, 45XXXXXX, 87XXXXXX, 88XXXXXX, 89XXXXXX + # + # Cf: Plan national de numérotation téléphonique, + # https://www.arcep.fr/uploads/tx_gsavis/05-1085.pdf “Numéros mobiles à 10 chiffres”, page 6 + # + # See issue #6996. + DEFAULT_COUNTRY_CODES = [:FR, :GP, :GF, :MQ, :RE, :YT, :NC, :PF].freeze + validates :value, phone: { possible: true, allow_blank: true, message: I18n.t(:not_a_phone, scope: 'activerecord.errors.messages') - }, unless: -> { Phonelib.valid_for_country?(value, :pf) } + }, unless: -> { Phonelib.valid_for_countries?(value, DEFAULT_COUNTRY_CODES) } def to_s value.present? ? Phonelib.parse(value).full_national : '' diff --git a/lib/core_ext/phonelib.rb b/lib/core_ext/phonelib.rb new file mode 100644 index 000000000..1345a0c75 --- /dev/null +++ b/lib/core_ext/phonelib.rb @@ -0,0 +1,14 @@ +# Class extensions to the Phonelib module, which allow parsing using several countries at once. +module Phonelib + # Variation of `.valid_for_country`, that can check several countries at once. + def self.valid_for_countries?(value, countries) + countries.any? { |country| valid_for_country?(value, country) } + end + + # Variation of `Phonelib.parse`, which parses the given string using the first country + # for which the phone number is valid. + def self.parse_for_countries(value, passed_countries = []) + valid_country = passed_countries.find { |country| valid_for_country?(value, country) } + parse(value, valid_country) + end +end diff --git a/spec/models/champs/phone_champ_spec.rb b/spec/models/champs/phone_champ_spec.rb index 4d1f12f6e..9b2bd7bdd 100644 --- a/spec/models/champs/phone_champ_spec.rb +++ b/spec/models/champs/phone_champ_spec.rb @@ -22,6 +22,10 @@ describe Champs::PhoneChamp do expect(champ_with_value("+1(0) - 123456789")).to be_valid expect(champ_with_value("+49 2109 87654321")).to be_valid expect(champ_with_value("012345678")).to be_valid + # DROM numbers should be valid + expect(champ_with_value("06 96 04 78 07")).to be_valid + expect(champ_with_value("05 94 22 31 31")).to be_valid + expect(champ_with_value("+594 5 94 22 31 31")).to be_valid # polynesian numbers should not return errors in any way ## landline numbers start with 40 or 45 expect(champ_with_value("45187272")).to be_valid From f35d18cd5c7c6ea09bd9e2a4b5d5df1bbf58d73d Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 1 Mar 2022 16:44:16 +0100 Subject: [PATCH 11/12] models: stop truncating DROM phone numbers Fix #6996 --- app/models/champs/phone_champ.rb | 3 ++- spec/models/champs/phone_champ_spec.rb | 16 ++++++++++++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/app/models/champs/phone_champ.rb b/app/models/champs/phone_champ.rb index 3e4fe39ee..ae06abe14 100644 --- a/app/models/champs/phone_champ.rb +++ b/app/models/champs/phone_champ.rb @@ -49,6 +49,7 @@ class Champs::PhoneChamp < Champs::TextChamp }, unless: -> { Phonelib.valid_for_countries?(value, DEFAULT_COUNTRY_CODES) } def to_s - value.present? ? Phonelib.parse(value).full_national : '' + return '' if value.blank? + Phonelib.parse_for_countries(value, DEFAULT_COUNTRY_CODES).full_national end end diff --git a/spec/models/champs/phone_champ_spec.rb b/spec/models/champs/phone_champ_spec.rb index 9b2bd7bdd..df7e9095a 100644 --- a/spec/models/champs/phone_champ_spec.rb +++ b/spec/models/champs/phone_champ_spec.rb @@ -40,9 +40,21 @@ describe Champs::PhoneChamp do expect(champ_with_value("88473500")).to be_valid expect(champ_with_value("89473500")).to be_valid end + end - def champ_with_value(number) - phone_champ.tap { |c| c.value = number } + describe '#to_s' do + context 'for valid phone numbers' do + it 'returns the national part of the number, formatted nicely' do + expect(champ_with_value("0115789055").to_s).to eq("01 15 78 90 55") + expect(champ_with_value("+33115789055").to_s).to eq("01 15 78 90 55") + # DROM phone numbers are formatted differently – but still formatted + expect(champ_with_value("0696047807").to_s).to eq("0696 04 78 07") + expect(champ_with_value("45187272").to_s).to eq("45187272") + end end end + + def champ_with_value(number) + phone_champ.tap { |c| c.value = number } + end end From e32c9a9f94e88321003d4b615804da49c957bed2 Mon Sep 17 00:00:00 2001 From: Pierre de La Morinerie Date: Tue, 1 Mar 2022 16:45:44 +0100 Subject: [PATCH 12/12] models: don't attempt to format invalid phone numbers This is a defensive-programming measure, because formatting an invalid phone number may truncate some leading numbers. --- app/models/champs/phone_champ.rb | 9 ++++++++- spec/models/champs/phone_champ_spec.rb | 6 ++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/models/champs/phone_champ.rb b/app/models/champs/phone_champ.rb index ae06abe14..ac2c51449 100644 --- a/app/models/champs/phone_champ.rb +++ b/app/models/champs/phone_champ.rb @@ -50,6 +50,13 @@ class Champs::PhoneChamp < Champs::TextChamp def to_s return '' if value.blank? - Phonelib.parse_for_countries(value, DEFAULT_COUNTRY_CODES).full_national + + if Phonelib.valid_for_countries?(value, DEFAULT_COUNTRY_CODES) + Phonelib.parse_for_countries(value, DEFAULT_COUNTRY_CODES).full_national + else + # When he phone number is possible for the default countries, but not strictly valid, + # `full_national` could mess up the formatting. In this case just return the original. + value + end end end diff --git a/spec/models/champs/phone_champ_spec.rb b/spec/models/champs/phone_champ_spec.rb index df7e9095a..275986c33 100644 --- a/spec/models/champs/phone_champ_spec.rb +++ b/spec/models/champs/phone_champ_spec.rb @@ -52,6 +52,12 @@ describe Champs::PhoneChamp do expect(champ_with_value("45187272").to_s).to eq("45187272") end end + + context 'for possible (but not valid) phone numbers' do + it 'returns the original' do + expect(champ_with_value("1234").to_s).to eq("1234") + end + end end def champ_with_value(number)