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

tweak(datatrakWeb): RN-1400: save task due date in unix time #5838

Merged
merged 22 commits into from
Aug 16, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,14 @@ import React, { useState } from 'react';
import PropTypes from 'prop-types';
import styled from 'styled-components';
import InsertDriveFileIcon from '@material-ui/icons/InsertDriveFile';
import { FlexEnd, FlexSpaceBetween, FlexStart, ImportModal } from '@tupaia/ui-components';
import { FlexEnd, FlexSpaceBetween, FlexStart } from '@tupaia/ui-components';
import { usePreviewDataContext, useVizConfigContext } from '../../context';
import { LinkButton } from '../LinkButton';
import { useUploadTestData } from '../../api';
import { ProjectField } from './ProjectField';
import { LocationField } from './LocationField';
import { DateRangeField } from './DateRangeField';
import { ImportModal } from '../../../importExport';

const Container = styled(FlexSpaceBetween)`
padding: 24px 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ describe('Permissions checker for CreateTaskComment', async () => {
};
await findOrCreateDummyRecord(models.user, assignee);

const dueDate = new Date('2021-12-31');
const dueDate = new Date('2021-12-31').getTime();

tasks = [
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ describe('Permissions checker for GETTaskComments', async () => {
};
await findOrCreateDummyRecord(models.user, user);

const dueDate = new Date('2021-12-31');
const dueDate = new Date('2021-12-31').getTime();

const task = {
id: generateId(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,12 @@ describe('Permissions checker for CreateTask', async () => {
last_name: 'Pan',
};

const dueDate = new Date('2021-12-31').getTime();

const BASE_TASK = {
assignee_id: assignee.id,
repeat_schedule: null,
due_date: new Date('2021-12-31'),
due_date: dueDate,
status: 'to_do',
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('Permissions checker for EditTask', async () => {
last_name: 'Pan',
};

const dueDate = new Date('2021-12-31');
const dueDate = new Date('2021-12-31').getTime();

let tasks;

Expand Down Expand Up @@ -218,7 +218,7 @@ describe('Permissions checker for EditTask', async () => {
});
await app.put(`tasks/${tasks[1].id}`, {
body: {
due_date: new Date('2021-11-30').toISOString(),
due_date: new Date('2021-11-30').getTime(),
},
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe('Permissions checker for GETTasks', async () => {
};
await findOrCreateDummyRecord(models.user, assignee);

const dueDate = new Date('2021-12-31');
const dueDate = new Date('2021-12-31').getTime();

tasks = [
{
Expand Down
1 change: 1 addition & 0 deletions packages/database/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"@tupaia/tsutils": "workspace:*",
"@tupaia/utils": "workspace:*",
"date-fns": "^2.29.2",
"date-fns-tz": "^2.0.1",
"db-migrate": "^0.11.5",
"db-migrate-pg": "^1.2.2",
"dotenv": "^16.4.5",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const TASK = {
status: 'to_do',
repeat_schedule: null,
assignee_id: null,
due_date: new Date('2021-01-01 00:00:00'),
due_date: new Date('2021-01-01 00:00:00').getTime(),
id: generateId(),
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ describe('TaskCompletionHandler', () => {
survey_id: SURVEY.id,
created_at: '2024-07-08',
status: 'to_do',
due_date: '2024-07-25',
due_date: new Date('2024-07-20').getTime(),
survey_response_id: null,
});
await upsertDummyRecord(models.user, { id: userId });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ const buildSurveyResponse = async (models, surveyCode, answers) => {
surveyCode,
answers,
id: generateId(),
timezone: 'Pacific/Auckland',
};

const surveyResponses = await buildAndInsertSurveyResponses(models, [surveyResponse]);
Expand All @@ -103,13 +104,15 @@ const TEST_DATA = [
answers: {
TEST_CODE_00: entityId,
TEST_CODE_01: 'Yes',
TEST_CODE_02: '2024/06/06 00:00:00+00',
// answers come in iso format
TEST_CODE_02: new Date('2024-06-06 00:00:00+12:00').toISOString(),
TEST_CODE_03: userId,
},
},
{
survey_id: taskSurveyId,
due_date: '2024-06-06 00:00:00',
// due date will be set to the last second of the day in the survey response timezone and converted to a timestamp
due_date: new Date('2024-06-06 23:59:59+12:00').getTime(),
assignee_id: userId,
entity_id: entityId,
},
Expand Down
8 changes: 5 additions & 3 deletions packages/database/src/__tests__/modelClasses/Task.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,27 +100,29 @@ describe('TaskModel', () => {

await Promise.all(SURVEYS.map(survey => upsertDummyRecord(models.survey, survey)));

const dueDate = new Date('2020-01-01 00:00:00+00').getTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it just be easier if we didn't have all these tests to worry about and maintain ;)


const TASKS = [
{
id: generateId(),
survey_id: SURVEYS[0].id,
status: 'to_do',
entity_id: facilities[0].id,
due_date: new Date('2020-01-01'),
due_date: dueDate,
},
{
id: generateId(),
survey_id: SURVEYS[1].id,
status: 'to_do',
entity_id: facilities[1].id,
due_date: new Date('2020-01-01'),
due_date: dueDate,
},
{
id: generateId(),
survey_id: SURVEYS[0].id,
status: 'to_do',
entity_id: facilities[1].id,
due_date: new Date('2020-01-01'),
due_date: dueDate,
},
];

Expand Down
23 changes: 21 additions & 2 deletions packages/database/src/changeHandlers/TaskCreationHandler.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
* Copyright (c) 2017 - 2024 Beyond Essential Systems Pty Ltd
*/
import keyBy from 'lodash.keyby';
import { formatInTimeZone } from 'date-fns-tz';
import { ChangeHandler } from './ChangeHandler';

const getAnswerWrapper = (config, answers) => {
Expand Down Expand Up @@ -71,6 +72,7 @@ export class TaskCreationHandler extends ChangeHandler {

for (const response of changedResponses) {
const sr = await models.surveyResponse.findById(response.id);
const { timezone } = sr;
const questions = await getQuestions(models, response.survey_id);

const taskQuestions = questions.filter(question => question.type === 'Task');
Expand All @@ -84,7 +86,7 @@ export class TaskCreationHandler extends ChangeHandler {
for (const taskQuestion of taskQuestions) {
const config = taskQuestion.config.task;
const getAnswer = getAnswerWrapper(config, answers);

if (
!config ||
getAnswer('shouldCreateTask') === null ||
Expand All @@ -100,12 +102,29 @@ export class TaskCreationHandler extends ChangeHandler {
: getAnswer('entityId');
const surveyId = await getSurveyId(models, config);

const dueDateAnswer = getAnswer('dueDate');

let dueDate = null;
if (dueDateAnswer) {
// Convert the due date to the timezone of the survey response and set the time to the last second of the day
const dateInTimezone = formatInTimeZone(
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a nice way of getting the end of day value for the timezone

dueDateAnswer,
timezone,
"yyyy-MM-dd'T23:59:59'XXX",
);

// Convert the date to a timestamp
const timestamp = new Date(dateInTimezone).getTime();

dueDate = timestamp;
}

await models.task.create({
initial_request_id: response.id,
survey_id: surveyId,
entity_id: entityId,
assignee_id: getAnswer('assignee'),
due_date: getAnswer('dueDate'),
due_date: dueDate,
status: 'to_do',
});
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
'use strict';

var dbm;
var type;
var seed;

/**
* We receive the dbmigrate dependency from dbmigrate initially.
* This enables us to not have to rely on NODE_PATH.
*/
exports.setup = function (options, seedLink) {
dbm = options.dbmigrate;
type = dbm.dataType;
seed = seedLink;
};

exports.up = async function (db) {
return db.runSql(`
ALTER TABLE task
RENAME COLUMN due_date TO old_due_date;

