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.

Sign up for GitHub

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

N21 1309 fix empty class sync #4443

Merged
merged 13 commits into from
Oct 6, 2023
Merged

N21 1309 fix empty class sync #4443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a8fbe10
fix empty ldapDns WIP
IgorCapCoder Sep 25, 2023
2f5f66c
unit tests WIP2
IgorCapCoder Sep 27, 2023
baa6a11
fix handling undefined uniqueMembers
IgorCapCoder Sep 27, 2023
80668de
also handle null
IgorCapCoder Sep 27, 2023
f34e949
also handle null
IgorCapCoder Sep 27, 2023
b3f85d9
Merge branch 'main' into N21-1309-fix-empty-class-sync
IgorCapCoder Sep 28, 2023
bd94198
request changes
IgorCapCoder Sep 28, 2023
14a9e43
logging WIP
IgorCapCoder Sep 28, 2023
9895685
handle empty, null and undefined lists
IgorCapCoder Sep 29, 2023
2f93e27
Merge branch 'main' into N21-1309-fix-empty-class-sync
IgorCapCoder Sep 29, 2023
66ea800
Merge branch 'main' into N21-1309-fix-empty-class-sync
IgorCapCoder Oct 4, 2023
eca8570
change scope of unique members assertion to fix current classes
IgorCapCoder Oct 4, 2023
1d4ed95
fix assertion
IgorCapCoder Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/services/sync/strategies/consumerActions/ClassAction.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should return here in case condition is not a valid DN to skip execute the extra update class with empty teacher and students

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually this is needed to fix current classes, which have been affected already

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);
Expand Down
72 changes: 49 additions & 23 deletions test/services/sync/repo/user.repo.integration.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
});
});
});
Expand Down
48 changes: 48 additions & 0 deletions test/services/sync/strategies/consumerActions/ClassAction.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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([]);
});
});
});