Showing a table for open and closed tickets

Question

This shows a table for open and closed tickets. The code for open tickets is identical with the closed tickets. I was trying to figure out a way to just write one code segment that will show both tables and use half of the code lines. With some changes I ended up to this code. With this code, if I iterate it twice and at at first use the word "open" and then rename it to "closed" then I will accomplish what I am after.

I looked around and figured out that I need to use a dynamic variable name. Something like sum#{status}tickets (pretty sure that something like that doesn't work) where the "status" variable will be at the first iteration "open" and then "closed" at the second one.

I was wondering if this is somehow possible (to iterate twice) and then dynamically set the variable's name.

 <% if sumopentickets == 0 %>
 <h3>Open Tickets</h3>
 <h4>No open tickets.</h3>
 <% else %>
 <h3>Open Tickets</h3>
 <table class="table">
 <tr>
 <th>Ticket Number</th>
 <th>Title</th>
 <th>Submitted by</th>
 <th>Created at</th>
 <th colspan="2">Status</th>
 </tr>
 <% @tickets.each do |ticket| %>
 <% if ticket.status == "open" %>
 <% user = User.find(ticket.user_id) %>
 <tr>
 <td><%= ticket.id %></td>
 <td><%= link_to ticket.title, ticket_path(ticket.url_token) %></td>
 <td><%= user.username %></td>
 <td><%= ticket.created_at.strftime("%H:%M, %B %d, %Y") %></td>
 <% if ticket.ticketmessages.last && ticket.ticketmessages.last.user_id %>
 <td>(user waiting for reply...)</td>
 <% end %>
 </tr>
 <% end %>
 <% end %>
 </table>
 <% end %>
 <% if sumclosedtickets == 0 %>
 <h3>Closed Tickets</h3>
 <h4>No closed tickets.</h3>
 <% else %>
 <h3>Closed Tickets</h3>
 <table class="table">
 <tr>
 <th>Ticket Number</th>
 <th>Title</th>
 <th>Submitted by</th>
 <th>Created at</th>
 <th colspan="2">Status</th>
 </tr>
 <% @tickets.each do |ticket| %>
 <% if ticket.status == "closed" %>
 <% user = User.find(ticket.user_id) %>
 <tr>
 <td><%= ticket.id %></td>
 <td><%= link_to ticket.title, ticket_path(ticket.url_token) %></td>
 <td><%= user.username %></td>
 <td><%= ticket.created_at.strftime("%H:%M, %B %d, %Y") %></td>
 <% if ticket.ticketmessages.last && ticket.ticketmessages.last.user_id %>
 <td>(user waiting for reply...)</td>
 <% end %>
 </tr>
 <% end %>
 <% end %>
 </table>
 <% end %>

• RubberDuck: This doesn't look like "just plain Ruby". Is this ruby-on-rails?

Answer 1

I'm going to assume that this is in fact a ruby-on-rails project - not just plain Ruby.

In that case, long story short: Stop doing everything in the view. Use Rails and its conventions.

In your model, add a proper relationship to the user, and a couple of scopes

belongs_to :user
scope :open, where(status: :open)
scope :closed, where(status: :closed)

Also add a method

def waiting_for_reply?
 # I don't quite understand your logic (or your naming) here,
 # so I'll just shorten it, but otherwise leave it
 ticketmessages.last.try(:user_id).present?
end

I say I don't understand your logic, because something called ticketmessages implies there's a class called Ticketmessage - when it should be called TicketMessage as it's two words (or simply Message). And in that case, the relation should be ticket_messages with an underscore (or, again, simply messages).
Secondly, you check whether there is a (last) ticket, and whether its user_id is non-nil/non-false. But what does that prove? If there's any system to how it's set up, a message will always have a user_id because every message was written by a user. The real question (I assume) is whether that user_id is equal to the ticket's user_id - otherwise someone else wrote it. But that's not what you're checking, so I'm confused.

Anyway, in your controller, load two collections (and eagerly load the associated users):

@open_tickets = Ticket.open.includes(:user)
@closed_tickets = Ticket.closed.includes(:user)

In your view, extract the table itself into a partial, and only include HTML for the parts that actually differ. Note the call to render where we point it to the partial template it should use, and the local variables that should be made available to that template.

<h3>Open Tickets</h3>
<% if @open_tickets.any? %>
 <%= render partial: "tickets", locals: { tickets: @open_tickets } %>
<% else %>
 <h4>No open tickets</h4> <!-- this should just be a P element by the way -->
<% end %>
<h3>Closed Tickets</h3>
<% if @closed_tickets.any? %>
 <%= render partial: "tickets", locals: { tickets: @closed_tickets } %>
<% else %>
 <h4>No closed tickets</h4> <!-- see above -->
<% end %>

And here's the partial

<table class="table">
 <thead>
 <tr>
 <th>Ticket Number</th>
 <th>Title</th>
 <th>Submitted by</th>
 <th>Created at</th>
 <th>Status</th>
 </tr>
 </thead>
 <tbody>
 <% tickets.each do |ticket| %> <!-- here we use the local "tickets" -->
 <tr>
 <td><%= ticket.id %></td>
 <td><%= link_to ticket.title, ticket_path(ticket.url_token) %></td>
 <td><%= ticket.user.username %></td>
 <td><%= ticket.created_at.strftime("%H:%M, %B %d, %Y") %></td> <!-- this formatting stuff should be a helper method -->
 <td>
 <% if ticket.waiting_for_reply? %>
 (user waiting for reply...)
 <% end %>
 </td>
 </tr>
 <% end %>
 </tbody>
</table>