ALTER TABLE task
ADD COLUMN due_date DOUBLE PRECISION;

UPDATE task
SET due_date = (EXTRACT(EPOCH FROM old_due_date) * 1000)::DOUBLE PRECISION;

ALTER TABLE task
DROP COLUMN old_due_date;
`);
};

exports.down = function (db) {
return db.runSql(`
ALTER TABLE task
RENAME COLUMN due_date TO old_due_date;

ALTER TABLE task
ADD COLUMN due_date TIMESTAMP WITH TIME ZONE;

UPDATE task
SET due_date = to_timestamp(CAST(old_due_date AS DOUBLE PRECISION) / 1000);

ALTER TABLE task
DROP COLUMN old_due_date;
`);
};

exports._meta = {
version: 1,
};
2 changes: 1 addition & 1 deletion packages/database/src/modelClasses/Task.js
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ export class TaskModel extends DatabaseModel {
CASE
WHEN repeat_schedule IS NOT NULL THEN 'repeating'
WHEN due_date IS NULL THEN 'to_do'
WHEN due_date < '${format(new Date(), 'yyyy-MM-dd')}' THEN 'overdue'
WHEN due_date < ${new Date().getTime()} THEN 'overdue'
ELSE 'to_do'
END
ELSE 'to_do'
Expand Down
29 changes: 28 additions & 1 deletion packages/datatrak-web-server/src/__tests__/TasksRoute.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ describe('TaskRoute', () => {
},
},
{
filter: { 'survey.name': { comparator: 'ilike', comparisonValue: '%a%' } },
filter: { 'survey.name': { comparator: 'ilike', comparisonValue: 'a%' } },
},
],
[
Expand Down Expand Up @@ -164,6 +164,33 @@ describe('TaskRoute', () => {
},
},
],
[
'Due date filter is between start and end of day',
{
headers: {
cookie: 'show_completed_tasks=true;show_cancelled_tasks=true;all_assignees_tasks=true',
},
query: {
filters: [
{
id: 'due_date',
value: '2021-01-01 23:59:59.000+12:00',
},
],
},
},
{
filter: {
due_date: {
comparator: 'BETWEEN',
comparisonValue: [
new Date('2021-01-01T00:00:00.000+12:00').getTime(),
new Date('2021-01-01T23:59:59.000+12:00').getTime(),
],
},
},
},
],
];

// @ts-ignore
Expand Down
16 changes: 15 additions & 1 deletion packages/datatrak-web-server/src/routes/TasksRoute.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* Tupaia
* Copyright (c) 2017 - 2024 Beyond Essential Systems Pty Ltd
*/
import { sub } from 'date-fns';
import { Request } from 'express';
import { Route } from '@tupaia/server-boilerplate';
import { parse } from 'cookie';
Expand Down Expand Up @@ -35,7 +36,7 @@ const DEFAULT_PAGE_SIZE = 20;

type FormattedFilters = Record<string, any>;

const EQUALITY_FILTERS = ['due_date', 'survey.project_id', 'task_status'];
const EQUALITY_FILTERS = ['survey.project_id', 'task_status'];

const getFilterSettings = (cookieString: string) => {
const cookies = parse(cookieString);
Expand All @@ -54,6 +55,19 @@ export class TasksRoute extends Route<TasksRequest> {

filters.forEach(({ id, value }) => {
if (value === '' || value === undefined || value === null) return;
if (id === 'due_date') {
// set the time to the end of the day to get the full range of the day, and apply milliseconds to ensure the range is inclusive
const endDateObj = new Date(value);
// subtract 23 hours, 59 minutes, 59 seconds to get the start of the day. This is because the filters always send the end of the day, and we need a range to handle the values being saved in the database as unix timestamps based on the user's timezone.
const startDate = sub(endDateObj, { hours: 23, minutes: 59, seconds: 59 }).getTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use the date-fns endOfDay function here to make this more readable? endOfDay(new Date()).getTime()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried using the startOfDay function here but because the server time is in UTC always it didn't actually work as expected, so I just subtracted the hours

const endDate = endDateObj.getTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just the same value as value ie. endDate = value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, this would be from when I was debugging

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, I was wrong, this is correct as value is a date string, but we need the unix time for comparison

this.filters[id] = {
comparator: 'BETWEEN',
comparisonValue: [startDate, endDate],
};

return;
}

if (EQUALITY_FILTERS.includes(id)) {
this.filters[id] = value;
Expand Down
11 changes: 3 additions & 8 deletions packages/datatrak-web-server/src/utils/formatTaskChanges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ import { stripTimezoneFromDate } from '@tupaia/utils';
type Input = Partial<DatatrakWebTaskChangeRequest.ReqBody> &
Partial<Pick<Task, 'entity_id' | 'survey_id' | 'status'>>;

type Output = Partial<Omit<Task, 'due_date'>> & {
due_date?: string | null;
type Output = Partial<Task> & {
comment?: string;
};

Expand All @@ -28,13 +27,9 @@ export const formatTaskChanges = (task: Input) => {
taskDetails.due_date = null;
} else if (dueDate) {
// apply status and due date only if not a repeating task
// set due date to end of day
const endOfDay = new Date(new Date(dueDate).setHours(23, 59, 59, 999));
const unix = new Date(dueDate).getTime();
Copy link
Contributor

Choose a reason for hiding this comment

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

So it looks like every time we want to save a value to the due date col we will have to convert the date to the unix timestamp by calling getTime and then most times we want to read the value, we will have to convert it back using new Date(). I wonder if it would be possible to do that conversion at the model level so that we always get or send a date object or date iso string when working with the Task. In previous ORMs I've used that is a pretty common thing to do. @rohan-bes do you think there is a nice way to do this with our models?

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 this would be great, I don't think we have a pattern of doing this anywhere else.
But, what I could do is, for fetching, add a custom column selector so that we don't have to convert this to a date object every time.

Copy link
Contributor

Choose a reason for hiding this comment

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

yep that sounds like a good improvement to me 👍


// strip timezone from date so that the returned date is always in the user's timezone
const withoutTimezone = stripTimezoneFromDate(endOfDay);

taskDetails.due_date = withoutTimezone;
taskDetails.due_date = unix;
taskDetails.repeat_schedule = null;
}

Expand Down
Loading