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

Candidature : N'utiliser que les diagnostiques considérés comme valides pour la SIAE #4229

Conversation

rsebille
Copy link
Contributor

Note

  • Je pense que les tests ne devraient pas être sur les vues mais directement le modèle afin de ne pas avoir à tout dupliquer à chaque fois.
  • Il y a sûrement une mise en commun à faire au niveau du setup des tests.
  • Est-ce que la logique même de with_jobseeker_eligibility_diagnosis() est toujours bonne et donc doit-on la garder ? Car ça me semble bizarre (et capillotracté) d'afficher une info alors que celle-ci n'est pas présente sur la candidature.

🤔 Pourquoi ?

Suite à un retour de recette pour #4197.
Ne pas afficher un diagnostique considéré comme non-valide, typiquement un diagnostique fait par la SIAE A alors que la candidature est pour la SIAE B.

🏝️ Comment tester

Avec les tests

@rsebille rsebille self-assigned this Jun 14, 2024
@@ -223,8 +223,8 @@ def with_jobseeker_eligibility_diagnosis(self):
"""
sub_query = Subquery(
(
EligibilityDiagnosis.objects.filter(job_seeker=OuterRef("job_seeker"))
.order_by("-created_at")
EligibilityDiagnosis.objects.valid()
Copy link
Contributor

Choose a reason for hiding this comment

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

Je vais changer légèrement cette logique pour https://www.notion.so/plateforme-inclusion/Investigation-Cacher-les-diags-SIAE-aux-prescripteurs-lors-des-candidatures-3227290de5b248759a5fe43079868292

On va avoir un viewing_user qui va décider de la visibilité d’un diag pour l’utilisateur courant. Mais a priori, je mettrais juste à jour cette section pour passer le viewing_user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'était ma supposition, d'où ton ajout à la PR pour être sûr que ça allais plus ou moins dans le même sens ;).

EligibilityDiagnosis.objects.filter(job_seeker=OuterRef("job_seeker"))
.order_by("-created_at")
EligibilityDiagnosis.objects.valid()
.for_job_seeker_and_siae(job_seeker=OuterRef("job_seeker"), siae=OuterRef("to_company"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Je me demande si on ne voudrait pas prendre le diagnostique du prescripteur en premier, comme dans EligibilityDiagnosis.objects.last_considered_valid() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sans doute, mais alors faut aussi modifier le .eligibility_validated() juste en dessous, ou mettre le tri directement dans .for_job_seeker_and_siae() ?

tests/eligibility/factories.py Outdated Show resolved Hide resolved
tests/eligibility/factories.py Outdated Show resolved Hide resolved
tests/eligibility/factories.py Outdated Show resolved Hide resolved
tests/www/apply/test_list_for_siae.py Outdated Show resolved Hide resolved
tests/www/apply/test_list_for_siae.py Outdated Show resolved Hide resolved
Copy link
Contributor

@francoisfreitag francoisfreitag left a comment

Choose a reason for hiding this comment

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

Commit “tests: Refactor eligibility diagnosis factories” : ça vaudrait le coup d’expliquer ce qui a changé (dans les grandes lignes) dans le corps du commit.

@rsebille rsebille force-pushed the rsebille/limit-eligibility-diagnosis-visibility-for-job-applications-list branch from a916514 to fc26640 Compare June 17, 2024 13:53
@rsebille rsebille force-pushed the rsebille/limit-eligibility-diagnosis-visibility-for-job-applications-list branch from fc26640 to b2952c2 Compare June 17, 2024 14:30
Copy link
Contributor Author

@rsebille rsebille left a comment

Choose a reason for hiding this comment

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

Commit “tests: Refactor eligibility diagnosis factories” : ça vaudrait le coup d’expliquer ce qui a changé (dans les grandes lignes) dans le corps du commit.

C'est fait.

@@ -223,8 +223,8 @@ def with_jobseeker_eligibility_diagnosis(self):
"""
sub_query = Subquery(
(
EligibilityDiagnosis.objects.filter(job_seeker=OuterRef("job_seeker"))
.order_by("-created_at")
EligibilityDiagnosis.objects.valid()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'était ma supposition, d'où ton ajout à la PR pour être sûr que ça allais plus ou moins dans le même sens ;).

EligibilityDiagnosis.objects.filter(job_seeker=OuterRef("job_seeker"))
.order_by("-created_at")
EligibilityDiagnosis.objects.valid()
.for_job_seeker_and_siae(job_seeker=OuterRef("job_seeker"), siae=OuterRef("to_company"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sans doute, mais alors faut aussi modifier le .eligibility_validated() juste en dessous, ou mettre le tri directement dans .for_job_seeker_and_siae() ?

tests/eligibility/factories.py Outdated Show resolved Hide resolved
tests/eligibility/factories.py Outdated Show resolved Hide resolved
tests/eligibility/factories.py Outdated Show resolved Hide resolved
tests/www/apply/test_list_for_siae.py Outdated Show resolved Hide resolved
tests/www/apply/test_list_for_siae.py Outdated Show resolved Hide resolved
Copy link
Contributor

@xavfernandez xavfernandez left a comment

Choose a reason for hiding this comment

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

Pourquoi c'est en draft ? Ça me semble déjà bien. Et les 2 premiers commits de nettoyage pourraient être mergés 👍

@rsebille rsebille force-pushed the rsebille/limit-eligibility-diagnosis-visibility-for-job-applications-list branch from b2952c2 to 53c2fe8 Compare June 24, 2024 14:26
@rsebille rsebille marked this pull request as ready for review June 24, 2024 15:30
@rsebille rsebille force-pushed the rsebille/limit-eligibility-diagnosis-visibility-for-job-applications-list branch from 9455392 to aa2dec6 Compare June 24, 2024 17:27
@rsebille
Copy link
Contributor Author

Normalement on est tout bon !

Copy link
Contributor

Choose a reason for hiding this comment

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

Il n'y a pas les snapshots de test_list_for_prescriber à supprimer à tout hasard ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai le cerveau cramé... Ouais je comprend pas pourquoi --snapshot-update le supprime pas 🤔.

- Create an abstract factory to follow the abstract model logic
- Create and use a prefixed factory for IAE diagnosis
- Replace behavioral factories (`Expired`, `MadeBySiae`) with traits
- Delete the unused `AdministrativeCriteriaFactory()`
@rsebille rsebille force-pushed the rsebille/limit-eligibility-diagnosis-visibility-for-job-applications-list branch from aa2dec6 to d0a766f Compare June 25, 2024 16:27
@rsebille rsebille added this pull request to the merge queue Jun 25, 2024
Merged via the queue into master with commit ea7bd8c Jun 25, 2024
11 checks passed
@rsebille rsebille deleted the rsebille/limit-eligibility-diagnosis-visibility-for-job-applications-list branch June 25, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants