CODEWITHSUNDEEP
ruby-on-railsmodel-view-controllermodelcontroller
Sean Magyar
Sean Magyar

Reputation: 2380

rails controller action + rendering bug

I have notification system in my app. When the user goes to other user's page (users/show page), the notification count gets decreased since he/she can see the common chat window. So for instance if sby texted you then you have 1 notification. When you get on the sender's show page your notifications gets decreased to 0.

Everything worked fine, but I wanted to refactor the code since it was getting cluttered.

Below you can see the old and the new code. The old code works fine but there is a weird problem with the new code. When I get to the user page and I print out the current_user.new_chat_notification attribute in the controller show action or in the template (I show it in the header so it's available on every page) it shows 1. In the meantime if I print out the number in the console it shows 0. So for some reason the number gets decreased in the db, but the controller action and the view don't know about it in time. If I go to an other page then the number goes down to zero. So the next controller action already knows that the number was decreased and shows 0. I don't really understand what's the difference between the old and new code that can causes something like this.

schema.rb

create_table "users", force: :cascade do |t|
  t.integer  "new_chat_notification",  default: 0
end

users controller

def show
  @user = User.find(params[:id])
  #FOLLOWING 3 LINES ARE PART OF THE UPDATE
  @conversation = Conversation.create_or_find_conversation(current_user.id, @user.id)
  @tasks = Task.uncompleted.between(current_user.id, @user.id).order("created_at DESC").includes(:assigner, :executor).paginate(page: params[:page], per_page: 14)
  @messages = @conversation.messages.includes(:user).order(created_at: :desc).limit(50).reverse
  current_user.decreasing_chat_notification_number(@user)
  respond_to do |format|
    format.html
    format.js { render template: "tasks/between.js.erb" }
  end
end

user.rb

#FOLLOWING 2 LINES ARE PART OF THE UPDATE
has_many :notifications, foreign_key: "recipient_id", dependent: :destroy
validates :new_chat_notification, numericality: { only_integer: true, greater_than_or_equal_to: 0 }

def decreasing_chat_notification_number(sender)
  notification = notifications.between_chat_recipient(sender).unchecked.first
  checking_and_decreasing_notification(notification) if notification.present?
end

def checking_and_decreasing_notification(notification)
  notification.check_notification
  if notification.notifiable_type == "Message"
    # decrease_new_chat_notifications --> OLD CODE THAT WORKING PROPERLY
    NotificationSender.new(notification).decrease_new_chat_notifications # --> NEW CODE NOT WORKING PROPERLY
    ....
  else
    ....
  end
end

def decrease_new_chat_notifications
  decrement!(:new_chat_notification) if new_chat_notification > 0
end

notification_sender.rb (for new code)

class NotificationSender
  attr_reader :notification, :recipient

  def initialize(notification)
    @notification = notification
    @recipient = notification.recipient
  end

  def decrease_new_chat_notifications
    recipient.decrement!(:new_chat_notification) if recipient.new_chat_notification > 0
  end
end

Upvotes: 0

Views: 35

Answers (1)

Nic Nilov
Nic Nilov

Reputation: 5156

The difference may be in the fact that this line:

@recipient = notification.recipient

may load the recipient from the database, resetting the state. It's not obvious from your code whether there is any state change in the User model before the decreasing_chat_notification_number gets called.

UPDATE

If this is in fact the reason for the issue (which is currently just an unsupported suggestion), you could load your referenced recipient along with the notification. This way the it's state will not unexpectedly be updated from the database when you're initializing the NotificationSender.

Instead of notification = notifications.between_chat_recipient(sender).unchecked.first, try this:

notification = notifications.
    between_chat_recipient(sender).
    unchecked.
    includes(:recipient).
    first

The exact query may be different depending on your associations, but again, the idea is to load the recipient in the same database call as the notification, thus removing the potential race condition.

Upvotes: 1

Related Questions