ER-659 Nudge emails #726

jack-coggin · 2023-07-19T15:12:56Z

ER-659 - complete module nudge emails
ER-660 - start training nudge emails
ER-658 - complete registration nudge emails

viezly · 2023-07-19T15:13:19Z

Changes preview:

Legend:

👀 Review pull request on Viezly

peterdavidhamilton

Are we testing for duplicate mail jobs?

peterdavidhamilton · 2023-08-10T15:04:59Z

app/services/nudge_mail.rb

+
+  # @param user [User]
+  # @return [Training::Module]
+  def module_in_progress(user)


I'd say because this isn't performing its own logic anymore and is just calling a chine of methods, we could remove it.

peterdavidhamilton · 2023-08-10T15:07:58Z

spec/services/nudge_mail_spec.rb

+      allow(NotifyMailer).to receive(:continue_training)
+    end
+
+    it 'The notify mailer is called with the correct users' do


Not essential but it can be useful to write as if the audience is not a developer.

it 'emails...

peterdavidhamilton · 2023-08-10T15:08:51Z

app/jobs/mail_job.rb

+private
+
+  def queued?
+    Que.job_stats.any? { |job| job[:job_class] == 'MailJob' && job[:count] > 1 }


HMW get the same check for our dashboard?

I was looking at a way to stop duplicate jobs being added to the queue in the first place, but wasn't able to make much progress. Instead, I've added a helper to guard against duplicate jobs in the run method of the jobs dbbefa7

peterdavidhamilton · 2023-08-10T15:10:02Z

app/models/user.rb

@@ -321,6 +331,11 @@ def dashboard_row
    data_attributes.dup.merge(module_ttc)
  end

+  # @return [Training::Module]


I think we should let the course object answer and not add an extra method on user. Is the doc accurate?

peterdavidhamilton

Would creating a reusable mail spec matcher be a good idea at some point? With two variables, a list of users it should contact, and a list it should exclude.

peterdavidhamilton · 2023-08-11T17:56:10Z

app/jobs/scheduler_helper.rb

@@ -0,0 +1,5 @@
+module SchedulerHelper


I suggest we instead create a base job for the application, similar to the convention Rails adopts with controllers.

The predicate could be more descriptive. It's queued with the first criteria, but what we are interested in is the second that checks for duplication.

When the run is skipped, how can we get notified that was the reason?

peterdavidhamilton · 2023-08-11T17:59:35Z

app/services/nudge_mail.rb

@@ -0,0 +1,27 @@
+class NudgeMail


What's the benefit of combining email nudges into one job, rather than dedicated jobs for each type?

peterdavidhamilton · 2023-08-11T18:00:23Z

app/services/nudge_mail.rb

+
+private
+
+  # @return [ActiveRecord::Relation]


Run .class and use the return value for the doc.

peterdavidhamilton · 2023-08-11T18:04:08Z

spec/services/nudge_mail_spec.rb

+    end
+  end
+
+  context 'when users are a month old and have completed registration but not started training' do


I'm imagining a month old baby learning about how they should be cared for. 🍼

peterdavidhamilton · 2023-08-11T18:06:35Z

spec/services/nudge_mail_spec.rb

@@ -0,0 +1,105 @@
+require 'rails_helper'
+
+RSpec.describe NudgeMail do


Can with progress be leveraged here?

peterdavidhamilton · 2023-08-11T18:13:30Z

app/services/nudge_mail.rb

+
+  # @return [Array<User>]
+  def continue_training_recipients
+    recent_visits = Ahoy::Visit.where(started_at: 4.weeks.ago.end_of_day..Time.zone.now)


If this were a scope on the visit I think it would help

peterdavidhamilton · 2023-08-11T18:19:22Z

db/schema.rb

@@ -150,6 +150,13 @@
    t.index ["user_id"], name: "index_responses_on_user_id"
  end

+  create_table "training_module_records", force: :cascade do |t|


I still think we need to articulate what this is for. I'd expect to see an explicit stamp for the date it went live.

Would a new Dev expect the updated_at time to track the modification time of the entry for example...

What is this thing responsible for?

peterdavidhamilton · 2023-08-11T18:22:36Z

spec/mailers/notify_mailer_spec.rb

@@ -87,4 +87,38 @@
      end
    end
  end
+
+  describe 'training email opt in' do


Can we collaborate and workshop the doc format this puts out.

peterdavidhamilton · 2023-08-11T18:28:01Z

spec/services/nudge_mail_spec.rb

+
+      allow(NotifyMailer).to receive(:continue_training)
+    end
+


My tactic is to use variables if I can make code/specs easier to skim read. Does using a variable to hold the module object improve your reading of this spec?

peterdavidhamilton · 2023-08-11T18:31:15Z

app/jobs/mail_job.rb

@@ -0,0 +1,8 @@
+class MailJob < Que::Job


Would this job benefit from any logging like the first job?

jack-coggin added 11 commits July 5, 2023 14:54

wip: add complete registration reminder email

37b574f

wip: update spec

7d1d9fc

add start training email notification

252fedb

add training emails condition to email recipients

c3e59ce

update recipient selector spec and user scope

0aff85a

rubocop

d5a3826

resolve merge conflicts

c4459d3

wip: add nudge email for new module

11f7313

wip: new module alert email

abef8a2

add nudge emails for new module

b35dd78

update recipient selector and spec

7eb3fe9

