From 8e561977287c133234bd09c43ed75c26abf01420 Mon Sep 17 00:00:00 2001
From: Vinoth Kannan <vinothkannan@vinkas.com>
Date: Mon, 25 May 2020 11:08:47 +0530
Subject: [PATCH] UX: use "icon-picker" & "image-uploader" fields to set group
 flair. (#9779)

---
 .../app/components/group-flair-inputs.js      | 53 +++++++++----
 .../javascripts/discourse/app/models/group.js |  9 ++-
 .../components/group-flair-inputs.hbs         | 37 +++++++--
 .../discourse/app/widgets/avatar-flair.js     |  2 +-
 app/assets/stylesheets/common/base/group.scss | 16 ++++
 app/controllers/admin/groups_controller.rb    |  3 +-
 app/controllers/groups_controller.rb          |  3 +-
 .../onceoff/migrate_group_flair_images.rb     | 78 +++++++++++++++++++
 app/jobs/scheduled/clean_up_uploads.rb        |  2 +
 app/models/group.rb                           | 12 ++-
 app/serializers/group_show_serializer.rb      | 10 ++-
 config/locales/client.en.yml                  |  6 +-
 ...1043818_add_more_flair_columns_to_group.rb | 36 +++++++++
 lib/primary_group_lookup.rb                   |  4 +-
 lib/svg_sprite/svg_sprite.rb                  |  2 +-
 spec/components/svg_sprite/svg_sprite_spec.rb |  4 +-
 spec/jobs/migrate_group_flair_images_spec.rb  | 26 +++++++
 spec/requests/groups_controller_spec.rb       |  2 +-
 18 files changed, 270 insertions(+), 35 deletions(-)
 create mode 100644 app/jobs/onceoff/migrate_group_flair_images.rb
 create mode 100644 db/migrate/20200511043818_add_more_flair_columns_to_group.rb
 create mode 100644 spec/jobs/migrate_group_flair_images_spec.rb

diff --git a/app/assets/javascripts/discourse/app/components/group-flair-inputs.js b/app/assets/javascripts/discourse/app/components/group-flair-inputs.js
index 7b432e12713..b5ecb07bfc1 100644
--- a/app/assets/javascripts/discourse/app/components/group-flair-inputs.js
+++ b/app/assets/javascripts/discourse/app/components/group-flair-inputs.js
@@ -2,11 +2,12 @@ import I18n from "I18n";
 import discourseComputed from "discourse-common/utils/decorators";
 import { debounce } from "@ember/runloop";
 import Component from "@ember/component";
-import { observes } from "discourse-common/utils/decorators";
+import { on, observes } from "discourse-common/utils/decorators";
 import { escapeExpression } from "discourse/lib/utilities";
 import { convertIconClass } from "discourse-common/lib/icon-library";
 import { ajax } from "discourse/lib/ajax";
 import { htmlSafe } from "@ember/template";
+import { action } from "@ember/object";
 
 export default Component.extend({
   classNames: ["group-flair-inputs"],
@@ -16,23 +17,26 @@ export default Component.extend({
     return Discourse.getURL("/images/avatar.png");
   },
 
-  @discourseComputed("model.flair_url")
-  flairPreviewIcon(flairURL) {
-    return flairURL && /fa(r|b?)-/.test(flairURL);
+  @discourseComputed("model.flair_type")
+  flairPreviewIcon(flairType) {
+    return flairType && flairType === "icon";
   },
 
-  @discourseComputed("model.flair_url", "flairPreviewIcon")
-  flairPreviewIconUrl(flairURL, flairPreviewIcon) {
-    return flairPreviewIcon ? convertIconClass(flairURL) : "";
+  @discourseComputed("model.flair_icon")
+  flairPreviewIconUrl(flairIcon) {
+    return flairIcon ? convertIconClass(flairIcon) : "";
   },
 
-  @observes("model.flair_url")
-  _loadSVGIcon() {
-    debounce(this, this._loadIcon, 1000);
+  @on("didInsertElement")
+  @observes("model.flair_icon")
+  _loadSVGIcon(flairIcon) {
+    if (flairIcon) {
+      debounce(this, this._loadIcon, 1000);
+    }
   },
 
   _loadIcon() {
-    const icon = convertIconClass(this.get("model.flair_url")),
+    const icon = convertIconClass(this.model.flair_icon),
       c = "#svg-sprites",
       h = "ajax-icon-holder",
       singleIconEl = `${c} .${h}`;
@@ -50,9 +54,14 @@ export default Component.extend({
     }
   },
 
-  @discourseComputed("model.flair_url", "flairPreviewIcon")
-  flairPreviewImage(flairURL, flairPreviewIcon) {
-    return flairURL && !flairPreviewIcon;
+  @discourseComputed("model.flair_type")
+  flairPreviewImage(flairType) {
+    return flairType && flairType === "image";
+  },
+
+  @discourseComputed("model.flair_url")
+  flairImageUrl(flairURL) {
+    return flairURL && flairURL.match(/\//) ? flairURL : null;
   },
 
   @discourseComputed(
@@ -91,5 +100,21 @@ export default Component.extend({
   flairPreviewLabel(flairPreviewImage) {
     const key = flairPreviewImage ? "image" : "icon";
     return I18n.t(`groups.flair_preview_${key}`);
+  },
+
+  @action
+  setFlairImage(upload) {
+    this.model.setProperties({
+      flair_url: Discourse.getURL(upload.url),
+      flair_upload_id: upload.id
+    });
+  },
+
+  @action
+  removeFlairImage() {
+    this.model.setProperties({
+      flair_url: null,
+      flair_upload_id: null
+    });
   }
 });
diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js
index ea4d03d0158..142341499ea 100644
--- a/app/assets/javascripts/discourse/app/models/group.js
+++ b/app/assets/javascripts/discourse/app/models/group.js
@@ -188,7 +188,8 @@ const Group = RestModel.extend({
       primary_group: !!this.primary_group,
       grant_trust_level: this.grant_trust_level,
       incoming_email: this.incoming_email,
-      flair_url: this.flair_url,
+      flair_icon: null,
+      flair_upload_id: null,
       flair_bg_color: this.flairBackgroundHexColor,
       flair_color: this.flairHexColor,
       bio_raw: this.bio_raw,
@@ -201,6 +202,12 @@ const Group = RestModel.extend({
       publish_read_state: this.publish_read_state
     };
 
+    if (this.flair_type === "icon") {
+      attrs["flair_icon"] = this.flair_icon;
+    } else if (this.flair_type === "image") {
+      attrs["flair_upload_id"] = this.flair_upload_id;
+    }
+
     if (!this.id) {
       attrs["usernames"] = this.usernames;
       attrs["owner_usernames"] = this.ownerUsernames;
diff --git a/app/assets/javascripts/discourse/app/templates/components/group-flair-inputs.hbs b/app/assets/javascripts/discourse/app/templates/components/group-flair-inputs.hbs
index 36dfc262026..f198b7bb636 100644
--- a/app/assets/javascripts/discourse/app/templates/components/group-flair-inputs.hbs
+++ b/app/assets/javascripts/discourse/app/templates/components/group-flair-inputs.hbs
@@ -1,13 +1,36 @@
 <div class="control-group">
   <label class="control-label" for="flair_url">{{i18n "groups.flair_url"}}</label>
 
-  {{text-field name="flair_url"
-      class="input-xxlarge"
-      value=model.flair_url
-      placeholderKey="groups.flair_url_placeholder"}}
-  <div class="control-instructions">
-    {{i18n "groups.flair_url_description"}}
+  <div class="radios">
+    <label class="radio-label" for="avatar-flair">
+      {{radio-button name="avatar-flair" value="icon" selection=model.flair_type}}
+      <b>{{i18n "groups.flair_type.icon"}}</b>
+    </label>
+
+    <label class="radio-label" for="avatar-flair">
+      {{radio-button name="avatar-flair" value="image" selection=model.flair_type}}
+      <b>{{i18n "groups.flair_type.image"}}</b>
+    </label>
   </div>
+
+  {{#if flairPreviewIcon}}
+    {{icon-picker
+      name="icon"
+      value=model.flair_icon
+      options=(hash maximum=1)
+      onChange=(action (mut model.flair_icon))
+    }}
+  {{else if flairPreviewImage}}
+    {{image-uploader
+      imageUrl=flairImageUrl
+      onUploadDone=(action "setFlairImage")
+      onUploadDeleted=(action "removeFlairImage")
+      type="group_flair"
+      class="no-repeat contain-image"}}
+    <div class="control-instructions">
+      {{i18n "groups.flair_upload_description"}}
+    </div>
+  {{/if}}
 </div>
 
 <div class="control-group">
@@ -40,7 +63,7 @@
 
     {{#if flairPreviewImage}}
       <div class="avatar-flair demo {{flairPreviewClasses}}" style={{flairPreviewStyle}}></div>
-    {{else}}
+    {{else if flairPreviewIcon}}
       <div class="avatar-flair demo {{flairPreviewClasses}}" style={{flairPreviewStyle}} role="presentation">
         {{d-icon flairPreviewIconUrl}}
       </div>
diff --git a/app/assets/javascripts/discourse/app/widgets/avatar-flair.js b/app/assets/javascripts/discourse/app/widgets/avatar-flair.js
index 0da095de00d..94e015fbc4c 100644
--- a/app/assets/javascripts/discourse/app/widgets/avatar-flair.js
+++ b/app/assets/javascripts/discourse/app/widgets/avatar-flair.js
@@ -8,7 +8,7 @@ createWidget("avatar-flair", {
   isIcon(attrs) {
     return (
       attrs.primary_group_flair_url &&
-      attrs.primary_group_flair_url.includes("fa-")
+      !attrs.primary_group_flair_url.includes("/")
     );
   },
 
diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss
index 013acd086c2..998a82a0e9b 100644
--- a/app/assets/stylesheets/common/base/group.scss
+++ b/app/assets/stylesheets/common/base/group.scss
@@ -153,6 +153,22 @@ table.group-members {
       background-color: #f4f4f4;
     }
   }
+
+  .radios {
+    margin-bottom: 5px;
+  }
+
+  .radio-label {
+    display: inline-flex;
+    padding-right: 10px;
+    margin-bottom: 0;
+    align-items: center;
+  }
+
+  .uploaded-image-preview {
+    height: 75px;
+    width: 275px;
+  }
 }
 
 .group-form-save {
diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb
index 50cef291640..32114f33699 100644
--- a/app/controllers/admin/groups_controller.rb
+++ b/app/controllers/admin/groups_controller.rb
@@ -163,7 +163,8 @@ class Admin::GroupsController < Admin::AdminController
       :primary_group,
       :grant_trust_level,
       :incoming_email,
-      :flair_url,
+      :flair_icon,
+      :flair_upload_id,
       :flair_bg_color,
       :flair_color,
       :bio_raw,
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 2716282d6ad..3d4b828ad1c 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -521,7 +521,8 @@ class GroupsController < ApplicationController
           mentionable_level
           messageable_level
           title
-          flair_url
+          flair_icon
+          flair_upload_id
           flair_bg_color
           flair_color
           bio_raw
diff --git a/app/jobs/onceoff/migrate_group_flair_images.rb b/app/jobs/onceoff/migrate_group_flair_images.rb
new file mode 100644
index 00000000000..d400cfa17d2
--- /dev/null
+++ b/app/jobs/onceoff/migrate_group_flair_images.rb
@@ -0,0 +1,78 @@
+# frozen_string_literal: true
+
+module Jobs
+  class MigrateGroupFlairImages < ::Jobs::Onceoff
+    def execute_onceoff(args)
+      return if Group.column_names.exclude?("flair_url")
+
+      Group.where.not(flair_url: nil).each do |group|
+        if group.flair_upload.present?
+          g.update_column(:flair_url, nil)
+          next
+        end
+
+        old_url = group[:flair_url]
+        group_name = group.name
+
+        count = 0
+        file = nil
+        sleep_interval = 5
+
+        loop do
+          url = UrlHelper.absolute_without_cdn(old_url)
+
+          begin
+            file = FileHelper.download(
+              url,
+              max_file_size: [
+                SiteSetting.max_image_size_kb.kilobytes,
+                20.megabytes
+              ].max,
+              tmp_file_name: 'tmp_group_flair_upload',
+              skip_rate_limit: true,
+              follow_redirect: true
+            )
+          rescue OpenURI::HTTPError,
+                 OpenSSL::SSL::SSLError,
+                 Net::OpenTimeout,
+                 Net::ReadTimeout,
+                 Errno::ECONNREFUSED,
+                 EOFError,
+                 SocketError,
+                 Discourse::InvalidParameters => e
+
+            logger.warn(
+              "Error encountered when trying to download file " +
+              "for group '#{group_name}'.\n#{e.class}: #{e.message}\n#{e.backtrace.join("\n")}"
+            )
+          end
+
+          count += 1
+          break if file || (file.blank? && count >= 3)
+
+          logger.info(
+            "Failed to download upload from #{url} for group '#{group_name}'. Retrying..."
+          )
+
+          sleep(count * sleep_interval)
+        end
+
+        next if file.blank?
+
+        upload = UploadCreator.new(
+          file,
+          "group_#{group_name}",
+          origin: UrlHelper.absolute(old_url)
+        ).create_for(Discourse.system_user.id)
+
+        group.update_columns(flair_upload_id: upload.id, flair_url: nil) if upload.present?
+      end
+    end
+
+    private
+
+    def logger
+      Rails.logger
+    end
+  end
+end
diff --git a/app/jobs/scheduled/clean_up_uploads.rb b/app/jobs/scheduled/clean_up_uploads.rb
index acf986a4141..95f528c5440 100644
--- a/app/jobs/scheduled/clean_up_uploads.rb
+++ b/app/jobs/scheduled/clean_up_uploads.rb
@@ -68,6 +68,7 @@ module Jobs
         .joins("LEFT JOIN custom_emojis ce ON ce.upload_id = uploads.id")
         .joins("LEFT JOIN theme_fields tf ON tf.upload_id = uploads.id")
         .joins("LEFT JOIN user_exports ue ON ue.upload_id = uploads.id")
+        .joins("LEFT JOIN groups g ON g.flair_upload_id = uploads.id")
         .where("pu.upload_id IS NULL")
         .where("u.uploaded_avatar_id IS NULL")
         .where("ua.gravatar_upload_id IS NULL AND ua.custom_upload_id IS NULL")
@@ -76,6 +77,7 @@ module Jobs
         .where("ce.upload_id IS NULL")
         .where("tf.upload_id IS NULL")
         .where("ue.upload_id IS NULL")
+        .where("g.flair_upload_id IS NULL")
         .where("ss.value IS NULL")
 
       result = result.where("uploads.url NOT IN (?)", ignore_urls) if ignore_urls.present?
diff --git a/app/models/group.rb b/app/models/group.rb
index 80c6080bc1b..b2d37283efc 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -27,6 +27,8 @@ class Group < ActiveRecord::Base
   has_many :category_reviews, class_name: 'Category', foreign_key: :reviewable_by_group_id, dependent: :nullify
   has_many :reviewables, foreign_key: :reviewable_by_group_id, dependent: :nullify
 
+  belongs_to :flair_upload, class_name: 'Upload'
+
   has_and_belongs_to_many :web_hooks
 
   before_save :downcase_incoming_email
@@ -62,7 +64,6 @@ class Group < ActiveRecord::Base
   validate :automatic_membership_email_domains_format_validator
   validate :incoming_email_validator
   validate :can_allow_membership_requests, if: :allow_membership_requests
-  validates :flair_url, url: true, if: Proc.new { |g| g.flair_url && g.flair_url.exclude?('fa-') }
   validate :validate_grant_trust_level, if: :will_save_change_to_grant_trust_level?
 
   AUTO_GROUPS = {
@@ -741,6 +742,15 @@ class Group < ActiveRecord::Base
     end
   end
 
+  def flair_type
+    return :icon if flair_icon.present?
+    return :image if flair_upload_id.present?
+  end
+
+  def flair_url
+    flair_icon.presence || flair_upload&.url || self[:flair_url].presence
+  end
+
   protected
 
   def name_format_validator
diff --git a/app/serializers/group_show_serializer.rb b/app/serializers/group_show_serializer.rb
index f1440c1e9df..d3400df6588 100644
--- a/app/serializers/group_show_serializer.rb
+++ b/app/serializers/group_show_serializer.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class GroupShowSerializer < BasicGroupSerializer
-  attributes :is_group_user, :is_group_owner, :is_group_owner_display, :mentionable, :messageable
+  attributes :is_group_user, :is_group_owner, :is_group_owner_display, :mentionable, :messageable, :flair_icon, :flair_type
 
   def include_is_group_user?
     authenticated?
@@ -43,6 +43,14 @@ class GroupShowSerializer < BasicGroupSerializer
     Group.messageable(scope.user).exists?(id: object.id)
   end
 
+  def include_flair_icon?
+    is_group_owner && flair_icon.present?
+  end
+
+  def include_flair_type?
+    is_group_owner && flair_type.present?
+  end
+
   private
 
   def authenticated?
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 3c3618ef96e..673cb435240 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -751,14 +751,16 @@ en:
           title: "Muted"
           description: "You will not be notified of anything about messages in this group."
       flair_url: "Avatar Flair Image"
-      flair_url_placeholder: "(Optional) Image URL or Font Awesome class"
-      flair_url_description: 'Use square images no smaller than 20px by 20px or FontAwesome icons (accepted formats: "fa-icon", "far fa-icon" or "fab fa-icon").'
+      flair_upload_description: "Use square images no smaller than 20px by 20px."
       flair_bg_color: "Avatar Flair Background Color"
       flair_bg_color_placeholder: "(Optional) Hex color value"
       flair_color: "Avatar Flair Color"
       flair_color_placeholder: "(Optional) Hex color value"
       flair_preview_icon: "Preview Icon"
       flair_preview_image: "Preview Image"
+      flair_type:
+        icon: "Select an icon"
+        image: "Upload an image"
 
     user_action_groups:
       "1": "Likes"
diff --git a/db/migrate/20200511043818_add_more_flair_columns_to_group.rb b/db/migrate/20200511043818_add_more_flair_columns_to_group.rb
new file mode 100644
index 00000000000..70c1b530439
--- /dev/null
+++ b/db/migrate/20200511043818_add_more_flair_columns_to_group.rb
@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+class AddMoreFlairColumnsToGroup < ActiveRecord::Migration[6.0]
+  def change
+    add_column :groups, :flair_icon, :string
+    add_column :groups, :flair_upload_id, :integer
+
+    reversible do |dir|
+      dir.up do
+        DB.exec(
+          <<~SQL
+            UPDATE groups SET flair_icon = REPLACE(REPLACE(flair_url, 'fas fa-', ''), ' fa-', '-')
+            WHERE flair_url LIKE 'fa%'
+          SQL
+        )
+
+        DB.exec(
+          <<~SQL
+            UPDATE groups g1
+            SET flair_upload_id = u.id
+            FROM groups g2
+            INNER JOIN uploads u ON g2.flair_url ~ CONCAT('\/', u.sha1, '[\.\w]*')
+            WHERE g1.id = g2.id
+          SQL
+        )
+
+        DB.exec(
+          <<~SQL
+            UPDATE groups SET flair_url = NULL
+            WHERE flair_icon IS NOT NULL OR flair_upload_id IS NOT NULL
+          SQL
+        )
+      end
+    end
+  end
+end
diff --git a/lib/primary_group_lookup.rb b/lib/primary_group_lookup.rb
index 4dd9ce1020b..8110a3792ba 100644
--- a/lib/primary_group_lookup.rb
+++ b/lib/primary_group_lookup.rb
@@ -13,7 +13,7 @@ class PrimaryGroupLookup
   private
 
   def self.lookup_columns
-    @lookup_columns ||= %i{id name flair_url flair_bg_color flair_color}
+    @lookup_columns ||= %i{id name flair_icon flair_upload_id flair_bg_color flair_color}
   end
 
   def users
@@ -29,7 +29,7 @@ class PrimaryGroupLookup
     group_ids = users_with_primary_group.map(&:primary_group_id)
     group_ids.uniq!
 
-    Group.where(id: group_ids).select(self.class.lookup_columns)
+    Group.includes(:flair_upload).where(id: group_ids).select(self.class.lookup_columns)
       .each { |g| group_lookup[g.id] = g }
 
     hash = {}
diff --git a/lib/svg_sprite/svg_sprite.rb b/lib/svg_sprite/svg_sprite.rb
index 918a28435cc..90571781d17 100644
--- a/lib/svg_sprite/svg_sprite.rb
+++ b/lib/svg_sprite/svg_sprite.rb
@@ -388,7 +388,7 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL
   end
 
   def self.group_icons
-    Group.where("flair_url LIKE '%fa-%'").pluck(:flair_url).uniq
+    Group.pluck(:flair_icon).uniq
   end
 
   def self.theme_icons(theme_ids)
diff --git a/spec/components/svg_sprite/svg_sprite_spec.rb b/spec/components/svg_sprite/svg_sprite_spec.rb
index 34e4d59b137..4a3100a5aab 100644
--- a/spec/components/svg_sprite/svg_sprite_spec.rb
+++ b/spec/components/svg_sprite/svg_sprite_spec.rb
@@ -216,12 +216,12 @@ describe SvgSprite do
   end
 
   it "includes Font Awesome 4.7 icons as group flair" do
-    group = Fabricate(:group, flair_url: "fa-air-freshener")
+    group = Fabricate(:group, flair_icon: "fa-air-freshener")
     expect(SvgSprite.bundle).to match(/air-freshener/)
   end
 
   it "includes Font Awesome 5 icons as group flair" do
-    group = Fabricate(:group, flair_url: "far fa-building")
+    group = Fabricate(:group, flair_icon: "far fa-building")
     expect(SvgSprite.bundle).to match(/building/)
   end
 end
diff --git a/spec/jobs/migrate_group_flair_images_spec.rb b/spec/jobs/migrate_group_flair_images_spec.rb
new file mode 100644
index 00000000000..a06b025d264
--- /dev/null
+++ b/spec/jobs/migrate_group_flair_images_spec.rb
@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Jobs::MigrateGroupFlairImages do
+  let(:image_url) { "https://omg.aws.somestack/test.png" }
+  let(:group) { Fabricate(:group, flair_url: image_url) }
+
+  before do
+    stub_request(:get, image_url).to_return(
+      status: 200, body: file_from_fixtures("smallest.png").read
+    )
+  end
+
+  it 'should migrate to the new group `flair_upload_id` column correctly' do
+    group
+
+    expect do
+      described_class.new.execute_onceoff({})
+    end.to change { Upload.count }.by(1)
+
+    group.reload
+    expect(group.flair_upload).to eq(Upload.last)
+    expect(group[:flair_url]).to eq(nil)
+  end
+end
diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb
index ff387a2ff37..862b9a631d8 100644
--- a/spec/requests/groups_controller_spec.rb
+++ b/spec/requests/groups_controller_spec.rb
@@ -674,7 +674,7 @@ describe GroupsController do
               incoming_email: 'test@mail.org',
               flair_bg_color: 'FFF',
               flair_color: 'BBB',
-              flair_url: 'fa-adjust',
+              flair_icon: 'fa-adjust',
               bio_raw: 'testing',
               full_name: 'awesome team',
               public_admission: true,