Extract email sending logic from deliver_poste_messages cron script into lib/cdo/poste.rb #31456
Conversation
…nto lib/cdo/poste.rb To make it easier to write unit tests
| end | ||
| end | ||
|
|
||
| class Deliverer |
There was a problem hiding this comment.
Is there a reason not to give each class its own file?
There was a problem hiding this comment.
mostly a desire not to clutter up the already-large lib/cdo directory
shared/test/test_deliverer.rb
Outdated
| @@ -0,0 +1,48 @@ | |||
| require_relative 'test_helper' | |||
There was a problem hiding this comment.
Why'd you put a test for a class in lib/cdo in shared/test instead of lib/test/cdo?
There was a problem hiding this comment.
oh, that's a much better location. I just looked for where the existing Poste tests were
shared/test/test_deliverer.rb
Outdated
| def setup | ||
| @fake_smtp = FakeSmtp.new | ||
| Deliverer.any_instance.stubs(:reset_connection).returns(@fake_smtp) | ||
| @deliverer = Deliverer.new({}) |
There was a problem hiding this comment.
thanks for pointing me in this direction!
shared/test/test_deliverer.rb
Outdated
| params: {}.to_json | ||
| } | ||
| @deliverer.send(delivery) | ||
| message = @fake_smtp.instance_variable_get(:@message) |
There was a problem hiding this comment.
It might read better to add
attr_reader :message, :from_address, :to_addressto FakeSmtp. My only concern would be whether you think the test becomes misleading as documentation of how you'd typically use this class.
There was a problem hiding this comment.
I'll add a comment explaining why I'm doing that
shared/test/test_deliverer.rb
Outdated
| # Sequel doesn't have a "find or create by", so we implement it manually | ||
| message = POSTE_DB[:poste_messages].where(name: "dashboard").first | ||
| message_id = message.nil? ? POSTE_DB[:poste_messages].insert({name: "dashboard"}) : message[:id] |
There was a problem hiding this comment.
TIL! Looks like it only exists if you're using Sequel models, which we don't actually do.
|
Thanks for your help! |
Description
To make it easier to write unit tests; also, add some unit tests!
Note that this change also requires that we lazily populate the MESSAGE_TEMPLATES constant: 3966745
Otherwise, circle will fail to build because this code expects the database to already be set up as soon as the file gets required, rather than not until use.
This is a follow-up to #31421, as it adds the unit tests that would have caught the error had they existed.
Reviewer Checklist: