feat: New developer notifications emailed to businesses

[jacobdaddario]

This is a work-in-progress PR that aims to resolve #208. In order to deploy these changes, the Heroku scheduler add-on will eventually need to be added to the website.

Tracking feature progress:

• Add a new column to the Business model,new_developer_notifications, as an enum with values none, daily, and weekly; defaulted to none
• Add an option for this field on /businesses/:id/edit
• Add a job that gathers developer profiles added in the last 24 hours and last 7 days and sends a respective email to paying customers based on their preference
• Run the daily job every day at 9am EST bundle exec rake developer_digest:daily
• Run the weekly daily job every day at 9am EST bundle exec rake developer_digest:weekly
• Add an unsubscribe link to the bottom of the email that links to editing their business profile
• Highlight the feature on the /pricing page

Pull request checklist

• Your code contains tests relevant for code you modified
• You have linted and tested the project with bin/check

[jacobdaddario]

@joemasilotti Ready to go. Just have a few things I wanted to make note of in advance of your review. Additionally, the only thing that isn't finished is scheduling the rake tasks. Unless you give me access to Heroku, you'll have to do it. It's a pretty simple configuration with the scheduler add-on. You should be able to set the appropriate frequency for each task using that tool as well.

[joemasilotti]

Amazing work, Jacob! Thanks so much for this.

I left a ton of comments, but they are mostly tiny nitpicky suggestions that you can directly accept (assuming they work).

The big bit of work I requested is moving the mailing logic out of the rake task and into a new model that can be tested directly. And a bit of changes to the actual email template.

Let me know if you'd like me to handle any of this and thanks again!

[jacobdaddario]

Looked over things really briefly. I think most of what you're saying makes sense. Like you said, the model is the main thing, and the rest come primarily down to copy and code style.

Only thing I wanted to make sure that you were aware of is the fact that you can't hyperlink in plain text emails. Other than that I can implement all of the suggested changes. I particularly like the bit about extracting the rake task logic out into a model. Good thinking with that.

I want to stick to my personal commitment about another project, so I'll be spending time on that this weekend when I have time, so I actually won't get around to this until next week. Up to you if you want to push this across this finish line. I'm fine either way.

[joemasilotti]

Only thing I wanted to make sure that you were aware of is the fact that you can't hyperlink in plain text emails.

That's correct, but you can add the URL for someone to copy/paste in their browser. I'm doing that in the AdminMailer text formats.

I want to stick to my personal commitment about another project, so I'll be spending time on that this weekend when I have time, so I actually won't get around to this until next week. Up to you if you want to push this across this finish line. I'm fine either way.

Sounds great, thanks for the heads up! I might try to pick up bits of this here and there and will make sure to push whenever I make changes.

[joemasilotti]

@jacobdaddario, a heads up that I'm directly testing a rake task in PR #225 and it seems to be much less work than it used to be. Check out this test for an example, which tests this task.

I'm happy to go with this approach or a custom object; your call!

[jacobdaddario]

I think I'm gonna go with a little command pattern/service object. Seems very suitable here. Any strong opinions on where that goes? I'm thinking about putting it under a Business namespace in the models folder.

[joemasilotti]

Sounds great on both accounts! A PORO that gets called from the rake task sounds 👌

[joemasilotti]

I applied all of the changes that I requested that were still unresolved. Most of the stuff was around copy/design in the emails, the enums, and naming. I think all that's left is is testing the rake tasks!

One thing that I overlooked is we need a paywall or CTA in the form for folks that don't have an active business subscription. I'm not sure what the best design is for that just yet, but wanted to call it out. We can merge this in with a feature flag if need be. (Also in the controller to reject the param.)

[jacobdaddario]

@joemasilotti Thank you! I really appreciate it. By the way, I did some searching about the timestamp precision thing again, and I found this: https://github.com/rails/rails/blob/2ea5268cd2994a8d4628fbdb02c64e4d25549c7f/activerecord/lib/active_record/migration/compatibility.rb#L220

This was the only compatibility change for timestamps. It changed in Rails 6 which is why we're seeing the backwards compatibility change for Rails 5.2. Is there a reason you think Rails 7 shouldn't add precision by default? From what I can see, it never changed back. At least as evidenced in this compatibility file.

Edit:
And here's the line in the source that adds the precision. Is your database not compatible with precision arguments? What version of Postgresql are you using? Are you using MySQL?

[joemasilotti]

Weird! Thanks for diving in. I'm using PostgreSQL 14.0, which is one minor release behind latest.

Are you saying that the latest versions remove the precision from db/schema or add them in? Because there are other spots in the schema where precisions are explicitly set. Ideally, I'd like to keep them all consistent.

[jacobdaddario]

I'm saying the latest version includes the precisions by default. You can see that the change kicked in at version 6.0. We can confirm that by seeing they added a backwards compatibility method for version 5.2 and below in that compatibility file.

