From 544a4e4b4896c1a84df69594c5dc9ab345740c59 Mon Sep 17 00:00:00 2001
From: Penar Musaraj <pmusaraj@gmail.com>
Date: Wed, 10 Feb 2021 13:12:04 -0500
Subject: [PATCH] UX: Changes to new features section in admin dashboard
 (#12029)

---
 .../components/dashboard-new-features.js      | 11 +++++--
 .../components/dashboard-new-features.hbs     | 32 +++++++++----------
 .../admin/addon/templates/dashboard.hbs       |  4 +--
 .../stylesheets/common/admin/dashboard.scss   | 23 ++++++++++++-
 app/assets/stylesheets/mobile/dashboard.scss  |  3 ++
 app/controllers/admin/dashboard_controller.rb |  7 ++--
 lib/discourse_updates.rb                      | 19 +++++++----
 spec/components/discourse_updates_spec.rb     | 32 ++++++-------------
 .../admin/dashboard_controller_spec.rb        | 13 ++++++++
 9 files changed, 92 insertions(+), 52 deletions(-)

diff --git a/app/assets/javascripts/admin/addon/components/dashboard-new-features.js b/app/assets/javascripts/admin/addon/components/dashboard-new-features.js
index 937340c1d36..059e138240a 100644
--- a/app/assets/javascripts/admin/addon/components/dashboard-new-features.js
+++ b/app/assets/javascripts/admin/addon/components/dashboard-new-features.js
@@ -1,9 +1,11 @@
 import Component from "@ember/component";
-import { action } from "@ember/object";
+import { action, computed } from "@ember/object";
 import { ajax } from "discourse/lib/ajax";
 
 export default Component.extend({
   newFeatures: null,
+  classNames: ["section", "dashboard-new-features"],
+  classNameBindings: ["hasUnseenFeatures:ordered-first"],
   releaseNotesLink: null,
 
   init() {
@@ -12,15 +14,20 @@ export default Component.extend({
     ajax("/admin/dashboard/new-features.json").then((json) => {
       this.setProperties({
         newFeatures: json.new_features,
+        hasUnseenFeatures: json.has_unseen_features,
         releaseNotesLink: json.release_notes_link,
       });
     });
   },
 
+  columnCountClass: computed("newFeatures", function () {
+    return this.newFeatures.length > 2 ? "three-or-more-items" : "";
+  }),
+
   @action
   dismissNewFeatures() {
     ajax("/admin/dashboard/mark-new-features-as-seen.json", {
       type: "PUT",
-    }).then(() => this.set("newFeatures", null));
+    }).then(() => this.set("hasUnseenFeatures", false));
   },
 });
diff --git a/app/assets/javascripts/admin/addon/templates/components/dashboard-new-features.hbs b/app/assets/javascripts/admin/addon/templates/components/dashboard-new-features.hbs
index 190e53a4326..8d7e1c78f30 100644
--- a/app/assets/javascripts/admin/addon/templates/components/dashboard-new-features.hbs
+++ b/app/assets/javascripts/admin/addon/templates/components/dashboard-new-features.hbs
@@ -1,21 +1,19 @@
 {{#if newFeatures}}
-  <div class="section dashboard-new-features">
-    <div class="section-title">
-      <h2>{{replace-emoji (i18n "admin.dashboard.new_features.title") }}</h2>
-    </div>
+  <div class="section-title">
+    <h2>{{replace-emoji (i18n "admin.dashboard.new_features.title") }}</h2>
+  </div>
 
-    <div class="section-body">
-      {{#each newFeatures as |feature|}}
-        {{dashboard-new-feature-item item=feature}}
-      {{/each}}
-    </div>
-    <div class="section-footer">
-      {{#if releaseNotesLink}}
-        <a rel="noopener noreferrer" target="_blank" href={{releaseNotesLink}} class="btn btn-primary new-features-release-notes">
-          {{i18n "admin.dashboard.new_features.learn_more"}}
-        </a>
-      {{/if}}
-      {{d-button label="admin.dashboard.new_features.dismiss" class="new-features-dismiss" action=dismissNewFeatures }}
-    </div>
+  <div class="section-body {{columnCountClass}}">
+    {{#each newFeatures as |feature|}}
+      {{dashboard-new-feature-item item=feature tagName=""}}
+    {{/each}}
+  </div>
+  <div class="section-footer">
+    {{#if releaseNotesLink}}
+      <a rel="noopener noreferrer" target="_blank" href={{releaseNotesLink}} class="btn btn-primary new-features-release-notes">
+        {{i18n "admin.dashboard.new_features.learn_more"}}
+      </a>
+    {{/if}}
+    {{d-button label="admin.dashboard.new_features.dismiss" class="new-features-dismiss" action=dismissNewFeatures }}
   </div>
 {{/if}}
diff --git a/app/assets/javascripts/admin/addon/templates/dashboard.hbs b/app/assets/javascripts/admin/addon/templates/dashboard.hbs
index cb87e84b471..95f26205e7c 100644
--- a/app/assets/javascripts/admin/addon/templates/dashboard.hbs
+++ b/app/assets/javascripts/admin/addon/templates/dashboard.hbs
@@ -1,5 +1,3 @@
-{{dashboard-new-features}}
-
 {{plugin-outlet name="admin-dashboard-top"}}
 
 {{#if showVersionChecks}}
@@ -52,4 +50,6 @@
 
 {{outlet}}
 
+{{dashboard-new-features tagName="div"}}
+
 {{plugin-outlet name="admin-dashboard-bottom"}}
diff --git a/app/assets/stylesheets/common/admin/dashboard.scss b/app/assets/stylesheets/common/admin/dashboard.scss
index 0e553c4a9a8..0e49cb27a41 100644
--- a/app/assets/stylesheets/common/admin/dashboard.scss
+++ b/app/assets/stylesheets/common/admin/dashboard.scss
@@ -613,11 +613,32 @@
   }
 }
 
+.dashboard-next.general {
+  display: flex;
+  flex-direction: column;
+}
 .dashboard-new-features {
+  &.ordered-first {
+    order: -1;
+  }
+
+  &:not(.ordered-first) {
+    .section-title {
+      margin-top: 1.5em;
+    }
+
+    .new-features-dismiss {
+      display: none;
+    }
+  }
+
   .section-body {
     display: grid;
-    grid-template-columns: repeat(auto-fill, minmax(300px, 1fr));
+    grid-template-columns: repeat(auto-fill, minmax(360px, 1fr));
     grid-gap: 1.5em;
+    &.three-or-more-items {
+      grid-template-columns: repeat(auto-fill, minmax(300px, 1fr));
+    }
   }
 
   .section-footer {
diff --git a/app/assets/stylesheets/mobile/dashboard.scss b/app/assets/stylesheets/mobile/dashboard.scss
index 71e8ef67610..3251b226690 100644
--- a/app/assets/stylesheets/mobile/dashboard.scss
+++ b/app/assets/stylesheets/mobile/dashboard.scss
@@ -6,4 +6,7 @@
   .navigation a.navigation-link {
     padding: 0.5em;
   }
+  .dashboard-new-features .section-body {
+    grid-template-columns: none;
+  }
 }
diff --git a/app/controllers/admin/dashboard_controller.rb b/app/controllers/admin/dashboard_controller.rb
index 1b75618f883..38e3fe0f074 100644
--- a/app/controllers/admin/dashboard_controller.rb
+++ b/app/controllers/admin/dashboard_controller.rb
@@ -24,8 +24,11 @@ class Admin::DashboardController < Admin::AdminController
   end
 
   def new_features
-    data = { new_features: DiscourseUpdates.unseen_new_features(current_user.id) }
-    data.merge!(release_notes_link: AdminDashboardGeneralData.fetch_cached_stats["release_notes_link"])
+    data = {
+      new_features: DiscourseUpdates.new_features,
+      has_unseen_features: DiscourseUpdates.has_unseen_features?(current_user.id),
+      release_notes_link: AdminDashboardGeneralData.fetch_cached_stats["release_notes_link"]
+    }
     render json: data
   end
 
diff --git a/lib/discourse_updates.rb b/lib/discourse_updates.rb
index a0274fe0929..91e5072bb78 100644
--- a/lib/discourse_updates.rb
+++ b/lib/discourse_updates.rb
@@ -121,21 +121,28 @@ module DiscourseUpdates
       Discourse.redis.set(new_features_key, response.body)
     end
 
-    def unseen_new_features(user_id)
+    def new_features
       entries = JSON.parse(Discourse.redis.get(new_features_key)) rescue nil
       return nil if entries.nil?
 
+      entries.select! do |item|
+        item["discourse_version"].nil? || Discourse.has_needed_version?(Discourse::VERSION::STRING, item["discourse_version"]) rescue nil
+      end
+
+      entries.sort { |item| Time.zone.parse(item["created_at"]) }
+    end
+
+    def has_unseen_features?(user_id)
+      entries = new_features
+      return false if entries.nil?
+
       last_seen = new_features_last_seen(user_id)
 
       if last_seen.present?
         entries.select! { |item| Time.zone.parse(item["created_at"]) > last_seen }
       end
 
-      entries.select! do |item|
-        item["discourse_version"].nil? || Discourse.has_needed_version?(Discourse::VERSION::STRING, item["discourse_version"]) rescue nil
-      end
-
-      entries.sort { |item| Time.zone.parse(item["created_at"]) }
+      entries.size > 0
     end
 
     def new_features_last_seen(user_id)
diff --git a/spec/components/discourse_updates_spec.rb b/spec/components/discourse_updates_spec.rb
index 41df2568636..3bdf7a683a3 100644
--- a/spec/components/discourse_updates_spec.rb
+++ b/spec/components/discourse_updates_spec.rb
@@ -158,46 +158,37 @@ describe DiscourseUpdates do
     before(:each) do
       Discourse.redis.del "new_features_last_seen_user_#{admin.id}"
       Discourse.redis.del "new_features_last_seen_user_#{admin2.id}"
-      Discourse.redis.del "new_features"
-
       Discourse.redis.set('new_features', MultiJson.dump(sample_features))
     end
 
     it 'returns all items on the first run' do
-      result = DiscourseUpdates.unseen_new_features(admin.id)
+      result = DiscourseUpdates.new_features
 
       expect(result.length).to eq(3)
       expect(result[2]["title"]).to eq("Super Fruits")
     end
 
-    it 'returns only unseen items by user' do
+    it 'correctly marks unseen items by user' do
       DiscourseUpdates.stubs(:new_features_last_seen).with(admin.id).returns(10.minutes.ago)
       DiscourseUpdates.stubs(:new_features_last_seen).with(admin2.id).returns(30.minutes.ago)
 
-      result = DiscourseUpdates.unseen_new_features(admin.id)
-      expect(result.length).to eq(1)
-      expect(result[0]["title"]).to eq("Quality Veggies")
-
-      result2 = DiscourseUpdates.unseen_new_features(admin2.id)
-      expect(result2.length).to eq(2)
-      expect(result2[0]["title"]).to eq("Quality Veggies")
-      expect(result2[1]["title"]).to eq("Fancy Legumes")
+      expect(DiscourseUpdates.has_unseen_features?(admin.id)).to eq(true)
+      expect(DiscourseUpdates.has_unseen_features?(admin2.id)).to eq(true)
     end
 
     it 'can mark features as seen for a given user' do
-      expect(DiscourseUpdates.unseen_new_features(admin.id)).to be_present
+      expect(DiscourseUpdates.has_unseen_features?(admin.id)).to be_truthy
 
       DiscourseUpdates.mark_new_features_as_seen(admin.id)
-      expect(DiscourseUpdates.unseen_new_features(admin.id)).to be_empty
+      expect(DiscourseUpdates.has_unseen_features?(admin.id)).to eq(false)
 
       # doesn't affect another user
-      expect(DiscourseUpdates.unseen_new_features(admin2.id)).to be_present
-
+      expect(DiscourseUpdates.has_unseen_features?(admin2.id)).to eq(true)
     end
 
     it 'correctly sees newly added features as unseen' do
       DiscourseUpdates.mark_new_features_as_seen(admin.id)
-      expect(DiscourseUpdates.unseen_new_features(admin.id)).to be_empty
+      expect(DiscourseUpdates.has_unseen_features?(admin.id)).to eq(false)
       expect(DiscourseUpdates.new_features_last_seen(admin.id)).to be_within(1.second).of (last_item_date)
 
       updated_features = [
@@ -206,10 +197,7 @@ describe DiscourseUpdates do
       updated_features += sample_features
 
       Discourse.redis.set('new_features', MultiJson.dump(updated_features))
-
-      result = DiscourseUpdates.unseen_new_features(admin.id)
-      expect(result.length).to eq(1)
-      expect(result[0]["title"]).to eq("Brand New Item")
+      expect(DiscourseUpdates.has_unseen_features?(admin.id)).to eq(true)
     end
 
     it 'correctly shows features by Discourse version' do
@@ -224,7 +212,7 @@ describe DiscourseUpdates do
 
       Discourse.redis.set('new_features', MultiJson.dump(features_with_versions))
       DiscourseUpdates.stubs(:last_installed_version).returns("2.7.0.beta2")
-      result = DiscourseUpdates.unseen_new_features(admin.id)
+      result = DiscourseUpdates.new_features
 
       expect(result.length).to eq(3)
       expect(result[0]["title"]).to eq("Confetti")
diff --git a/spec/requests/admin/dashboard_controller_spec.rb b/spec/requests/admin/dashboard_controller_spec.rb
index b838f169dc7..00ed3b3aaa5 100644
--- a/spec/requests/admin/dashboard_controller_spec.rb
+++ b/spec/requests/admin/dashboard_controller_spec.rb
@@ -118,6 +118,18 @@ describe Admin::DashboardController do
         expect(json['new_features'].length).to eq(2)
         expect(json['new_features'][0]["emoji"]).to eq("🙈")
         expect(json['new_features'][0]["title"]).to eq("Fancy Legumes")
+        expect(json['has_unseen_features']).to eq(true)
+      end
+
+      it 'passes unseen feature state' do
+        populate_new_features
+        DiscourseUpdates.mark_new_features_as_seen(admin.id)
+
+        get "/admin/dashboard/new-features.json"
+        expect(response.status).to eq(200)
+        json = response.parsed_body
+
+        expect(json['has_unseen_features']).to eq(false)
       end
     end
 
@@ -128,6 +140,7 @@ describe Admin::DashboardController do
 
         expect(response.status).to eq(200)
         expect(DiscourseUpdates.new_features_last_seen(admin.id)).not_to eq(nil)
+        expect(DiscourseUpdates.has_unseen_features?(admin.id)).to eq(false)
       end
     end
   end