-
Notifications
You must be signed in to change notification settings - Fork 7
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
feat(datatrakWeb): RN-1331: Task comments setup #5800
Changes from 9 commits
c9db4eb
743a53a
cc3bae3
580a6d4
ced6958
54f00c1
07e162d
fb63c4f
8daeb38
802e108
498b1be
baf0fa8
d0c82a2
b92b94f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,41 @@ | ||||||
/** | ||||||
* Tupaia | ||||||
* Copyright (c) 2017 - 2024 Beyond Essential Systems Pty Ltd | ||||||
*/ | ||||||
import { RECORDS } from '@tupaia/database'; | ||||||
import { CreateHandler } from '../CreateHandler'; | ||||||
import { assertAnyPermissions, assertBESAdminAccess } from '../../permissions'; | ||||||
import { assertUserHasTaskPermissions } from '../tasks/assertTaskPermissions'; | ||||||
|
||||||
/** | ||||||
* Handles POST endpoints: | ||||||
* - /tasks/:parentRecordId/taskComments | ||||||
*/ | ||||||
|
||||||
export class CreateTaskComment extends CreateHandler { | ||||||
parentRecordType = RECORDS.TASK; | ||||||
|
||||||
async assertUserHasAccess() { | ||||||
const createPermissionChecker = accessPolicy => | ||||||
assertUserHasTaskPermissions(accessPolicy, this.models, this.parentRecordId); | ||||||
|
||||||
await this.assertPermissions( | ||||||
assertAnyPermissions([assertBESAdminAccess, createPermissionChecker]), | ||||||
); | ||||||
} | ||||||
|
||||||
async createRecord() { | ||||||
const { id: userId } = this.req.user; | ||||||
const user = await this.models.user.findById(userId); // Check if user exists | ||||||
if (!user) { | ||||||
throw new Error(`User with id ${userId} not found`); | ||||||
} | ||||||
|
||||||
const { full_name: userFullName } = user; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this work? I would have thought you'd need to do something like:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep it does work :) I made a custom column selector a while ago called |
||||||
this.newRecordData.user_id = userId; | ||||||
this.newRecordData.user_name = userFullName; | ||||||
this.newRecordData.task_id = this.parentRecordId; | ||||||
|
||||||
return this.insertRecord(); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
/** | ||
* Tupaia | ||
* Copyright (c) 2017 - 2024 Beyond Essential Systems Pty Ltd | ||
*/ | ||
|
||
import { assertAnyPermissions, assertBESAdminAccess } from '../../permissions'; | ||
import { GETHandler } from '../GETHandler'; | ||
import { assertUserHasTaskPermissions } from '../tasks/assertTaskPermissions'; | ||
import { createTaskCommentDBFilter } from './assertTaskCommentPermissions'; | ||
|
||
/** | ||
* Handles endpoints: | ||
* - /tasks/:taskId/comments | ||
*/ | ||
|
||
export class GETTaskComments extends GETHandler { | ||
permissionsFilteredInternally = true; | ||
|
||
async getPermissionsFilter(criteria, options) { | ||
return createTaskCommentDBFilter(this.accessPolicy, this.models, criteria, options); | ||
} | ||
|
||
async getPermissionsViaParentFilter(criteria, options) { | ||
const taskPermissionsChecker = accessPolicy => | ||
assertUserHasTaskPermissions(accessPolicy, this.models, this.parentRecordId); | ||
await this.assertPermissions( | ||
assertAnyPermissions([assertBESAdminAccess, taskPermissionsChecker]), | ||
); | ||
// Filter by parent | ||
const dbConditions = { 'task_comment.task_id': this.parentRecordId, ...criteria }; | ||
|
||
// Apply regular permissions | ||
return { | ||
dbConditions, | ||
dbOptions: options, | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,38 @@ | ||||||
/** | ||||||
* Tupaia | ||||||
* Copyright (c) 2017 - 2024 Beyond Essential Systems Pty Ltd | ||||||
*/ | ||||||
|
||||||
import { hasBESAdminAccess } from '../../permissions'; | ||||||
import { createTaskDBFilter } from '../tasks/assertTaskPermissions'; | ||||||
|
||||||
export const createTaskCommentDBFilter = async (accessPolicy, models, criteria, options) => { | ||||||
if (hasBESAdminAccess(accessPolicy)) { | ||||||
return { dbConditions: criteria, dbOptions: options }; | ||||||
} | ||||||
const { dbConditions } = await createTaskDBFilter(accessPolicy, models, criteria, options); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Just eyeballing this one, but I think we'd need to do something like this? Otherwise we'd be filtering the |
||||||
|
||||||
const taskIds = await models.task.find( | ||||||
{ | ||||||
...dbConditions, | ||||||
id: criteria.task_id ?? undefined, | ||||||
}, | ||||||
{ columns: ['task.id'] }, | ||||||
); | ||||||
|
||||||
if (!taskIds.length) { | ||||||
// if the user doesn't have access to any tasks, return a condition that will return no results | ||||||
return { dbConditions: { id: -1 }, dbOptions: options }; | ||||||
} | ||||||
|
||||||
return { | ||||||
dbConditions: { | ||||||
...criteria, | ||||||
task_id: { | ||||||
comparator: 'IN', | ||||||
comparisonValue: taskIds.map(task => task.id), // this will include any task_id filters because the list of tasks was already filtered by the dbConditions | ||||||
}, | ||||||
}, | ||||||
dbOptions: options, | ||||||
}; | ||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
/** | ||
* Tupaia | ||
* Copyright (c) 2017 - 2024 Beyond Essential Systems Pty Ltd | ||
*/ | ||
|
||
export { GETTaskComments } from './GETTaskComments'; | ||
export { CreateTaskComment } from './CreateTaskComment'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,159 @@ | ||
/** | ||
* Tupaia | ||
* Copyright (c) 2017 - 2024 Beyond Essential Systems Pty Ltd | ||
*/ | ||
|
||
import { expect } from 'chai'; | ||
import { | ||
buildAndInsertSurvey, | ||
findOrCreateDummyCountryEntity, | ||
findOrCreateDummyRecord, | ||
generateId, | ||
} from '@tupaia/database'; | ||
import { TestableApp, resetTestData } from '../../testUtilities'; | ||
import { BES_ADMIN_PERMISSION_GROUP } from '../../../permissions'; | ||
|
||
describe('Permissions checker for CreateTaskComment', async () => { | ||
const BES_ADMIN_POLICY = { | ||
DL: [BES_ADMIN_PERMISSION_GROUP], | ||
}; | ||
|
||
const DEFAULT_POLICY = { | ||
DL: ['Donor'], | ||
}; | ||
|
||
const PUBLIC_POLICY = { | ||
DL: ['Public'], | ||
}; | ||
|
||
const app = new TestableApp(); | ||
const { models } = app; | ||
|
||
const BASE_COMMENT = { | ||
message: 'This is a test comment', | ||
type: 'user', | ||
}; | ||
|
||
const generateData = async () => { | ||
const { country: dlCountry } = await findOrCreateDummyCountryEntity(models, { | ||
code: 'DL', | ||
name: 'Demo Land', | ||
}); | ||
|
||
const donorPermission = await findOrCreateDummyRecord(models.permissionGroup, { | ||
name: 'Donor', | ||
}); | ||
|
||
const facility = { | ||
id: generateId(), | ||
code: 'TEST_FACILITY_2', | ||
name: 'Test Facility 2', | ||
country_code: dlCountry.code, | ||
}; | ||
|
||
await findOrCreateDummyRecord(models.entity, facility); | ||
|
||
const { survey } = await buildAndInsertSurvey(models, { | ||
code: 'TEST_SURVEY_1', | ||
name: 'Test Survey 1', | ||
permission_group_id: donorPermission.id, | ||
country_ids: [dlCountry.id], | ||
}); | ||
|
||
const user = { | ||
id: generateId(), | ||
first_name: 'Minnie', | ||
last_name: 'Mouse', | ||
}; | ||
|
||
await findOrCreateDummyRecord(models.user, user); | ||
|
||
const dueDate = new Date('2021-12-31'); | ||
|
||
const task = { | ||
id: generateId(), | ||
survey_id: survey.id, | ||
entity_id: facility.id, | ||
due_date: dueDate, | ||
status: 'to_do', | ||
repeat_schedule: null, | ||
}; | ||
|
||
await findOrCreateDummyRecord( | ||
models.task, | ||
{ | ||
'task.id': task.id, | ||
}, | ||
task, | ||
); | ||
|
||
return { | ||
task, | ||
user, | ||
}; | ||
}; | ||
|
||
let task; | ||
|
||
before(async () => { | ||
const data = await generateData(); | ||
task = data.task; | ||
}); | ||
|
||
afterEach(() => { | ||
app.revokeAccess(); | ||
}); | ||
|
||
after(async () => { | ||
await resetTestData(); | ||
}); | ||
|
||
describe('POST /taskComments', async () => { | ||
it('Sufficient permissions: allows a user to create a task comment if they have BES Admin permission', async () => { | ||
await app.grantAccess(BES_ADMIN_POLICY); | ||
const { body: result } = await app.post(`tasks/${task.id}/taskComments`, { | ||
body: BASE_COMMENT, | ||
}); | ||
|
||
expect(result.message).to.equal('Successfully created taskComments'); | ||
const taskComment = await models.taskComment.findOne({ | ||
task_id: task.id, | ||
message: BASE_COMMENT.message, | ||
}); | ||
|
||
expect(taskComment).to.not.be.undefined; | ||
|
||
expect(taskComment.type).to.equal(BASE_COMMENT.type); | ||
expect(taskComment.user_name).to.equal('Test User'); | ||
expect(taskComment.task_id).to.equal(task.id); | ||
}); | ||
|
||
it('Sufficient permissions: allows a user to create a task comment if they have access to the task', async () => { | ||
await app.grantAccess(DEFAULT_POLICY); | ||
const { body: result } = await app.post(`tasks/${task.id}/taskComments`, { | ||
body: BASE_COMMENT, | ||
}); | ||
|
||
expect(result.message).to.equal('Successfully created taskComments'); | ||
const taskComment = await models.taskComment.findOne({ | ||
task_id: task.id, | ||
message: BASE_COMMENT.message, | ||
}); | ||
|
||
expect(taskComment).to.not.be.undefined; | ||
|
||
expect(taskComment.type).to.equal(BASE_COMMENT.type); | ||
expect(taskComment.user_name).to.equal('Test User'); | ||
expect(taskComment.task_id).to.equal(task.id); | ||
}); | ||
|
||
it('Insufficient permissions: throws an error if the user does not have access to the task', async () => { | ||
await app.grantAccess(PUBLIC_POLICY); | ||
const { body: result } = await app.post(`tasks/${task.id}/taskComments`, { | ||
body: BASE_COMMENT, | ||
}); | ||
|
||
expect(result).to.have.keys('error'); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parentRecordId just refers the task_id relation on the comments table, right? Any reason that you didn't just use :taskId here? To me it seems that it would be more obvious what it is referring to if it was taskId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, yes I originally called it
taskId
but the central-server has built in logic that handlesparentRecordId
so that's what it needs to be to access that