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

Add more tests for instructor permissions to fetch ddahs #599

Merged
merged 17 commits into from
Jun 20, 2021
Merged
5 changes: 3 additions & 2 deletions frontend/src/tests/ddah-tests.js
Original file line number Diff line number Diff line change
@@ -87,8 +87,9 @@ export function ddahTests(api) {
it("get all ddahs associated with a session", async () => {
let resp = await apiGET(`/admin/sessions/${session.id}/ddahs`);
expect(resp).toHaveStatus("success");
// Originally only one ddah is seeded
expect(resp.payload.length).toEqual(1);
// Originally two ddahs are seeded and
// another one is added in this test suite
expect(resp.payload.length).toEqual(3);
expect(resp.payload).toContainObject(ddah);
});

352 changes: 254 additions & 98 deletions frontend/src/tests/instructor-permission-test.js
Original file line number Diff line number Diff line change
@@ -26,7 +26,7 @@ export function instructorsPermissionTests(api) {
* @returns {Promise<void>}
*/
async function switchToInstructorOnlyUser() {
let respSwitchToInstOnlyUser = await apiPOST(
const respSwitchToInstOnlyUser = await apiPOST(
`/debug/active_user`,
instructorUser
);
@@ -39,62 +39,13 @@ export function instructorsPermissionTests(api) {
* @returns {Promise<void>}
*/
async function restoreDefaultUser() {
let respSwitchBackUser = await apiPOST(
const respSwitchBackUser = await apiPOST(
`/debug/active_user`,
defaultUser
);
expect(respSwitchBackUser).toHaveStatus("success");
}

/**
* Updates a position to include instructor with `instructorId` and
* creates a DDAH for one of the assignments realted to that
* instructor.
*
* @param instructorId: int - the unique Id of an instructor
*
* @returns {Promise<Ddah>}
*/
async function createDdahWithFixedDuties(instructorId) {
// We first need to update position to include our instructor
const existingPosition = databaseSeeder.seededData.positions[0];
const positionWithInstructor = {
...existingPosition,
instructor_ids: [...existingPosition.instructor_ids, instructorId],
};
let resp = await apiPOST(`/admin/positions`, positionWithInstructor);
expect(resp).toHaveStatus("success");

// We then proceed to create a DDAH for that position
resp = await apiGET(`/instructor/sessions/${session.id}/assignments`);
expect(resp).toHaveStatus("success");
expect(resp.payload.length).toBeGreaterThan(0);
const newDdah = {
assignment_id: resp.payload[0].id,
duties: [
{
order: 2,
hours: 25,
description: "marking:Marking the midterm",
},
{
order: 1,
hours: 4,
description: "training:Initial training",
},
{
order: 3,
hours: 40,
description: "contact:Running tutorials",
},
],
};

resp = await apiPOST(`/admin/ddahs`, newDdah);
expect(resp).toHaveStatus("success");
return resp.payload;
}

beforeAll(async () => {
await databaseSeeder.seed(api);
await databaseSeeder.seedForInstructors(api);
@@ -116,7 +67,7 @@ export function instructorsPermissionTests(api) {
existingContractTemplateId = resp.payload[0].id;

const instructorOnlyUserData = {
utorid: "instructor_only_test_user_utorid",
...databaseSeeder.seededData.instructors[3],
roles: ["instructor"],
};

@@ -238,10 +189,10 @@ export function instructorsPermissionTests(api) {

it("can't update instructors except for self (i.e. active user)", async () => {
// we use the default user's admin privilege here to fetch an unrelated instructor
let respFetchInst = await apiGET(`/instructor/instructors`);
const respFetchInst = await apiGET(`/instructor/instructors`);
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
expect(respFetchInst).toHaveStatus("success");

let respFetchDefaultUser = await apiGET(`/instructor/active_user`);
const respFetchDefaultUser = await apiGET(`/instructor/active_user`);
expect(respFetchDefaultUser).toHaveStatus("success");
const defaultUser = respFetchDefaultUser.payload;

@@ -252,7 +203,7 @@ export function instructorsPermissionTests(api) {
// instructor does not have permission to update other instructors' information
// in this example, the instructor-only-user should not have permission to update the default user's information
await switchToInstructorOnlyUser();
let respInvalidRouteWithSession = await apiPOST(
const respInvalidRouteWithSession = await apiPOST(
`/instructor/instructors`,
defaultUserWithUpdatedInformation
);
@@ -262,7 +213,7 @@ export function instructorsPermissionTests(api) {
await restoreDefaultUser();

// instructors have permission to modify their own information
let respModDefaultUser = await apiPOST(
const respModDefaultUser = await apiPOST(
`/instructor/instructors`,
defaultUserWithUpdatedInformation
);
@@ -274,7 +225,7 @@ export function instructorsPermissionTests(api) {

it("fetch sessions", async () => {
await switchToInstructorOnlyUser();
let resp = await apiGET("/instructor/sessions");
const resp = await apiGET("/instructor/sessions");
expect(resp).toHaveStatus("success");

await restoreDefaultUser();
@@ -315,7 +266,9 @@ export function instructorsPermissionTests(api) {

it("fetch positions", async () => {
await switchToInstructorOnlyUser();
let resp = await apiGET(`/instructor/sessions/${session.id}/positions`);
const resp = await apiGET(
`/instructor/sessions/${session.id}/positions`
);
expect(resp).toHaveStatus("success");

await restoreDefaultUser();
@@ -409,7 +362,7 @@ export function instructorsPermissionTests(api) {

it("fetch contract templates", async () => {
await switchToInstructorOnlyUser();
let resp = await apiGET(
const resp = await apiGET(
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
`/instructor/sessions/${session.id}/contract_templates`
);
expect(resp).toHaveStatus("success");
@@ -443,7 +396,7 @@ export function instructorsPermissionTests(api) {

it("fetch applicants", async () => {
await switchToInstructorOnlyUser();
let resp = await apiGET(
const resp = await apiGET(
`/instructor/sessions/${session.id}/applicants`
);
expect(resp).toHaveStatus("success");
@@ -490,7 +443,7 @@ export function instructorsPermissionTests(api) {

it("fetch assignments", async () => {
await switchToInstructorOnlyUser();
let resp = await apiGET(
const resp = await apiGET(
`/instructor/sessions/${session.id}/applicants`
);
expect(resp).toHaveStatus("success");
@@ -516,51 +469,254 @@ export function instructorsPermissionTests(api) {

it("fetch applications", async () => {
await switchToInstructorOnlyUser();
let resp = await apiGET(
const resp = await apiGET(
`/instructor/sessions/${session.id}/applications`
);
expect(resp).toHaveStatus("success");

await restoreDefaultUser();
});

it("fetch Ddahs", async () => {
// If a user is not in Instructors table, it is not considered an instructor
// for the purpose of fetching DDAHs - so we create one
const newInstructor = {
first_name: "Jane",
last_name: "Smith",
email: "jane.smith@gmail.com",
utorid: instructorUser.utorid,
};
let resp = await apiPOST(`/admin/instructors`, newInstructor);
expect(resp).toHaveStatus("success");
describe("Ddah permissions", () => {
let position;
let instructor;
let assignment;
let foreignAssignment;

beforeAll(async () => {
await restoreDefaultUser();

// Get our instructor
let resp = await apiGET(`/admin/instructors`);
expect(resp).toHaveStatus("success");
instructor = resp.payload.find(
(instructor) => instructor.utorid === instructorUser.utorid
);
expect(instructor).toBeDefined();
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved

// Fetch the position related to our instructor
resp = await apiGET(`/admin/sessions/${session.id}/positions`);
position = resp.payload.find((position) =>
position.instructor_ids.find((id) => id === instructor.id)
);
expect(position).toBeDefined();
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved

// Create another assignment (with no DDAH)
// for position associated with our instructor
const newAssignment = {
note: "",
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
applicant_id: databaseSeeder.seededData.applicant.id,
position_id: position.id,
start_date: "2019-09-02T00:00:00.000Z",
end_date: "2019-12-31T00:00:00.000Z",
};
resp = await apiPOST("/admin/assignments", newAssignment);
expect(resp).toHaveStatus("success");
assignment = resp.payload;
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved

// Fetch the position related to other instructor
resp = await apiGET(`/admin/instructors`);
const otherInstructor = resp.payload.find(
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
(instructor) =>
instructor.utorid ===
databaseSeeder.seededData.instructors[4].utorid
);
expect(otherInstructor).toBeDefined();
resp = await apiGET(`/admin/sessions/${session.id}/positions`);
const otherPosition = resp.payload.find((position) =>
position.instructor_ids.find((id) => id === otherInstructor.id)
);
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
expect(otherPosition).toBeDefined();

// Create another assignment (with no DDAH)
// for position associated with other instructor
const otherAssignment = {
note: "",
applicant_id: databaseSeeder.seededData.applicant.id,
position_id: otherPosition.id,
start_date: "2019-09-02T00:00:00.000Z",
end_date: "2019-12-31T00:00:00.000Z",
};
resp = await apiPOST("/admin/assignments", otherAssignment);
expect(resp).toHaveStatus("success");
foreignAssignment = resp.payload;
});

// Get newly created instructor and create a DDAH for them
resp = await apiGET(`/admin/instructors`);
expect(resp).toHaveStatus("success");
const instructorId = resp.payload.find(
(instructor) => instructor.utorid === instructorUser.utorid
)?.id;
expect(instructorId).toBeDefined();
const newDDAH = await createDdahWithFixedDuties(instructorId);
beforeEach(async () => {
await switchToInstructorOnlyUser();
});
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved

// Test the DDAH is fetched properly
await switchToInstructorOnlyUser();
resp = await apiGET(`/instructor/sessions/${session.id}/ddahs`);
expect(resp).toHaveStatus("success");
expect(resp.payload).toHaveLength(1);
expect(resp.payload[0]).toStrictEqual(newDDAH);
});
it("fetch Ddahs associated with self only", async () => {
// Our instructor should only be able to fetch ddahs associated with them
const resp = await apiGET(
`/instructor/sessions/${session.id}/ddahs`
);
expect(resp).toHaveStatus("success");

// Only one DDAH is intially seeded for this instructor
expect(resp.payload).toHaveLength(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though this implementation should be fine, I think you should not assume that only one DDAH is initially seeded for the instructor unless you are seeding the instructor within the test. This is because someone may need to seed more DDAH for this particular instructor in the future and might run into problems with this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I don't like this approach either, that is similar to what's done here for the ddah tests. I see before it used .toContain() and I like that better too. I am not sure that's the direction we will be going in though. @siefkenj do you have a preference?

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 you should fetch all DDAHs as admin and filter that list to contain only those for that instructor. Then check the list the instructor retrieves is the same.

Copy link
Contributor Author

@alex-kozin alex-kozin Jun 14, 2021

Choose a reason for hiding this comment

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

It's a great idea! For the implementation though, we don't have an endpoint to get all DDAHs per instructor? That means we will have to fetch all positions, filter them per instructor, then get all assignments for these positions and then get their respective DDAHs? That would complicate the test a lot imo, even though that will yield the most predictable outcome regardless of the seeded data.

That also conflicts with the idea that we "know the seeded data" and can rely on it unless I am missing something of course


// We get duties returned sorted in ascending order, so
// we need to sort the seeded ones before comparing
const sortedSeededDuties = databaseSeeder.seededData.ddahs[0].duties.sort(
(first, second) => first.order - second.order
);
expect(resp.payload[0].duties).toEqual(sortedSeededDuties);
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
});

it.todo("fetch Ddahs a position associated with self");
it.todo("fetch Ddahs an assignment associated with self");
it.todo("cannot fetch Ddahs for assignment not associated with self");
it.todo("create a Ddah for an assignment associated with self");
it.todo("update a Ddah for an assignment associated with self");
it.todo(
"cannot set approved_date/accepted_date/revised_date/emailed_ate/signature for a Ddah associated with self"
);
it.todo("cannot create a Ddah for an assignment not associated with self");
it.todo("cannot update a Ddah for an assignment not associated with self");
it("create a Ddah for an assignment associated with self", async () => {
const ddahToCreate = {
// This assignment has no DDAH initially
assignment_id: assignment.id,
duties: [
{
order: 1,
hours: 20,
description: "other:Facilitating workshops",
},
{
order: 2,
hours: 50,
description: "other:Lecture support",
},
],
};
let resp = await apiPOST(`/instructor/ddahs`, ddahToCreate);
expect(resp).toHaveStatus("success");
expect(resp.payload).toEqual(expect.objectContaining(ddahToCreate));
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this work? Isn't resp.payload an array? We have a toContainObject extension to expect for just this case..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case resp.payload is an object and that was the reason why the test was failing! toContainObject would not work with objects.

For .toEqual Jest allows its matchers to be passed in the function. It can match a literal or match based on Jest's expect.matcher() expressions. In this case we are matching any object that recursively has the properties of the ddahToCreate!


// Test we can fetch both DDAHs now: the seeded one and the newly created one
resp = await apiGET(`/instructor/sessions/${session.id}/ddahs`);
expect(resp).toHaveStatus("success");
expect(resp.payload).toHaveLength(2);
});

it("update a Ddah for an assignment associated with self", async () => {
// Fetch the first assignment associated with our instructor
let resp = await apiGET(
`/instructor/sessions/${session.id}/assignments`
);
expect(resp).toHaveStatus("success");
const assignment_id = resp.payload[0].id;

// Update the DDAH and check it was updated correctly
const updatedDdah = {
assignment_id,
duties: [
{
order: 1,
hours: 20,
description: "marking:Test Marking",
},
{
order: 2,
hours: 50,
description: "other:Additional duties",
},
],
};
resp = await apiPOST(`/instructor/ddahs`, updatedDdah);
expect(resp).toHaveStatus("success");
expect(resp.payload).toEqual(expect.objectContaining(updatedDdah));
});

// This test indeed fails and allows to set these fields 0w0
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
it.skip("cannot set approved_date/accepted_date/revised_date/emailed_date/signature for a Ddah associated with self", async () => {
// Fetch the DDAH related to instructor's assignment
let resp = await apiGET(`/instructor/sessions/${session.id}/ddahs`);
expect(resp).toHaveStatus("success");
const originalDdah = resp.payload.find(
(ddah) => ddah.assignment_id === assignment.id
);

// Update the DDAH and check it was updated correctly
const ddahWithRestrictedFields = {
assignment_id: assignment.id,
duties: [
{
order: 1,
hours: 40,
description: "marking:Test Marking",
},
{
order: 2,
hours: 30,
description: "other:Additional duties",
},
],
approved_date: new Date().toISOString(),
accepted_date: "2020-09-02T00:00:00.000Z",
revised_date: "2020-09-01T00:00:00.000Z",
emailed_date: "2020-08-28T00:00:00.000Z",
signature: "Harry Potter",
};
resp = await apiPOST(`/instructor/ddahs`, ddahWithRestrictedFields);
expect(resp).toHaveStatus("success");
expect(resp.payload).toEqual(expect.objectContaining(originalDdah));

resp = await apiGET(`/instructor/sessions/${session.id}/ddahs`);
expect(resp).toHaveStatus("success");
const newDdah = resp.payload.find(
(ddah) => ddah.assignment_id === assignment.id
);
expect(newDdah).toEqual(expect.objectContaining(originalDdah));
}); // still a success, but does not change the underlying data

it("cannot create a Ddah for an assignment not associated with self", async () => {
const ddahToCreate = {
// This assignment has no DDAH initially
assignment_id: foreignAssignment.id,
duties: [
{
order: 1,
hours: 20,
description: "other:Facilitating workshops",
},
],
};
let resp = await apiPOST(`/instructor/ddahs`, ddahToCreate);
expect(resp).toHaveStatus("error");
expect(resp.message).toStrictEqual(
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
"Cannot create a DDAH without an assignment_id OR improper permissions to access/create a DDAH"
);
});

it("cannot update a Ddah for an assignment not associated with self", async () => {
// First we add the DDAH to the assignment
// associated with another instructor
await restoreDefaultUser();
const ddahToCreate = {
assignment_id: foreignAssignment.id,
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
duties: [
{
order: 1,
hours: 30,
description: "contact:Practice",
},
],
};
let resp = await apiPOST(`/admin/ddahs`, ddahToCreate);
expect(resp).toHaveStatus("success");
expect(resp.payload.duties).toStrictEqual(ddahToCreate.duties);
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved

// Then we try to update the existing DDAH with our instructor
await switchToInstructorOnlyUser();
const ddahToUpdate = {
// This assignment has a DDAH initially
assignment_id: foreignAssignment.id,
duties: [
{
order: 1,
hours: 20,
description: "other:Facilitating workshops",
},
],
};
resp = await apiPOST(`/instructor/ddahs`, ddahToUpdate);
expect(resp).toHaveStatus("error");
expect(resp.message).toStrictEqual(
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
"Cannot create a DDAH without an assignment_id OR improper permissions to access/create a DDAH"
);
});
});
}
115 changes: 115 additions & 0 deletions frontend/src/tests/setup.js
Original file line number Diff line number Diff line change
@@ -66,6 +66,18 @@ class DatabaseSeeder {
email: "megan.miller@utoronto.ca",
utorid: "millerm",
},
{
last_name: "Lucas",
first_name: "George",
email: "george.lucas@utoronto.ca",
utorid: "lucasg",
},
{
last_name: "Bellford",
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
first_name: "Jordan",
email: "jordan.bell@utoronto.ca",
utorid: "belljo",
},
],
applicants: [
{
@@ -147,6 +159,22 @@ class DatabaseSeeder {
instructor_ids: [],
instructor_utorids: [],
},
{
position_code: "CSC555Y1",
position_title: "Computer Networks",
hours_per_assignment: 70,
contract_template_id: null,
instructor_ids: [],
instructor_utorids: ["lucasg", "millerm"],
},
{
position_code: "ECO101H1F",
position_title: "Microeconomics",
hours_per_assignment: 50,
contract_template_id: null,
instructor_ids: [],
instructor_utorids: ["millerm", "belljo"],
},
],
assignments: [
{
@@ -173,6 +201,58 @@ class DatabaseSeeder {
position_code: "MAT235H1F",
applicant_utorid: "weasleyr",
},
{
ddah_seed_id: 1,
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
position_code: "CSC555Y1",
applicant_utorid: "molinat",
},
{
ddah_seed_id: 2,
position_code: "ECO101H1F",
applicant_utorid: "brownd",
},
],
ddahs: [
{
assignment_seed_id: 1,
duties: [
{
order: 2,
hours: 18,
description: "marking:Marking midterms",
},
{
order: 1,
hours: 2,
description: "training:Initial TA training",
},
{
order: 3,
hours: 50,
description: "contact:Tutorials",
},
],
},
{
assignment_seed_id: 2,
duties: [
{
order: 2,
hours: 18,
description: "marking:Marking midterms",
},
{
order: 1,
hours: 2,
description: "training:Initial TA training",
},
{
order: 3,
hours: 50,
description: "contact:Tutorials",
},
],
},
],
};
}
@@ -400,6 +480,9 @@ async function seedDatabaseForInstructors(
}

// Assignment
// We will keep track of the inserted assignments for the purpose
// of creating DDAHs for some of them later
const processedAssignments = [];
for (const assignment of seeded.assignments) {
// The seed data is written in terms of position codes
// and applicant utorids, so we need to mangle the data
@@ -431,5 +514,37 @@ async function seedDatabaseForInstructors(
resp = await apiPOST(`/admin/assignments`, assignment);
expect(resp).toHaveStatus("success");
Object.assign(assignment, resp.payload);
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved
processedAssignments.push(assignment);
}

// DDAH
for (const ddah of seeded.ddahs) {
// assignment_seed_id in ddahs should correspond to ddah_seed_id in assignments
if (!ddah.assignment_seed_id) {
throw new Error(
`Inconsistency in seed data: could not create ddah without assignment_seed_id`
);
}

// The seed data is written in terms of position codes
// and applicant utorids, since unique applicant can apply to
// at most one position
const { id: assignment_id } =
processedAssignments.find(
(assignment) =>
assignment.ddah_seed_id === ddah.assignment_seed_id
) || {};
if (!assignment_id) {
throw new Error(
`Inconsistency in seed data: could not find assignment with ddah_seed_id ${ddah.assignment_seed_id}`
);
}

const newDdah = {
...ddah,
assignment_id,
};
let resp = await apiPOST(`/admin/ddahs`, newDdah);
expect(resp).toHaveStatus("success");
}
}