Adding OKR checkin reminder email notifications
What does this MR do and why?
This MR is part of https://gitlab.com/gitlab-org/incubation-engineering/okr/meta/-/issues/19+, and enables the reminder notifications for which the setting was added in Adding checkin reminder setting quick action (!130371 - merged)
This MR adds a new Sidekiq cron job that runs once per day. The job uses Okrs::CheckinReminderKeyResultFinder
to return an array of all opened
work items of type key_result
with assignees that have been configured with the notification cadence (weekly
, twice-monthly
, or monthly
).
It then iterates over each item and sends individual emails to each assignee.
Screenshots or screen recordings
Email Screenshot
Database Review
The daily cron job introduces the three queries below via the finder class:
Finder Query
SELECT
"issues".*
FROM
"issues"
INNER JOIN "issue_assignees" ON "issue_assignees"."issue_id" = "issues"."id"
INNER JOIN "work_item_parent_links" ON "work_item_parent_links"."work_item_id" = "issues"."id"
INNER JOIN "issues" "work_item_parents_issues" ON "work_item_parents_issues"."id" = "work_item_parent_links"."work_item_parent_id"
LEFT OUTER JOIN "work_item_progresses" ON "work_item_progresses"."issue_id" = "issues"."id"
WHERE
("issues"."state_id" IN (1))
AND (
"issues"."work_item_type_id" = (
SELECT
"work_item_types"."id"
FROM
"work_item_types"
WHERE
"work_item_types"."base_type" = 6
LIMIT
1
)
)
AND "work_item_parent_links"."work_item_parent_id" IN (
WITH RECURSIVE "base_and_descendants" AS (
(
SELECT
"issues"."id"
FROM
"issues"
INNER JOIN "work_item_progresses" ON "work_item_progresses"."issue_id" = "issues"."id"
WHERE
"work_item_progresses"."reminder_frequency" = 1
AND ("issues"."state_id" IN (1))
AND (
"issues"."work_item_type_id" = (
SELECT
"work_item_types"."id"
FROM
"work_item_types"
WHERE
"work_item_types"."base_type" = 5
LIMIT
1
)
)
AND (
NOT EXISTS (
SELECT
FROM
work_item_parent_links
WHERE
work_item_id = issues.id
)
)
)
UNION
(
SELECT
"issues"."id"
FROM
"issues",
"base_and_descendants",
"work_item_parent_links"
WHERE
"work_item_parent_links"."work_item_id" = "issues"."id"
AND "work_item_parent_links"."work_item_parent_id" = "base_and_descendants"."id"
)
)
SELECT
"issues"."id"
FROM
"base_and_descendants" AS "issues"
)
AND (
work_item_progresses.last_reminder_sent_at IS NULL
OR work_item_progresses.last_reminder_sent_at <= '2023-09-13'
)
GROUP BY
"issues"."id"
ORDER BY
"issues"."id" ASC
LIMIT
1000
Query plan:
cold cache: https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/22655/commands/73037
warm cache: https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/22655/commands/73038
Update Query
The following update query is introduced in Okrs::CheckinReminderEmailsCronWorker#deliver_reminder
.
UPDATE
"work_item_progresses"
SET
"updated_at" = '2023-09-13 18:22:46.209470',
"last_reminder_sent_at" = '2023-09-13 18:22:46.208466'
WHERE
"work_item_progresses"."issue_id" = 134631298
Query Plan: https://console.postgres.ai/gitlab/gitlab-production-tunnel-pg12/sessions/22174/commands/71674
How to set up and validate locally
-
Enable the
okrs_mvc
andokr_checkin_reminders
feature flag locally. In the rails console, run:Feature.enable(:okrs_mvc) Feature.enable(:okr_checkin_reminders)
-
Create some OKR seed data. In the rails console run:
objective = WorkItem.create( title: "Sample Objective", author: User.first, project: Project.first, work_item_type_id: ::WorkItems::Type.default_by_type(:objective).id ) objective.create_progress(reminder_frequency: 'weekly') key_result = WorkItem.create( title: "Sample Key Result", author: User.first, project: Project.first, work_item_type_id: ::WorkItems::Type.default_by_type(:key_result).id ) WorkItems::ParentLink.create( work_item: key_result, work_item_parent: objective ) key_result.assignees<<User.first
-
Run the cron job. In the rails console, run:
date = Date.today.next_week(:tuesday) Okrs::CheckinReminderEmailsCronWorker.new(date: date).perform
-
Load Letter Opener in the browser and you should see the checkin notification email
http://127.0.0.1:3000/rails/letter_opener/
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.
Merge request reports
Activity
assigned to @darbyfrey
mentioned in commit 4ea53aa7
added 1 commit
- 4ea53aa7 - Adding OKR checkin reminder email notifications
- A deleted user
added backend database databasereview pending frontend labels
- Resolved by Darby Frey
7 Warnings This merge request is quite big (502 lines changed), please consider splitting it into multiple merge requests. f0b9b21d: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines. 67760c40: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. e8d40a41: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 3ba0796a: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. 49083942: Commits that change 30 or more lines across at least 3 files should describe these changes in the commit body. For more information, take a look at our Commit message guidelines. featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
Reviewer roulette
Changes that require review have been detected!
Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:
Category Reviewer Maintainer backend Mo Khan (
@mokhax
) (UTC-6)Avielle Wolfe (
@avielle
) (UTC+2)database Jarka Košanová (
@jarka
) (UTC+2)Prabakaran Murugesan (
@praba.m7n
) (UTC+2)frontend Alper Akgun (
@a_akgun
) (UTC+3)Olena HK. (
@ohoral
) (UTC+2)Please check reviewer's status!
Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.
To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.
Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.
Sidekiq queue changes
This merge request contains changes to Sidekiq queues. Please follow the documentation on changing a queue's urgency.
These queues were added:
cronjob:okrs_checkin_reminder_emails_cron
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Danger- A deleted user
added Data WarehouseImpact Check label
mentioned in commit 18a814be
added 1 commit
- 18a814be - Adding OKR checkin reminder email notifications
mentioned in commit 72725086
added 22 commits
-
18a814be...22767151 - 20 commits from branch
master
- 72725086 - Adding OKR checkin reminder email notifications
- f518e629 - Refactored finder, updated tests
-
18a814be...22767151 - 20 commits from branch
added 1 commit
- 9625faea - Fixing issue with unsupported cross table join, updated tests
Allure report
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for b23c27d5expand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Plan | 55 | 0 | 0 | 0 | 55 | ✅ | | Create | 45 | 0 | 2 | 0 | 47 | ✅ | | Manage | 13 | 0 | 1 | 0 | 14 | ✅ | | Data Stores | 18 | 0 | 4 | 1 | 22 | ❗ | | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | | Govern | 36 | 0 | 0 | 0 | 36 | ✅ | | Verify | 8 | 0 | 0 | 0 | 8 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Framework sanity | 0 | 0 | 1 | 0 | 1 | ➖ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 179 | 0 | 9 | 1 | 188 | ❗ | +------------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for b23c27d5expand test summary
+-----------------------------------------------------------------------+ | suites summary | +------------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +------------------+--------+--------+---------+-------+-------+--------+ | Create | 543 | 0 | 105 | 16 | 648 | ❗ | | Monitor | 36 | 0 | 7 | 0 | 43 | ✅ | | Verify | 147 | 0 | 15 | 0 | 162 | ✅ | | Manage | 151 | 7 | 13 | 9 | 171 | ❌ | | Package | 226 | 0 | 17 | 7 | 243 | ❗ | | Plan | 246 | 0 | 10 | 0 | 256 | ✅ | | Data Stores | 101 | 0 | 19 | 3 | 120 | ❗ | | Fulfillment | 8 | 0 | 69 | 0 | 77 | ✅ | | Govern | 188 | 0 | 0 | 6 | 188 | ❗ | | Growth | 0 | 0 | 6 | 0 | 6 | ➖ | | Systems | 8 | 0 | 0 | 0 | 8 | ✅ | | Release | 15 | 0 | 3 | 0 | 18 | ✅ | | Configure | 1 | 0 | 9 | 0 | 10 | ✅ | | Analytics | 7 | 0 | 0 | 0 | 7 | ✅ | | GitLab Metrics | 2 | 0 | 1 | 0 | 3 | ✅ | | ModelOps | 0 | 0 | 6 | 0 | 6 | ➖ | | Framework sanity | 0 | 0 | 5 | 0 | 5 | ➖ | | Secure | 6 | 0 | 3 | 0 | 9 | ✅ | +------------------+--------+--------+---------+-------+-------+--------+ | Total | 1685 | 7 | 288 | 41 | 1980 | ❌ | +------------------+--------+--------+---------+-------+-------+--------+