FIX: Badge image uploader (#28188) (#28521)

In the formkit conversion in 2ca06ba236
we missed setting a type for the UppyImageUploader for badges. Also,
we were not passing down the `image_url` as form data, so when we used
`data.image` for that field the badge was not updating in the UI after
page loads and the image URL was not loading for preview.

Co-authored-by: Martin Brennan <martin@discourse.org>

app/assets/javascripts/admin/addon/controllers/admin-badges/show.js

@@ -21,6 +21,7 @@ const FORM_FIELDS = [
   "long_description",
   "icon",
   "image_upload_id",
+  "image_url",
   "query",
   "badge_grouping_id",
   "trigger",

app/assets/javascripts/admin/addon/templates/admin-badges/show.hbs

@@ -65,7 +65,7 @@
     </form.Field>
 
     <form.ConditionalContent
-      @activeName={{if data.image "upload-image" "choose-icon"}}
+      @activeName={{if data.image_url "upload-image" "choose-icon"}}
       as |cc|
     >
       <cc.Conditions as |Condition|>
@@ -90,14 +90,14 @@
         </Content>
         <Content @name="upload-image">
           <form.Field
-            @name="image"
+            @name="image_url"
             @showTitle={{false}}
             @title={{i18n "admin.badges.image"}}
             @onSet={{this.onSetImage}}
             @onUnset={{this.onUnsetImage}}
             as |field|
           >
-            <field.Image />
+            <field.Image @type="badge_image" />
           </form.Field>
         </Content>
       </cc.Contents>

app/assets/javascripts/discourse/app/form-kit/components/fk/control/image.gjs

@@ -1,14 +1,17 @@
 import Component from "@glimmer/component";
+import { tracked } from "@glimmer/tracking";
 import { concat } from "@ember/helper";
 import { action } from "@ember/object";
 import UppyImageUploader from "discourse/components/uppy-image-uploader";
 
 export default class FKControlImage extends Component {
   static controlType = "image";
+  @tracked imageUrl = this.args.value;
 
   @action
   setImage(upload) {
     this.args.field.set(upload);
+    this.imageUrl = upload?.url;
   }
 
   @action
@@ -19,9 +22,10 @@ export default class FKControlImage extends Component {
   <template>
     <UppyImageUploader
       @id={{concat @field.id "-" @field.name}}
-      @imageUrl={{readonly @value}}
+      @imageUrl={{this.imageUrl}}
       @onUploadDone={{this.setImage}}
       @onUploadDeleted={{this.removeImage}}
+      @type={{@type}}
       class="form-kit__control-image no-repeat contain-image"
     />
   </template>

app/serializers/admin_badge_serializer.rb

@@ -1,7 +1,13 @@
 # frozen_string_literal: true
 
 class AdminBadgeSerializer < BadgeSerializer
-  attributes :query, :trigger, :target_posts, :auto_revoke, :show_posts, :i18n_name
+  attributes :query,
+             :trigger,
+             :target_posts,
+             :auto_revoke,
+             :show_posts,
+             :i18n_name,
+             :image_upload_id
 
   def include_long_description?
     true

spec/requests/api/schemas/json/badge_create_response.json

@@ -56,6 +56,12 @@
             "null"
           ]
         },
+        "image_upload_id": {
+          "type": [
+            "integer",
+            "null"
+          ]
+        },
         "listable": {
           "type": "boolean"
         },
@@ -123,7 +129,8 @@
         "target_posts",
         "auto_revoke",
         "show_posts",
-        "badge_type_id"
+        "badge_type_id",
+        "image_upload_id"
       ]
     }
   },

spec/requests/api/schemas/json/badge_list_response.json

@@ -83,6 +83,12 @@
                 "null"
               ]
             },
+            "image_upload_id": {
+              "type": [
+                "integer",
+                "null"
+              ]
+            },
             "badge_type_id": {
               "type": "integer"
             }
@@ -108,7 +114,8 @@
             "target_posts",
             "auto_revoke",
             "show_posts",
-            "badge_type_id"
+            "badge_type_id",
+            "image_upload_id"
           ]
         }
     },

spec/requests/api/schemas/json/badge_update_response.json

@@ -56,6 +56,12 @@
             "null"
           ]
         },
+        "image_upload_id": {
+          "type": [
+            "integer",
+            "null"
+          ]
+        },
         "listable": {
           "type": "boolean"
         },
@@ -123,7 +129,8 @@
         "target_posts",
         "auto_revoke",
         "show_posts",
-        "badge_type_id"
+        "badge_type_id",
+        "image_upload_id"
       ]
     }
   },

spec/system/admin_badges_spec.rb

@@ -6,7 +6,6 @@ describe "Admin Badges Page", type: :system do
   fab!(:current_user) { Fabricate(:admin) }
 
   let(:badges_page) { PageObjects::Pages::AdminBadges.new }
-  let(:form) { PageObjects::Components::FormKit.new("form") }
 
   before { sign_in(current_user) }
 
@@ -15,6 +14,7 @@ describe "Admin Badges Page", type: :system do
       badges_page.visit_page(Badge::Autobiographer)
 
       badge = Badge.find(Badge::Autobiographer)
+      form = badges_page.form
 
       expect(form.field("enabled")).to be_enabled
       expect(form.field("badge_type_id")).to be_disabled
@@ -31,48 +31,66 @@ describe "Admin Badges Page", type: :system do
       expect(form.field("show_posts")).to be_unchecked
       expect(form.field("icon")).to be_enabled
       expect(form.field("icon")).to have_value("user-edit")