The second file, linked in the edit, shows where the current definition for timestamps is. If you follow the conditional to see if it supports it, it shows that the Postgres adapter supports it for all versions.

[joemasilotti]

Ahhh gotcha! Then is it safe to remove all of the precision options in the schema?

[jacobdaddario]

My thought process was that they'd generate in the schema by default, since that's the behavior for all Postgres versions since Rail 6.0, as far as I can tell.

[joemasilotti]

Interesting. And if you run rake db:migrate:reset does it add all the instances of precision: back in, remove them all, or some weird mix?

[jacobdaddario]

@joemasilotti It adds all of the datetime precision options back in.

[joemasilotti]

Oh man, for real? And then if you re-run the most recent migration it... removes a few instances of precision:?

[jacobdaddario]

I just pulled from main and ran it. I don't see anything different. Most recent commit seems to affect timezone, not precision. What makes you think it'd be different?

[joemasilotti]

What makes you think it'd be different?

Didn't this whole conversation spark from this PR removing a could instances of precision: in the schema? Or am I going crazy today.

[jacobdaddario]

What makes you think it'd be different?

Didn't this whole conversation spark from this PR removing a could instances of precision: in the schema? Or am I going crazy today.

Oh sorry for the confusion. I'm remarking about the problem in general.

[joemasilotti]

Gotcha. What do you propose moving forward? I think you have more knowledge around this than I do at this point.

[jacobdaddario]

I'm in favor of going with the default. I just don't get why your machine isn't producing that result anyways.

Also, had less time to work on this today than I thought I would. Pushed my changes, but it's unfinished. Got hung up on something and I'm too tired to debug it.

Considering reverting back to not having a command object and just testing the rake tasks directly.

[jacobdaddario]

@joemasilotti Alright, I think I've taken this as far as I can. There's a couple of things I want to mention before leaving this in your hands.

1. I've soft feature flagged email preferences for the time being. I imagine we'll need a separate PR to figure out how we want to gate premium content from companies that have not yet paid for a subscription. Obviously the field is still reachable, but it won't appear on the business form for now.
2. You'll need to set this up in Heroku's scheduler to run at the desired time whenever we complete this full feature.
3. I see you changed the query for developers into a date range as a matter of code quality. I'm not sure why you switched to Date.current though. This was breaking tests on my machine because Date.current is not timezone aware unlike Time.now. My PSQL database saves thing in UTC time, and without the timezone aware Time.now, developers were actually being created before the current time on my system. On a production environment this won't be a problem, but if there's no particular reason you used Date.current over Time.now, it might be best to keep the timezone aware methods.
4. Regarding the datetime precision thing. I'm fairly confident that default behavior should be having precision: 6, but it's up to you how you handle this. I'll say anecdotally, after resolving merge conflicts on this branch, whenever I tried to revert the precision values added to the schema, I'd get database integrity issues on my end. Rails would keep telling me I had pending migrations and wouldn't let me run tests. Consequently, I've committed he precision values in this PR.

After I get #220 merged, I'll probably take a break from committing for a while. I'll be honest that I struggled with this branch for some reason, and I don't want to take away from your development time by making do code reviews. Plus, it'll let me focus on my personal project that I've been neglecting.

[joemasilotti]

1. Awesome, this is perfect for now. I'm going to change it to a "real" Feature.rb flag so we don't have the environment check in the view. From there, I'm hoping to add a Subscribe button to /developers.
2. 👍
3. Hmm, I that's backwards. Both Date.current and Time.current take into account time zone. Time.now does not. Could they be failing because the created developers were "in between" UTC's midnight and your current time zone?
4. Gotcha, sounds good. I'll keep what you have and if I run into a different diff I'll address it later.

Outside of hearing your feedback on number 3. I can take this over the finish line. Thanks again for the contribution, this was a lot of work and I really appreciate it!

[jacobdaddario]

Oh shoot, you're totally right about Date.current. I was in a rush last night, and I must have skimmed TimeZone.now as Time.now.

I'd hate to quit on you, so I can investigate this and the feature flag. Whatever you feel is most time efficient. I want to be respectful to your codebase and to your time.

[joemasilotti]

All good! Don't ever feel obligated to work on this, every line of code you write is one less for me :)

If you get around to making changes go for it. If not, I'm hoping to dive into some work later this week and can take this over the finish line.

[joemasilotti]

Holy crap, dates and time zones suuuuuuuuuuck.

I ended up reworking the logic of the mailer around dates. Using 1.day.ago and friends uses the current time zone – that combined with #beginning_of_day and #end_of_day makes it a bit easier (IMHO) to reason about what days we are talking about. I also dropped the enum-ish param for two separate methods.

Thanks for the work you put in on this! I'm going to sleep on the PR, review it tomorrow, and hopefully merge this weekend if all still looks good.

[joemasilotti]

And we're live! 🎉

Wow that was a big one. That definitely snuck up on me, I did not expect this to be such a huge commitment. Thanks again for doing the majority of the work on this, @jacobdaddario! I always appreciate it :)