Skip to content

Conversation

@pyDez pyDez requested a review from thibault December 11, 2025 09:12
@tristanrobert
Copy link

tristanrobert commented Dec 11, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@pyDez pyDez requested a review from numahell December 11, 2025 09:12
Copy link
Collaborator

@numahell numahell left a comment

Choose a reason for hiding this comment

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

J'ai des commentaires pour plus de commentaires :)
à part ça c'est bon pour moi.

@pyDez pyDez requested a review from numahell January 5, 2026 08:13
Copy link
Collaborator

@numahell numahell left a comment

Choose a reason for hiding this comment

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

Merci pour l'ajout des commentaires et des docstrings :)

Copy link
Collaborator

@thibault thibault left a comment

Choose a reason for hiding this comment

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

J'ai posé quelques commentaires, j'ai l'intuition que la solution choisie risque de créer des problèmes quand on prendra en compte plus d'éléments dans l'historique de modification du dossier. Qu'en penses-tu ?


def get_context_data(self, **kwargs):

def extract_entries(log):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Je suis un peu mitigé quant à ce choix technique. À ce stade, on a une belle correspondance ou chaque entrée StatusLog correspond à un changement d'état. C'est clair, explicite et propre.

Est-ce que ça n'aurait pas été plus simple de garder cette correspondance en créant tout simplement une nouvelle entrée au moment de la reprise de l'instruction ? Ça aurait évité de bidouiller pour créer ce tableau et épargné la nécessité de créer une nouvelle structure de données.

Par ailleurs, la présente solution créé un petit bug de cohérence sur le nombre de changements indiqué.

Image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pour ce besoin précis d'affichage des changements, on prefererait effectivement avoir un objet distinct pour chaque changement: changement d'état, demande de suppression, reception des compléments.
Mais il y a peut etre eu d'autre raison technique ou metier qui ont mené à la decision de faire porter les demandes de supplement et les reception directement sur les changements d'état.
Je ne suis pas fermé à lancer la discussion, mais peut etre en dehors de ce ticket que je preferais merger tel quel.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Je reste super mitigé vis à vis de ça, parce qu'on intègre au dépôt du code relativement complexe pour quelque chose qui devrait être trivial : une liste d'états = un tableau d'historique. Pour moi ça frise pas mal la dette technique parce que dès qu'on devra toucher à ce changement d'état, il faudra retoucher cette boucle. Je trouve dommage d'intégrer ça au dépôt si ça doit être retouché plus tard.

D'ailleurs, il me semble qu'il y a déjà un ticket dans les tuyaux qui va obliger à se poser la question.

https://trello.com/c/ecg5IsPW/2057-conserver-l%C3%A9tat-suspendu-lors-dun-changement-d%C3%A9tape

Maintenant, si tu juges que c'est mieux de fusionner en état pour des raisons de mep et que tu veux y revenir plus tard, je ne bloque pas la fusion :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

J'ai suivi la voie de la sagesse (aka @thibault) voici la séparation des logs en différents objets ! Une nouvelle revue n'est pas de refus :)

{% if log.previous_log %}
{{ log.previous_log.get_stage_display }}
{% if log.previous_log.decision != "unset" %}
{% if log.type == "status_change" %}
Copy link
Collaborator

@thibault thibault Jan 5, 2026

Choose a reason for hiding this comment

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

Ça me semble beaucoup de logique pour de l'affichage. Est-ce que ça n'aurait pas plutôt sa place dans le modèle, avec un champ « type » de type Choices ?

```{{ log.get_type_display }}``

pyDez and others added 6 commits January 7, 2026 06:32
Add data migration step to set resumed_by for existing StatusLog entries
that have info_receipt_date but were missing resumed_by. Use atomic=False
to avoid pending trigger events error.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@pyDez pyDez requested a review from thibault January 7, 2026 10:03

def get_context_data(self, **kwargs):

def extract_entries(log):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Je reste super mitigé vis à vis de ça, parce qu'on intègre au dépôt du code relativement complexe pour quelque chose qui devrait être trivial : une liste d'états = un tableau d'historique. Pour moi ça frise pas mal la dette technique parce que dès qu'on devra toucher à ce changement d'état, il faudra retoucher cette boucle. Je trouve dommage d'intégrer ça au dépôt si ça doit être retouché plus tard.

D'ailleurs, il me semble qu'il y a déjà un ticket dans les tuyaux qui va obliger à se poser la question.

https://trello.com/c/ecg5IsPW/2057-conserver-l%C3%A9tat-suspendu-lors-dun-changement-d%C3%A9tape

Maintenant, si tu juges que c'est mieux de fusionner en état pour des raisons de mep et que tu veux y revenir plus tard, je ne bloque pas la fusion :)

@pyDez pyDez requested a review from thibault January 15, 2026 08:48
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.

5 participants