From e9c6ed80e4beb45823d2c50562c463c1b460008e Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Tue, 17 Sep 2019 15:55:35 +0200 Subject: [PATCH 01/11] Migration: Index and make Procedure.path nonnull --- app/models/procedure.rb | 2 +- db/migrate/20190917151652_make_path_nonnull.rb | 6 ++++++ db/schema.rb | 5 +++-- spec/factories/procedure.rb | 1 + 4 files changed, 11 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20190917151652_make_path_nonnull.rb diff --git a/app/models/procedure.rb b/app/models/procedure.rb index a93d73a5f..b9760fa41 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -66,7 +66,7 @@ class Procedure < ApplicationRecord validates :description, presence: true, allow_blank: false, allow_nil: false validates :administrateurs, presence: true validate :check_juridique - validates :path, format: { with: /\A[a-z0-9_\-]{3,50}\z/ }, uniqueness: { scope: :aasm_state, case_sensitive: false }, presence: true, allow_blank: false, allow_nil: true + validates :path, presence: true, format: { with: /\A[a-z0-9_\-]{3,50}\z/ }, uniqueness: { scope: [:path, :archived_at, :hidden_at], case_sensitive: false } # FIXME: remove duree_conservation_required flag once all procedures are converted to the new style validates :duree_conservation_dossiers_dans_ds, allow_nil: false, numericality: { only_integer: true, greater_than_or_equal_to: 1, less_than_or_equal_to: MAX_DUREE_CONSERVATION }, if: :durees_conservation_required validates :duree_conservation_dossiers_hors_ds, allow_nil: false, numericality: { only_integer: true, greater_than_or_equal_to: 0 }, if: :durees_conservation_required diff --git a/db/migrate/20190917151652_make_path_nonnull.rb b/db/migrate/20190917151652_make_path_nonnull.rb new file mode 100644 index 000000000..401a13c84 --- /dev/null +++ b/db/migrate/20190917151652_make_path_nonnull.rb @@ -0,0 +1,6 @@ +class MakePathNonnull < ActiveRecord::Migration[5.2] + def change + change_column_null :procedures, :path, false + add_index :procedures, [:path, :archived_at, :hidden_at], unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index 330d17d99..9914c4180 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_09_17_120856) do +ActiveRecord::Schema.define(version: 2019_09_17_151652) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -480,12 +480,13 @@ ActiveRecord::Schema.define(version: 2019_09_17_120856) do t.string "cadre_juridique" t.boolean "juridique_required", default: true t.boolean "durees_conservation_required", default: true - t.string "path" + t.string "path", null: false t.string "declarative_with_state" t.text "monavis_embed" t.index ["declarative_with_state"], name: "index_procedures_on_declarative_with_state" t.index ["hidden_at"], name: "index_procedures_on_hidden_at" t.index ["parent_procedure_id"], name: "index_procedures_on_parent_procedure_id" + t.index ["path", "archived_at", "hidden_at"], name: "index_procedures_on_path_and_archived_at_and_hidden_at", unique: true t.index ["service_id"], name: "index_procedures_on_service_id" end diff --git a/spec/factories/procedure.rb b/spec/factories/procedure.rb index 118b1c7d5..23b32efa7 100644 --- a/spec/factories/procedure.rb +++ b/spec/factories/procedure.rb @@ -11,6 +11,7 @@ FactoryBot.define do duree_conservation_dossiers_hors_ds { 6 } ask_birthday { false } lien_site_web { "https://mon-site.gouv" } + path { SecureRandom.uuid } transient do administrateur {} From 8fa630d2bb03a591ae3c3cb2cc58c41328eae08e Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Tue, 17 Sep 2019 15:07:58 +0200 Subject: [PATCH 02/11] Remove Gestionnaire.visible_procedures It is actually the same thing as Gestionnaire.procedures. It already included the procedures with paths as well as the archived procedures, and in production, there were no Gestionnaire for who procedures was returning a different result than visible_procedures (expect for two baddata brouillon procedures with a nil path). In addition, Procedure.path is now nonnull, which means the Procedure.avec_lien scope is pointless. Finally, the current spec showed that the only procedure not visible to the gestion was the one he was not assigned to. --- .../instructeurs/procedures_controller.rb | 4 +-- app/models/instructeur.rb | 4 --- app/models/procedure.rb | 1 - app/views/layouts/_new_header.haml | 2 +- spec/models/instructeur_spec.rb | 25 ------------------- 5 files changed, 3 insertions(+), 33 deletions(-) diff --git a/app/controllers/instructeurs/procedures_controller.rb b/app/controllers/instructeurs/procedures_controller.rb index 83cc2d03d..fe323dee4 100644 --- a/app/controllers/instructeurs/procedures_controller.rb +++ b/app/controllers/instructeurs/procedures_controller.rb @@ -7,7 +7,7 @@ module Instructeurs def index @procedures = current_instructeur - .visible_procedures + .procedures .with_attached_logo .includes(:defaut_groupe_instructeur) .order(archived_at: :desc, published_at: :desc, created_at: :desc) @@ -235,7 +235,7 @@ module Instructeurs end def redirect_to_avis_if_needed - if current_instructeur.visible_procedures.count == 0 && current_instructeur.avis.count > 0 + if current_instructeur.procedures.count == 0 && current_instructeur.avis.count > 0 redirect_to instructeur_avis_index_path end end diff --git a/app/models/instructeur.rb b/app/models/instructeur.rb index 0657032be..3cdfa45c9 100644 --- a/app/models/instructeur.rb +++ b/app/models/instructeur.rb @@ -24,10 +24,6 @@ class Instructeur < ApplicationRecord has_one :user - def visible_procedures - procedures.merge(Procedure.avec_lien.or(Procedure.archivees)) - end - def follow(dossier) begin followed_dossiers << dossier diff --git a/app/models/procedure.rb b/app/models/procedure.rb index b9760fa41..b425e0b60 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -45,7 +45,6 @@ class Procedure < ApplicationRecord scope :by_libelle, -> { order(libelle: :asc) } scope :created_during, -> (range) { where(created_at: range) } scope :cloned_from_library, -> { where(cloned_from_library: true) } - scope :avec_lien, -> { where.not(path: nil) } scope :declarative, -> { where.not(declarative_with_state: nil) } scope :for_api, -> { diff --git a/app/views/layouts/_new_header.haml b/app/views/layouts/_new_header.haml index eb3c7eb97..440a01b4a 100644 --- a/app/views/layouts/_new_header.haml +++ b/app/views/layouts/_new_header.haml @@ -15,7 +15,7 @@ - if nav_bar_profile == :instructeur && instructeur_signed_in? - current_url = request.path_info %ul.header-tabs - - if current_instructeur.visible_procedures.count > 0 + - if current_instructeur.procedures.count > 0 %li = active_link_to "Démarches", instructeur_procedures_path, active: :inclusive, class: 'tab-link' - if current_instructeur.avis.count > 0 diff --git a/spec/models/instructeur_spec.rb b/spec/models/instructeur_spec.rb index 281123918..5b0ba0ed4 100644 --- a/spec/models/instructeur_spec.rb +++ b/spec/models/instructeur_spec.rb @@ -12,31 +12,6 @@ describe Instructeur, type: :model do assign(procedure_2) end - describe '#visible_procedures' do - let(:procedure_not_assigned) { create :procedure, administrateur: admin } - let(:procedure_with_default_path) { create :procedure, administrateur: admin } - let(:procedure_with_custom_path) { create :procedure, :with_path, administrateur: admin } - let(:procedure_archived_manually) { create :procedure, :archived, administrateur: admin } - let(:procedure_archived_automatically) { create :procedure, :archived_automatically, administrateur: admin } - - before do - assign(procedure_with_default_path) - assign(procedure_with_custom_path) - assign(procedure_archived_manually) - assign(procedure_archived_automatically) - end - - subject { instructeur.visible_procedures } - - it do - expect(subject).not_to include(procedure_not_assigned) - expect(subject).to include(procedure_with_default_path) - expect(subject).to include(procedure_with_custom_path) - expect(subject).to include(procedure_archived_manually) - expect(subject).to include(procedure_archived_automatically) - end - end - describe 'follow' do let(:dossier) { create :dossier } let(:already_followed_dossier) { create :dossier } From b9968b76b0a4767973376a4cd0b2c9005518bf39 Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Mon, 16 Sep 2019 16:59:10 +0200 Subject: [PATCH 03/11] =?UTF-8?q?Remove=20=E2=80=9Cavec=5Flien=E2=80=9D=20?= =?UTF-8?q?helpers=20now=20that=20Procedure.path=20cannot=20be=20nil?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/controllers/admin_controller.rb | 2 +- .../new_administrateur/administrateur_controller.rb | 2 +- app/helpers/procedure_helper.rb | 10 ++++------ app/models/procedure.rb | 5 ----- app/serializers/procedure_serializer.rb | 10 ++++------ app/views/admin/procedures/show.html.haml | 4 ++-- ...t_panel_admin_procedurescontroller_navbar.html.haml | 2 +- 7 files changed, 13 insertions(+), 22 deletions(-) diff --git a/app/controllers/admin_controller.rb b/app/controllers/admin_controller.rb index 926bb0ff4..608c1fcef 100644 --- a/app/controllers/admin_controller.rb +++ b/app/controllers/admin_controller.rb @@ -25,7 +25,7 @@ class AdminController < ApplicationController end def reset_procedure - if @procedure.brouillon_avec_lien? + if @procedure.brouillon? @procedure.reset! end end diff --git a/app/controllers/new_administrateur/administrateur_controller.rb b/app/controllers/new_administrateur/administrateur_controller.rb index 2e1ad0eb4..d8828ba1c 100644 --- a/app/controllers/new_administrateur/administrateur_controller.rb +++ b/app/controllers/new_administrateur/administrateur_controller.rb @@ -20,7 +20,7 @@ module NewAdministrateur end def reset_procedure - if @procedure.brouillon_avec_lien? + if @procedure.brouillon? @procedure.reset! end end diff --git a/app/helpers/procedure_helper.rb b/app/helpers/procedure_helper.rb index 52a834f16..38cab945b 100644 --- a/app/helpers/procedure_helper.rb +++ b/app/helpers/procedure_helper.rb @@ -1,11 +1,9 @@ module ProcedureHelper def procedure_lien(procedure) - if procedure.path.present? - if procedure.brouillon_avec_lien? - commencer_test_url(path: procedure.path) - else - commencer_url(path: procedure.path) - end + if procedure.brouillon? + commencer_test_url(path: procedure.path) + else + commencer_url(path: procedure.path) end end diff --git a/app/models/procedure.rb b/app/models/procedure.rb index b425e0b60..42bb93ba6 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -133,11 +133,6 @@ class Procedure < ApplicationRecord !archivee? end - # This method is needed for transition. Eventually this will be the same as brouillon?. - def brouillon_avec_lien? - brouillon? && path.present? - end - def publiee_ou_archivee? publiee? || archivee? end diff --git a/app/serializers/procedure_serializer.rb b/app/serializers/procedure_serializer.rb index c2608a9ff..778e236e6 100644 --- a/app/serializers/procedure_serializer.rb +++ b/app/serializers/procedure_serializer.rb @@ -24,12 +24,10 @@ class ProcedureSerializer < ActiveModel::Serializer end def link - if object.path.present? - if object.brouillon_avec_lien? - commencer_test_url(path: object.path) - else - commencer_url(path: object.path) - end + if object.brouillon? + commencer_test_url(path: object.path) + else + commencer_url(path: object.path) end end diff --git a/app/views/admin/procedures/show.html.haml b/app/views/admin/procedures/show.html.haml index f1e353499..9301644a3 100644 --- a/app/views/admin/procedures/show.html.haml +++ b/app/views/admin/procedures/show.html.haml @@ -44,7 +44,7 @@ .lien-demarche %h3 - - if @procedure.brouillon_avec_lien? + - if @procedure.brouillon? Test et publication - else Publication @@ -58,7 +58,7 @@ %br %br Attention, diffusez toujours le lien complet affiché ci-dessus, et non pas un lien générique vers demarches-simplifiees.fr. Ne dites pas non plus aux usagers de se rendre sur le site générique demarches-simplifiees.fr, donnez-leur toujours le lien complet. - - elsif @procedure.brouillon_avec_lien? + - elsif @procedure.brouillon? - if @procedure.missing_steps.empty? %p Cette démarche est actuellement en test, diff --git a/app/views/layouts/left_panels/_left_panel_admin_procedurescontroller_navbar.html.haml b/app/views/layouts/left_panels/_left_panel_admin_procedurescontroller_navbar.html.haml index f4b646548..18d34a23d 100644 --- a/app/views/layouts/left_panels/_left_panel_admin_procedurescontroller_navbar.html.haml +++ b/app/views/layouts/left_panels/_left_panel_admin_procedurescontroller_navbar.html.haml @@ -11,7 +11,7 @@ #procedure-list %a#onglet-infos{ href: url_for(admin_procedure_path(@procedure)) } .procedure-list-element{ class: ('active' if active == 'Informations') } - - if @procedure.brouillon_avec_lien? + - if @procedure.brouillon? Test et publication - else Publication From 92e60321157283b2da293c847505b967776f6843 Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Fri, 5 Jul 2019 10:37:29 +0200 Subject: [PATCH 04/11] Remove path availability dead code when creating/editing a procedure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s no “path” field anymore in the Procedure form, it can only be set when publishing. --- .../admin/procedures_controller.rb | 21 ------- .../admin/procedures/_unavailable.html.haml | 11 ---- .../procedures/check_availability.js.erb | 11 ---- config/routes.rb | 1 - .../admin/procedures_controller_spec.rb | 57 ------------------- 5 files changed, 101 deletions(-) delete mode 100644 app/views/admin/procedures/_unavailable.html.haml delete mode 100644 app/views/admin/procedures/check_availability.js.erb diff --git a/app/controllers/admin/procedures_controller.rb b/app/controllers/admin/procedures_controller.rb index c2c061e75..1fc919b07 100644 --- a/app/controllers/admin/procedures_controller.rb +++ b/app/controllers/admin/procedures_controller.rb @@ -43,8 +43,6 @@ class Admin::ProceduresController < AdminController end def edit - @path = @procedure.path || @procedure.default_path - @availability = @procedure.path_availability(current_administrateur, @path) end def destroy @@ -63,13 +61,10 @@ class Admin::ProceduresController < AdminController def new @procedure ||= Procedure.new(for_individual: true) - @availability = Procedure::PATH_AVAILABLE end def create @procedure = Procedure.new(procedure_params.merge(administrateurs: [current_administrateur])) - @path = @procedure.path - @availability = Procedure.path_availability(current_administrateur, @procedure.path) if !@procedure.save flash.now.alert = @procedure.errors.full_messages @@ -87,10 +82,6 @@ class Admin::ProceduresController < AdminController if !@procedure.update(procedure_params) flash.now.alert = @procedure.errors.full_messages - @path = procedure_params[:path] - if @path.present? - @availability = @procedure.path_availability(current_administrateur, @path) - end render 'edit' elsif @procedure.brouillon? reset_procedure @@ -234,18 +225,6 @@ class Admin::ProceduresController < AdminController render json: json_path_list end - def check_availability - path = params[:procedure][:path] - procedure_id = params[:procedure][:id] - - if procedure_id.present? - procedure = current_administrateur.procedures.find(procedure_id) - @availability = procedure.path_availability(current_administrateur, path) - else - @availability = Procedure.path_availability(current_administrateur, path) - end - end - def delete_logo @procedure.logo.purge_later @procedure.logo_active_storage.purge_later diff --git a/app/views/admin/procedures/_unavailable.html.haml b/app/views/admin/procedures/_unavailable.html.haml deleted file mode 100644 index d3c2ce71d..000000000 --- a/app/views/admin/procedures/_unavailable.html.haml +++ /dev/null @@ -1,11 +0,0 @@ -- case availability -- when Procedure::PATH_AVAILABLE_PUBLIEE - Ce lien est déjà utilisé par une de vos démarche. - %br - Si vous voulez l’utiliser, l’ancienne démarche sera archivée lors de la publication de la démarche (plus accessible du public). -- when Procedure::PATH_NOT_AVAILABLE_BROUILLON - Une démarche en test existe déjà avec ce lien. -- when Procedure::PATH_NOT_AVAILABLE - Ce lien est déjà utilisé par une démarche. - %br - Vous ne pouvez pas l’utiliser car il appartient à un autre administrateur. diff --git a/app/views/admin/procedures/check_availability.js.erb b/app/views/admin/procedures/check_availability.js.erb deleted file mode 100644 index f5eed5960..000000000 --- a/app/views/admin/procedures/check_availability.js.erb +++ /dev/null @@ -1,11 +0,0 @@ -<% if @availability == Procedure::PATH_AVAILABLE %> - <%= remove_element('.unavailable-path-message', inner: true) %> -<% else %> - <%= render_to_element('.unavailable-path-message', partial: 'unavailable', locals: { availability: @availability }) %> -<% end %> - -<% if @availability.in?(Procedure::PATH_CAN_PUBLISH) %> - <%= enable_element('button[type=submit]') %> -<% else %> - <%= disable_element('button[type=submit]') %> -<% end %> diff --git a/config/routes.rb b/config/routes.rb index 0bb11a794..9214c6d5d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -160,7 +160,6 @@ Rails.application.routes.draw do get 'procedures/archived' => 'procedures#archived' get 'procedures/draft' => 'procedures#draft' get 'procedures/path_list' => 'procedures#path_list' - get 'procedures/available' => 'procedures#check_availability' resources :procedures do collection do diff --git a/spec/controllers/admin/procedures_controller_spec.rb b/spec/controllers/admin/procedures_controller_spec.rb index 7df3010d7..ede09f394 100644 --- a/spec/controllers/admin/procedures_controller_spec.rb +++ b/spec/controllers/admin/procedures_controller_spec.rb @@ -716,63 +716,6 @@ describe Admin::ProceduresController, type: :controller do end end - describe "GET #check_availability" do - render_views - let(:procedure) { create(:procedure, :with_path, administrateur: admin) } - let(:params) { - { - procedure: { - path: path, - id: procedure.id - } - } - } - let(:path) { generate(:published_path) } - - before do - get :check_availability, params: params, format: 'js' - end - - context 'self path' do - let(:path) { procedure.path } - - it { expect(response.body).to include("innerHTML = ''") } - end - - context 'available path' do - it { expect(response.body).to include("innerHTML = ''") } - end - - context 'my path (brouillon)' do - let(:procedure_owned) { create(:procedure, :with_path, administrateur: admin) } - let(:path) { procedure_owned.path } - - it { - expect(response.body).to include('Une démarche en test existe déjà avec ce lien.') - } - end - - context 'my path' do - let(:procedure_owned) { create(:procedure, :published, administrateur: admin) } - let(:path) { procedure_owned.path } - - it { - expect(response.body).to include('Ce lien est déjà utilisé par une de vos démarche.') - expect(response.body).to include('Si vous voulez l’utiliser, l’ancienne démarche sera archivée') - } - end - - context 'unavailable path' do - let(:procedure_not_owned) { create(:procedure, :with_path, administrateur: create(:administrateur)) } - let(:path) { procedure_not_owned.path } - - it { - expect(response.body).to include('Ce lien est déjà utilisé par une démarche.') - expect(response.body).to include('Vous ne pouvez pas l’utiliser car il appartient à un autre administrateur.') - } - end - end - describe 'PATCH #monavis' do let!(:procedure) { create(:procedure, administrateur: admin) } let(:procedure_params) { From 99f986b815d67d22a898ad4cfa05c22b7d152801 Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Fri, 5 Jul 2019 10:37:29 +0200 Subject: [PATCH 05/11] =?UTF-8?q?Cleanup=20Procedure=20retrieval=20in=20?= =?UTF-8?q?=E2=80=9CCommencer=E2=80=9D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make sure to only fetch active (brouillons or published) procedures. --- app/controllers/users/commencer_controller.rb | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/app/controllers/users/commencer_controller.rb b/app/controllers/users/commencer_controller.rb index 9a16ee2aa..841acd58a 100644 --- a/app/controllers/users/commencer_controller.rb +++ b/app/controllers/users/commencer_controller.rb @@ -3,21 +3,21 @@ module Users layout 'procedure_context' def commencer - @procedure = Procedure.publiees.find_by(path: params[:path]) - return procedure_not_found if @procedure.blank? + @procedure = retrieve_procedure + return procedure_not_found if @procedure.blank? || @procedure.brouillon? render 'commencer/show' end def commencer_test - @procedure = Procedure.brouillons.find_by(path: params[:path]) - return procedure_not_found if @procedure.blank? + @procedure = retrieve_procedure + return procedure_not_found if @procedure.blank? || @procedure.publiee? render 'commencer/show' end def sign_in - @procedure = Procedure.find_by(path: params[:path]) + @procedure = retrieve_procedure return procedure_not_found if @procedure.blank? store_user_location!(@procedure) @@ -25,7 +25,7 @@ module Users end def sign_up - @procedure = Procedure.find_by(path: params[:path]) + @procedure = retrieve_procedure return procedure_not_found if @procedure.blank? store_user_location!(@procedure) @@ -33,7 +33,7 @@ module Users end def france_connect - @procedure = Procedure.find_by(path: params[:path]) + @procedure = retrieve_procedure return procedure_not_found if @procedure.blank? store_user_location!(@procedure) @@ -41,23 +41,25 @@ module Users end def procedure_for_help - Procedure.publiees.find_by(path: params[:path]) || Procedure.brouillons.find_by(path: params[:path]) + retrieve_procedure end private + def retrieve_procedure + Procedure.publiees.or(Procedure.brouillons).find_by(path: params[:path]) + end + def procedure_not_found procedure = Procedure.find_by(path: params[:path]) if procedure&.archivee? flash.alert = t('errors.messages.procedure_archived') - elsif procedure&.publiee? - flash.alert = t('errors.messages.procedure_not_draft') else flash.alert = t('errors.messages.procedure_not_found') end - return redirect_to root_path + redirect_to root_path end def store_user_location!(procedure) From 00c37eccb300858017fc4d25f643fe530e121701 Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Mon, 16 Sep 2019 17:00:37 +0200 Subject: [PATCH 06/11] Simplify procedure.path and publish event MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Get rid of the “reopen” event, merge it with “publish” (it’s the same code) * Remove the “availability” states; “available with brouillon” makes no sense since the brouillons path are always uuids * Instead of checking if publish can happen, just try it and handle the errors --- .../admin/procedures_controller.rb | 32 ++++---- app/models/procedure.rb | 81 ++++++------------- app/views/admin/procedures/publish.js.erb | 2 +- .../admin/procedures_controller_spec.rb | 6 +- 4 files changed, 44 insertions(+), 77 deletions(-) diff --git a/app/controllers/admin/procedures_controller.rb b/app/controllers/admin/procedures_controller.rb index 1fc919b07..2056ff8ea 100644 --- a/app/controllers/admin/procedures_controller.rb +++ b/app/controllers/admin/procedures_controller.rb @@ -98,22 +98,24 @@ class Admin::ProceduresController < AdminController lien_site_web = params[:lien_site_web] procedure = current_administrateur.procedures.find(params[:procedure_id]) - procedure.path = path - procedure.lien_site_web = lien_site_web - - if !procedure.validate - flash.alert = 'Lien de la démarche invalide ou lien vers la démarche manquant' - return redirect_to admin_procedures_path - else - procedure.path = nil - end - - if procedure.publish_or_reopen!(current_administrateur, path, lien_site_web) - flash.notice = "Démarche publiée" - redirect_to admin_procedures_path - else - @mine = false + procedure.publish_or_reopen!(current_administrateur, path, lien_site_web) + flash.notice = "Démarche publiée" + redirect_to admin_procedures_path + rescue ActiveRecord::RecordInvalid => e + errors = e.record.errors + if errors.details.key?(:path) + path_error = errors.details.dig(:path, 0, :error) + case path_error + when :invalid + @valid = false + when :taken + @valid = true + @mine = false + end render '/admin/procedures/publish', formats: 'js' + else + flash.alert = errors.full_messages + return redirect_to admin_procedure_path(procedure) end rescue ActiveRecord::RecordNotFound flash.alert = 'Démarche inexistante' diff --git a/app/models/procedure.rb b/app/models/procedure.rb index 42bb93ba6..75f353220 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -74,7 +74,8 @@ class Procedure < ApplicationRecord validates_with MonAvisEmbedValidator before_save :update_juridique_required before_save :update_durees_conservation_required - before_create :ensure_path_exists + after_initialize :ensure_path_exists + before_save :ensure_path_exists after_create :ensure_default_groupe_instructeur include AASM @@ -85,11 +86,8 @@ class Procedure < ApplicationRecord state :archivee state :hidden - event :publish, after: :after_publish, guard: :can_publish? do + event :publish, before: :before_publish, after: :after_publish do transitions from: :brouillon, to: :publiee - end - - event :reopen, after: :after_reopen, guard: :can_publish? do transitions from: :archivee, to: :publiee end @@ -109,10 +107,10 @@ class Procedure < ApplicationRecord end def publish_or_reopen!(administrateur, path, lien_site_web) - if archivee? && may_reopen?(administrateur, path, lien_site_web) - reopen!(administrateur, path, lien_site_web) - elsif may_publish?(administrateur, path, lien_site_web) - reset! + Procedure.transaction do + if brouillon? + reset! + end publish!(administrateur, path, lien_site_web) end end @@ -364,34 +362,20 @@ class Procedure < ApplicationRecord percentile_time(:en_construction_at, :processed_at, 90) end - PATH_AVAILABLE = :available - PATH_AVAILABLE_PUBLIEE = :available_publiee - PATH_NOT_AVAILABLE = :not_available - PATH_NOT_AVAILABLE_BROUILLON = :not_available_brouillon - PATH_CAN_PUBLISH = [PATH_AVAILABLE, PATH_AVAILABLE_PUBLIEE] + def path_available?(administrateur, path) + procedure = Procedure.publiees + .where.not(id: self.id) + .find_by(path: path) - def path_availability(administrateur, path) - Procedure.path_availability(administrateur, path, id) + procedure.blank? || administrateur.owns?(procedure) end - def self.path_availability(administrateur, path, exclude_id = nil) - if exclude_id.present? - procedure = where.not(id: exclude_id).find_by(path: path) - else - procedure = find_by(path: path) - end + def auto_archive_procedure_with_same_path(administrateur, path) + procedure = administrateur.procedures.publiees + .where.not(id: self.id) + .find_by(path: path) - if procedure.blank? - PATH_AVAILABLE - elsif administrateur.owns?(procedure) - if procedure.brouillon? - PATH_NOT_AVAILABLE_BROUILLON - else - PATH_AVAILABLE_PUBLIEE - end - else - PATH_NOT_AVAILABLE - end + procedure&.archive! end def self.find_with_path(path) @@ -491,41 +475,22 @@ class Procedure < ApplicationRecord end end - def claim_path_ownership!(path) - procedure = Procedure.joins(:administrateurs) - .where(administrateurs: { id: administrateur_ids }) - .find_by(path: path) - - if procedure&.publiee? && procedure != self - procedure.archive! - end - - update!(path: path) - end - - def can_publish?(administrateur, path, lien_site_web) - path_availability(administrateur, path).in?(PATH_CAN_PUBLISH) && lien_site_web.present? + def before_publish(administrateur, path, lien_site_web) + auto_archive_procedure_with_same_path(administrateur, path) + update!(path: path, lien_site_web: lien_site_web, archived_at: nil) end def after_publish(administrateur, path, lien_site_web) update!(published_at: Time.zone.now) - - claim_path_ownership!(path) - end - - def after_reopen(administrateur, path, lien_site_web) - update!(published_at: Time.zone.now, archived_at: nil) - - claim_path_ownership!(path) end def after_archive - update!(archived_at: Time.zone.now, path: SecureRandom.uuid) + update!(archived_at: Time.zone.now) end def after_hide now = Time.zone.now - update!(hidden_at: now, path: SecureRandom.uuid) + update!(hidden_at: now) dossiers.update_all(hidden_at: now) end @@ -562,7 +527,7 @@ class Procedure < ApplicationRecord end def ensure_path_exists - if self.path.nil? + if self.path.blank? self.path = SecureRandom.uuid end end diff --git a/app/views/admin/procedures/publish.js.erb b/app/views/admin/procedures/publish.js.erb index 9c11731ef..d5df8f14b 100644 --- a/app/views/admin/procedures/publish.js.erb +++ b/app/views/admin/procedures/publish.js.erb @@ -1 +1 @@ -<%= "togglePathMessage(true, #{@mine})" %> +<%= "togglePathMessage(#{@valid}, #{@mine})" %> diff --git a/spec/controllers/admin/procedures_controller_spec.rb b/spec/controllers/admin/procedures_controller_spec.rb index ede09f394..1f05f798a 100644 --- a/spec/controllers/admin/procedures_controller_spec.rb +++ b/spec/controllers/admin/procedures_controller_spec.rb @@ -383,7 +383,8 @@ describe Admin::ProceduresController, type: :controller do expect(procedure.publiee?).to be_falsey expect(procedure.path).not_to match(path) expect(procedure.lien_site_web).to match(lien_site_web) - expect(response.status).to eq 200 + expect(assigns(:valid)).to eq true + expect(assigns(:mine)).to eq false end it 'previous procedure remains published' do @@ -401,8 +402,7 @@ describe Admin::ProceduresController, type: :controller do expect(procedure.publiee?).to be_falsey expect(procedure.path).not_to match(path) expect(procedure.lien_site_web).to match(lien_site_web) - expect(response).to redirect_to :admin_procedures - expect(flash[:alert]).to have_content 'Lien de la démarche invalide' + expect(assigns(:valid)).to eq false end end end From 8806eab8d6b90871745c23e99a1e1dbe94147323 Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Mon, 29 Jul 2019 16:01:13 +0200 Subject: [PATCH 07/11] Properly validate Procedure.lien_site_web is set when publishing --- app/models/procedure.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/procedure.rb b/app/models/procedure.rb index 75f353220..e3431a3a8 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -64,6 +64,7 @@ class Procedure < ApplicationRecord validates :libelle, presence: true, allow_blank: false, allow_nil: false validates :description, presence: true, allow_blank: false, allow_nil: false validates :administrateurs, presence: true + validates :lien_site_web, presence: true, if: :publiee? validate :check_juridique validates :path, presence: true, format: { with: /\A[a-z0-9_\-]{3,50}\z/ }, uniqueness: { scope: [:path, :archived_at, :hidden_at], case_sensitive: false } # FIXME: remove duree_conservation_required flag once all procedures are converted to the new style From bd1e0aba3856baa8bbe06388617beb6c17499cfb Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Fri, 5 Jul 2019 14:38:20 +0200 Subject: [PATCH 08/11] Add Procedure path suggestion --- .../admin/procedures_controller.rb | 1 + app/models/procedure.rb | 18 +++++- .../admin/procedures/_modal_publish.html.haml | 2 +- spec/models/procedure_spec.rb | 57 +++++++++++++++++-- 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/app/controllers/admin/procedures_controller.rb b/app/controllers/admin/procedures_controller.rb index 2056ff8ea..20784c050 100644 --- a/app/controllers/admin/procedures_controller.rb +++ b/app/controllers/admin/procedures_controller.rb @@ -40,6 +40,7 @@ class Admin::ProceduresController < AdminController end def show + @suggested_path = @procedure.suggested_path(current_administrateur) end def edit diff --git a/app/models/procedure.rb b/app/models/procedure.rb index e3431a3a8..61c9a8a44 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -167,8 +167,22 @@ class Procedure < ApplicationRecord types_de_champ_private.map(&:build_champ) end - def default_path - libelle&.parameterize&.first(50) + def path_customized? + !path.match?(/[[:xdigit:]]{8}-[[:xdigit:]]{4}-[[:xdigit:]]{4}-[[:xdigit:]]{4}-[[:xdigit:]]{12}/) + end + + def suggested_path(administrateur) + if path_customized? + return path + end + slug = libelle&.parameterize&.first(50) + suggestion = slug + counter = 1 + while !path_available?(administrateur, suggestion) + counter = counter + 1 + suggestion = "#{slug}-#{counter}" + end + suggestion end def organisation_name diff --git a/app/views/admin/procedures/_modal_publish.html.haml b/app/views/admin/procedures/_modal_publish.html.haml index 102e0c234..e1d981e13 100644 --- a/app/views/admin/procedures/_modal_publish.html.haml +++ b/app/views/admin/procedures/_modal_publish.html.haml @@ -21,7 +21,7 @@ %h4 Lien de la démarche %p.center = commencer_url(path: '') - = text_field_tag(:path, @procedure.path || @procedure.default_path, + = text_field_tag(:path, @suggested_path, id: 'procedure_path', placeholder: 'Chemin vers la démarche', data: { autocomplete: 'path' }, diff --git a/spec/models/procedure_spec.rb b/spec/models/procedure_spec.rb index 0ddc65d61..df3899ef8 100644 --- a/spec/models/procedure_spec.rb +++ b/spec/models/procedure_spec.rb @@ -616,6 +616,22 @@ describe Procedure do it { expect(procedure.archived_at).to eq(now) } end + describe 'path_customized?' do + let(:procedure) { create :procedure } + + subject { procedure.path_customized? } + + context 'when the path is still the default' do + it { is_expected.to be_falsey } + end + + context 'when the path has been changed' do + before { procedure.path = 'custom_path' } + + it { is_expected.to be_truthy } + end + end + describe 'total_dossier' do let(:procedure) { create :procedure } @@ -630,12 +646,45 @@ describe Procedure do it { is_expected.to eq 2 } end - describe '#default_path' do - let(:procedure) { create(:procedure, libelle: 'A long libelle with àccênts, blabla coucou hello un deux trois voila') } + describe 'suggested_path' do + let(:procedure) { create :procedure, aasm_state: :publiee, libelle: 'Inscription au Collège' } - subject { procedure.default_path } + subject { procedure.suggested_path(procedure.administrateurs.first) } - it { is_expected.to eq('a-long-libelle-with-accents-blabla-coucou-hello-un') } + context 'when the path has been customized' do + before { procedure.path = 'custom_path' } + + it { is_expected.to eq 'custom_path' } + end + + context 'when the suggestion does not conflict' do + it { is_expected.to eq 'inscription-au-college' } + end + + context 'when the suggestion conflicts with one procedure' do + before do + create :procedure, aasm_state: :publiee, path: 'inscription-au-college' + end + + it { is_expected.to eq 'inscription-au-college-2' } + end + + context 'when the suggestion conflicts with several procedures' do + before do + create :procedure, aasm_state: :publiee, path: 'inscription-au-college' + create :procedure, aasm_state: :publiee, path: 'inscription-au-college-2' + end + + it { is_expected.to eq 'inscription-au-college-3' } + end + + context 'when the suggestion conflicts with another procedure of the same admin' do + before do + create :procedure, aasm_state: :publiee, path: 'inscription-au-college', administrateurs: procedure.administrateurs + end + + it { is_expected.to eq 'inscription-au-college' } + end end describe ".default_scope" do From c26a701a178dd86424f5797f93bfcde1a465b9dd Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Tue, 30 Jul 2019 16:54:43 +0200 Subject: [PATCH 09/11] Refactor and redesign publish modal * remove the autocomplete menu * use ujs to pre-validate the procedure * tweak the UI --- .../old_design/admin_procedures_modal.js | 70 --------------- .../stylesheets/admin_procedures_modal.scss | 24 ----- app/assets/stylesheets/application.scss | 1 - .../admin/procedures_controller.rb | 57 ++++-------- app/javascript/shared/autocomplete.js | 4 - app/models/procedure.rb | 90 ++++++++++--------- .../admin/procedures/_modal_publish.html.haml | 78 +++++++--------- .../procedures/_publish_buttons.html.haml | 13 +++ .../_publish_path_message.html.haml | 11 +++ app/views/admin/procedures/publish.js.erb | 1 - .../admin/procedures/publish_validate.js.haml | 4 + app/views/admin/procedures/show.html.haml | 4 +- config/locales/fr.yml | 2 + config/routes.rb | 2 +- .../admin/procedures_controller_spec.rb | 56 +----------- spec/factories/procedure.rb | 15 ++-- spec/models/procedure_spec.rb | 4 +- .../admin/procedures/show.html.haml_spec.rb | 4 +- 18 files changed, 148 insertions(+), 292 deletions(-) delete mode 100644 app/assets/javascripts/old_design/admin_procedures_modal.js delete mode 100644 app/assets/stylesheets/admin_procedures_modal.scss create mode 100644 app/views/admin/procedures/_publish_buttons.html.haml create mode 100644 app/views/admin/procedures/_publish_path_message.html.haml delete mode 100644 app/views/admin/procedures/publish.js.erb create mode 100644 app/views/admin/procedures/publish_validate.js.haml diff --git a/app/assets/javascripts/old_design/admin_procedures_modal.js b/app/assets/javascripts/old_design/admin_procedures_modal.js deleted file mode 100644 index 5a013cb53..000000000 --- a/app/assets/javascripts/old_design/admin_procedures_modal.js +++ /dev/null @@ -1,70 +0,0 @@ -/* globals $ */ - -$(document).on('turbolinks:load', init_path_modal); - -var PROCEDURE_PATH_SELECTOR = 'input[data-autocomplete=path]'; - -function init_path_modal() { - path_modal_action(); - path_validation_action(); - path_type_init(); - path_validation($(PROCEDURE_PATH_SELECTOR)); -} - -function path_modal_action() { - $('#publish-modal').on('show.bs.modal', function(event) { - $('#publish-modal .modal-body .table .tr-content').hide(); - - var button = $(event.relatedTarget); // Button that triggered the modal - var modal_title = button.data('modal_title'); // Extract info from data-* attributes - var modal_index = button.data('modal_index'); // Extract info from data-* attributes - - var modal = $(this); - modal.find('#publish-modal-title').html(modal_title); - $('#publish-modal .modal-body .table #' + modal_index).show(); - }); -} - -function path_validation_action() { - $(PROCEDURE_PATH_SELECTOR).keyup(function(key) { - if (key.keyCode != 13) path_validation(this); - }); -} - -function togglePathMessage(valid, mine) { - $('#path-messages .message').hide(); - - if (valid === true && mine === true) { - $('#path_is_mine').show(); - } else if (valid === true && mine === false) { - $('#path_is_not_mine').show(); - } else if (valid === false && mine === null) { - $('#path_is_invalid').show(); - } - - if ((valid && mine === null) || mine === true) - $('#publish-modal #publish').removeAttr('disabled'); - else $('#publish-modal #publish').attr('disabled', 'disabled'); -} - -function path_validation(el) { - var valid = validatePath($(el).val()); - toggleErrorClass(el, valid); - togglePathMessage(valid, null); -} - -function toggleErrorClass(node, boolean) { - if (boolean) $(node).removeClass('input-error'); - else $(node).addClass('input-error'); -} - -function validatePath(path) { - var re = /^[a-z0-9_-]{3,50}$/; - return re.test(path); -} - -function path_type_init() { - $(PROCEDURE_PATH_SELECTOR).on('autocomplete:select', function(event) { - togglePathMessage(true, event.detail['mine']); - }); -} diff --git a/app/assets/stylesheets/admin_procedures_modal.scss b/app/assets/stylesheets/admin_procedures_modal.scss deleted file mode 100644 index b6b2aa84e..000000000 --- a/app/assets/stylesheets/admin_procedures_modal.scss +++ /dev/null @@ -1,24 +0,0 @@ -.path-mine-false { - color: #FF0000; -} - -#path-messages { - .message { - display: none; - } -} - -#publish-modal { - .algolia-autocomplete { - width: 300px; - } - - .aa-dropdown-menu { - width: 300px; - } - - // Fix the input not being displayed on the same line than the text before - .aa-input { - vertical-align: initial !important; - } -} diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 5eec7e869..84ef7d75e 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -13,7 +13,6 @@ // = require _card // = require _helpers // = require _turbolinks -// = require admin_procedures_modal // = require admin_type_de_champ // = require carte // = require custom_mails diff --git a/app/controllers/admin/procedures_controller.rb b/app/controllers/admin/procedures_controller.rb index 20784c050..3449336e1 100644 --- a/app/controllers/admin/procedures_controller.rb +++ b/app/controllers/admin/procedures_controller.rb @@ -2,7 +2,7 @@ class Admin::ProceduresController < AdminController include SmartListing::Helper::ControllerExtensions helper SmartListing::Helper - before_action :retrieve_procedure, only: [:show, :edit, :delete_logo, :delete_deliberation, :delete_notice, :monavis, :update_monavis] + before_action :retrieve_procedure, only: [:show, :edit, :delete_logo, :delete_deliberation, :delete_notice, :monavis, :update_monavis, :publish_validate, :publish] def index if current_administrateur.procedures.count != 0 @@ -40,7 +40,8 @@ class Admin::ProceduresController < AdminController end def show - @suggested_path = @procedure.suggested_path(current_administrateur) + @procedure.path = @procedure.suggested_path(current_administrateur) + @current_administrateur = current_administrateur end def edit @@ -94,33 +95,19 @@ class Admin::ProceduresController < AdminController end end - def publish - path = params[:path] - lien_site_web = params[:lien_site_web] - procedure = current_administrateur.procedures.find(params[:procedure_id]) + def publish_validate + @procedure.assign_attributes(publish_params) + end + + def publish + @procedure.assign_attributes(publish_params) + + @procedure.publish_or_reopen!(current_administrateur) - procedure.publish_or_reopen!(current_administrateur, path, lien_site_web) flash.notice = "Démarche publiée" redirect_to admin_procedures_path - rescue ActiveRecord::RecordInvalid => e - errors = e.record.errors - if errors.details.key?(:path) - path_error = errors.details.dig(:path, 0, :error) - case path_error - when :invalid - @valid = false - when :taken - @valid = true - @mine = false - end - render '/admin/procedures/publish', formats: 'js' - else - flash.alert = errors.full_messages - return redirect_to admin_procedure_path(procedure) - end - rescue ActiveRecord::RecordNotFound - flash.alert = 'Démarche inexistante' - redirect_to admin_procedures_path + rescue ActiveRecord::RecordInvalid + render 'publish_validate', formats: :js end def transfer @@ -214,20 +201,6 @@ class Admin::ProceduresController < AdminController @draft_class = 'active' end - def path_list - json_path_list = Procedure - .find_with_path(params[:request]) - .order(:id) - .map do |procedure| - { - label: procedure.path, - mine: current_administrateur.owns?(procedure) - } - end.to_json - - render json: json_path_list - end - def delete_logo @procedure.logo.purge_later @procedure.logo_active_storage.purge_later @@ -256,6 +229,10 @@ class Admin::ProceduresController < AdminController params[:from_new_from_existing].present? end + def publish_params + params.permit(:path, :lien_site_web) + end + def procedure_params editable_params = [:libelle, :description, :organisation, :direction, :lien_site_web, :cadre_juridique, :deliberation, :notice, :web_hook_url, :euro_flag, :logo, :auto_archive_on, :monavis_embed] permited_params = if @procedure&.locked? diff --git a/app/javascript/shared/autocomplete.js b/app/javascript/shared/autocomplete.js index 4aefffd57..e37dec4dc 100644 --- a/app/javascript/shared/autocomplete.js +++ b/app/javascript/shared/autocomplete.js @@ -5,10 +5,6 @@ const sources = [ { type: 'address', url: '/address/suggestions' - }, - { - type: 'path', - url: '/admin/procedures/path_list' } ]; diff --git a/app/models/procedure.rb b/app/models/procedure.rb index 61c9a8a44..16ff6a9ee 100644 --- a/app/models/procedure.rb +++ b/app/models/procedure.rb @@ -65,6 +65,7 @@ class Procedure < ApplicationRecord validates :description, presence: true, allow_blank: false, allow_nil: false validates :administrateurs, presence: true validates :lien_site_web, presence: true, if: :publiee? + validate :validate_for_publication, on: :publication validate :check_juridique validates :path, presence: true, format: { with: /\A[a-z0-9_\-]{3,50}\z/ }, uniqueness: { scope: [:path, :archived_at, :hidden_at], case_sensitive: false } # FIXME: remove duree_conservation_required flag once all procedures are converted to the new style @@ -107,12 +108,18 @@ class Procedure < ApplicationRecord end end - def publish_or_reopen!(administrateur, path, lien_site_web) + def publish_or_reopen!(administrateur) Procedure.transaction do if brouillon? reset! end - publish!(administrateur, path, lien_site_web) + + other_procedure = other_procedure_with_path(path) + if other_procedure.present? && administrateur.owns?(other_procedure) + other_procedure.archive! + end + + publish! end end @@ -124,6 +131,44 @@ class Procedure < ApplicationRecord end end + def validate_for_publication + old_attributes = self.slice(:aasm_state, :archived_at) + self.aasm_state = :publiee + self.archived_at = nil + + is_valid = validate + + self.attributes = old_attributes + + is_valid + end + + def suggested_path(administrateur) + if path_customized? + return path + end + slug = libelle&.parameterize&.first(50) + suggestion = slug + counter = 1 + while !path_available?(administrateur, suggestion) + counter = counter + 1 + suggestion = "#{slug}-#{counter}" + end + suggestion + end + + def other_procedure_with_path(path) + Procedure.publiees + .where.not(id: self.id) + .find_by(path: path) + end + + def path_available?(administrateur, path) + procedure = other_procedure_with_path(path) + + procedure.blank? || administrateur.owns?(procedure) + end + def locked? publiee_ou_archivee? end @@ -171,20 +216,6 @@ class Procedure < ApplicationRecord !path.match?(/[[:xdigit:]]{8}-[[:xdigit:]]{4}-[[:xdigit:]]{4}-[[:xdigit:]]{4}-[[:xdigit:]]{12}/) end - def suggested_path(administrateur) - if path_customized? - return path - end - slug = libelle&.parameterize&.first(50) - suggestion = slug - counter = 1 - while !path_available?(administrateur, suggestion) - counter = counter + 1 - suggestion = "#{slug}-#{counter}" - end - suggestion - end - def organisation_name service&.nom || organisation end @@ -377,26 +408,6 @@ class Procedure < ApplicationRecord percentile_time(:en_construction_at, :processed_at, 90) end - def path_available?(administrateur, path) - procedure = Procedure.publiees - .where.not(id: self.id) - .find_by(path: path) - - procedure.blank? || administrateur.owns?(procedure) - end - - def auto_archive_procedure_with_same_path(administrateur, path) - procedure = administrateur.procedures.publiees - .where.not(id: self.id) - .find_by(path: path) - - procedure&.archive! - end - - def self.find_with_path(path) - where.not(aasm_state: :archivee).where("path LIKE ?", "%#{path}%") - end - def populate_champ_stable_ids TypeDeChamp.where(procedure: self, stable_id: nil).find_each do |type_de_champ| type_de_champ.update_column(:stable_id, type_de_champ.id) @@ -490,12 +501,11 @@ class Procedure < ApplicationRecord end end - def before_publish(administrateur, path, lien_site_web) - auto_archive_procedure_with_same_path(administrateur, path) - update!(path: path, lien_site_web: lien_site_web, archived_at: nil) + def before_publish + update!(archived_at: nil) end - def after_publish(administrateur, path, lien_site_web) + def after_publish update!(published_at: Time.zone.now) end diff --git a/app/views/admin/procedures/_modal_publish.html.haml b/app/views/admin/procedures/_modal_publish.html.haml index e1d981e13..cfcfaf6cc 100644 --- a/app/views/admin/procedures/_modal_publish.html.haml +++ b/app/views/admin/procedures/_modal_publish.html.haml @@ -1,63 +1,49 @@ #publish-modal.modal{ "aria-labelledby" => "myModalLabel", :role => "dialog", :tabindex => "-1" } .modal-dialog.modal-lg{ :role => "document" } - = form_tag admin_procedure_publish_path(procedure_id: @procedure.id), method: :put, remote: true do + = form_tag admin_procedure_publish_path(procedure_id: procedure.id), method: :put, remote: true do .modal-content .modal-header %button.close{ "aria-label" => "Close", "data-dismiss" => "modal", :type => "button" } %span{ "aria-hidden" => "true" } × %h4#myModalLabel.modal-title - = procedure_modal_text(@procedure, :title) + = procedure_modal_text(procedure, :title) %span#publish-modal-title .modal-body - = procedure_modal_text(@procedure, :body) - - if !@procedure.archivee? - %b - Elle ne pourra plus être modifiée à l’issue de cette publication. - %br - Afin de faciliter l’accès à la démarche, vous êtes invité à personnaliser l’adresse d'accès si vous le souhaitez. - %br + .text-info + %p + = procedure_modal_text(procedure, :body) + %b Elle ne pourra plus être modifiée à l’issue de cette publication. .form-group - %br - %h4 Lien de la démarche - %p.center + %h4 Adresse de la démarche + %p Vous pouvez personnaliser le lien public de la démarche pour en faciliter l’accès. + %p = commencer_url(path: '') - = text_field_tag(:path, @suggested_path, - id: 'procedure_path', - placeholder: 'Chemin vers la démarche', - data: { autocomplete: 'path' }, - class: 'form-control', - maxlength: 50, - style: 'width: 300px; display: inline;') - %br - .alert.alert-info - Attention, diffusez toujours le lien complet affiché ci-dessus, et non pas un lien générique vers demarches-simplifiees.fr. Ne dites pas non plus aux usagers de se rendre sur le site générique demarches-simplifiees.fr, donnez-leur toujours le lien complet. - %br - %br - Prenez quelques minutes pour savoir comment établir une bonne relation avec les usagers de votre démarche : - = link_to "Regarder la vidéo de 5 minutes", - "https://vimeo.com/334463514", - target: "_blank" - + = text_field_tag(:path, procedure.path, + id: 'procedure_path', + placeholder: 'chemin-de-la-démarche', + required: true, + class: 'form-control', + pattern: '^[a-z0-9_-]{3,50}$', + title: "De 3 à 50 caractères; minuscules, chiffres et tiret seulement", + data: { remote: true, debounce: true, url: admin_procedure_publish_validate_path(procedure) }, + autocomplete: 'off', + style: 'width: 300px; display: inline;') + = render 'publish_path_message', procedure: procedure, administrateur: administrateur + .text-info + Attention, diffusez toujours le lien complet affiché ci-dessus, et non pas un lien générique vers demarches-simplifiees.fr. Ne dites pas non plus aux usagers de se rendre sur le site générique demarches-simplifiees.fr, donnez-leur toujours le lien complet. .form-group - %h4 Où les usagers trouveront-ils le lien vers cette démarche ? * + %h4 Diffusion de la démarche + %p Où les utilisateurs trouveront-ils le lien de la démarche ? Typiquement, il s’agit d’une page de votre site web. %p.center - = text_field_tag(:lien_site_web, @procedure.lien_site_web, + = text_field_tag(:lien_site_web, procedure.lien_site_web, required: true, class: 'form-control', + autocomplete: 'off', placeholder: 'https://exemple.gouv.fr/ma_demarche') - - #path-messages - #path_is_mine.text-warning.center.message - Ce lien est déjà utilisé par une de vos démarche. - %br - Si vous voulez l’utiliser, l’ancienne démarche sera archivée (plus accessible du public). - #path_is_not_mine.text-danger.center.message - Ce lien est déjà utilisé par une démarche. - %br - Vous ne pouvez pas l’utiliser car il appartient à un autre administrateur. - #path_is_invalid.text-danger.center.message - Ce lien - = t('activerecord.errors.models.procedure.attributes.path.invalid') + .text-info + Prenez quelques minutes pour savoir comment établir une bonne relation avec les usagers de votre démarche : + = link_to "Regarder la vidéo de 5 minutes.", + "https://vimeo.com/334463514", + target: "_blank" .modal-footer - = submit_tag procedure_modal_text(@procedure, :submit), class: %w(btn btn btn-success), disabled: :disabled, id: 'publish' - = button_tag "Annuler", class: %w(btn btn btn-default), data: { dismiss: :modal }, id: 'cancel' + = render 'publish_buttons', procedure: procedure, administrateur: administrateur diff --git a/app/views/admin/procedures/_publish_buttons.html.haml b/app/views/admin/procedures/_publish_buttons.html.haml new file mode 100644 index 000000000..6630763d8 --- /dev/null +++ b/app/views/admin/procedures/_publish_buttons.html.haml @@ -0,0 +1,13 @@ +#publish-buttons + = button_tag "Annuler", class: %w(btn btn-default), data: { dismiss: :modal } + + - procedure.validate(:publication) + - errors = procedure.errors + -# Ignore the :taken error if the path can be claimed + - if errors.details[:path]&.pluck(:error)&.include?(:taken) && procedure.path_available?(administrateur, procedure.path) + - errors.delete(:path) + + - options = { class: %w(btn btn-success), id: 'publish' } + - if errors.details[:path].present? + - options[:disabled] = :disabled + = submit_tag procedure_modal_text(@procedure, :submit), options diff --git a/app/views/admin/procedures/_publish_path_message.html.haml b/app/views/admin/procedures/_publish_path_message.html.haml new file mode 100644 index 000000000..5ed0a1dcd --- /dev/null +++ b/app/views/admin/procedures/_publish_path_message.html.haml @@ -0,0 +1,11 @@ +#publish-path-message + - procedure.validate(:publication) + - errors = procedure.errors + -# Ignore the :taken error if the path can be claimed, and instead display the :taken_can_be_claimed error message. + - if errors.details[:path]&.pluck(:error)&.include?(:taken) && procedure.path_available?(administrateur, procedure.path) + .alert.alert-warning + = errors.full_message('Le lien public', errors.generate_message(:path, :taken_can_be_claimed)) + - elsif errors.messages[:path].present? + -# Display the actual errors for :path + .alert.alert-danger + = errors.full_message('Le lien public', errors.messages[:path].first) diff --git a/app/views/admin/procedures/publish.js.erb b/app/views/admin/procedures/publish.js.erb deleted file mode 100644 index d5df8f14b..000000000 --- a/app/views/admin/procedures/publish.js.erb +++ /dev/null @@ -1 +0,0 @@ -<%= "togglePathMessage(#{@valid}, #{@mine})" %> diff --git a/app/views/admin/procedures/publish_validate.js.haml b/app/views/admin/procedures/publish_validate.js.haml new file mode 100644 index 000000000..74c0681c3 --- /dev/null +++ b/app/views/admin/procedures/publish_validate.js.haml @@ -0,0 +1,4 @@ += render_to_element("#publish-path-message", partial: 'publish_path_message', outer: true, + locals: { procedure: @procedure, administrateur: current_administrateur }) += render_to_element("#publish-buttons", partial: 'publish_buttons', outer: true, + locals: { procedure: @procedure, administrateur: current_administrateur }) diff --git a/app/views/admin/procedures/show.html.haml b/app/views/admin/procedures/show.html.haml index 9301644a3..ccd27f46f 100644 --- a/app/views/admin/procedures/show.html.haml +++ b/app/views/admin/procedures/show.html.haml @@ -1,7 +1,7 @@ = render partial: 'admin/closed_mail_template_attestation_inconsistency_alert' .row.white-back #procedure_show - = render partial: '/admin/procedures/modal_publish' + = render 'modal_publish', procedure: @procedure, administrateur: @current_administrateur = render partial: '/admin/procedures/modal_transfer' - if @procedure.brouillon? @@ -117,7 +117,7 @@ - if @procedure.missing_steps.include?(:service) %p.alert.alert-danger Vous devez renseigner les coordonnées de votre Service administratif avant de pouvoir publier votre démarche. - = link_to 'Cliquez ici.', (current_administrateur.services.present? ? url_for(services_path(procedure_id: @procedure.id)) : url_for(new_service_path(procedure_id: @procedure.id))) + = link_to 'Cliquez ici.', (@current_administrateur.services.present? ? url_for(services_path(procedure_id: @procedure.id)) : url_for(new_service_path(procedure_id: @procedure.id))) - if @procedure.missing_steps.include?(:instructeurs) %p.alert.alert-danger diff --git a/config/locales/fr.yml b/config/locales/fr.yml index f48b60ed6..c44b18967 100644 --- a/config/locales/fr.yml +++ b/config/locales/fr.yml @@ -163,6 +163,8 @@ fr: procedure: attributes: path: + taken: est déjà utilisé par une démarche. Vous ne pouvez pas l’utiliser car il appartient à un autre administrateur. + taken_can_be_claimed: est identique à celui d’une autre de vos démarches publiées. Si vous publiez cette démarche, l’ancienne sera archivée et ne sera plus accessible au public. invalid: n'est pas valide. Il doit comporter au moins 3 caractères, au plus 50 caractères et seuls les caractères a-z, 0-9, '_' et '-' sont autorisés. errors: diff --git a/config/routes.rb b/config/routes.rb index 9214c6d5d..e48f85db9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -159,7 +159,6 @@ Rails.application.routes.draw do patch 'activate' => '/administrateurs/activate#create' get 'procedures/archived' => 'procedures#archived' get 'procedures/draft' => 'procedures#draft' - get 'procedures/path_list' => 'procedures#path_list' resources :procedures do collection do @@ -175,6 +174,7 @@ Rails.application.routes.draw do resources :mail_templates, only: [:index, :edit, :update] put 'archive' => 'procedures#archive', as: :archive + get 'publish_validate' => 'procedures#publish_validate', as: :publish_validate put 'publish' => 'procedures#publish', as: :publish post 'transfer' => 'procedures#transfer', as: :transfer put 'clone' => 'procedures#clone', as: :clone diff --git a/spec/controllers/admin/procedures_controller_spec.rb b/spec/controllers/admin/procedures_controller_spec.rb index 1f05f798a..b5c5fc87c 100644 --- a/spec/controllers/admin/procedures_controller_spec.rb +++ b/spec/controllers/admin/procedures_controller_spec.rb @@ -340,7 +340,7 @@ describe Admin::ProceduresController, type: :controller do context 'when admin is the owner of the procedure' do before do - put :publish, params: { procedure_id: procedure.id, path: path, lien_site_web: lien_site_web } + put :publish, format: :js, params: { procedure_id: procedure.id, path: path, lien_site_web: lien_site_web } procedure.reload procedure2.reload end @@ -383,8 +383,6 @@ describe Admin::ProceduresController, type: :controller do expect(procedure.publiee?).to be_falsey expect(procedure.path).not_to match(path) expect(procedure.lien_site_web).to match(lien_site_web) - expect(assigns(:valid)).to eq true - expect(assigns(:mine)).to eq false end it 'previous procedure remains published' do @@ -402,7 +400,6 @@ describe Admin::ProceduresController, type: :controller do expect(procedure.publiee?).to be_falsey expect(procedure.path).not_to match(path) expect(procedure.lien_site_web).to match(lien_site_web) - expect(assigns(:valid)).to eq false end end end @@ -419,8 +416,7 @@ describe Admin::ProceduresController, type: :controller do end it 'fails' do - expect(response).to redirect_to :admin_procedures - expect(flash[:alert]).to have_content 'Démarche inexistante' + expect(response).to have_http_status(404) end end @@ -573,54 +569,6 @@ describe Admin::ProceduresController, type: :controller do end end - describe 'GET #path_list' do - let!(:procedure) { create(:procedure, :published, administrateur: admin) } - let(:admin2) { create(:administrateur) } - let!(:procedure2) { create(:procedure, :published, administrateur: admin2) } - let!(:procedure3) { create(:procedure, :published, administrateur: admin2) } - - subject { get :path_list } - - let(:body) { JSON.parse(response.body) } - - describe 'when no params' do - before do - subject - end - - it { expect(response.status).to eq(200) } - it { expect(body.size).to eq(3) } - it { expect(body.first['label']).to eq(procedure.path) } - it { expect(body.first['mine']).to be_truthy } - it { expect(body.second['label']).to eq(procedure2.path) } - it { expect(body.second['mine']).to be_falsy } - end - - context 'filtered' do - before do - subject - end - - subject { get :path_list, params: { request: URI.encode(procedure2.path) } } - - it { expect(response.status).to eq(200) } - it { expect(body.size).to eq(1) } - it { expect(body.first['label']).to eq(procedure2.path) } - it { expect(body.first['mine']).to be_falsy } - end - - context 'when procedure is archived' do - let!(:procedure3) { create(:procedure, :archived, administrateur: admin2) } - before do - subject - end - - it 'do not return on the json' do - expect(body.size).to eq(2) - end - end - end - describe 'POST #transfer' do let!(:procedure) { create :procedure, :with_service, administrateur: admin } diff --git a/spec/factories/procedure.rb b/spec/factories/procedure.rb index 23b32efa7..3cfdd52dc 100644 --- a/spec/factories/procedure.rb +++ b/spec/factories/procedure.rb @@ -49,7 +49,8 @@ FactoryBot.define do after(:build) do |procedure, _evaluator| procedure.for_individual = true procedure.types_de_champ << create(:type_de_champ, libelle: 'Texte obligatoire', mandatory: true) - procedure.publish!(procedure.administrateurs.first, generate(:published_path), procedure.lien_site_web) + procedure.path = generate(:published_path) + procedure.publish! end end @@ -148,13 +149,15 @@ FactoryBot.define do trait :published do after(:build) do |procedure, _evaluator| - procedure.publish!(procedure.administrateurs.first, generate(:published_path), procedure.lien_site_web) + procedure.path = generate(:published_path) + procedure.publish! end end trait :archived do after(:build) do |procedure, _evaluator| - procedure.publish!(procedure.administrateurs.first, generate(:published_path), procedure.lien_site_web) + procedure.path = generate(:published_path) + procedure.publish! procedure.archive! end end @@ -163,14 +166,16 @@ FactoryBot.define do # For now the behavior is the same than :archived # (it may be different in the future though) after(:build) do |procedure, _evaluator| - procedure.publish!(procedure.administrateurs.first, generate(:published_path), procedure.lien_site_web) + procedure.path = generate(:published_path) + procedure.publish! procedure.archive! end end trait :hidden do after(:build) do |procedure, _evaluator| - procedure.publish!(procedure.administrateurs.first, generate(:published_path), procedure.lien_site_web) + procedure.path = generate(:published_path) + procedure.publish! procedure.hide! end end diff --git a/spec/models/procedure_spec.rb b/spec/models/procedure_spec.rb index df3899ef8..c95879de4 100644 --- a/spec/models/procedure_spec.rb +++ b/spec/models/procedure_spec.rb @@ -547,12 +547,12 @@ describe Procedure do end describe '#publish!' do - let(:procedure) { create(:procedure) } + let(:procedure) { create(:procedure, path: 'example-path') } let(:now) { Time.zone.now.beginning_of_minute } before do Timecop.freeze(now) - procedure.publish!(procedure.administrateurs.first, "example-path", procedure.lien_site_web) + procedure.publish! end after { Timecop.return } diff --git a/spec/views/admin/procedures/show.html.haml_spec.rb b/spec/views/admin/procedures/show.html.haml_spec.rb index f98d1eb91..8f724c6a7 100644 --- a/spec/views/admin/procedures/show.html.haml_spec.rb +++ b/spec/views/admin/procedures/show.html.haml_spec.rb @@ -41,7 +41,7 @@ describe 'admin/procedures/show.html.haml', type: :view do describe 'procedure is published' do before do - procedure.publish!(procedure.administrateurs.first, 'fake_path', procedure.lien_site_web) + procedure.publish! procedure.reload render end @@ -59,7 +59,7 @@ describe 'admin/procedures/show.html.haml', type: :view do describe 'procedure is archived' do before do - procedure.publish!(procedure.administrateurs.first, 'fake_path', procedure.lien_site_web) + procedure.publish! procedure.archive! procedure.reload render From 543f9894c1661e9254c6776aa88388e4adceeb25 Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Tue, 13 Aug 2019 15:43:23 +0200 Subject: [PATCH 10/11] =?UTF-8?q?Make=20sure=20the=20=E2=80=9Ccommencer?= =?UTF-8?q?=E2=80=9D=20link=20uses=20the=20proper=20test=20path?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/controllers/admin/procedures_controller.rb | 5 +++++ app/views/admin/procedures/show.html.haml | 4 ++-- spec/views/admin/procedures/show.html.haml_spec.rb | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/controllers/admin/procedures_controller.rb b/app/controllers/admin/procedures_controller.rb index 3449336e1..05230f168 100644 --- a/app/controllers/admin/procedures_controller.rb +++ b/app/controllers/admin/procedures_controller.rb @@ -40,6 +40,11 @@ class Admin::ProceduresController < AdminController end def show + if @procedure.brouillon? + @procedure_lien = commencer_test_url(path: @procedure.path) + else + @procedure_lien = commencer_url(path: @procedure.path) + end @procedure.path = @procedure.suggested_path(current_administrateur) @current_administrateur = current_administrateur end diff --git a/app/views/admin/procedures/show.html.haml b/app/views/admin/procedures/show.html.haml index ccd27f46f..13c81bac4 100644 --- a/app/views/admin/procedures/show.html.haml +++ b/app/views/admin/procedures/show.html.haml @@ -54,7 +54,7 @@ - elsif @procedure.publiee? Cette démarche est publiée, certains éléments ne peuvent plus être modifiés. Pour y accéder vous pouvez utiliser le lien : - = link_to procedure_lien(@procedure), sanitize_url(procedure_lien(@procedure)), target: :blank, rel: :noopener + = link_to @procedure_lien, sanitize_url(@procedure_lien), target: :blank, rel: :noopener %br %br Attention, diffusez toujours le lien complet affiché ci-dessus, et non pas un lien générique vers demarches-simplifiees.fr. Ne dites pas non plus aux usagers de se rendre sur le site générique demarches-simplifiees.fr, donnez-leur toujours le lien complet. @@ -63,7 +63,7 @@ %p Cette démarche est actuellement en test, pour y accéder vous pouvez utiliser le lien : - = link_to procedure_lien(@procedure), sanitize_url(procedure_lien(@procedure)), target: :blank, rel: :noopener + = link_to @procedure_lien, sanitize_url(@procedure_lien), target: :blank, rel: :noopener %p Toute personne ayant la connaissance de ce lien pourra ainsi remplir des dossiers de test sur votre démarche. %br diff --git a/spec/views/admin/procedures/show.html.haml_spec.rb b/spec/views/admin/procedures/show.html.haml_spec.rb index 8f724c6a7..c1ec1bffe 100644 --- a/spec/views/admin/procedures/show.html.haml_spec.rb +++ b/spec/views/admin/procedures/show.html.haml_spec.rb @@ -6,6 +6,7 @@ describe 'admin/procedures/show.html.haml', type: :view do before do assign(:procedure, procedure) + assign(:procedure_lien, commencer_url(path: procedure.path)) end describe 'procedure is draft' do From 78da268dedcb5e17454f3875938bc66f623ea6cc Mon Sep 17 00:00:00 2001 From: Nicolas Bouilleaud Date: Wed, 14 Aug 2019 18:05:01 +0200 Subject: [PATCH 11/11] Improve feature tests for procedure cloning and creation --- spec/features/admin/procedure_cloning_spec.rb | 38 +++++++++++++------ .../features/admin/procedure_creation_spec.rb | 7 +++- 2 files changed, 33 insertions(+), 12 deletions(-) diff --git a/spec/features/admin/procedure_cloning_spec.rb b/spec/features/admin/procedure_cloning_spec.rb index eb908a012..74442d89f 100644 --- a/spec/features/admin/procedure_cloning_spec.rb +++ b/spec/features/admin/procedure_cloning_spec.rb @@ -7,23 +7,39 @@ feature 'As an administrateur I wanna clone a procedure', js: true do let(:administrateur) { create(:administrateur) } before do + create :procedure, :with_service, + aasm_state: :publiee, published_at: Time.zone.now, + administrateurs: [administrateur], + libelle: 'libellé de la procédure', + path: 'libelle-de-la-procedure' login_as administrateur.user, scope: :user - visit new_from_existing_admin_procedures_path end - context 'Cloning procedure' do - before 'Create procedure' do - page.find_by_id('from-scratch').click - fill_in_dummy_procedure_details - page.find_by_id('save-procedure').click - end - - scenario 'Cloning' do - visit admin_procedures_draft_path + context 'Cloning a procedure owned by the current admin' do + scenario do + visit admin_procedures_path expect(page.find_by_id('procedures')['data-item-count']).to eq('1') page.all('.clone-btn').first.click visit admin_procedures_draft_path - expect(page.find_by_id('procedures')['data-item-count']).to eq('2') + expect(page.find_by_id('procedures')['data-item-count']).to eq('1') + click_on Procedure.last.libelle + expect(page).to have_current_path(admin_procedure_path(Procedure.last)) + + find('#publish-procedure').click + + within '#publish-modal' do + expect(find_field('procedure_path').value).to eq 'libelle-de-la-procedure' + expect(page).to have_text('ancienne sera archivée') + fill_in 'lien_site_web', with: 'http://some.website' + click_on 'publish' + end + + page.refresh + + visit admin_procedures_archived_path + expect(page.find_by_id('procedures')['data-item-count']).to eq('1') + visit admin_procedures_draft_path + expect(page.find_by_id('procedures')['data-item-count']).to eq('0') end end end diff --git a/spec/features/admin/procedure_creation_spec.rb b/spec/features/admin/procedure_creation_spec.rb index afd26564c..5abfec21c 100644 --- a/spec/features/admin/procedure_creation_spec.rb +++ b/spec/features/admin/procedure_creation_spec.rb @@ -98,11 +98,16 @@ feature 'As an administrateur I wanna create a new procedure', js: true do click_on Procedure.last.libelle expect(page).to have_current_path(admin_procedure_path(Procedure.last)) + expect(page).to have_content('en test') + # Only check the path even though the link is the complete URL + # (Capybara runs the app on an arbitrary host/port.) + expect(page).to have_link(nil, href: /#{commencer_test_path(Procedure.last.path)}/) + expect(page).to have_selector('#publish-procedure', visible: true) find('#publish-procedure').click within '#publish-modal' do - expect(page).to have_field('procedure_path') + expect(find_field('procedure_path').value).to eq 'libelle-de-la-procedure' fill_in 'lien_site_web', with: 'http://some.website' click_on 'publish' end