[FEATURE] Ajouter un nom interne pour les profils cibles (PIX-16685) #11581
+255
−137
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Une fois les applications déployées, elles seront accessibles via les liens suivants :
Les variables d'environnement seront accessibles via les liens suivants : |
La-toile-cosmique
changed the title
La-toile-cosmique
force-pushed
the
pix-16685-add-target-profile-internal-name
branch
from
4f0565c to
7996272
Compare
La-toile-cosmique
force-pushed
the
pix-16685-add-target-profile-internal-name
branch
from
7996272 to
da37807
Compare
La-toile-cosmique
force-pushed
the
pix-16685-add-target-profile-internal-name
branch
2 times, most recently
from
7a32994 to
6d430b2
Compare
| @@ -0,0 +1,38 @@ | |||
| const TABLE_NAME = 'target-profiles'; | |||
There was a problem hiding this comment.
thought: Il me semble que les captains préconisent de faire ce genre de migrations dans une PR a part, en cas de soucis ça serait plus simple de revert.
Egalement je sais pas si on veut pas split la création et la copie dans deux migrations séparée ? Peut être demander l'avis aux @1024pix/team-captains
There was a problem hiding this comment.
| @@ -506,6 +507,7 @@ const register = async function (server) { | |||
| description: Joi.string().allow(null).max(500), | |||
| 'image-url': Joi.string().uri().allow(null), | |||
| name: Joi.string(), | |||
| 'internal-name': Joi.string(), | |||
There was a problem hiding this comment.
suggestion: rajouter un required()
Comment on lines
741
to
+743
| const { name } = await knex('target-profiles').select('name').where('id', targetProfile.id).first(); | ||
| expect(name).to.equal(targetProfile.name); | ||
| expect(name).to.equal(targetProfile.name); |
There was a problem hiding this comment.
Suggested change
| const { name } = await knex('target-profiles').select('name').where('id', targetProfile.id).first(); | |
| expect(name).to.equal(targetProfile.name); | |
| expect(name).to.equal(targetProfile.name); | |
| const { name, internalName } = await knex('target-profiles').select('name').where('id', targetProfile.id).first(); | |
| expect(name).to.equal(targetProfile.name); expect(name).to.equal(targetProfile.name); | |
| expect(internalName).to.equal(targetProfile.internalName) |
Comment on lines
23
to
+24
| @attr('nullable-string') name; | ||
| @attr('nullable-string') internalName; |
There was a problem hiding this comment.
thought: C'est string tout court vu que c'est pas nullable 🤔
Alexandre-Monney
approved these changes
Mar 7, 2025
Alexandre-Monney
added
Tech Review OK
Func Review OK
La-toile-cosmique
force-pushed
the
pix-16685-add-target-profile-internal-name
branch
from
6d430b2 to
99a9123
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🥞 Problème
Il y a beaucoup de profils cibles et apparemment il devient complique de s'y retrouver.
🥓 Proposition
Ajouter un nom interne aux profil cibles uniquement visibles sur l'interface d'administration pour aider le tri.
🧃 Remarques
Il y a une copie de la colonne nom en nom interne dans la migration. Ainsi il est possible par la suite d'appliquer un contrainte "not null" sur le champ internalName. Cette migration est a surveiller a la mise en integration/prod.
Je ne sais pas si il existe d'autres moyens de créer des profils cibles que via l'administration ( script ? autre étrangeté ? )
Dans l'administration, le nom affiche au prescripteur apparaît seulement au détail du profil cible et n'est pas beaucoup mis en valeur.
😋 Pour tester
Cas de tests: