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.

Sign up for GitHub

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

Jump to bottom

Résolution de soucis mineurs d'héritage de classes #616

Merged
merged 2 commits into from
Apr 19, 2022
Merged

Résolution de soucis mineurs d'héritage de classes #616

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c35dc21
Fix inheritance issues
tut-tuuut Apr 12, 2022
11df410
Declare abstract method in abstract class
tut-tuuut Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions aidants_connect_habilitation/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,14 @@ def dispatch(self, request, *args, **kwargs):


class LateStageRequestView(VerifiedEmailIssuerView, View):
Copy link
Member Author

Choose a reason for hiding this comment

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

Ces vues peuvent concerner des formulaires ou non, donc pas forcément besoin de fil d'Ariane => pas de step attendu, pas d'héritage de habilitationstep

@property
def step(self) -> HabilitationFormStep:
raise NotImplementedError()

def setup(self, request, *args, **kwargs):
super().setup(request, *args, **kwargs)
self.organisation = get_object_or_404(
OrganisationRequest, uuid=kwargs.get("uuid"), issuer=self.issuer
)


class OnlyNewRequestsView(LateStageRequestView):
class OnlyNewRequestsView(HabilitationStepMixin, LateStageRequestView):
Copy link
Member Author

Choose a reason for hiding this comment

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

Comme ces vues concernent seulement des brouillons, on a forcément un formulaire donc on s'attend à un fil d'Ariane => on peut hériter directement du habilitationstepmixin au lieu de dupliquer la logique

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm du coup l'IDE râle qu'on n'implémente pas la méthode abstraite. Pas moyen de déclarer que la classe est abstraite sans dupliquer le step ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Bah l'IDE propoe aussi de rajouter abc.ABC aux classes héritées. Mais je sais pas trop ce que c'est ni comment ça fonctionne alors j'ai évité.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Après, ça sert à rien de multiplier les classes abstraites à 1 méthode. Moi mon but à la base c'était surtout d'avoir 1 FormView commune pour presque toutes les vues de l'application et éviter qu'on fasse des erreurs bêtes en modifiant, genre oublier de remettre le step dans le contexte et péter le rail sans s'en rendre compte. Mais le code actuel fait plus vraiment ça donc je sais pas 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Le problème à la base c'est qu'on est loin d'avoir un fil d'ariane dans toutes les view, donc à mon avis ça n'avait pas grand intérêt de sortir un mixin pour ça.
Par contre l'héritage a de l'intérêt car plus on avance dans le process, plus on vérifie de choses (le demandeur, puis son mail, plus le statut de la demande, etc.) et on n'a pas envie de dupliquer la vérif. Côté aidants_connect_web on gérait ça avec des décorateurs et ça tournait bien je trouve, mais je ne sais pas si c'est possible avec des CBV ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

les CBV sont globalement un nids a mixin :)

C'est même parfois un reproche qui leur ait fait.

Copy link
Collaborator

Choose a reason for hiding this comment

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

par contre j'ai pas fini mon deuxieme café, je viens de voir que le sujet était les décorateurs !

c'est aussi possible sur les CBV, un peu plus compliqué que juste sur des fonctions, mais oui ça se fait

@property
def step(self) -> HabilitationFormStep:
raise NotImplementedError()
Expand Down Expand Up @@ -287,7 +283,7 @@ def get_form_kwargs(self):
return {**super().get_form_kwargs(), "instance": self.organisation}


class PersonnelRequestFormView(OnlyNewRequestsView, HabilitationStepMixin, FormView):
class PersonnelRequestFormView(OnlyNewRequestsView, FormView):
Copy link
Member Author

Choose a reason for hiding this comment

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

Et ici et sur la suivante, comme OnlyNewRequestsView hérite de HabilitationStepMixin, pas besoin de le rappeler ici

template_name = "personnel_form.html"
form_class = PersonnelForm

Expand Down Expand Up @@ -338,7 +334,7 @@ def get_success_url(self):
)


class ValidationRequestFormView(OnlyNewRequestsView, HabilitationStepMixin, FormView):
class ValidationRequestFormView(OnlyNewRequestsView, FormView):
template_name = "validation_form.html"
form_class = ValidationForm

Expand Down