Merge pull request #5043 from coalest/fix-organization-show-n-plus-ones

Fix N+1s in OrganizationController#show

Author: cielf

app/controllers/admin/organizations_controller.rb

@@ -68,6 +68,9 @@ def create
   def show
     @organization = Organization.find(params[:id])
     @header_link = admin_dashboard_path
+    @default_storage_location = StorageLocation.find_by(id: @organization.default_storage_location) if @organization.default_storage_location
+    @intake_storage_location = StorageLocation.find_by(id: @organization.storage_locations) if @organization.intake_location
+    @users = @organization.users.with_discarded.includes(:roles, :organization).alphabetized
   end
 
   def destroy

app/controllers/organizations_controller.rb

@@ -6,6 +6,9 @@ class OrganizationsController < ApplicationController
   def show
     @organization = current_organization
     @header_link = dashboard_path
+    @default_storage_location = StorageLocation.find_by(id: @organization.default_storage_location) if @organization.default_storage_location
+    @intake_storage_location = StorageLocation.find_by(id: @organization.intake_location) if @organization.intake_location
+    @users = @organization.users.with_discarded.includes(:roles, :organization).alphabetized
   end
 
   def edit

app/models/user.rb

@@ -112,16 +112,16 @@ def invitation_status
   end
 
   def kind
-    return "super" if has_role?(Role::SUPER_ADMIN)
-    return "admin" if has_role?(Role::ORG_ADMIN, organization)
-    return "normal" if has_role?(Role::ORG_USER, organization)
-    return "partner" if has_role?(Role::PARTNER, partner)
+    return "super" if has_cached_role?(Role::SUPER_ADMIN)
+    return "admin" if has_cached_role?(Role::ORG_ADMIN, organization)
+    return "normal" if has_cached_role?(Role::ORG_USER, organization)
+    return "partner" if has_cached_role?(Role::PARTNER, partner)
 
     "normal"
   end
 
   def is_admin?(org)
-    has_role?(Role::ORG_ADMIN, org) || has_role?(Role::SUPER_ADMIN)
+    has_cached_role?(Role::ORG_ADMIN, org) || has_cached_role?(Role::SUPER_ADMIN)
   end
 
   def switchable_roles

app/views/organizations/_details.html.erb

@@ -80,7 +80,7 @@
                 <h3 class='font-bold'>Default Intake Location</h3>
                 <p>
                   <%= fa_icon "building" %>
-                  <%= StorageLocation.find_by(id: @organization.intake_location)&.name || "Not defined" %>
+                  <%= @intake_storage_location&.name || "Not defined" %>
                 </p>
               </div>
               <div class="mb-4">
@@ -102,7 +102,7 @@
                 <h3 class='font-bold'>Default Storage Location</h3>
                 <p>
                   <%= fa_icon "building-o" %>
-                  <%= StorageLocation.find_by(id: @organization.default_storage_location)&.name || "Not defined" %>
+                  <%= @default_storage_location&.name || "Not defined" %>
                 </p>
               </div>
               <div class="mb-4">

app/views/users/_organization_user.html.erb

@@ -8,7 +8,7 @@
   <td><%= user.invitation_status %></td>
   <td nowrap><%= reinvite_user_link(user) %></td>
   <td>
-    <% unless user.has_role?(Role::SUPER_ADMIN) || user.has_role?(Role::ORG_ADMIN, current_organization) %>
+    <% unless user.has_cached_role?(Role::SUPER_ADMIN) || user.has_cached_role?(Role::ORG_ADMIN, current_organization) %>
       <div class="dropdown">
         <button class="btn btn-default dropdown-toggle" type="button" id="<%= dom_id(user, "dropdownMenu") %>" data-bs-toggle="dropdown" aria-haspopup="true" aria-expanded="true">
           Actions
@@ -33,7 +33,7 @@
         </ul>
       </div>
     <% end %>
-    <% if current_user.is_admin?(current_organization) && user.has_role?(Role::ORG_ADMIN, current_organization) %>
+    <% if current_user.is_admin?(current_organization) && user.has_cached_role?(Role::ORG_ADMIN, current_organization) %>
       <%= edit_button_to demote_to_user_organization_path(user_id: user.id, organization_name: current_organization.short_name),
                  {text: 'Demote to User'},
                  {method: :post, rel: "nofollow", data: {confirm: 'This will demote the admin to user status. Are you sure that you want to submit this?', size: 'xs'}} unless user.id == current_user.id %>

app/views/users/_organization_users_table.html.erb

@@ -24,7 +24,7 @@
 
               <tbody>
                 <%= render partial: "/users/organization_user",
-                           collection: @organization.users.with_discarded.alphabetized,
+                           collection: @users,
                            as: :user,
                            locals: { current_organization: current_organization || @organization } %>
               </tbody>

spec/requests/admin/organizations_requests_spec.rb

@@ -166,6 +166,18 @@
         expect(response).to be_successful
       end
 
+      it "displays the correct organization details" do
+        intake_storage_location = create(:storage_location, organization:, name: "Intake Center")
+        default_storage_location = create(:storage_location, organization:, name: "Default Center")
+
+        organization.update!(intake_location: intake_storage_location.id, default_storage_location: default_storage_location.id)
+
+        get admin_organization_path({ id: organization.id })
+
+        expect(response.body).to include("Intake Center")
+        expect(response.body).to include("Default Center")
+      end
+
       context "with an organization user" do
         let!(:user) { create(:user, organization: organization) }
 

spec/requests/organization_requests_spec.rb

@@ -47,6 +47,18 @@
         expect(html.text).to include("Receive email when partner makes a request:")
       end
 
+      it "displays the correct organization details" do
+        intake_storage_location = create(:storage_location, organization:, name: "Intake Center")
+        default_storage_location = create(:storage_location, organization:, name: "Default Center")
+
+        organization.update!(intake_location: intake_storage_location.id, default_storage_location: default_storage_location.id)
+
+        get organization_path
+
+        expect(response.body).to include("Intake Center")
+        expect(response.body).to include("Default Center")
+      end
+
       context "when enable_packs flipper is on" do
         it "displays organization's custom units" do
           Flipper.enable(:enable_packs)