HiveBrain v1.2.0
Get Started
← Back to all entries
patternrubyrailsMinor

Email notifications system in Rails project

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
emailsystemnotificationsrailsproject

Problem

I'm working on the Rails project where I have many-to-many :through relationship. Each setting has many notifications through email_notification model where the notification can be primary or a secondary.

schema.rb

create_table "email_notifications", :force => true do |t|
  t.integer "setting_id"
  t.integer "notification_id"
  t.boolean "primary"
  t.boolean "secondary"

create_table "notifications", :force => true do |t|
  t.string   "name"

create_table "settings", :force => true do |t|
  t.string   "truck_identification"
  t.string   "email_periods"
  t.integer  "email_recepient_id"
  t.datetime "created_at",                             :null => false
  t.datetime "updated_at",                             :null => false
  t.integer  "truck_fleet_id"


In models

class EmailNotification  :email_notifications
  has_many :email_notifications
end

class Setting  :email_notifications
  has_many :email_notifications
  validates_uniqueness_of :truck_fleet_id 
  # validates :email, :email_format => true
end


I used this many-to-many relationship because I need flexibility to add up new Notifications over the time, and keep ability to make notifications flexible as possible. For example, I might add a column frequency, or interval etc.

Let me know if you can find a better solution.

Solution

# TODO: refactor this bullshit


While amusing to devs, swear words probably don't belong in production code.

In your copypasta of schema.rb, I do believe you forgot to paste the end keywords.

As a side note, you can also write null: false instead of :null => false; as far as I can tell, this is the accepted syntax. You can do the same thing with through: :email_notifications and anything of the form Symbol => BasicObject. This syntax was added after this question was posted, but if you're still interested in updating it, that's what I'd do.

Personally, I prefer to use Symbols instead of Strings as table names, though that's entirely personal preference.

In your models, you also forgot an end at the end of your first class.

Aside from that... Well, honestly, it looks good to me. Since I can't really tell what Setting and Notification are supposed to represent on a more concrete level than their names alone can give, I can't give good advice on how to link them together. However, I don't see why you need a many-to-many, if I'm understanding the purpose of Setting and Notification correctly. It seems like it could be a one-to-many relationship, though I'm not sure which side should be the one.

Code Snippets

# TODO: refactor this bullshit

Context

StackExchange Code Review Q#19807, answer score: 2

Revisions (0)

No revisions yet.