Merge branch 'main' into ER-659-complete-module-email

d9dbebe

jack-coggin had a problem deploying to test July 19, 2023 15:14 — with GitHub Actions Failure

Merge branch 'main' into ER-659-complete-module-email

f3b2245

jack-coggin had a problem deploying to test July 20, 2023 08:01 — with GitHub Actions Failure

Merge branch 'main' into ER-659-complete-module-email

14c6f27

jack-coggin had a problem deploying to test July 21, 2023 10:41 — with GitHub Actions Failure

jack-coggin marked this pull request as ready for review July 21, 2023 10:45

jack-coggin had a problem deploying to test July 21, 2023 10:46 — with GitHub Actions Failure

fix syntax error

9246796

jack-coggin had a problem deploying to test July 21, 2023 11:05 — with GitHub Actions Failure

remove new module nudge email

88a9781

jack-coggin had a problem deploying to test July 21, 2023 14:26 — with GitHub Actions Failure

refactor nudge mails

6fd6c0a

jack-coggin had a problem deploying to test July 24, 2023 14:11 — with GitHub Actions Failure

jack-coggin had a problem deploying to test July 24, 2023 14:50 — with GitHub Actions Failure

jack-coggin added 2 commits July 24, 2023 16:20

Merge branch 'main' into ER-659-complete-module-email

70d9889

update dashboard spec update time

7ff6bbc

jack-coggin temporarily deployed to test July 24, 2023 15:33 — with GitHub Actions Inactive

jack-coggin temporarily deployed to test August 9, 2023 12:21 — with GitHub Actions Inactive

change que version

6b520f9

jack-coggin had a problem deploying to test August 9, 2023 12:49 — with GitHub Actions Failure

protect against duplicate mail jobs in queue

80daaf4

jack-coggin had a problem deploying to test August 10, 2023 12:34 — with GitHub Actions Failure

rubocop

1be52c9

jack-coggin temporarily deployed to test August 10, 2023 12:50 — with GitHub Actions Inactive

Merge branch 'main' into ER-659-complete-module-email

19cf386

jack-coggin had a problem deploying to test August 10, 2023 12:51 — with GitHub Actions Failure

jack-coggin requested a review from peterdavidhamilton August 10, 2023 13:09

jack-coggin temporarily deployed to test August 10, 2023 14:31 — with GitHub Actions Inactive

peterdavidhamilton suggested changes Aug 10, 2023

View reviewed changes

add yard annotation

cb5e978

jack-coggin temporarily deployed to test August 10, 2023 15:12 — with GitHub Actions Inactive

add job helper to guard against duplicate queued jobs

dbbefa7

jack-coggin had a problem deploying to test August 11, 2023 11:10 — with GitHub Actions Failure

refactor nudge mail service and spec

62482be

jack-coggin had a problem deploying to test August 11, 2023 11:31 — with GitHub Actions Failure

Merge branch 'main' into ER-659-complete-module-email

71331c5

jack-coggin had a problem deploying to test August 11, 2023 12:26 — with GitHub Actions Failure

jack-coggin requested a review from peterdavidhamilton August 11, 2023 13:17

jack-coggin temporarily deployed to test August 11, 2023 13:17 — with GitHub Actions Inactive

peterdavidhamilton reviewed Aug 11, 2023

View reviewed changes

peterdavidhamilton suggested changes Aug 14, 2023

View reviewed changes

jack-coggin closed this Aug 18, 2023

jack-coggin temporarily deployed to development August 18, 2023 12:54 — with GitHub Actions Inactive

jack-coggin had a problem deploying to production August 18, 2023 12:54 — with GitHub Actions Failure

peterdavidhamilton modified the milestones: rc0.9.0, email Aug 22, 2023

jack-coggin deleted the ER-659-complete-module-email branch September 11, 2023 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ER-659 Nudge emails #726

ER-659 Nudge emails #726

jack-coggin commented Jul 19, 2023 •

edited by jira bot

Loading

viezly bot commented Jul 19, 2023 •

edited

Loading

peterdavidhamilton left a comment

peterdavidhamilton Aug 10, 2023

jack-coggin Aug 11, 2023

peterdavidhamilton Aug 10, 2023

jack-coggin Aug 11, 2023

peterdavidhamilton Aug 10, 2023

jack-coggin Aug 11, 2023

peterdavidhamilton Aug 10, 2023

jack-coggin Aug 11, 2023

peterdavidhamilton left a comment

peterdavidhamilton Aug 11, 2023

peterdavidhamilton Aug 11, 2023

peterdavidhamilton Aug 11, 2023

peterdavidhamilton Aug 11, 2023

peterdavidhamilton Aug 11, 2023

peterdavidhamilton Aug 11, 2023

peterdavidhamilton Aug 11, 2023

peterdavidhamilton Aug 11, 2023

peterdavidhamilton Aug 11, 2023

peterdavidhamilton Aug 11, 2023

		@@ -0,0 +1,105 @@
		require 'rails_helper'

		RSpec.describe NudgeMail do

ER-659 Nudge emails #726

ER-659 Nudge emails #726

Conversation

jack-coggin commented Jul 19, 2023 • edited by jira bot Loading

viezly bot commented Jul 19, 2023 • edited Loading

peterdavidhamilton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterdavidhamilton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-coggin commented Jul 19, 2023 •

edited by jira bot

Loading

viezly bot commented Jul 19, 2023 •

edited

Loading