(2/X) ETQ administrateur, je veux pouvoir modifier le lien URL de ma démarche sans avoir à la cloturer #11051

mmagn · 2024-11-15T14:02:57Z

Cette PR permet à l'administrateur de modifier le lien de la démarche.
Il est toujours possible pour un administrateur de récupérer un ancien lien qui était associé à une autre de ses démarches. Pas possible de récupérer le lien d'une démarche qui n'appartient pas à l'administrateur.
On garde un historique des anciens liens associés à une démarche, ceux ci redirigent vers le dernier lien en cours (sauf si un ancien lien a été associé à une autre démarche).

Il y a une tâche de migration de données associée à cette PR, en attendant que la tâche ait tourné, l'ancien attribut path pointe toujours sur la démarche.

Il faudra ensuite une autre PR pour gérer les PathRewrite, et supprimer les références à l'ancien attribut path

codecov · 2024-11-15T14:09:29Z

Codecov Report

Attention: Patch coverage is 92.63158% with 7 lines in your changes missing coverage. Please review.

Project coverage is 84.37%. Comparing base (8ba2c5c) to head (cebe11d).

Files with missing lines	Patch %	Lines
.../controllers/manager/procedure_paths_controller.rb	0.00%	4 Missing ⚠️
app/controllers/users/commencer_controller.rb	80.00%	1 Missing ⚠️
app/controllers/users/statistiques_controller.rb	0.00%	1 Missing ⚠️
...intenance/migrate_existing_procedure_paths_task.rb	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #11051      +/-   ##
==========================================
+ Coverage   84.35%   84.37%   +0.02%     
==========================================
  Files        1178     1182       +4     
  Lines       25962    26033      +71     
  Branches     4901     4918      +17     
==========================================
+ Hits        21900    21966      +66     
- Misses       4062     4067       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

LeSim

je n'arrive plus a aller sur les démarches en tests qui utilise l'url en uuid

LeSim · 2024-11-28T09:40:21Z

app/models/concerns/procedure_path_concern.rb

-    validates :path, presence: true, format: { with: /\A[a-z0-9_\-]{3,200}\z/ }, uniqueness: { scope: [:path, :closed_at, :hidden_at, :unpublished_at], case_sensitive: false }
+    has_many :procedure_paths, dependent: :destroy, autosave: true
+
+    # validates :path, presence: true, format: { with: /\A[a-z0-9_\-]{3,200}\z/ }, uniqueness: { scope: [:path, :closed_at, :hidden_at, :unpublished_at], case_sensitive: false }


a supprimer ?

LeSim · 2024-11-28T09:42:58Z

app/models/procedure_path.rb

+  scope :by_updated_at, -> { order(updated_at: :desc) }
+
+  def ensure_one_path
+    return if procedure.procedure_paths.count > 1 || destroyed_by_association


TIL destroyed_by_association

LeSim · 2024-11-28T09:47:48Z

app/models/procedure_path.rb

+    return if procedure.procedure_paths.count > 1 || destroyed_by_association
+
+    errors.add(:base, :at_least_one_path)
+    throw(:abort)


Alors j'ai l'impression que throw et raise ne sont pas exactement interchangeable en ruby. et d'hab on met du raise

yes alors là j'ai mis des throw car c'est ce qu'il faut pour interrompre un callback AR https://guides.rubyonrails.org/active_record_callbacks.html, sinon je mets des raise en effet

LeSim · 2024-11-28T09:57:15Z

app/controllers/administrateurs/procedures_controller.rb

        @current_administrateur = current_administrateur
        @closed_procedures = current_administrateur.procedures.with_discarded.closes.map { |p| ["#{p.libelle} (#{p.id})", p.id] }.to_h
      end
    end

    def check_path
-      @path_available = @procedure.path_available?(params[:path])
-      @other_procedure = @procedure.other_procedure_with_path(params[:path])
+      path = params.permit(:path)[:path]


Suggested change

path = params.permit(:path)[:path]

path = params[:path]

LeSim · 2024-11-28T10:02:25Z

app/controllers/administrateurs/procedures_controller.rb

+
+      if other_procedure.present? && !current_administrateur.owns?(other_procedure)
+        flash.alert = "Cette URL de démarche n'est pas disponible"
+        return redirect_to admin_procedure_path_path(@procedure)


