3

I'm writing a simple chat, and I need to list out users online. I don't use devise for authentication, there's a custom user model which authenticates via omniauth.

user.rb

class User < ActiveRecord::Base
    has_many :messages, dependent: :delete_all
    class << self
        def from_omniauth(auth)
            provider = auth.provider
            uid = auth.uid
            info = auth.info.symbolize_keys!
            user = User.find_or_initialize_by(uid: uid, provider: provider)
            user.name = info.name
            user.avatar_url = info.image
            user.profile_url = info.urls.send(provider.capitalize.to_sym)
            user.save!
            user
        end
    end
end

application_controller.rb

def current_user
    @current_user ||= User.find_by(id: cookies[:user_id]) if cookies[:user_id]
end
helper_method :current_user

I tried to do that in such way: add to application_controller.rb a show_online method:

def show_online
    @users = User.where(status: online)
end

helper_method :online_users

and then add to a view:

<%= online_users.each do |user| %>
<ul>
    <li><%= user.name %></li>
</ul>
<%end%>

but it throws an exception ActionView::Template::Error (undefined method 'online_users' for #<MessagesController:0x007f52d7f82740>)

source code here

EDIT

the best solution as for me I found here, but I completely don't get how to implement it correctly :( But that's definitely what I need

Community
  • 1
  • 1
AlexNikolaev94
  • 1,169
  • 2
  • 16
  • 40
  • probably `Users.find(:all, :conditions => ["status = ?", "online"])` returns nil thats why. – 7urkm3n Apr 04 '16 at 08:09
  • Don't quote me on that but I'm pretty sure that this find syntax you are using got removed a long time ago. Also provide a proper error message. It should tell you _what object_ has no each method which would help debugging a lot. – 2called-chaos Apr 04 '16 at 08:09
  • @2called-chaos it returns `ActionView::Template::Error (undefined method 'each' for nil:NilClass)` – AlexNikolaev94 Apr 04 '16 at 08:13
  • @AlexNikolaev94 This is because the find method takes an ID to find one single record (and I guess you don't have an ID `:all`). Please lookup the documentation here http://guides.rubyonrails.org/active_record_querying.html – 2called-chaos Apr 04 '16 at 08:15
  • do you check my post do you use the possibilities I have mentioned @AlexNikolaev94 – Rajarshi Das Apr 04 '16 at 08:31
  • Post your `Helper` file too. – 7urkm3n Apr 04 '16 at 08:56
  • @AlexNikolaev94 Is the controller that's contains the action used to render the view (the one that calls online_users) inheriting from ApplicationController? – Codebeef Apr 04 '16 at 09:45
  • @AlexNikolaev94 I have edited my answer and it should now solve your problem. – Matt C Apr 04 '16 at 09:48
  • @AlexNikolaev94 please check my answer – Codebeef Apr 04 '16 at 09:48

4 Answers4

0

It should be <% %> not <%= %>

<% @users.each do |user| %>
    <ul>
    <li><%= user.name %></li>
</ul>
<% end%>

Secondly

but also you need to check whether @users is nil so nil.each each will throw that error ActionView::Template::Error (undefined method 'each' for nil:NilClass)

so it will be look like

<% if @users %>
 <% @users.each do |user| %>
  <ul>
    <li><%= user.name %></li>
  </ul>
 <% end%>
<% end %>

or in controller

def show_online
  @users = User.where(status: 'Online')
end

and

<% @users.each do |user| %>
<ul>
    <li><%= user.try(:name) %></li>
</ul>
<%end%>

why I choose where not find all

Community
  • 1
  • 1
Rajarshi Das
  • 11,778
  • 6
  • 46
  • 74
0

EDIT:

The error you get can be solved in one of two ways.
You can use helper methods and call them from your view, as it seems you want to do.
Or, you can avoid using them completely and just call the show_online method from whichever method is currently being when the view is loaded. If you're going to the show, it'll be the show method, and so on and so on.

Your own answer does correctly fix the errors using the first method, but I recommend this way.

What needs to be done to implement these fixes:

  • Call show_online when the new is being loaded so that view can access the @users variable. We can do that with before_action

  • In the view, you have a loop which iterates over online_users, but it should iterate over @users

  • In the that same loop in the view, you have a simple syntax error. The first line starts with <%= but should start with <% without the =. This should be changed no matter what way you write the code.

So all together the code is:

application_controller.rb

#put this line at the top of the controller, just below the line ApplicationController:: .....
before_action :show_online, only: [:new]

def show_online
    @users = User.where(online: true)

the view file

<% @users.each do |user| %>
  <ul>
      <li><%= user.name %></li>
  </ul>
<% end %>

Why this method?

  • Making one method to get the online users means the logic is only one one place
  • Having your logic/code in one place means you don't ever repeat yourself, and you know where to look if a problem occurs
  • Using before_action means the call won't be made unless it is needed
  • If you later add pages which need to get the list of online users, you just add those to the list of methods in the brackets here: only: [:new]
  • When choosing between placing logic in a view or in a controller, the correct answer is almost always the controller
Matt C
  • 4,470
  • 5
  • 26
  • 44
  • 1
    thank you! but I actually don't get properly `show` method - the `current_user` method must be nested into it? – AlexNikolaev94 Apr 04 '16 at 10:14
  • @AlexNikolaev94 You can call the **show_online** method from whichever method is loading the view, it doesn't have to be the show method. What is the name of your view file? That will tell us what method we need. – Matt C Apr 04 '16 at 10:18
  • I've just done as you've said - `<% @users.each %>` and now it returns me `ActionView::Template::Error (undefined method 'each' for nil:NilClass)`. while when I've done that as I've said in my answer, there were no errors, but the online users were just not shown in the list. – AlexNikolaev94 Apr 04 '16 at 10:22
  • What is the name of the view file? – Matt C Apr 04 '16 at 10:22
  • now there's a `NoMethodError (undefined method 'show_online' for #)` – AlexNikolaev94 Apr 04 '16 at 10:40
  • I see. Make sure `show_online` is not private. If it is not private and you get the same error, change the word **before_action** to **before_filter**. But only if you still get the error after you make show_online not private. – Matt C Apr 04 '16 at 10:51
  • The only reason you might need to change it to **before_filter** is because that was the name of **before_action** until it was recently changed in a new rails version. – Matt C Apr 04 '16 at 10:53
  • @AlexNikolaev94 Can you post a link to the entire controller code as well as the entire view file? – Matt C Apr 04 '16 at 13:23
  • I found an [answer](http://stackoverflow.com/questions/10805479/rails-how-to-check-for-online-users) that does correctly what I need, but I don't completely get HOW to implement it :( If you could help, please e-mail @ alexandernikolaev94@gmail.com me or write me in [facebook](https://www.facebook.com/profile.php?id=100011665566894) – AlexNikolaev94 Apr 04 '16 at 14:06
  • I will email you if you continue to have trouble, but for right now just try to do this: add that code from the answer you found to your user model. Make a migration to add a time stamp attribute to the user table called `last_online`. Every time the a page is loaded or a user does any action, set the user's `last_online` attribute to Time.now – Matt C Apr 04 '16 at 14:35
  • Once you do that, you'll be able to just change your logic in your application controller to get the online users. From before when you did `User.where(online: true)`, you'll change that to `User.where(online_now: true)` – Matt C Apr 04 '16 at 14:38
  • should the migration look like `add_last_online_to_users last_online:timestamp`? – AlexNikolaev94 Apr 04 '16 at 14:58
  • @AlexNikolaev94 No, see why [here](http://stackoverflow.com/questions/7542976/add-timestamps-to-an-existing-table) – Matt C Apr 04 '16 at 14:59
  • so I need to create the migration file manually and then migrate my database, right? – AlexNikolaev94 Apr 04 '16 at 15:18
  • @AlexNikolaev94 Yeah. You can name the migration whatever then go find it in db/migrate/ and edit it. – Matt C Apr 04 '16 at 15:19
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/108204/discussion-between-alexnikolaev94-and-matthew-cliatt). – AlexNikolaev94 Apr 04 '16 at 17:19
0

Your application controller code is wrong:

class ApplicationController < ActionController::Base
  def show_online
    @users = User.where(status: 'online')
  end
  helper_method :online_users
end

Should be:

class ApplicationController < ActionController::Base
  def online_users
    @users ||= User.where(status: 'online')
  end
  helper_method :online_users
end
Codebeef
  • 43,508
  • 23
  • 86
  • 119
-1

From the error message it seems like your @users is not an array or ActiveRecord::Relation.

I would print out the @users on the view to debug. Also, find(:all, :conditions => ["status = ?", "online"]) is not the preferable way to query.

Use User.where(:status => "online"). Reference - http://guides.rubyonrails.org/active_record_querying.html#conditions

  • still it doesn't help :( – AlexNikolaev94 Apr 04 '16 at 08:27
  • What's the outcome of your debug then? The same error? And the class if `@users`? – Jiazhen Xie Apr 04 '16 at 08:43
  • Thanks. A couple of errors: 1. I think you mean `@online_users` not `online_users`. 2. change `@users = User.where(status: online)` to `@online_users = User.where(status: "online")`. Also, I suggest you to add `<%= @online_users.class %>` on the view to see what class is it. – Jiazhen Xie Apr 04 '16 at 09:04