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

Ajout le l'état clôturé #262

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Ajout le l'état clôturé #262

wants to merge 1 commit into from

Conversation

alanzirek
Copy link
Collaborator

@alanzirek alanzirek commented Sep 12, 2024

Cette PR ajoute l'état clôturé sur la fiche détection.
J'ai aussi ajouté un fichier constants.py contenant des constantes utilisées dans différentes parties du code.

@alanzirek alanzirek self-assigned this Sep 12, 2024
@alanzirek alanzirek linked an issue Sep 12, 2024 that may be closed by this pull request
@alanzirek alanzirek changed the title Ajout le l'état clôturé Ajout le l'état clôturé Sep 13, 2024
Ajout du fichier constants.py contenant des constantes utilisées dans différentes parties du code.
Copy link
Collaborator

@Anto59290 Anto59290 left a comment

Choose a reason for hiding this comment

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

Dispo pour en parler, en particulier sur les perfs

@@ -5,8 +5,11 @@
<div class="fr-collapse fr-translate__menu fr-menu" id="action-1">
<ul class="fr-menu__list">
<li><a class="fr-translate__language fr-nav__link" href="#" data-fr-opened="false" aria-controls="fr-modal-freelink"><span class="fr-icon-git-pull-request-fill fr-mr-2v fr-icon--sm" aria-hidden="true"></span>Ajouter un lien libre</a></li>
{% if user.agent.structure.is_ac and not fichedetection.etat.is_cloture %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Je me demande dans quelle mesure ceci n'introduit pas des nouvelle requetes SQL (voir autres commentaires)

<p>Vous ne pouvez pas clôturer la fiche n° {{ fichedetection.numero }} car les structures suivantes n'ont pas signalées la fin de suivi : </p>
<ul>
{% for contact in contacts_not_in_fin_suivi %}
<li>{{ contact }}</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Je pense que ceci va introduire un requete N+1 sur la page, surtout avec l'affichage de contact qui est de mémoire assez gourmand

)
expect(page.get_by_label("Clôturer une fiche").get_by_role("listitem")).to_contain_text(contact2.structure.libelle)
assert FicheDetection.objects.get(id=fiche_detection.id).etat == Etat.objects.get(libelle=Etat.NOUVEAU)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Je pense qu'il serait intéressant d'ajouter un test de perf (dans l'autre fichier peut être) pour tester le cas suivant:

  • Une page fiche détection fait X requetes
  • J'ajoute 10 structures qui n'ont pas fait de fin de suivi, combien de requetes sont faites.

@@ -81,8 +81,8 @@ def test_add_multiple_structures_to_a_fiche(live_server, page, fiche_detection):
page.get_by_role("button", name="Ajouter les structures sélectionnées").click()

expect(page.get_by_text("Les 2 structures ont été ajoutées avec succès.")).to_be_visible()
expect(page.get_by_text(contact1.structure.libelle, exact=True)).to_be_visible()
expect(page.get_by_text(contact2.structure.libelle, exact=True)).to_be_visible()
expect(page.locator("p").filter(has_text=contact1.structure.libelle)).to_be_visible()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Je n'ai pas compris pourquoi ces changements, est ce qu'on ne risque pas de tester le mauvais élément ?

class FicheDetecionCloturerView(View):
def post(self, request, pk):
fiche = FicheDetection.objects.get(pk=pk)
fiche.cloturer()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Est ce que dans l'idéal il ne faudrait pas aussi une protection côté backend ? Car ici la détection de est ce que cette action est autorisée n'est fait que dans le front non ?

Comment on lines 374 to +380
@classmethod
def get_etat_initial(cls):
return cls.objects.get(libelle="nouveau").id
return cls.objects.get(libelle=cls.NOUVEAU).id

@classmethod
def get_etat_cloture(cls):
return cls.objects.get(libelle=cls.CLOTURE)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

à déplacer dans un manager ?

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.

État clôturé
2 participants