path_path ?

Tu savais qu'on peut faire ça ? le futur non ?

Suggested change

return redirect_to admin_procedure_path_path(@procedure)

return redirect_to [:admin, @procedure, :path]

le turfu en effet, je change

LeSim · 2024-11-28T10:12:17Z

app/models/concerns/procedure_path_concern.rb

+
+    scope :find_with_path, -> (path) do
+      normalized_path = path.downcase.strip
+      joins(:procedure_paths).where(procedure_paths: { path: normalized_path }).or(where(path: normalized_path)).limit(1)


Alors la, tu me dois 2 carambars.

Tu peux pas faire de joins car certaines procédure n'ont pas de ProcedurePath, du coup, tu les exclues de ta recherche et ca fait du caca.

Je me demande si tu peux pas mettre directement un first également dans le find

Suggested change

joins(:procedure_paths).where(procedure_paths: { path: normalized_path }).or(where(path: normalized_path)).limit(1)

includes(:procedure_paths).where(procedure_paths: { path: normalized_path }).or(where(path: normalized_path)).first

?

Bien vu ! En plus j'avais un cas de test pour ce problème spécifique, mais j'ai zappé que pour toute nouvelle procédure un ProcedurePath était créé. 🤦
C'est corrigé !
J'ai pas de carambars sous la main, alors je te donne des chocolats 🍫🍫.

Par contre c'est pas possible de mettre le first qui serait je trouve aussi plus élégant, car il risque de retourner nil s'il ne trouve pas de Procedure et ça renverrait toutes les Procedure => https://stackoverflow.com/questions/20942672/rails-scope-returns-all-instead-of-nil

LeSim · 2024-11-28T11:04:53Z

app/controllers/administrateurs/procedures_controller.rb

@@ -286,26 +286,51 @@ def publication
        flash.alert = "La date limite de dépôt des dossiers doit être postérieure à la date du jour pour réactiver la procédure. #{view_context.link_to('Veuillez la modifier', edit_admin_procedure_path(@procedure))}"
        redirect_to admin_procedure_path(@procedure)
      else
-        @procedure.path = @procedure.suggested_path(current_administrateur)
+        @procedure.claim_path(current_administrateur, @procedure.suggested_path)


par construction, suggested_path n'est utilisé par personne, du coup, j'ai l'impression que tu pourrais faire
@procedure.procedure_paths.create(@procedure.suggested_path)

voir mm qu'on pourrait directement ajouter une methode ensure_custom_path dans le path concern qui comprendrait le code de suggested_path

~~en fait ici on écrit pas en DB le procedure path, on le prérempli pour la page de publication.~~

mais je ne suis pas sûr de comprendre à 100% ton point, on peut en discuter en visio ?

je pense avoir trouvé mieux, je l'utilise dans le formulaire de publication

LeSim · 2024-11-28T11:20:05Z

app/views/administrateurs/procedures/path.html.haml

