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

Jump to bottom

Add more tests for instructor permissions to fetch ddahs #599

Merged
merged 17 commits into from
Jun 20, 2021
Merged

Add more tests for instructor permissions to fetch ddahs #599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
05e7b6f
More tests.
alex-kozin May 31, 2021
e6f3e64
Failing ddah creation...
alex-kozin May 31, 2021
cfdd994
Work from before.
alex-kozin Jun 4, 2021
60df9a7
Seed ddahs for two instructors, fix some tests.
alex-kozin Jun 8, 2021
1e92a33
Use default jest method instead of custom ToContainObject.
alex-kozin Jun 8, 2021
d96f085
One more test.
alex-kozin Jun 8, 2021
cc290d7
Add a failing test.
alex-kozin Jun 8, 2021
35aa1e8
Added another test for updating the DDAH.
alex-kozin Jun 8, 2021
407a95c
Lint
alex-kozin Jun 8, 2021
4697c51
Remove .only
alex-kozin Jun 8, 2021
e88b52a
Add spacing.
alex-kozin Jun 11, 2021
ed81b31
Address feedback.
alex-kozin Jun 14, 2021
c79b3af
Rename foreignAssignment.
alex-kozin Jun 14, 2021
7bcb145
Rename temp ids.
alex-kozin Jun 18, 2021
debb745
Delete unnecessary calls, grab data from seeder.
alex-kozin Jun 18, 2021
0f7b174
Trivial change.
alex-kozin Jun 20, 2021
c88d272
Lint setup please.
alex-kozin Jun 20, 2021
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
5 changes: 3 additions & 2 deletions frontend/src/tests/ddah-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});

Expand Down
292 changes: 206 additions & 86 deletions frontend/src/tests/instructor-permission-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,55 +46,6 @@ export function instructorsPermissionTests(api) {
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);
Expand All @@ -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"],
};

Expand Down Expand Up @@ -524,43 +475,212 @@ export function instructorsPermissionTests(api) {
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: "[email protected]",
utorid: instructorUser.utorid,
};
let resp = await apiPOST(`/admin/instructors`, newInstructor);
expect(resp).toHaveStatus("success");
describe("Ddah permissions", () => {
let position = databaseSeeder.seededData.positions[3];
let assignment;
let assignmentInstructorCantAccess;

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

// Create another assignment (with no DDAH)
// for position associated with our instructor
const newAssignment = {
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",
};
let resp = await apiPOST("/admin/assignments", newAssignment);
expect(resp).toHaveStatus("success");
expect(resp.payload).toBeDefined();
assignment = resp.payload;
alex-kozin marked this conversation as resolved.
Show resolved Hide resolved

// Create another assignment (with no DDAH)
// for position associated with other instructor
const otherPosition = databaseSeeder.seededData.positions[4];
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");
assignmentInstructorCantAccess = 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 •
edited
Loading

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
);
const firstDdah = resp.payload[0];
expect(firstDdah.duties).toEqual(sortedSeededDuties);
});

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.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("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, check Issue #608
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`);
// still a success, but does not change the underlying data
expect(resp).toHaveStatus("success");
const newDdah = resp.payload.find(
(ddah) => ddah.assignment_id === assignment.id
);
expect(newDdah).toEqual(expect.objectContaining(originalDdah));
});

it("cannot create a Ddah for an assignment not associated with self", async () => {
const ddahToCreate = {
// This assignment has no DDAH initially
assignment_id: assignmentInstructorCantAccess.id,
duties: [
{
order: 1,
hours: 20,
description: "other:Facilitating workshops",
},
],
};
let resp = await apiPOST(`/instructor/ddahs`, ddahToCreate);
expect(resp).toHaveStatus("error");
});

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: assignmentInstructorCantAccess.id,
duties: [
{
order: 1,
hours: 30,
description: "contact:Practice",
},
],
};
let resp = await apiPOST(`/admin/ddahs`, ddahToCreate);
expect(resp).toHaveStatus("success");

// Then we try to update the existing DDAH with our instructor
await switchToInstructorOnlyUser();
const ddahToUpdate = {
// This assignment has a DDAH initially
assignment_id: assignmentInstructorCantAccess.id,
duties: [
{
order: 1,
hours: 20,
description: "other:Facilitating workshops",
},
],
};
resp = await apiPOST(`/instructor/ddahs`, ddahToUpdate);
expect(resp).toHaveStatus("error");
});
});
}
Loading