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

[FEATURE] Permettre de rattacher un profil cible à une certification complementaire (PIX-12404) #8945

Merged
merged 3 commits into from
May 22, 2024

Conversation

aceol
Copy link
Contributor

@aceol aceol commented May 16, 2024

🦄 Problème

On peut modifier le profil cible lié à une certification complémentaire mais ce n'est pas possible de faire l'ajout via l'interface

🤖 Proposition

Permettre de lier un profil cible à une nouvelle certification complementaire

🌈 Remarques

💯 Pour tester

  • Ajouter une certification complémentaire Pix Pro santé

INSERT INTO "complementary-certifications" ("label", "minimumReproducibilityRate", "key", "hasComplementaryReferential", "hasExternalJury", "certificationExtraTime", "minimumReproducibilityRateLowerLevel") values ('Pix+ Santé', 70, 'PRO_SANTE', true, false, 45, 60);

  • Ajouter un profil cible pour Pix+ pro santé avec badge certifiant
  • Mettre à jour le profil cible pour la certification complémentaire crée plus haut

@aceol aceol self-assigned this May 16, 2024
@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@aceol aceol force-pushed the pix-12404-add-script-attach-badges branch 2 times, most recently from ab053b5 to 160237d Compare May 16, 2024 16:38
@aceol aceol marked this pull request as ready for review May 16, 2024 16:41
@aceol aceol force-pushed the pix-12404-add-script-attach-badges branch from 160237d to 7e368c2 Compare May 17, 2024 07:46
@P-Jeremy
Copy link
Contributor

P-Jeremy commented May 21, 2024

Test func KO

Test func OK en mob 💚

image

@P-Jeremy P-Jeremy closed this May 21, 2024
@P-Jeremy P-Jeremy reopened this May 21, 2024
@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@aceol aceol added Func Review OK PO validated functionally the PR and removed 👀 Func Review Needed labels May 21, 2024
@aceol aceol requested a review from P-Jeremy May 21, 2024 13:32
@aceol aceol force-pushed the pix-12404-add-script-attach-badges branch 2 times, most recently from 25896ef to 0af419a Compare May 21, 2024 13:37
@@ -17,7 +17,7 @@ const attachTargetProfile = async function (request, h, dependencies = { complem
complementaryCertificationBadgesToAttachDTO: complementaryCertificationBadges,
});

if (notifyOrganizations) {
if (!!targetProfileId && notifyOrganizations) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pas obligé de caster en boolean avec !! ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

!! cast en Boolean, mais tu as raison ca equivaut à Boolean(targetProfileId)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha non je voulais dire on peut faire ça direct ?

Suggested change
if (!!targetProfileId && notifyOrganizations) {
if (targetProfileId && notifyOrganizations) {

Comment on lines +52 to +53
});
context('when there is no target profile to remove', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Petit saut de ligne pour aérer 😬 (fais moi savoir si ce genre de retour "relou" je peux le traiter moi même pour gagner du temps). Peut être le mieux serait d'avoir une règle eslint pour gérer ça automatiquement 🤷‍♂️

Copy link
Contributor Author

@aceol aceol May 21, 2024

Choose a reason for hiding this comment

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

Le top serait de faire la suggestion, comme ca ont peut les prendre en compte via github (et un petit rebase)

Suggested change
});
context('when there is no target profile to remove', function () {
});
context('when there is no target profile to remove', function () {

this.route('attach-target-profile', { path: '/attach-target-profile/:target_profile_id' });
this.route('attach-target-profile', function () {
this.route('update', { path: '/:target_profile_id' });
this.route('new');
Copy link
Contributor

Choose a reason for hiding this comment

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

J'ai du mal à comprendre la chorégraphie ici. Il n'y aurait pas possibilité d'utiliser le même template, plutôt que de créer deux fois les même pour un id null ou défini ?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Si tu sais comment faire je suis preneur!

@@ -103,6 +103,7 @@ export default class AttachTargetProfileController extends Controller {
`Profil cible rattaché à la certification ${complementaryCertification.label} mis à jour avec succès !`,
);
} catch (error) {
console.error({ error });
Copy link
Contributor

Choose a reason for hiding this comment

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

Un oubli ?

Suggested change
console.error({ error });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Etonnant que ca pete pas le build. Apres je trouve ca quand meme cool d'avoir les logs quand ca pete (mais bon c'est pas necessaire en prod)

@pix-service-auto-merge pix-service-auto-merge merged commit 9a97cea into dev May 22, 2024
6 of 7 checks passed
@pix-service-auto-merge pix-service-auto-merge deleted the pix-12404-add-script-attach-badges branch May 22, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants