N21 1309 fix empty class sync (#4443)

* handle empty, null and undefined lists * change scope of unique members assertion to fix current classes
hpi-schul-cloud · Oct 6, 2023 · 3923edf · 3923edf
1 parent 13f4d7a
commit 3923edf
Show file tree

Hide file tree

Showing 3 changed files with 105 additions and 29 deletions.
diff --git a/src/services/sync/strategies/consumerActions/ClassAction.js b/src/services/sync/strategies/consumerActions/ClassAction.js
@@ -90,14 +90,16 @@ class ClassAction extends BaseConsumerAction {
 		const teachers = [];
 		const ldapDns = !Array.isArray(uniqueMembers) ? [uniqueMembers] : uniqueMembers;
 
-		const users = await UserRepo.findByLdapDnsAndSchool(ldapDns, schoolId);
+		if (ldapDns[0]) {
+			const users = await UserRepo.findByLdapDnsAndSchool(ldapDns, schoolId);
 
-		users.forEach((user) => {
-			user.roles.forEach((role) => {
-				if (role.name === 'student') students.push(user._id);
-				if (role.name === 'teacher') teachers.push(user._id);
+			users.forEach((user) => {
+				user.roles.forEach((role) => {
+					if (role.name === 'student') students.push(user._id);
+					if (role.name === 'teacher') teachers.push(user._id);
+				});
 			});
-		});
+		}
 
 		await ClassRepo.updateClassStudents(classId, students);
 		await ClassRepo.updateClassTeachers(classId, teachers);

diff --git a/test/services/sync/repo/user.repo.integration.test.js b/test/services/sync/repo/user.repo.integration.test.js
@@ -188,45 +188,71 @@ describe('user repo', () => {
 	});
 
 	describe('findByLdapDnsAndSchool', () => {
-		it('should return empty list if not found', async () => {
-			const testSchool = await testObjects.createTestSchool();
-			const res = await UserRepo.findByLdapDnsAndSchool('Not existed dn', testSchool._id);
+		const setup = async () => {
+			const ldapDn = 'TEST_LDAP_DN';
+			const ldapDn2 = 'TEST_LDAP_DN2';
+			const previousLdapDn = 'PREVIOUS_LDAP_DN';
+			const notExistingLdapDn = 'NOT_EXISTING_LDAP_DN';
+			const ldapDns = [ldapDn, ldapDn2];
+
+			const school = await testObjects.createTestSchool();
+
+			const migratedUser = await testObjects.createTestUser({
+				previousExternalId: previousLdapDn,
+				schoolId: school._id,
+				ldapDn: 'NEW_ID',
+			});
+			const createdUsers = [
+				await testObjects.createTestUser({ ldapDn, schoolId: school._id }),
+				await testObjects.createTestUser({ ldapDn2, schoolId: school._id }),
+			];
+
+			return {
+				ldapDns,
+				notExistingLdapDn,
+				previousLdapDn,
+				migratedUser,
+				createdUsers,
+				school,
+			};
+		};
+
+		it('should return empty list if user with ldapDn does not exist', async () => {
+			const { school, notExistingLdapDn } = await setup();
+
+			const res = await UserRepo.findByLdapDnsAndSchool([notExistingLdapDn], school._id);
+
+			expect(res).to.eql([]);
+		});
+
+		it('should return empty list if ldapDns are empty', async () => {
+			const { school } = await setup();
+
+			const res = await UserRepo.findByLdapDnsAndSchool([], school._id);
+
 			expect(res).to.eql([]);
 		});
 
 		it('should find user by ldap dn and school', async () => {
-			const ldapDns = ['TEST_LDAP_DN', 'TEST_LDAP_DN2'];
-			const school = await testObjects.createTestSchool();
-			const createdUsers = await Promise.all(
-				ldapDns.map((ldapDn) => testObjects.createTestUser({ ldapDn, schoolId: school._id }))
-			);
+			const { school, ldapDns, createdUsers } = await setup();
+
 			const res = await UserRepo.findByLdapDnsAndSchool(ldapDns, school._id);
+
 			const user1 = res.filter((user) => createdUsers[0]._id.toString() === user._id.toString());
 			const user2 = res.filter((user) => createdUsers[1]._id.toString() === user._id.toString());
+
 			expect(user1).not.to.be.undefined;
 			expect(user2).not.to.be.undefined;
 		});
 
 		describe('when the user has migrated', () => {
-			const setup = async () => {
-				const ldapDn = 'TEST_LDAP_DN';
-				const school = await testObjects.createTestSchool();
-				const user = await testObjects.createTestUser({ previousExternalId: ldapDn, schoolId: school._id });
-
-				return {
-					ldapDn,
-					user,
-					school,
-				};
-			};
-
 			it('should find the user by its old ldap dn and school', async () => {
-				const { ldapDn, school, user } = await setup();
+				const { previousLdapDn, school, migratedUser } = await setup();
 
-				const res = await UserRepo.findByLdapDnsAndSchool([ldapDn], school._id);
+				const res = await UserRepo.findByLdapDnsAndSchool([previousLdapDn], school._id);
 
 				expect(res.length).to.equal(1);
-				expect(res[0]._id.toString()).to.equal(user._id.toString());
+				expect(res[0]._id.toString()).to.equal(migratedUser._id.toString());
 			});
 		});
 	});

diff --git a/test/services/sync/strategies/consumerActions/ClassAction.test.js b/test/services/sync/strategies/consumerActions/ClassAction.test.js
@@ -342,5 +342,53 @@ describe('Class Actions', () => {
 			expect(updateClassTeachersStub.getCall(0).firstArg.toString()).to.be.equal(mockClass._id.toString());
 			expect(updateClassTeachersStub.getCall(0).lastArg).to.eql(['user2', 'user3']);
 		});
+
+		it('should not add any user to the class, when uniqueMembers are []', async () => {
+			const uniqueMembers = [];
+			const schoolObj = { _id: new ObjectId(), currentYear: new ObjectId() };
+			const findByLdapDnsAndSchoolStub = sinon.stub(UserRepo, 'findByLdapDnsAndSchool');
+
+			await classAction.addUsersToClass(schoolObj._id, mockClass._id, uniqueMembers);
+
+			expect(findByLdapDnsAndSchoolStub.notCalled).to.be.true;
+
+			expect(updateClassStudentsStub.getCall(0).firstArg.toString()).to.be.equal(mockClass._id.toString());
+			expect(updateClassStudentsStub.getCall(0).lastArg).to.eql([]);
+
+			expect(updateClassTeachersStub.getCall(0).firstArg.toString()).to.be.equal(mockClass._id.toString());
+			expect(updateClassTeachersStub.getCall(0).lastArg).to.eql([]);
+		});
+
+		it('should not add any user to the class, when uniqueMembers are [undefined]', async () => {
+			const uniqueMembers = [undefined];
+			const schoolObj = { _id: new ObjectId(), currentYear: new ObjectId() };
+			const findByLdapDnsAndSchoolStub = sinon.stub(UserRepo, 'findByLdapDnsAndSchool');
+
+			await classAction.addUsersToClass(schoolObj._id, mockClass._id, uniqueMembers);
+
+			expect(findByLdapDnsAndSchoolStub.notCalled).to.be.true;
+
+			expect(updateClassStudentsStub.getCall(0).firstArg.toString()).to.be.equal(mockClass._id.toString());
+			expect(updateClassStudentsStub.getCall(0).lastArg).to.eql([]);
+
+			expect(updateClassTeachersStub.getCall(0).firstArg.toString()).to.be.equal(mockClass._id.toString());
+			expect(updateClassTeachersStub.getCall(0).lastArg).to.eql([]);
+		});
+
+		it('should not add any user to the class, when uniqueMembers are [null]', async () => {
+			const uniqueMembers = [null];
+			const schoolObj = { _id: new ObjectId(), currentYear: new ObjectId() };
+			const findByLdapDnsAndSchoolStub = sinon.stub(UserRepo, 'findByLdapDnsAndSchool');
+
+			await classAction.addUsersToClass(schoolObj._id, mockClass._id, uniqueMembers);
+
+			expect(findByLdapDnsAndSchoolStub.notCalled).to.be.true;
+
+			expect(updateClassStudentsStub.getCall(0).firstArg.toString()).to.be.equal(mockClass._id.toString());
+			expect(updateClassStudentsStub.getCall(0).lastArg).to.eql([]);
+
+			expect(updateClassTeachersStub.getCall(0).firstArg.toString()).to.be.equal(mockClass._id.toString());
+			expect(updateClassTeachersStub.getCall(0).lastArg).to.eql([]);
+		});
 	});
 });