-      expect(find(".form-kit__container[data-name='name']")).to have_content(badge.name.strip)
-      expect(find(".form-kit__container[data-name='description']")).to have_content(
-        badge.description.strip,
-      )
-      expect(find(".form-kit__container[data-name='long_description']")).to have_content(
-        badge.long_description.strip,
-      )
+      expect(form.container("name")).to have_content(badge.name.strip)
+      expect(form.container("description")).to have_content(badge.description.strip)
+      expect(form.container("long_description")).to have_content(badge.long_description.strip)
     end
   end
 
   context "when creating a badge" do
     it "creates a badge" do
       badges_page.new_page
+      badges_page.form.field("enabled").accept
+      badges_page.form.field("name").fill_in("a name")
+      badges_page.form.field("badge_type_id").select(BadgeType::Bronze)
+      badges_page.form.field("icon").select("ambulance")
+      badges_page.form.field("description").fill_in("a description")
+      badges_page.form.field("long_description").fill_in("a long_description")
+      badges_page.form.field("badge_grouping_id").select(BadgeGrouping::GettingStarted)
+      badges_page.form.field("allow_title").toggle
+      badges_page.form.field("multiple_grant").toggle
+      badges_page.form.field("listable").toggle
+      badges_page.form.field("show_posts").toggle
+      badges_page.submit_form
 
-      form.field("enabled").accept
-      form.field("name").fill_in("a name")
-      form.field("badge_type_id").select(BadgeType::Bronze)
-      form.field("icon").select("ambulance")
-      form.field("description").fill_in("a description")
-      form.field("long_description").fill_in("a long_description")
-      form.field("badge_grouping_id").select(BadgeGrouping::GettingStarted)
-      form.field("allow_title").toggle
-      form.field("multiple_grant").toggle
-      form.field("listable").toggle
-      form.field("show_posts").toggle
-      form.submit
-
-      expect(PageObjects::Components::Toasts.new).to have_success(I18n.t("js.saved"))
+      expect(badges_page).to have_saved_form
       expect(badges_page).to have_badge("a name")
     end
   end
 
+  context "when updating a badge" do
+    it "can upload an image for the badge" do
+      badges_page.visit_page(Badge::Autobiographer).upload_image("logo.jpg").submit_form
+
+      expect(badges_page).to have_saved_form
+
+      badge = Badge.find(Badge::Autobiographer)
+      try_until_success do
+        expect(badge.image_upload_id).to be_present
+        expect(badge.icon).to be_blank
+      end
+    end
+
+    it "can change to an icon for the badge" do
+      badge = Badge.find(Badge::Autobiographer)
+      badge.update!(image_upload_id: Fabricate(:image_upload).id)
+
+      badges_page.visit_page(Badge::Autobiographer).choose_icon("ambulance").submit_form
+
+      expect(badges_page).to have_saved_form
+      expect(badge.reload.image_upload_id).to be_blank
+      expect(badge.icon).to eq("ambulance")
+    end
+  end
+
   context "with enable_badge_sql" do
     before { SiteSetting.enable_badge_sql = true }
 
     it "shows the sql section" do
-      badges_page.new_page
+      badges_page.new_page.fill_query("a query")
 
-      form.field("query").fill_in("a query")
-
-      expect(form.field("auto_revoke")).to be_unchecked
-      expect(form.field("target_posts")).to be_unchecked
+      expect(badges_page.form.field("auto_revoke")).to be_unchecked
+      expect(badges_page.form.field("target_posts")).to be_unchecked
     end
   end
 end

spec/system/page_objects/admin_badges.rb

@@ -18,6 +18,41 @@ module PageObjects
       def has_badge?(title)
         page.has_css?(".current-badge-header .badge-display-name", text: title)
       end
+
+      def has_saved_form?
+        expect(PageObjects::Components::Toasts.new).to have_success(I18n.t("js.saved"))
+      end
+
+      def submit_form
+        form.submit
+      end
+
+      def choose_icon(name)
+        form.choose_conditional("choose-icon")
+        form.field("icon").select("ambulance")
+        self
+      end
+
+      def fill_query(query)
+        form.field("query").fill_in(query)
+        self
+      end
+
+      def upload_image(name)
+        form.choose_conditional("upload-image")
+
+        attach_file(File.absolute_path(file_from_fixtures(name))) do
+          form.field("image_url").find(".image-upload-controls .btn").click
+        end
+
+        expect(form.field("image_url")).to have_css(".btn-danger")
+
+        self
+      end
+
+      def form
+        @form ||= PageObjects::Components::FormKit.new("form")
+      end
     end
   end
 end

spec/system/page_objects/components/form_kit.rb

@@ -2,6 +2,22 @@
 
 module PageObjects
   module Components
+    class FormKitContainer < PageObjects::Components::Base
+      attr_reader :component
+
+      def initialize(input)
+        if input.is_a?(Capybara::Node::Element)
+          @component = input
+        else
+          @component = find(input)
+        end
+      end
+
+      def has_content?(content)
+        component.has_content?(content)
+      end
+    end
+
     class FormKitField < PageObjects::Components::Base
       attr_reader :component
 
@@ -166,6 +182,16 @@ module PageObjects
           FormKitField.new(find(".form-kit__field[data-name='#{name}']"))
         end
       end
+
+      def container(name)
+        within component do
+          FormKitContainer.new(find(".form-kit__container[data-name='#{name}']"))
+        end
+      end
+
+      def choose_conditional(name)
+        find(".form-kit__conditional-display .form-kit__control-radio[value='#{name}']").click
+      end
     end
   end
 end