Skip to content
Snippets Groups Projects

Adding OKR checkin reminder email notifications

Merged Darby Frey requested to merge okr-checkin-reminder-notifications into master

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

Screenshot_2023-09-12_at_3.39.36_PM

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

  1. Enable the okrs_mvc and okr_checkin_reminders feature flag locally. In the rails console, run:

    Feature.enable(:okrs_mvc)
    Feature.enable(:okr_checkin_reminders)
  2. 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
  3. Run the cron job. In the rails console, run:

    date = Date.today.next_week(:tuesday)
    Okrs::CheckinReminderEmailsCronWorker.new(date: date).perform
  4. 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.

Edited by Darby Frey

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Contributor
    7 Warnings
    :warning: This merge request is quite big (502 lines changed), please consider splitting it into multiple merge requests.
    :warning: f0b9b21d: The commit subject must contain at least 3 words. For more information, take a look at our Commit message guidelines.
    :warning: 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.
    :warning: 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.
    :warning: 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.
    :warning: 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.
    :warning:

    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:

    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 current availability (@mokhax) (UTC-6) Avielle Wolfe current availability (@avielle) (UTC+2)
    database Jarka Košanová current availability (@jarka) (UTC+2) Prabakaran Murugesan current availability (@praba.m7n) (UTC+2)
    frontend Alper Akgun current availability (@a_akgun) (UTC+3) Olena HK. current availability (@ohoral) (UTC+2)

    Please check reviewer's status!

    • available Reviewer is available!
    • unavailable Reviewer is unavailable!

    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 :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • A deleted user added Data WarehouseImpact Check label
  • Darby Frey mentioned in commit 18a814be

    mentioned in commit 18a814be

  • Darby Frey added 1 commit

    added 1 commit

    • 18a814be - Adding OKR checkin reminder email notifications

    Compare with previous version

  • Darby Frey mentioned in commit 72725086

    mentioned in commit 72725086

  • Darby Frey added 22 commits

    added 22 commits

    Compare with previous version

  • Darby Frey added 1 commit

    added 1 commit

    • 9625faea - Fixing issue with unsupported cross table join, updated tests

    Compare with previous version

  • Contributor

    Allure report

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :exclamation: test report for b23c27d5

    expand 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: :x: test report for b23c27d5

    expand 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  | ❌     |
    +------------------+--------+--------+---------+-------+-------+--------+
  • Darby Frey added 1 commit

    added 1 commit

    • 80e2d2b6 - Updated key_results finder and specs

    Compare with previous version

  • Darby Frey changed the description

    changed the description

  • Darby Frey changed the description

    changed the description

  • Darby Frey added 1 commit

    added 1 commit

    • 2494693b - Refactoring and cleaning up tests

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading