Skip to content

Commit

Permalink
fix: Don't schedule konnector alerts too soon
Browse files Browse the repository at this point in the history
  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.
  • Loading branch information
taratatach committed Oct 11, 2022
1 parent 161658d commit fe27012
Show file tree
Hide file tree
Showing 4 changed files with 115 additions and 21 deletions.
27 changes: 18 additions & 9 deletions src/targets/services/konnectorAlerts/createTriggerAt.js
Original file line number Diff line number Diff line change
@@ -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',
Expand Down Expand Up @@ -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
})
}
Expand Down
62 changes: 51 additions & 11 deletions src/targets/services/konnectorAlerts/createTriggerAt.spec.js
Original file line number Diff line number Diff line change
@@ -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'),
Expand All @@ -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 } }
})
Expand All @@ -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 }
Expand All @@ -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)
})
})
})
7 changes: 7 additions & 0 deletions src/targets/services/konnectorAlerts/helpers.js
Original file line number Diff line number Diff line change
@@ -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'
Expand Down Expand Up @@ -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)

Expand Down
40 changes: 39 additions & 1 deletion src/targets/services/konnectorAlerts/helpers.spec.js
Original file line number Diff line number Diff line change
@@ -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 = [
{
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit fe27012

Please sign in to comment.