Rake task to send users a reminder to post with conditions

Question

I am implementing a feature that reminds users to make a post via email if

• the user has set daily reminders to true
• he has not posted yet today
• and if the current hour matches when he would like to receive the daily reminder in his time zone.

I am going to write a Rake task for it and then schedule it to run every hour.

Currently, the rake tasks looks like this:

namespace :mail do
  desc "Send daily reminder to users"
  task :daily_reminder => :environment do
    User.all.each do |user|
      Time.zone = user.time_zone
      if user.reminder == true && Time.current.strftime("%H").to_i == user.reminder_time && user.reminded == false
        UserMailer.test_email(user).deliver_now
        user.update_attribute(:reminded, true)

      elsif Time.current.strftime("%H") == user.reminder_time + 1 && user.reminded == true
        user.update_attribute(:reminded, false)
        puts "Update to false"
      end
    end
  end
end

I am resetting the field reminded back to false the hour after they were reminded.

Since I am rather new to Ruby and Ruby on Rails I would appreciate feedback on the setup of this feature a lot.

Please critique away on anything you think is broken or needs improvement.

Also, right now it is not working and it's giving me back a undefined method + for nil:NilClass which tells me that the user hasn't an ActiveRecord assigned to it? Because the reminder_time column exists and is set (to 16:00).

---

Follow-up question:

• The if clause looks kind of large and unclean. Is that the proper way to go about this?
• Shall I make the whole logic more clean by putting some of the conditions in methods?

Answer 1

1. Never User.all.each in the real world. That loads your entire table into memory and instantiates ActiveRecord models for every record. You want User.find_each do |user|, which uses find_in_batches to load 1000 records at a time.

2. Don't do simple filtering in Ruby, do it in the database. This...

   if user.reminder == true 
   

   should be a simple scope that prevents these records from ever being loaded. Something like this:

   class User < ActiveRecord::Base
     scope :with_reminder, -> { where(reminder: true) }
   end
   

   Now, your top-level loop can use the scope:

   User.with_reminder.find_each do |user|
   

   This filters out a huge number of users who would otherwise be loaded from the database needlessly.

3. Your if/else aren't actually correct, since your intent is to load all users with reminder = true. You load all users, and then your if looks for user.reminder == true, but your else doesn't. It affects all users with reminder of true or false, and then sets their reminded to false.

4. ... which tells me that the user hasn't an ActiveRecord assigned to it? Because the reminder_time column exists and is set (to 16:00).

   Err, no, that's not at all what that tells you. Nothing has "an ActiveRecord" assigned to it, that isn't at all how ActiveRecord works. Every record in the database is wrapped in an ActiveRecord object as you read it from the database, it's impossible for some users to "have" ActiveRecords and others not to.

   See point 3. Your else currently checks all records, including those with reminder == false, and presumably those records don't have a reminder_time set.

5. It's really not clear what you're trying to do with Time.current.strftime("%H") == user.reminder_time + 1. What is "1"? You have no unit, so it means "1 second". Your probably never going to hit that.