+      .flex
+        %span.placeholder
+          = commencer_url(path: '')
+        = text_field_tag(:path, @procedure.path,


mettre un label,

expliciter les regles dans la description ? (j'ai pas eu le droit à l'url a)

indiquer que les anciens liens marchent toujours ?

Yes je reprends ce qui est déjà en place sur la page de publication. Et je vais ping la chef UX avant la MEP pour validation

LeSim · 2024-11-28T11:38:11Z

app/models/procedure_path.rb

+
+  validates :path, presence: true, format: { with: /\A[a-z0-9_\-]{3,200}\z/ }, uniqueness: { case_sensitive: false }
+
+  scope :by_updated_at, -> { order(updated_at: :desc) }


Suggested change

scope :by_updated_at, -> { order(updated_at: :desc) }

tu ne peux le mettre order by defaut dans la relation décrite dans le concern ?

un truc du genre

has_many :procedure_paths, -> { order(updated_at: :desc) }, dependent: :destroy, autosave: true

ca permettrait d'éviter de spécifier l'ordre à droite à gauche

ouais, normalement on est pas trop sensé taper sur cette association en direct, mais t'as raison ce sera plus simple

… procedure

…ites data

mmagn changed the base branch from main to fix-9513-part-1 November 15, 2024 14:03

mmagn force-pushed the fix-9513-part-2 branch 10 times, most recently from 05500a0 to 6b22737 Compare November 15, 2024 17:36

mmagn changed the base branch from fix-9513-part-1 to main November 15, 2024 18:33

mmagn force-pushed the fix-9513-part-2 branch 3 times, most recently from 5db3e7f to 5a9c80a Compare November 18, 2024 22:17

mmagn changed the base branch from main to fix-9513-part-1 November 19, 2024 08:54

mmagn force-pushed the fix-9513-part-2 branch from 5a9c80a to e8c49a3 Compare November 19, 2024 08:58

mmagn changed the base branch from fix-9513-part-1 to main November 19, 2024 08:59

mmagn marked this pull request as ready for review November 19, 2024 09:29

LeSim reviewed Nov 28, 2024

View reviewed changes

[#9513] initialize ProcedurePath

99c29b4

mmagn force-pushed the fix-9513-part-2 branch 3 times, most recently from 49d70e1 to e16bc64 Compare November 29, 2024 13:22

mmagn added 5 commits November 29, 2024 15:22

[#9513] Define canonical_path (and override path)

094db85

[#9513] Add ProcedurePath to Manager

005f47a

[#9513] Update path in UI

d19f4d5

[#9513] Use find_with_path

7fb73f0

[#9513] Redirect to canonical path if the path is an old path

c0e14b4

mmagn added 11 commits November 29, 2024 15:22

[#9513] Sync path with Procedure Path

e2a2bb2

[#9513] claim_path method

974e274

[#9513] I18n & Fix tests

098bf01

[#9513] Task to migrate old data

59ba1c0

[#9513] fix tests

39c84ae

[#9513] strip and downcase when using find_with_path

f56b8e2

[#9513] add test case

a8e1483

[#9513] when migration is pending find_with_path should not return no…

02eb53c

… procedure

[#9513] Add tests on ProcedurePath #destroy

6bcf68b

[#9513] Fix issue with inner join potentially causing 💩

3941238

[#9513] Fixes after review

95ca13f

mmagn force-pushed the fix-9513-part-2 branch from e16bc64 to 95ca13f Compare November 29, 2024 14:23

[#9513] claim_path should raise an exception if invalid because it wr…

cebe11d

…ites data

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(2/X) ETQ administrateur, je veux pouvoir modifier le lien URL de ma démarche sans avoir à la cloturer #11051

(2/X) ETQ administrateur, je veux pouvoir modifier le lien URL de ma démarche sans avoir à la cloturer #11051

mmagn commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading

LeSim left a comment

LeSim Nov 28, 2024

LeSim Nov 28, 2024

LeSim Nov 28, 2024

mmagn Nov 29, 2024

LeSim Nov 28, 2024

LeSim Nov 28, 2024

mmagn Nov 29, 2024

LeSim Nov 28, 2024

mmagn Nov 29, 2024

LeSim Nov 28, 2024

mmagn Nov 29, 2024 •

edited

Loading

mmagn Nov 29, 2024

LeSim Nov 28, 2024

mmagn Nov 29, 2024

LeSim Nov 28, 2024

mmagn Nov 29, 2024

	return redirect_to admin_procedure_path_path(@procedure)
	return redirect_to [:admin, @procedure, :path]

	joins(:procedure_paths).where(procedure_paths: { path: normalized_path }).or(where(path: normalized_path)).limit(1)
	includes(:procedure_paths).where(procedure_paths: { path: normalized_path }).or(where(path: normalized_path)).first


		validates :path, presence: true, format: { with: /\A[a-z0-9_\-]{3,200}\z/ }, uniqueness: { case_sensitive: false }

		scope :by_updated_at, -> { order(updated_at: :desc) }

(2/X) ETQ administrateur, je veux pouvoir modifier le lien URL de ma démarche sans avoir à la cloturer #11051

Are you sure you want to change the base?

(2/X) ETQ administrateur, je veux pouvoir modifier le lien URL de ma démarche sans avoir à la cloturer #11051

Conversation

mmagn commented Nov 15, 2024 • edited Loading

codecov bot commented Nov 15, 2024 • edited Loading

Codecov Report

LeSim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmagn Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmagn commented Nov 15, 2024 •

edited

Loading

codecov bot commented Nov 15, 2024 •

edited

Loading

mmagn Nov 29, 2024 •

edited

Loading