Gate perf optimization (#167)

* remove N+1 queries when fetching group expiration date

* update code that use user.group_expiration_date

* update spec to prevent race condition

* add apt-key for travis

* fix apt-key to be received

* use xenial instead of trusty because mysql package in trusty won't be generated anymore

* remove unnecessary step

.travis.yml

@@ -1,17 +1,11 @@
+dist: xenial
 sudo: required
 language: ruby
 rvm:
   - ruby-2.4.4
 services:
   - redis-server
   - mysql
-addons:
-  apt:
-    sources:
-      - mysql-5.7-trusty
-    packages:
-      - mysql-server
-      - mysql-client
 
 env:
   global:

app/controllers/groups_controller.rb

@@ -40,11 +40,16 @@ def new
   end
 
   def show
-    # This is set in before_action filter
-    # @group = Group.find(params[:id])
-    @vpns = Vpn.all.select { |vpn| vpn.groups.count.zero? }
-    @users = User.where(active: true)
-    @host_machines = HostMachine.all
+    @group_users = User.
+      select(%Q{
+        users.id AS id,
+        name,
+        email,
+        active,
+        group_associations.expiration_date AS group_expiration_date
+      }).
+      joins('LEFT OUTER JOIN group_associations ON users.id = group_associations.user_id').
+      where('group_associations.group_id = ?', @group.id)
   end
 
   def delete_machine

app/controllers/users_controller.rb

@@ -11,6 +11,16 @@ def index
 
   def show
     @user = User.where(id: params[:id]).first
+    @user_groups = Group.
+      select(%{
+        groups.id AS id,
+        gid,
+        name,
+        deleted_at,
+        group_associations.expiration_date AS group_expiration_date
+      }).
+      joins('INNER JOIN group_associations ON groups.id = group_associations.group_id').
+      where('group_associations.user_id = ?', @user.id)
 
     if @user.access_token.blank?
       access_token = AccessToken.new
@@ -23,7 +33,6 @@ def show
 
     return unless current_user.admin? || current_user == @user
 
-    @groups = Group.all
     render_404 if @user.blank?
 
     return unless @user.present? && (current_user.admin? || current_user.id == @user.id)

app/models/user.rb

@@ -403,10 +403,6 @@ def group_admin?
     GroupAdmin.find_by_user_id(self.id).present?
   end
 
-  def group_expiration_date(group_id)
-    group_associations.detect { |assoc| assoc.group_id == group_id }.expiration_date
-  end
-
   private
 
   def remove_default_admin

app/views/groups/show.html.slim

@@ -36,7 +36,7 @@
   br
   .card
     .card-body
-      h6.card-title Groups Members (#{@group.users.count})
+      h6.card-title Groups Members (#{@group_users.length})
       .table-responsive
         table.table.table-striped
           thead
@@ -46,7 +46,7 @@
               th Expiration Date
               th
           tbody
-            - @group.users.sort_by{ |user| user.email}.each do |user|
+            - @group_users.sort_by{ |user| user.email}.each do |user|
               tr
                 td
                   - if user.active?
@@ -56,12 +56,12 @@
                 td
                   = "#{user.email}"
                 td
-                  = "#{user.group_expiration_date @group.id}"
+                  = "#{user.group_expiration_date}"
                 td
                   = link_to "Delete", [@group, user], method: :delete, data: {confirm: 'Are you sure to remove this user from the group?'} if current_user.admin or @group.admin?(current_user)
 
       br
-      = "*This group does not have any group members" if @group.users.count == 0
+      = "*This group does not have any group members" if @group_users.length == 0
       br
       - if current_user.admin or @group.admin?(current_user)
         .h7 Assign members

app/views/users/show.html.slim

@@ -60,13 +60,13 @@
               th Expiration Date
               th Action
           tbody
-            - @user.groups.each do |group|
+            - @user_groups.each do |group|
               - unless group.deleted?
               tr
                 td
                   = "#{group.name} (#{group.gid})"
                 td
-                  = "#{@user.group_expiration_date group.id}"
+                  = "#{group.group_expiration_date}"
                 td
                   = link_to "Remove group", [@user, group], method: :delete, data: {confirm: 'Are you sure to delete this group?'} if current_user.admin?
       br

spec/controllers/groups_controller_spec.rb

@@ -58,6 +58,44 @@
     end
   end
 
+  describe 'GET #show' do
+    context 'unauthenticated' do
+      it 'should return 302' do
+        get :index
+
+        expect(response).to have_http_status(302)
+      end
+    end
+
+    context 'authenticated as admin' do
+      context '' do
+        it 'should return specified group' do
+          sign_in admin
+          group = create(:group)
+          get :show, params: { id: group.id }
+          expect(response).to have_http_status(200)
+        end
+
+        it 'should populate group_users instance variable' do
+          sign_in admin
+          user = create(:user)
+          group = create(:group)
+          create(:group_association, group_id: group.id, user_id: user.id, expiration_date: '2020-01-01')
+          get :show, params: { id: group.id }
+          expect(assigns(:group_users).first.to_json).to eq(
+            {
+              id: user.id,
+              email: user.email,
+              name: user.name,
+              active: user.active,
+              group_expiration_date: '2020-01-01'
+            }.to_json
+          )
+        end
+      end
+    end
+  end
+
   describe 'POST #add_user' do
     context 'unauthenticated' do
       it 'should return 302' do

spec/controllers/users_controller_spec.rb

@@ -5,6 +5,40 @@
   let!(:group) { FactoryBot.create(:group)  }
   let(:user) { FactoryBot.create(:user, name: "foobar", user_login_id: "foobar", email: "[email protected]")  }
 
+  describe 'GET #show' do
+    context 'unauthenticated' do
+      it 'should return 302' do
+        get :index
+
+        expect(response).to have_http_status(302)
+      end
+    end
+
+    context 'authenticated' do
+      it 'should return specified user' do
+        sign_in user
+        get :show, params: { id: user.id }
+        expect(response).to have_http_status(200)
+      end
+
+      it 'should populate user_groups instance variable' do
+        sign_in user
+        create(:group_association, group_id: group.id, user_id: user.id, expiration_date: '2020-01-01')
+        get :show, params: { id: user.id }
+        expected_group = assigns(:user_groups).select { |user_group| user_group.id == group.id }
+        expect(expected_group.first.to_json).to eq(
+          {
+            id: group.id,
+            name: group.name,
+            gid: group.gid,
+            deleted_at: group.deleted_at,
+            group_expiration_date: '2020-01-01',
+          }.to_json
+        )
+      end
+    end
+  end
+
   context "update user profile" do
     it "should update profile with product name" do
       sign_in user

spec/models/user_spec.rb

@@ -311,19 +311,6 @@
     end
   end
 
-  describe '#group_expiration_date' do
-    it 'should return expiration date from group' do
-      user = create(:user)
-      group = create(:group)
-      expiration_date = Date.parse('2019-10-20')
-      group.add_user_with_expiration(user.id, expiration_date)
-
-      user_expiration_date = user.group_expiration_date group.id
-
-      expect(user_expiration_date).to eq(expiration_date)
-    end
-  end
-
   describe '#uid' do
     it 'should check uid creation with offset' do
       user = create(:user)

spec/views/groups/show.html.slim_spec.rb

@@ -6,6 +6,7 @@
     it 'should renders form to add user to group' do
       sign_in admin
       assign(:group, group)
+      assign(:group_users, [])
 
       render