From 77081de02711e204b16b591b09a10f3905d82bc3 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 1 Aug 2024 23:36:17 +1000 Subject: [PATCH] FIX: Badge image uploader (#28188) In the formkit conversion in 2ca06ba2360c200b2be8e0718fcc04c64ca14935 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: Joffrey JAFFEUX --- .../addon/controllers/admin-badges/show.js | 1 + .../addon/templates/admin-badges/show.hbs | 6 +- .../form-kit/components/fk/control/image.gjs | 6 +- app/serializers/admin_badge_serializer.rb | 8 ++- .../schemas/json/badge_create_response.json | 9 ++- .../api/schemas/json/badge_list_response.json | 9 ++- .../schemas/json/badge_update_response.json | 9 ++- spec/system/admin_badges_spec.rb | 72 ++++++++++++------- spec/system/page_objects/admin_badges.rb | 35 +++++++++ .../page_objects/components/form_kit.rb | 26 +++++++ 10 files changed, 146 insertions(+), 35 deletions(-) diff --git a/app/assets/javascripts/admin/addon/controllers/admin-badges/show.js b/app/assets/javascripts/admin/addon/controllers/admin-badges/show.js index 78af7fb4f97..098f8defa87 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin-badges/show.js +++ b/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", diff --git a/app/assets/javascripts/admin/addon/templates/admin-badges/show.hbs b/app/assets/javascripts/admin/addon/templates/admin-badges/show.hbs index 9fa3017bd79..fa1847ffb64 100644 --- a/app/assets/javascripts/admin/addon/templates/admin-badges/show.hbs +++ b/app/assets/javascripts/admin/addon/templates/admin-badges/show.hbs @@ -65,7 +65,7 @@ @@ -90,14 +90,14 @@ - + diff --git a/app/assets/javascripts/discourse/app/form-kit/components/fk/control/image.gjs b/app/assets/javascripts/discourse/app/form-kit/components/fk/control/image.gjs index b200002b358..94344604783 100644 --- a/app/assets/javascripts/discourse/app/form-kit/components/fk/control/image.gjs +++ b/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 { diff --git a/app/serializers/admin_badge_serializer.rb b/app/serializers/admin_badge_serializer.rb index fc75706c594..f848efd5ea6 100644 --- a/app/serializers/admin_badge_serializer.rb +++ b/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 diff --git a/spec/requests/api/schemas/json/badge_create_response.json b/spec/requests/api/schemas/json/badge_create_response.json index 55fa58b411f..57408db5f25 100644 --- a/spec/requests/api/schemas/json/badge_create_response.json +++ b/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" ] } }, diff --git a/spec/requests/api/schemas/json/badge_list_response.json b/spec/requests/api/schemas/json/badge_list_response.json index 9c0fa2b16bb..3229e08c641 100644 --- a/spec/requests/api/schemas/json/badge_list_response.json +++ b/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" ] } }, diff --git a/spec/requests/api/schemas/json/badge_update_response.json b/spec/requests/api/schemas/json/badge_update_response.json index 55fa58b411f..57408db5f25 100644 --- a/spec/requests/api/schemas/json/badge_update_response.json +++ b/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" ] } }, diff --git a/spec/system/admin_badges_spec.rb b/spec/system/admin_badges_spec.rb index 91039998715..45a50187926 100644 --- a/spec/system/admin_badges_spec.rb +++ b/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 diff --git a/spec/system/page_objects/admin_badges.rb b/spec/system/page_objects/admin_badges.rb index bc2e96ab144..dc7462ab1ed 100644 --- a/spec/system/page_objects/admin_badges.rb +++ b/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 diff --git a/spec/system/page_objects/components/form_kit.rb b/spec/system/page_objects/components/form_kit.rb index c7d5ee4570d..b4d226a460b 100644 --- a/spec/system/page_objects/components/form_kit.rb +++ b/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