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

PASS IAE: création d’une page de détails [GEN-12] #4719

Merged
merged 6 commits into from
Oct 15, 2024

Conversation

xavfernandez
Copy link
Contributor

🤔 Pourquoi ?

Pour alléger un peu l'encart du PASS IAE

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?

🏝️ Comment tester

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

@xavfernandez xavfernandez added the ajouté Ajouté dans le changelog. label Sep 11, 2024
@xavfernandez xavfernandez self-assigned this Sep 11, 2024
Copy link

@xavfernandez xavfernandez added the 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC label Sep 11, 2024
@xavfernandez xavfernandez force-pushed the xfernandez/approval_details branch from f375063 to 15eb475 Compare September 11, 2024 10:57
Copy link

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

@xavfernandez xavfernandez force-pushed the xfernandez/approval_details branch 4 times, most recently from 9186d2a to d7ce214 Compare September 13, 2024 15:21
@xavfernandez xavfernandez force-pushed the xfernandez/approval_details branch 20 times, most recently from 97003e9 to 2042054 Compare September 25, 2024 09:50
@xavfernandez xavfernandez force-pushed the xfernandez/approval_details branch 9 times, most recently from 1f9b791 to f845d68 Compare October 4, 2024 09:59
@xavfernandez xavfernandez requested a review from tonial October 4, 2024 12:49
@xavfernandez xavfernandez force-pushed the xfernandez/approval_details branch 2 times, most recently from 5151000 to bbf886d Compare October 4, 2024 15:23
itou/templates/approvals/details.html Outdated Show resolved Hide resolved
itou/templates/approvals/details.html Outdated Show resolved Hide resolved
itou/templates/approvals/details.html Outdated Show resolved Hide resolved
itou/templates/approvals/details.html Outdated Show resolved Hide resolved
itou/templates/approvals/details.html Outdated Show resolved Hide resolved
itou/www/approvals_views/views.py Outdated Show resolved Hide resolved
Comment on lines 194 to 196
if application_states := self.object.user.job_applications.filter(
to_company=self.request.current_organization,
).values_list("state", flat=True):
if any(state == JobApplicationState.ACCEPTED for state in application_states):
is_employer_with_accepted_application = True
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if application_states := self.object.user.job_applications.filter(
to_company=self.request.current_organization,
).values_list("state", flat=True):
if any(state == JobApplicationState.ACCEPTED for state in application_states):
is_employer_with_accepted_application = True
if self.object.user.job_applications.filter(
to_company=self.request.current_organization, state=JobApplicationState.ACCEPTED
).exists():
is_employer_with_accepted_application = True

Copy link
Contributor Author

@xavfernandez xavfernandez Oct 14, 2024

Choose a reason for hiding this comment

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

Alors non car je veux différencier les cas d'un employeur:

  • ayant reçu une candidature pour ce candidat mais sans avoir accepté de candidature (droit d'accès mais sans voir les boutons)
  • ayant reçu une candidature pour ce candidat et ayant déjà accepté une candidature (droit d'accès et les boutons)

Mais je suis d'accord que le code est piégeux, je vais rajouter une commentaire.

Copy link
Contributor

Choose a reason for hiding this comment

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

Et même un test si possible 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

(j’avais vérifié la suggestion avec la suite de test et de mémoire tout était passé)

Copy link
Contributor Author

@xavfernandez xavfernandez Oct 15, 2024

Choose a reason for hiding this comment

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

👍 Test ajouté dans fe5d2b2

itou/www/approvals_views/views.py Outdated Show resolved Hide resolved
itou/templates/approvals/details.html Outdated Show resolved Hide resolved
tests/www/approvals_views/test_detail.py Show resolved Hide resolved
@xavfernandez xavfernandez force-pushed the xfernandez/approval_details branch 2 times, most recently from d42037a to b1c33ae Compare October 14, 2024 10:00
@xavfernandez xavfernandez force-pushed the xfernandez/approval_details branch 2 times, most recently from abb3dd2 to fe5d2b2 Compare October 15, 2024 12:48
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.

Très propre et bien organisé, un plaisir 💯

Comment on lines +207 to +210
else:
# test_func should prevent this case from happening but let's be safe
logger.exception("This should never happen")
raise PermissionDenied
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Je trouve que test_func complexifie plus qu’il n’aide. On pourrait virer le mixin et commencer par : if request.user.kind not in (EMPLOYER, PRESCRIBER): raise PermissionDenied.

Si on ne veut pas faire les tests au moment de get_context_data, on pourrait faire le check dans le dispatch avec un check sur request.user.is_authenticated 🤷

tests/www/apply/test_process.py Outdated Show resolved Hide resolved
tests/www/apply/test_process.py Outdated Show resolved Hide resolved
from tests.utils.test import assert_previous_step, assertSnapshotQueries, parse_response_to_soup


class TestEmployeeDetailView:
APPROVAL_NUMBER_LABEL = "Numéro de PASS IAE"
APPROVAL_NUMBER_LABEL = "Numéro de PASS IAE"
Copy link
Contributor

Choose a reason for hiding this comment

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

On devrait garder l’espace insécable, PASS IAE va ensemble.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oui, mais le nouveau template itou/templates/approvals/includes/box.html ne le contient pas 😬
Il faudrait soit un commit préparatoire (mais je trouve 115 occurences de PASS IAE sans espace insécable dans les templates 👀 ) soit un commit en suivant: je laisse la seconde option ouverte 👼

@xavfernandez xavfernandez force-pushed the xfernandez/approval_details branch from fe5d2b2 to 4056357 Compare October 15, 2024 14:10
@xavfernandez xavfernandez added this pull request to the merge queue Oct 15, 2024
Merged via the queue into master with commit df1aa2f Oct 15, 2024
11 checks passed
@xavfernandez xavfernandez deleted the xfernandez/approval_details branch October 15, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC ajouté Ajouté dans le changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants