Merge pull request #660 from YaleSTC/190_refactor_views_users_new

Refactor UsersController#new (controller and view)

Author: squidgetx

app/controllers/users_controller.rb

@@ -43,17 +43,43 @@ def show
   end
 
   def new
-    if can? :create, User
-      if params[:possible_netid]
-        @user = User.new(User.search_ldap(params[:possible_netid]))
-      else
-        @user = User.new
-      end
-    else
+    @can_edit_login = current_user.present? && (can? :create, User) # used in view
+
+    if current_user.nil?
+      # This is a new user -> create an account for them
       @user = User.new(User.search_ldap(session[:cas_user]))
       @user.login = session[:cas_user] #default to current login
+
+      # TODO: What should it render?
+      @partial_to_render = 'form'
+    else
+      # Someone with permissions is creating a new user
+      ldap_result = User.search_ldap(params[:possible_netid])
+      @user = User.new(ldap_result)
+
+      # Does netID exist?
+      if ldap_result.nil?
+        @message = 'Sorry, the netID that you entered does not exist.
+        You cannot create a user profile without a valid netID.'
+        render :new and return
+      end
+
+      # Is there a user record already?
+      if User.exists?(login: params[:possible_netid])
+        @message = 'You cannot create a new user, as the netID you entered
+        is already associated with a user. If you would like to reserve for
+        them, please select their name from the drop-down options in the cart.'
+        render :new and return
+      end
+
+      # With existing netID and no user record, what's the context of creation?
+      # FIXME: can the check be replaced by params[:from_cart].present?
+      if params[:from_cart] == 'true'
+        @partial_to_render = 'short_form' # Display short_form
+      else
+        @partial_to_render = 'form' # Display (normal) form
+      end
     end
-    @can_edit_login = (can? :create, User)
   end
 
   def create

app/models/user.rb

@@ -76,6 +76,8 @@ def checked_out_models
   end
 
   def self.search_ldap(login)
+    return nil if login.blank?
+
     ldap = Net::LDAP.new(host: "directory.yale.edu", port: 389)
     filter = Net::LDAP::Filter.eq("uid", login)
     attrs = ["givenname", "sn", "eduPersonNickname", "telephoneNumber", "uid",

app/views/users/new.html.erb

@@ -1,25 +1,11 @@
 <% title "New User" unless params[:from_cart] == "true" %>
 
-<% if params[:from_cart] == "true" && User.search_ldap(params[:possible_netid]) && !User.exists?(login: params[:possible_netid]) %>
-  <%= render :partial => 'short_form' %>
-<% elsif params[:from_cart] == "true" && !User.search_ldap(params[:possible_netid]) %>
-  <div class="span5">
-  <p>Sorry, the netID that you entered does not exist.
-  You cannot create a user profile without a valid netID.</p>
-  </div>
-  <div class="span5">
-  </div>
-<% elsif params[:from_cart] == "true" && User.exists?(login: params[:possible_netid]) %>
-  <div class="span5">
-    <p>This user already exists! If you would like to reserve for them, please select their name from the drop-down
-      options.</p>
-  </div>
+<% if flash[:notice].nil? && flash[:error].nil? %>
+  <%= render :partial => @partial_to_render %>
+  <%= content_tag(:p, link_to("Back to List", users_path)) if current_user && params[:from_cart] != "true" %>
+<% else %>
   <div class="span5">
+    <%= content_tag(:p, @message) %>
   </div>
-<% else %>
-  <%= render :partial => 'form' %>
-<% end%>
-
-<% if current_user && params[:from_cart] != "true" %>
-  <p><%= link_to "Back to List", users_path %></p>
+  <div class="span5"></div>
 <% end %>

app/views/users/show.html.erb

@@ -53,12 +53,14 @@
 
   <div id="user_stats" class="span5">
 
-  <% @show_equipment.each do |key, value| %>
-    <%= content_tag :div, :id => "#{key}_count", :class => 'span2' do %>
-      <%= content_tag :h4, key.to_s.titleize %>
-      <%= content_tag :div, :class => "#{key}_num" do %>
-        <%= content_tag :i, nil, :class => stats_icons(key) %>
-        <%= value.size %>
+  <%# @show_equipment has reservation status (:overdue, ...) as key, full
+  reservations in question as value %>
+  <% @show_equipment.each do |status, reservations| %>
+    <%= content_tag :div, :id => "#{status}_count", :class => 'span2' do %>
+      <%= content_tag :h4, status.to_s.titleize %>
+      <%= content_tag :div, :class => "#{status}_num" do %>
+        <%= content_tag :i, nil, :class => stats_icons(status) %>
+        <%= reservations.size %>
       <% end %>
     <% end %>
   <% end %>
@@ -98,22 +100,22 @@
   <div class="tabbable tabs-left span9">
 
     <ul class="nav nav-tabs">
-      <% @show_equipment.each do |key, value| %>
-        <%= content_tag :li, :class => active_tab(key) do %>
-          <%= link_to "##{key.to_s}", :class => 'tab-text', :"data-toggle" => 'tab' do %>
-            <%= key.to_s.titleize %>
-            <%= content_tag :span, value.size, :class => 'badge tab-badge' %>
+      <% @show_equipment.each do |status, reservations| %>
+        <%= content_tag :li, :class => active_tab(status) do %>
+          <%= link_to "##{status.to_s}", :class => 'tab-text', :"data-toggle" => 'tab' do %>
+            <%= status.to_s.titleize %>
+            <%= content_tag :span, reservations.size, :class => 'badge tab-badge' %>
           <% end %>
         <% end %>
       <% end %>
     </ul>
 
     <div class="tab-content">
-      <% @show_equipment.each do |key, value| %>
+      <% @show_equipment.each do |status, reservations| %>
         <p>
-          <%= content_tag :div, {:class => "tab-pane #{active_tab(key)}", :id => key}, false do %>
-            <% unless value.empty? %>
-              <%= render partial: 'history_table', locals: { :key => key, :value => value } %>
+          <%= content_tag :div, {:class => "tab-pane #{active_tab(status)}", :id => status}, false do %>
+            <% unless reservations.empty? %>
+              <%= render partial: 'history_table', locals: { :key => status, :value => reservations } %>
             <% end %>
           <% end %>
         </p>