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

Feat/clin 2095 #35

Merged
merged 14 commits into from
Aug 7, 2023
Merged

Feat/clin 2095 #35

merged 14 commits into from
Aug 7, 2023

Conversation

ethienneroy
Copy link
Contributor

No description provided.

isUUID: 4,
},
},
title: {
Copy link
Contributor

Choose a reason for hiding this comment

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

sommes-nous sûr qu'il n'y a pas de titre funky avec des tirets "-" ou autres caractères du genre?

},
keycloak_id: {
type: DataTypes.STRING,
allowNull: false,
validate: {
isUUID: 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

Magic number.
const VERSION_4 = 4 ou const UUID_VERSION = 4 ou ...
ou regarder dans la lib sequilize si il existe une constante que tu pourrais importer au lieu de t'en créer une

"keycloak-connect": "^15.0.2",
"node-pg-migrate": "^6.0.0",
"pg": "^8.7.1",
"sequelize": "^6.14.1",
"sequelize": "^6.28.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Comment on lines 82 to 89
len: [1, 20],
isAlpha: true,
},
},
last_name: {
type: DataTypes.CITEXT,
validate: {
len: [1, 20],
Copy link
Contributor

Choose a reason for hiding this comment

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

go with MAX=70

Comment on lines 96 to 108
isAlpha: true,
},
},
nih_ned_id: {
type: DataTypes.STRING,
validate: {
isAlpha: true,
},
},
commercial_use_reason: {
type: DataTypes.STRING,
validate: {
isAlpha: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

alphanumeric

Copy link
Contributor

Choose a reason for hiding this comment

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

Je pense qu'on devrait ajouter plus de validation de longeur, mais on pourra le faire dans un 2ième temps. Faut s'assurer que dans les BDs actuelles, on mette pas des contraintes qui ne soient pas compatibles avec le contenu de certains champs

Comment on lines 38 to 45
isUUID: 4,
},
},
keycloak_id: {
type: DataTypes.STRING,
allowNull: false,
validate: {
isUUID: 4,
Copy link
Contributor

Choose a reason for hiding this comment

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

même commentaire que plus haut

name: 'Invalid data',
errors: err.errors.map((error) => error.message.replace('%s', error.path)),
};
res.status(422).json({
Copy link
Contributor

Choose a reason for hiding this comment

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

  • magic number
  • tu peux le laisser sur un 422, mais pas sûr que ce soit nécéssairement le bon code...

Copy link
Contributor

@atoulous atoulous left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a custom validator for roles? making sure every role is alphanumerical only

@ethienneroy ethienneroy merged commit e71c8b1 into main Aug 7, 2023
1 check passed
@ethienneroy ethienneroy deleted the feat/clin-2095 branch August 7, 2023 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants