Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Etq admin je peux relancer le routage sur tous les dossiers #11312

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

E-L-T
Copy link
Contributor

@E-L-T E-L-T commented Feb 14, 2025

Voir #10766

Si un admin route sa démarche alors que des dossiers ont déjà été déposés, on lui affiche une alerte avec un bouton permettant de lancer le routage sur tous les dossiers (quel que soit leur statut) :

Capture d’écran 2025-02-14 à 15 07 16

Si toutes les règles de routage ne sont pas valides, on désactive le bouton et le texte est un peu différent :

Capture d’écran 2025-02-14 à 15 06 18

NB : J'hésite à lancer ce bulk_routing dans un job assynchrone parce que ça risque de faire un timeout si on a beaucoup de dossiers. En même temps je m'attends plutôt à ce que les admins veuillent router quand il y a encore peu de dossiers déposés. Donc je serais partant de laisser en synchrone pour l'instant

Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 33.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 44.59%. Comparing base (cb58be7) to head (6216f3c).
Report is 81 commits behind head on main.

Files with missing lines Patch % Lines
.../administrateurs/groupe_instructeurs_controller.rb 27.27% 8 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11312      +/-   ##
==========================================
- Coverage   48.21%   44.59%   -3.62%     
==========================================
  Files        1214     1232      +18     
  Lines       30805    32163    +1358     
  Branches     4328     4206     -122     
==========================================
- Hits        14853    14344     -509     
- Misses      15952    17819    +1867     

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

@E-L-T E-L-T marked this pull request as ready for review February 14, 2025 15:23
Copy link
Member

@LeSim LeSim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A part une modification concernant la perf, c'est bon pour moi.

@@ -220,6 +220,7 @@ def classer_sans_suite(motivation: nil, instructeur: nil, processed_at: Time.zon
scope :state_accepte, -> { where(state: states.fetch(:accepte)) }
scope :state_refuse, -> { where(state: states.fetch(:refuse)) }
scope :state_sans_suite, -> { where(state: states.fetch(:sans_suite)) }
scope :soumis, -> { where(state: SOUMIS) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

j'ai l'impression que ce scope est équivalent à not_brouillon. Je trouve le tiens plus clair et je ne me demande s'il n'y aura pas matière à faire une autre pr de nettoyage pour remplacer not_brouillon par soumis

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finalement, suite au retour de @tchak signalant le risque à re-router les dossiers terminés, je ne vais plus avoir besoin de ce nouveau scope (j'avais pas vu not_brouillon, je trouvais ça louche d'avoir à créer un nouveau scope pour ça). Mais cela dit pourquoi pas faire quand même une PR de renommage de state_not_brouillon par state_soumis

@@ -392,6 +392,22 @@ def export_groupe_instructeurs
end
end

def bulk_route
dossiers = procedure.dossiers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

au niveau perf, ca doit être la fête au n+1. Est ce que tu pourrais essayer avec une procédure importante genre fond vert ?

J'ai l'impression qu'il faut aussi que tu batches et que tu passes par le DossierPreloader


dossiers.update_all(forced_groupe_instructeur: false)

dossiers.each do |dossier|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pour les dossiers terminés, les règles de routage ne sont pas garanties d'être compatibles avec la révision du dossier. Je pense que c'est une mauvaise idée de rerouter ces dossiers

@@ -88,6 +88,8 @@ def create_simple_routing
defaut.destroy!
end

procedure.update!(routing_alert: true) if procedure.dossiers.soumis.any?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'explique plus bas pourquoi c'est une mauvaise idée de redispatcher les dossiers terminés. Si on veut vraiment le faire, il faut soit appliquer le reroutage uniquement sur les dossiers terminés qui sont en dernière révision, soit vérifier si la révision du dossier terminé est compatible avec les nouvelles règles de routage.

Copy link
Member

@tchak tchak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pour les dossiers terminés, les règles de routage ne sont pas garanties d'être compatibles avec la révision du dossier. C'est une mauvaise idée de redispatcher les dossiers terminés. Si on veut vraiment le faire, il faut soit appliquer le reroutage uniquement sur les dossiers terminés qui sont en dernière révision, soit vérifier si la révision du dossier terminé est compatible avec les nouvelles règles de routage.

@E-L-T
Copy link
Contributor Author

E-L-T commented Mar 3, 2025

Pour les dossiers terminés, les règles de routage ne sont pas garanties d'être compatibles avec la révision du dossier. C'est une mauvaise idée de redispatcher les dossiers terminés. Si on veut vraiment le faire, il faut soit appliquer le reroutage uniquement sur les dossiers terminés qui sont en dernière révision, soit vérifier si la révision du dossier terminé est compatible avec les nouvelles règles de routage.

Ok, je crois que je vois le problème. C'est parce que lors de la révision d'une démarche, les changements sont appliqués aux dossiers en construction et en instruction mais pas aux dossiers terminés, c'est ça ?
Dans ce contexte, comme tu proposes, je pense que c'est mieux de ne pas du tout re-router les dossiers terminés. Ça risquerait d'être source de confusion pour l'admin que certains dossiers terminés soient re-routés et pas d'autres. Et a priori si le dossier est traité, c'est qu'on n'a plus besoin de le router à quelqu'un d'autre.

@E-L-T E-L-T force-pushed the etq_admin_je_peux_relancer_le_routage_sur_tous_les_dossiers branch from e8b7840 to a0dc47e Compare March 3, 2025 13:33
@E-L-T E-L-T force-pushed the etq_admin_je_peux_relancer_le_routage_sur_tous_les_dossiers branch from b0a3cd4 to 6216f3c Compare March 4, 2025 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants