From 161658d06ada26cee8f247d8fc918f16a0d5a344 Mon Sep 17 00:00:00 2001 From: Erwan Guyader Date: Mon, 3 Oct 2022 12:43:56 +0200 Subject: [PATCH 1/2] fix: Don't notify about old konnector failures We don't want users to be notified (neither by e-mail nor push notifications) when a konnector failed a long time ago since there's no real need for a quick action from their part. This will be particularly useful when activating these notifications for a large audience. To achieve that result, we create a specific reason not to notify that we should not ignore via the `forcedIgnored` argument of the service. We set the limit to 7 days and 15 minutes after the last failure as this is the maximum delay we can wait for before triggering the service to send the last reminder notification to the user. Setting the limit below this would prevent that last notification to be sent. --- src/ducks/transactions/queries.js | 24 +++--- .../services/konnectorAlerts/helpers.js | 11 +++ .../services/konnectorAlerts/helpers.spec.js | 45 +++++++++++- .../sendTriggerNotifications.spec.js | 6 ++ .../services/konnectorAlerts/shouldNotify.js | 20 ++++- .../konnectorAlerts/shouldNotify.spec.js | 73 +++++++++++++++++++ 6 files changed, 161 insertions(+), 18 deletions(-) create mode 100644 src/targets/services/konnectorAlerts/shouldNotify.spec.js diff --git a/src/ducks/transactions/queries.js b/src/ducks/transactions/queries.js index 365b862857..69abdf4378 100644 --- a/src/ducks/transactions/queries.js +++ b/src/ducks/transactions/queries.js @@ -100,35 +100,33 @@ export const makeFilteredTransactionsConn = options => { * @return {[QueryDefinition, QueryDefinition]} - Earliest and latest queries */ export const makeEarliestLatestQueries = baseQuery => { - /** - * Big hack. We worked to optimize this query see + * Big hack. We worked to optimize this query see * commit 2bb11660ec40c683898673b3270c763404552cd8 - * - * But by doing so, we had an issue where when selecting + * + * But by doing so, we had an issue where when selecting * "all accounts", the result was not right. This is because * we indexed by _id first and then date. - * So the current fix is : - * - If we have a selector on an account, then the index should - * be indexed by account first and then we ensure that we have + * So the current fix is : + * - If we have a selector on an account, then the index should + * be indexed by account first and then we ensure that we have * the date within the index * - If we don't have a slector on an account, then the index * should be indexed first by the date and then by the selector - * - * @todo: We should really split all this query creation to several + * + * @todo: We should really split all this query creation to several * methods. We can't have optimized query with such generic methods - */ + */ const selectors = Object.keys(baseQuery.selector) let indexedFields - if(selectors.includes('account')){ + if (selectors.includes('account')) { indexedFields = selectors - indexedFields.push('date') + indexedFields.push('date') } else { indexedFields = ['date'] indexedFields.push(...selectors) } - const sortByDesc = [] const sortByAsc = [] diff --git a/src/targets/services/konnectorAlerts/helpers.js b/src/targets/services/konnectorAlerts/helpers.js index de0f4fe02d..c209efac80 100644 --- a/src/targets/services/konnectorAlerts/helpers.js +++ b/src/targets/services/konnectorAlerts/helpers.js @@ -1,6 +1,11 @@ import memoize from 'lodash/memoize' import keyBy from 'lodash/keyBy' import get from 'lodash/get' +import parse from 'date-fns/parse' +import subDays from 'date-fns/sub_days' +import subHours from 'date-fns/sub_hours' +import subMinutes from 'date-fns/sub_minutes' +import subSeconds from 'date-fns/sub_seconds' import { Q } from 'cozy-client' @@ -115,3 +120,9 @@ export const fetchRelatedFuturAtTriggers = async (client, id) => { return data } + +export const sub = (base, { days = 0, hours = 0, minutes = 0, seconds = 0 }) => + subDays(subHours(subMinutes(subSeconds(base, seconds), minutes), hours), days) + +export const isOlderThan = (referenceDate, { days, hours, minutes, seconds }) => + parse(referenceDate) < sub(Date.now(), { days, hours, minutes, seconds }) diff --git a/src/targets/services/konnectorAlerts/helpers.spec.js b/src/targets/services/konnectorAlerts/helpers.spec.js index 1eb7f87621..382df2058e 100644 --- a/src/targets/services/konnectorAlerts/helpers.spec.js +++ b/src/targets/services/konnectorAlerts/helpers.spec.js @@ -1,5 +1,7 @@ import { createMockClient } from 'cozy-client' -import { storeTriggerStates } from './helpers' +import MockDate from 'mockdate' + +import { storeTriggerStates, isOlderThan, sub } from './helpers' const triggerStatesWithNotifsInfo = [ { @@ -55,3 +57,44 @@ describe('storeTriggerStates', () => { }) }) }) + +describe('sub', () => { + it('returns a Date object', () => { + expect( + sub(Date.now(), { days: 1, hours: 2, minutes: 3, seconds: 4 }) + ).toBeInstanceOf(Date) + expect( + sub(new Date(), { days: 1, hours: 2, minutes: 3, seconds: 4 }) + ).toBeInstanceOf(Date) + }) + + it('returns the expected past date', () => { + const base = Date.parse('28 Sep 2022 11:14:37 GMT') + + const past = sub(base, { days: 1, hours: 2, minutes: 3, seconds: 4 }) + expect(past.getUTCFullYear()).toBe(2022) + expect(past.getUTCMonth()).toBe(8) // Months are 0 indexed + expect(past.getUTCDate()).toBe(27) + expect(past.getUTCHours()).toBe(9) + expect(past.getUTCMinutes()).toBe(11) + expect(past.getUTCSeconds()).toBe(33) + }) +}) + +describe('isOlderThan', () => { + afterEach(() => { + MockDate.reset() + }) + + it('returns true if given date is older than given time parameters', () => { + MockDate.set(Date.parse('28 Sep 2022 11:14:37 GMT')) + + expect(isOlderThan('2022-09-28T11:14:33Z', { seconds: 3 })).toBe(true) + }) + + it('returns false if given date is not older than given time parameters', () => { + MockDate.set(Date.parse('28 Sep 2022 11:14:37 GMT')) + + expect(isOlderThan('2022-09-28T11:14:33Z', { seconds: 4 })).toBe(false) + }) +}) diff --git a/src/targets/services/konnectorAlerts/sendTriggerNotifications.spec.js b/src/targets/services/konnectorAlerts/sendTriggerNotifications.spec.js index c0616a79c3..b69c4f109c 100644 --- a/src/targets/services/konnectorAlerts/sendTriggerNotifications.spec.js +++ b/src/targets/services/konnectorAlerts/sendTriggerNotifications.spec.js @@ -1,5 +1,6 @@ import CozyClient from 'cozy-client' import { sendNotification } from 'cozy-notifications' +import MockDate from 'mockdate' import matchAll from 'utils/matchAll' import { sendTriggerNotifications } from './sendTriggerNotifications' @@ -222,6 +223,11 @@ describe('sendTriggerNotifications', () => { beforeEach(() => { sendNotification.mockClear() + MockDate.set('2020-01-02') + }) + + afterEach(() => { + MockDate.reset() }) const expectTriggerStatesToHaveBeenSaved = client => { diff --git a/src/targets/services/konnectorAlerts/shouldNotify.js b/src/targets/services/konnectorAlerts/shouldNotify.js index 8578062971..2700b88bf3 100644 --- a/src/targets/services/konnectorAlerts/shouldNotify.js +++ b/src/targets/services/konnectorAlerts/shouldNotify.js @@ -6,15 +6,16 @@ import { JOBS_DOCTYPE } from 'doctypes' import { getKonnectorSlug, fetchRegistryInfo, - isErrorActionable -} from './helpers' + isErrorActionable, + isOlderThan +} from 'targets/services/konnectorAlerts/helpers' /** * Returns whether we need to send a notification for a trigger * * @typedef {Object} ShouldNotifyResult - * @property {number} ok - Whether the trigger generates a notification - * @property {number} reason - If ok=false, describes why. + * @property {boolean} ok - Whether the trigger generates a notification + * @property {string} reason - If ok=false, describes why. * * @return {ShouldNotifyResult} */ @@ -37,6 +38,17 @@ export const shouldNotify = async ({ client, trigger, previousStates }) => { return { ok: false, reason: 'never-been-in-success' } } + // We do not want to send notifications for connectors that failed a long time + // ago without any retry since then. + // The last reminder is scheduled 7 days after the last failure plus a few + // minutes, with a maximum of 15 minutes, so we prevent notifications to be + // sent if we're past that delay. + if ( + isOlderThan(trigger.current_state.last_failure, { days: 7, minutes: 15 }) + ) { + return { ok: false, reason: 'last-failure-too-old' } + } + if ( previousState.status === 'errored' && isErrorActionable(previousState.last_error) diff --git a/src/targets/services/konnectorAlerts/shouldNotify.spec.js b/src/targets/services/konnectorAlerts/shouldNotify.spec.js new file mode 100644 index 0000000000..09729a5a61 --- /dev/null +++ b/src/targets/services/konnectorAlerts/shouldNotify.spec.js @@ -0,0 +1,73 @@ +import { createMockClient } from 'cozy-client' +import MockDate from 'mockdate' + +import { shouldNotify } from './shouldNotify' +import { sub } from './helpers' + +describe('shouldNotify', () => { + const setup = ({ last_failure } = {}) => { + const client = createMockClient({}) + client.query.mockResolvedValue({ data: {} }) + client.stackClient.fetchJSON.mockResolvedValue({ + latest_version: { manifest: { categories: ['banking'] } } + }) // connector's registry info + + const previousStates = { + fakeTrigger: {} + } + + const trigger = { + _id: 'fakeTrigger', + message: { + konnector: 'fakeBankingConnector' + }, + current_state: { + status: 'errored', + last_error: 'LOGIN_FAILED', // actionable error + last_success: sub(Date.now(), { days: 15 }).toISOString(), // has succeeded in the past + last_executed_job_id: 'fakeJob', + last_failure + } + } + + return { client, previousStates, trigger } + } + + beforeEach(() => { + MockDate.set(Date.now()) + }) + + afterEach(() => { + MockDate.reset() + }) + + describe('last failure date', () => { + it('returns a truthy result if given trigger failed less than 7 days and 15 minutes ago', async () => { + const { client, previousStates, trigger } = setup({ + last_failure: sub(Date.now(), { + days: 7, + minutes: 15 + }).toISOString() + }) + + expect(await shouldNotify({ client, trigger, previousStates })).toEqual({ + ok: true + }) + }) + + it('returns a falsy result if given trigger failed more than 7 days and 15 minutes ago', async () => { + const { client, previousStates, trigger } = setup({ + last_failure: sub(Date.now(), { + days: 7, + minutes: 15, + seconds: 1 + }).toISOString() + }) + + expect(await shouldNotify({ client, trigger, previousStates })).toEqual({ + ok: false, + reason: 'last-failure-too-old' + }) + }) + }) +}) From fe27012b4a12b9c3b89ef6319f621b28ed5012d7 Mon Sep 17 00:00:00 2001 From: Erwan Guyader Date: Mon, 3 Oct 2022 13:17:43 +0200 Subject: [PATCH 2/2] fix: Don't schedule konnector alerts too soon When a konnector fails for an actionnable reason (i.e. the user has the power to fix the underlying issue), we send a notification after the konnector has run and schedule 2 `konnectorAlerts` triggers 3 days and 7 days after to send notifications again in case the konnector is still failing to run. However, we set the starting point of these delays not at the moment when the `konnectorAlerts` service runs for the first time but at the moment the konnector failed for the last time. This date will always be in the past but it can be quite old thus generating reminders scheduled dates very close to the moment the service ran first and sent a notification to the user. It can even be in the past in which case `cozy-stack` will execute the trigger immediately after it is scheduled. In both situations, the user gets spammed with notifications. To prevent these, we will now make sure the reminders are scheduled to be sent at least 2 days after the moment the service first ran. --- .../konnectorAlerts/createTriggerAt.js | 27 +++++--- .../konnectorAlerts/createTriggerAt.spec.js | 62 +++++++++++++++---- .../services/konnectorAlerts/helpers.js | 7 +++ .../services/konnectorAlerts/helpers.spec.js | 40 +++++++++++- 4 files changed, 115 insertions(+), 21 deletions(-) diff --git a/src/targets/services/konnectorAlerts/createTriggerAt.js b/src/targets/services/konnectorAlerts/createTriggerAt.js index 43624e304a..fc3d8336b6 100644 --- a/src/targets/services/konnectorAlerts/createTriggerAt.js +++ b/src/targets/services/konnectorAlerts/createTriggerAt.js @@ -1,14 +1,23 @@ import { TRIGGER_DOCTYPE } from 'doctypes' import { logger } from 'ducks/konnectorAlerts' -import { ONE_DAY } from 'ducks/recurrence/constants' -import { getTriggerStates, fetchRelatedFuturAtTriggers } from './helpers' - -const dateInDays = (referenceDate, n) => { - return new Date(+new Date(referenceDate) + n * ONE_DAY) -} +import { + add, + getTriggerStates, + fetchRelatedFuturAtTriggers +} from 'targets/services/konnectorAlerts/helpers' const createTriggerAt = async ({ client, date, konnectorTriggerId }) => { try { + if (date <= add(Date.now(), { days: 2 })) { + // If the date is in the past or too close to the current execution of the + // service, we don't create a trigger. + logger( + 'info', + '@at trigger not created: this konnector trigger would be too close to this execution (less than 2 days)' + ) + return + } + await client.save({ _type: TRIGGER_DOCTYPE, type: '@at', @@ -56,19 +65,19 @@ export const createScheduledTrigger = async client => { if (relatedFuturAtTriggers.length > 0) { logger( 'info', - `@at triggers not created: @at triggers already existing in the futur for this konnector trigger` + `@at triggers not created: @at triggers already existing in the future for this konnector trigger` ) continue } await containerForTesting.createTriggerAt({ client, - date: dateInDays(triggerStates.last_failure, 3), + date: add(triggerStates.last_failure, { days: 3 }), konnectorTriggerId: id }) await containerForTesting.createTriggerAt({ client, - date: dateInDays(triggerStates.last_failure, 7), + date: add(triggerStates.last_failure, { days: 7 }), konnectorTriggerId: id }) } diff --git a/src/targets/services/konnectorAlerts/createTriggerAt.spec.js b/src/targets/services/konnectorAlerts/createTriggerAt.spec.js index 139e67199c..300a81bf93 100644 --- a/src/targets/services/konnectorAlerts/createTriggerAt.spec.js +++ b/src/targets/services/konnectorAlerts/createTriggerAt.spec.js @@ -1,7 +1,7 @@ import { createMockClient } from 'cozy-client' import { createScheduledTrigger, containerForTesting } from './createTriggerAt' -import { getTriggerStates, fetchRelatedFuturAtTriggers } from './helpers' +import { fetchRelatedFuturAtTriggers, getTriggerStates, sub } from './helpers' jest.mock('./helpers', () => ({ ...jest.requireActual('./helpers'), @@ -26,7 +26,7 @@ describe('createTriggerAt', () => { expect(containerForTesting.createTriggerAt).not.toHaveBeenCalled() }) - it("should not create @at triggers if the konnector doesn't sent notification", async () => { + it('should not create @at triggers if the service should not notify', async () => { getTriggerStates.mockResolvedValue({ trigger01Id: { shouldNotify: { ok: false } } }) @@ -36,7 +36,7 @@ describe('createTriggerAt', () => { expect(containerForTesting.createTriggerAt).not.toHaveBeenCalled() }) - it('should not create @at triggers if there are already created in the futur', async () => { + it('should not create @at triggers if some were already scheduled in the future', async () => { getTriggerStates.mockResolvedValue({ trigger01Id: { shouldNotify: { ok: true } @@ -51,16 +51,56 @@ describe('createTriggerAt', () => { expect(containerForTesting.createTriggerAt).not.toHaveBeenCalled() }) - it('should create two @at triggers', async () => { - getTriggerStates.mockResolvedValue({ - trigger01Id: { - shouldNotify: { ok: true } - } + describe('when the last trigger failure occurred less than 24 hours ago', () => { + it('should create two @at triggers', async () => { + getTriggerStates.mockResolvedValue({ + trigger01Id: { + shouldNotify: { ok: true }, + last_failure: sub(Date.now(), { hours: 23, minutes: 59, seconds: 59 }) + } + }) + fetchRelatedFuturAtTriggers.mockResolvedValue([]) + + await createScheduledTrigger(client) + + expect(client.save).toHaveBeenCalledTimes(2) }) - fetchRelatedFuturAtTriggers.mockResolvedValue([]) + }) - await createScheduledTrigger(client) + describe('when the last trigger failure occurred less than 5 days ago', () => { + it('should create one @at trigger', async () => { + getTriggerStates.mockResolvedValue({ + trigger01Id: { + shouldNotify: { ok: true }, + last_failure: sub(Date.now(), { + days: 4, + hours: 23, + minutes: 59, + seconds: 59 + }) + } + }) + fetchRelatedFuturAtTriggers.mockResolvedValue([]) + + await createScheduledTrigger(client) + + expect(client.save).toHaveBeenCalledTimes(1) + }) + }) - expect(containerForTesting.createTriggerAt).toHaveBeenCalledTimes(2) + describe('when the last trigger failure occurred 5 days ago or more', () => { + it('should not create @at triggers', async () => { + getTriggerStates.mockResolvedValue({ + trigger01Id: { + shouldNotify: { ok: true }, + last_failure: sub(Date.now(), { days: 5 }) + } + }) + fetchRelatedFuturAtTriggers.mockResolvedValue([]) + + await createScheduledTrigger(client) + + expect(client.save).toHaveBeenCalledTimes(0) + }) }) }) diff --git a/src/targets/services/konnectorAlerts/helpers.js b/src/targets/services/konnectorAlerts/helpers.js index c209efac80..8e6056a1ed 100644 --- a/src/targets/services/konnectorAlerts/helpers.js +++ b/src/targets/services/konnectorAlerts/helpers.js @@ -1,6 +1,10 @@ import memoize from 'lodash/memoize' import keyBy from 'lodash/keyBy' import get from 'lodash/get' +import addDays from 'date-fns/add_days' +import addHours from 'date-fns/add_hours' +import addMinutes from 'date-fns/add_minutes' +import addSeconds from 'date-fns/add_seconds' import parse from 'date-fns/parse' import subDays from 'date-fns/sub_days' import subHours from 'date-fns/sub_hours' @@ -121,6 +125,9 @@ export const fetchRelatedFuturAtTriggers = async (client, id) => { return data } +export const add = (base, { days = 0, hours = 0, minutes = 0, seconds = 0 }) => + addDays(addHours(addMinutes(addSeconds(base, seconds), minutes), hours), days) + export const sub = (base, { days = 0, hours = 0, minutes = 0, seconds = 0 }) => subDays(subHours(subMinutes(subSeconds(base, seconds), minutes), hours), days) diff --git a/src/targets/services/konnectorAlerts/helpers.spec.js b/src/targets/services/konnectorAlerts/helpers.spec.js index 382df2058e..4730192584 100644 --- a/src/targets/services/konnectorAlerts/helpers.spec.js +++ b/src/targets/services/konnectorAlerts/helpers.spec.js @@ -1,7 +1,7 @@ import { createMockClient } from 'cozy-client' import MockDate from 'mockdate' -import { storeTriggerStates, isOlderThan, sub } from './helpers' +import { add, storeTriggerStates, isOlderThan, sub } from './helpers' const triggerStatesWithNotifsInfo = [ { @@ -81,6 +81,44 @@ describe('sub', () => { }) }) +describe('add', () => { + it('returns a Date object', () => { + expect( + add(Date.now(), { + days: 1, + hours: 2, + minutes: 3, + seconds: 4 + }) + ).toBeInstanceOf(Date) + expect( + add(new Date(), { + days: 1, + hours: 2, + minutes: 3, + seconds: 4 + }) + ).toBeInstanceOf(Date) + }) + + it('returns the expected future date', () => { + const base = Date.parse('28 Sep 2022 11:14:37 GMT') + + const future = add(base, { + days: 1, + hours: 2, + minutes: 3, + seconds: 4 + }) + expect(future.getUTCFullYear()).toBe(2022) + expect(future.getUTCMonth()).toBe(8) // Months are 0 indexed + expect(future.getUTCDate()).toBe(29) + expect(future.getUTCHours()).toBe(13) + expect(future.getUTCMinutes()).toBe(17) + expect(future.getUTCSeconds()).toBe(41) + }) +}) + describe('isOlderThan', () => { afterEach(() => { MockDate.reset()