[FEATURE] Générer en masse les identifiants des élèves (PIX-12975). #9832
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 : |
mariannebost
force-pushed
the
pix-12975-generate-in-mass-sco-ids
branch
4 times, most recently
from
8ad97c1
to
33995f7
Compare
mariannebost
force-pushed
the
pix-12975-generate-in-mass-sco-ids
branch
2 times, most recently
from
e263dda
to
a15273e
Compare
er-lim
reviewed
Aug 28, 2024
| @@ -29,6 +32,18 @@ const authenticationDomainErrorMappingConfiguration = [ | |||
| name: MissingUserAccountError.name, | |||
| httpErrorFn: (error) => new HttpErrors.BadRequestError(error.message), | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
A t'on vraiment besoin de toutes ces erreurs spécifiques ?
Comment on lines
+8
to
+10
| OrganizationLearnerDoesAlreadyHaveAnUsernameError, | ||
| OrganizationLearnerDoesNotBelongToOrganizationError, | ||
| OrganizationLearnerDoesNotHaveAPixAccountError, |
There was a problem hiding this comment.
Ces erreurs doivent-elles être dans ce scope ou bien côté prescription ?
| @@ -26,6 +26,30 @@ class MissingUserAccountError extends DomainError { | |||
| } | |||
| } | |||
|
|
|||
| class OrganizationLearnerDoesAlreadyHaveAnUsernameError extends DomainError { | |||
| constructor(message = "L'élève a déjà un identitfiant.", code = 'ORGANIZATION_LEARNER_DOES_ALREADY_HAVE_A_USERNAME') { | |||
There was a problem hiding this comment.
Plutôt réécrire l'erreur en anglais dans le champ message : https://github.com/1024pix/pix/blob/dev/docs/adr/0044-gestion-erreurs-i18n-reference.md
| } | ||
| } | ||
|
|
||
| function _checkIfUserAccountsHaveUsername(userAccounts) { |
There was a problem hiding this comment.
Suggested change
| function _checkIfUserAccountsHaveUsername(userAccounts) { | |
| function _assertUserAccountsHaveNoUsername(userAccounts) { |
| } | ||
| } | ||
|
|
||
| function _checkIfOrganizationLearnersHaveAPixAccount(organizationLearnerUserIds) { |
There was a problem hiding this comment.
Suggested change
| function _checkIfOrganizationLearnersHaveAPixAccount(organizationLearnerUserIds) { | |
| function _assertOrganizationLearnersHaveAPixAccount(organizationLearnerUserIds) { |
Plutôt utiliser le terme assert ici car on throw une exception.
| return updatedOrganizationLearners; | ||
| }; | ||
|
|
||
| function _checkIfOrganizationLearnersBelongsToOrganization(organizationLearners, organizationId) { |
There was a problem hiding this comment.
Suggested change
| function _checkIfOrganizationLearnersBelongsToOrganization(organizationLearners, organizationId) { | |
| function _assertOrganizationLearnersBelongsToOrganization(organizationLearners, organizationId) { |
| lastName: organizationLearner.lastName, | ||
| firstName: organizationLearner.firstName, | ||
| username, | ||
| password: '', |
There was a problem hiding this comment.
Est-ce nécessaire de passer un mot de passe vide à cet endroit ?
Comment on lines
+40
to
+69
| if (hasStudentAccountAnIdentityProviderPIX) { | ||
| await userRepository.updateUsername({ id: organizationLearnerUserAccount.id, username }); | ||
| updatedOrganizationLearners.push( | ||
| new OrganizationLearnerPasswordResetDTO({ | ||
| division: organizationLearner.division, | ||
| lastName: organizationLearner.lastName, | ||
| firstName: organizationLearner.firstName, | ||
| username, | ||
| password: '', | ||
| }), | ||
| ); | ||
| } else { | ||
| const generatedPassword = passwordGeneratorService.generateSimplePassword(); | ||
| const hashedPassword = await cryptoService.hashPassword(generatedPassword); | ||
|
|
||
| await userService.updateUsernameAndAddPassword({ | ||
| userId: organizationLearnerUserAccount.id, | ||
| username, | ||
| hashedPassword, | ||
| authenticationMethodRepository, | ||
| userRepository, | ||
| }); | ||
| updatedOrganizationLearners.push( | ||
| new OrganizationLearnerPasswordResetDTO({ | ||
| division: organizationLearner.division, | ||
| lastName: organizationLearner.lastName, | ||
| firstName: organizationLearner.firstName, | ||
| username, | ||
| password: generatedPassword, | ||
| }), |
There was a problem hiding this comment.
Se poser la question d'un éventuel refacto à la fin de la mob review.
Comment on lines
+95
to
+134
| const message = 'Test message error'; | ||
| const code = 'CODE_ERROR'; | ||
|
|
||
| //when | ||
| const error = httpErrorMapper.httpErrorFn(new OrganizationLearnerDoesAlreadyHaveAnUsernameError(message, code)); | ||
|
|
||
| //then | ||
| expect(error).to.be.instanceOf(HttpErrors.BadRequestError); | ||
| expect(error.message).to.equal(message); | ||
| expect(error.code).to.equal(code); | ||
| }); | ||
| }); | ||
|
|
||
| context('when mapping "OrganizationLearnerDoesNotBelongToOrganizationError"', function () { | ||
| it('returns an BadRequestError Http Error', function () { | ||
| //given | ||
| const httpErrorMapper = authenticationDomainErrorMappingConfiguration.find( | ||
| (httpErrorMapper) => httpErrorMapper.name === OrganizationLearnerDoesNotBelongToOrganizationError.name, | ||
| ); | ||
| const message = 'Test message error'; | ||
| const code = 'CODE_ERROR'; | ||
|
|
||
| //when | ||
| const error = httpErrorMapper.httpErrorFn(new OrganizationLearnerDoesNotBelongToOrganizationError(message, code)); | ||
|
|
||
| //then | ||
| expect(error).to.be.instanceOf(HttpErrors.UnauthorizedError); | ||
| expect(error.message).to.equal(message); | ||
| expect(error.code).to.equal(code); | ||
| }); | ||
| }); | ||
|
|
||
| context('when mapping "OrganizationLearnerDoesNotHaveAPixAccountError"', function () { | ||
| it('returns an BadRequestError Http Error', function () { | ||
| //given | ||
| const httpErrorMapper = authenticationDomainErrorMappingConfiguration.find( | ||
| (httpErrorMapper) => httpErrorMapper.name === OrganizationLearnerDoesNotHaveAPixAccountError.name, | ||
| ); | ||
| const message = 'Test message error'; | ||
| const code = 'CODE_ERROR'; |
There was a problem hiding this comment.
Pouvoir modifier le code de ces erreurs n'est pas une utilisation légitime cf. https://github.com/1024pix/pix/blob/dev/docs/adr/0044-gestion-erreurs-i18n-reference.md
Aussi il faudrait :
- ne pas permettre de modifier le code de ces erreurs dans le constructeur
- ne pas montrer des exemples qui feraient ça dans les tests
er-lim
reviewed
Aug 28, 2024
| import { OrganizationLearnerPasswordResetDTO } from '../../../../../src/shared/domain/models/OrganizationLearnerPasswordResetDTO.js'; | ||
| import { catchErr, expect, sinon } from '../../../../test-helper.js'; | ||
|
|
||
| describe('Unit | Identity Access Management | Domain | UseCase | Generate Usernames', function () { |
There was a problem hiding this comment.
Recommandation : réécrire le test en intégration pour une meilleure lisibilité et maintenabilité
…r generateOrganizationLearnersUsername
EmmanuelleBonnemay
force-pushed
the
pix-12975-generate-in-mass-sco-ids
branch
from
a15273e
to
3958fa3
Compare
|
Fonctionnalité gérée sur une autre PR : #10096 |
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
Beaucoup de demandes de la part des utilisateurs pour générer en masse des identifiants pour les élèves SCO (sous conditions). Actuellement, les professeurs doivent se rendre dans la pop-up de gestion des élèves et générer l’identifiant un par un.
🤖 Proposition
A l’instar de la fonctionnalité pour gérer des mots de passe en masse présente pour les orgas SCO (sous conditions), on veut pouvoir séléctionner les utilisateurs avec les checkboxs présentes et générer des identifiants en masse. La fonctionnalité de génération d’un identifiant à l’unité est déjà présente côté Pix Orga.
A la première sélection d’un élève → bandeau s’affiche en bas proposant les 2 fonctionnalités
Conditions à respecter pour générer un identifiant :
🌈 Remarques
Création du dossier sco-organization-learners dans le dossier src/IAM, est ce le meilleur endroit ?
TODO
💯 Pour tester
Différences entre les fonctionnalités de génération d'identifiant unitaire vs génération d'identifiants en masse :