From 216dff3ed97351431231f8d8afe5d3554a4e832c Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Thu, 8 Apr 2021 17:47:44 +0100
Subject: [PATCH] DEV: Improve logic for showing/hiding the reports tab in
 group screens (#107)

Previously this was adding an extra AJAX request to check if the group had any queries available. Now a boolean is included in the group serializer, so there is no need for the extra request.

Removing this ajax request will also stop other plugin JS integration tests from failing when the data-explorer plugin is installed.

This commit also fixes the HTML markup of the tab, so that it doesn't have a <ul> nested inside the existing <ul>. Also adds an icon for good measure.
---
 .../data_explorer/query_controller.rb         |  6 +--
 app/models/data_explorer/query.rb             |  7 +++
 .../components/group-reports-nav-item.js.es6  | 27 ------------
 .../group-reports-nav-item/nav-item.js.es6    |  5 +++
 .../components/group-reports-nav-item.hbs     |  9 ----
 .../group-reports-nav-item/nav-item.hbs       |  4 +-
 plugin.rb                                     |  8 ++++
 spec/requests/group_spec.rb                   | 44 +++++++++++++++++++
 8 files changed, 68 insertions(+), 42 deletions(-)
 delete mode 100644 assets/javascripts/discourse/components/group-reports-nav-item.js.es6
 create mode 100644 assets/javascripts/discourse/connectors/group-reports-nav-item/nav-item.js.es6
 delete mode 100644 assets/javascripts/discourse/templates/components/group-reports-nav-item.hbs
 create mode 100644 spec/requests/group_spec.rb

diff --git a/app/controllers/data_explorer/query_controller.rb b/app/controllers/data_explorer/query_controller.rb
index 3fe782b..d4b2c52 100644
--- a/app/controllers/data_explorer/query_controller.rb
+++ b/app/controllers/data_explorer/query_controller.rb
@@ -47,11 +47,7 @@ class DataExplorer::QueryController < ::ApplicationController
 
     respond_to do |format|
       format.json do
-        queries = DataExplorer::Query
-          .where(hidden: false)
-          .joins("INNER JOIN data_explorer_query_groups
-                    ON data_explorer_query_groups.query_id = data_explorer_queries.id
-                    AND data_explorer_query_groups.group_id = #{@group.id}")
+        queries = DataExplorer::Query.for_group(@group)
         render_serialized(queries, DataExplorer::QuerySerializer, root: 'queries')
       end
     end
diff --git a/app/models/data_explorer/query.rb b/app/models/data_explorer/query.rb
index d64db4b..3e0b07b 100644
--- a/app/models/data_explorer/query.rb
+++ b/app/models/data_explorer/query.rb
@@ -7,6 +7,13 @@ module DataExplorer
     has_many :groups, through: :query_groups
     belongs_to :user
 
+    scope :for_group, ->(group) do
+      where(hidden: false)
+        .joins("INNER JOIN data_explorer_query_groups
+              ON data_explorer_query_groups.query_id = data_explorer_queries.id
+              AND data_explorer_query_groups.group_id = #{group.id}")
+    end
+
     def params
       @params ||= DataExplorer::Parameter.create_from_sql(sql)
     end
diff --git a/assets/javascripts/discourse/components/group-reports-nav-item.js.es6 b/assets/javascripts/discourse/components/group-reports-nav-item.js.es6
deleted file mode 100644
index 8164c9e..0000000
--- a/assets/javascripts/discourse/components/group-reports-nav-item.js.es6
+++ /dev/null
@@ -1,27 +0,0 @@
-import { ajax } from "discourse/lib/ajax";
-
-export default Ember.Component.extend({
-  group: null,
-  showReportsTab: false,
-
-  checkForReports() {
-    return ajax(`/g/${this.group.name}/reports`).then((response) => {
-      return this.set("showReportsTab", response.queries.length > 0);
-    });
-  },
-
-  init(args) {
-    this.set("group", args.group);
-    if (
-      (this.get("currentUser.groups") || []).some(
-        (g) => g.id === this.group.id
-      ) ||
-      this.get("currentUser.admin")
-    ) {
-      // User is a part of the group (or an admin). Now check if the group has reports
-      this.checkForReports();
-    }
-
-    this._super(args);
-  },
-});
diff --git a/assets/javascripts/discourse/connectors/group-reports-nav-item/nav-item.js.es6 b/assets/javascripts/discourse/connectors/group-reports-nav-item/nav-item.js.es6
new file mode 100644
index 0000000..35745d6
--- /dev/null
+++ b/assets/javascripts/discourse/connectors/group-reports-nav-item/nav-item.js.es6
@@ -0,0 +1,5 @@
+export default {
+  shouldRender(args) {
+    return args.group.has_visible_data_explorer_queries;
+  },
+};
diff --git a/assets/javascripts/discourse/templates/components/group-reports-nav-item.hbs b/assets/javascripts/discourse/templates/components/group-reports-nav-item.hbs
deleted file mode 100644
index 5fd40cd..0000000
--- a/assets/javascripts/discourse/templates/components/group-reports-nav-item.hbs
+++ /dev/null
@@ -1,9 +0,0 @@
-{{#if showReportsTab}}
-  <ul class ='nav-pills'>
-    <li>
-      {{#link-to 'group.reports'}}
-        {{i18n 'group.reports'}}
-      {{/link-to}}
-    </li>
-  </ul>
-{{/if}}
\ No newline at end of file
diff --git a/assets/javascripts/discourse/templates/connectors/group-reports-nav-item/nav-item.hbs b/assets/javascripts/discourse/templates/connectors/group-reports-nav-item/nav-item.hbs
index f29f5b2..683464b 100644
--- a/assets/javascripts/discourse/templates/connectors/group-reports-nav-item/nav-item.hbs
+++ b/assets/javascripts/discourse/templates/connectors/group-reports-nav-item/nav-item.hbs
@@ -1 +1,3 @@
-{{group-reports-nav-item group = group}}
\ No newline at end of file
+{{#link-to 'group.reports'}}
+  {{d-icon 'chart-bar'}}{{i18n 'group.reports'}}
+{{/link-to}}
diff --git a/plugin.rb b/plugin.rb
index c59753c..8c543b3 100644
--- a/plugin.rb
+++ b/plugin.rb
@@ -60,6 +60,14 @@ after_initialize do
     return user_is_a_member_of_group?(group) && query.groups.exists?(id: group.id)
   end
 
+  add_to_serializer(:group_show, :has_visible_data_explorer_queries, false) do
+    DataExplorer::Query.for_group(object).exists?
+  end
+
+  add_to_serializer(:group_show, :include_has_visible_data_explorer_queries?, false) do
+    SiteSetting.data_explorer_enabled && scope.user_is_a_member_of_group?(object)
+  end
+
   module ::DataExplorer
     class Engine < ::Rails::Engine
       engine_name "data_explorer"
diff --git a/spec/requests/group_spec.rb b/spec/requests/group_spec.rb
new file mode 100644
index 0000000..cf4163b
--- /dev/null
+++ b/spec/requests/group_spec.rb
@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe "Data explorer group serializer additions" do
+  let(:group_user) { Fabricate(:user) }
+  let(:other_user) { Fabricate(:user) }
+  let(:group) { Fabricate(:group) }
+  let!(:query) { DataExplorer::Query.create!(name: "My query", sql: "") }
+
+  before do
+    SiteSetting.data_explorer_enabled = true
+    group.add(group_user)
+    DataExplorer::QueryGroup.create!(group: group, query: query)
+  end
+
+  it "query boolean is true for group user" do
+    sign_in group_user
+    get "/g/#{group.name}.json"
+    expect(response.status).to eq(200)
+    expect(response.parsed_body["group"]["has_visible_data_explorer_queries"]).to eq(true)
+  end
+
+  it "query boolean is false for group user when there are no queries" do
+    query.destroy!
+    sign_in group_user
+    get "/g/#{group.name}.json"
+    expect(response.status).to eq(200)
+    expect(response.parsed_body["group"]["has_visible_data_explorer_queries"]).to eq(false)
+  end
+
+  it "does not include query boolean for anon" do
+    get "/g/#{group.name}.json"
+    expect(response.status).to eq(200)
+    expect(response.parsed_body["group"]["has_visible_data_explorer_queries"]).to eq(nil)
+  end
+
+  it "does not include query boolean for non-group user" do
+    sign_in other_user
+    get "/g/#{group.name}.json"
+    expect(response.status).to eq(200)
+    expect(response.parsed_body["group"]["has_visible_data_explorer_queries"]).to eq(nil)
+  end
+end