From b30efb57c16b4f408c8f38c045dd4d42690b0e22 Mon Sep 17 00:00:00 2001 From: Jacob Steves Date: Tue, 9 Mar 2021 16:48:27 -0500 Subject: [PATCH] send script configuration ui definition during push (#1095) --- lib/project_types/script/commands/push.rb | 2 +- lib/project_types/script/errors.rb | 17 ++++ .../app_script_update_or_create.graphql | 2 + .../script/layers/application/build_script.rb | 8 +- .../script/layers/application/push_script.rb | 12 +-- .../script/layers/domain/push_package.rb | 6 +- .../infrastructure/push_package_repository.rb | 29 +++---- .../layers/infrastructure/script_service.rb | 4 +- lib/project_types/script/messages/messages.rb | 6 ++ lib/project_types/script/script_project.rb | 24 +++++- lib/project_types/script/ui/error_handler.rb | 12 +++ .../layers/application/build_script_test.rb | 9 +- .../layers/application/push_script_test.rb | 9 +- .../script/layers/domain/push_package_test.rb | 2 + .../fake_push_package_repository.rb | 21 +++-- .../infrastructure/script_service_test.rb | 5 ++ .../script/script_project_test.rb | 82 +++++++++++++++++-- .../test_helpers/fake_script_project.rb | 1 + .../script/ui/error_handler_test.rb | 16 +++- 19 files changed, 201 insertions(+), 66 deletions(-) diff --git a/lib/project_types/script/commands/push.rb b/lib/project_types/script/commands/push.rb index e048a78dd5..c229e3193f 100644 --- a/lib/project_types/script/commands/push.rb +++ b/lib/project_types/script/commands/push.rb @@ -16,7 +16,7 @@ def call(_args, _name) Layers::Application::PushScript.call(ctx: @ctx, force: options.flags.key?(:force)) @ctx.puts(@ctx.message("script.push.script_pushed", api_key: api_key)) rescue StandardError => e - msg = @ctx.message("script.push.error.operation_failed", api_key: ScriptProject.current.api_key) + msg = @ctx.message("script.push.error.operation_failed", api_key: ShopifyCli::Project.current.env.api_key) UI::ErrorHandler.pretty_print_and_raise(e, failed_op: msg) end diff --git a/lib/project_types/script/errors.rb b/lib/project_types/script/errors.rb index 831f9eef30..d92acfff42 100644 --- a/lib/project_types/script/errors.rb +++ b/lib/project_types/script/errors.rb @@ -4,6 +4,23 @@ module Script module Errors class InvalidContextError < ScriptProjectError; end class InvalidScriptNameError < ScriptProjectError; end + + class InvalidConfigUiDefinitionError < ScriptProjectError + attr_reader :filename + def initialize(filename) + super() + @filename = filename + end + end + + class MissingSpecifiedConfigUiDefinitionError < ScriptProjectError + attr_reader :filename + def initialize(filename) + super() + @filename = filename + end + end + class NoExistingAppsError < ScriptProjectError; end class NoExistingOrganizationsError < ScriptProjectError; end diff --git a/lib/project_types/script/graphql/app_script_update_or_create.graphql b/lib/project_types/script/graphql/app_script_update_or_create.graphql index e29e03311c..699cd41519 100644 --- a/lib/project_types/script/graphql/app_script_update_or_create.graphql +++ b/lib/project_types/script/graphql/app_script_update_or_create.graphql @@ -2,6 +2,7 @@ mutation AppScriptUpdateOrCreate( $extensionPointName: ExtensionPointName!, $title: String, $description: String, + $configUi: String, $sourceCode: String, $language: String, $force: Boolean, @@ -13,6 +14,7 @@ mutation AppScriptUpdateOrCreate( extensionPointName: $extensionPointName title: $title description: $description + configUi: $configUi sourceCode: $sourceCode language: $language force: $force diff --git a/lib/project_types/script/layers/application/build_script.rb b/lib/project_types/script/layers/application/build_script.rb index 963e0f9c7d..15b07f0f3e 100644 --- a/lib/project_types/script/layers/application/build_script.rb +++ b/lib/project_types/script/layers/application/build_script.rb @@ -5,17 +5,15 @@ module Layers module Application class BuildScript class << self - def call(ctx:, task_runner:, script_name:, description:, extension_point_type:) + def call(ctx:, task_runner:, script_project:) CLI::UI::Frame.open(ctx.message("script.application.building")) do begin UI::StrictSpinner.spin(ctx.message("script.application.building_script")) do |spinner| Infrastructure::PushPackageRepository.new(ctx: ctx).create_push_package( - extension_point_type: extension_point_type, - script_name: script_name, - description: description, + script_project: script_project, script_content: task_runner.build, compiled_type: task_runner.compiled_type, - metadata: task_runner.metadata + metadata: task_runner.metadata, ) spinner.update_title(ctx.message("script.application.built")) end diff --git a/lib/project_types/script/layers/application/push_script.rb b/lib/project_types/script/layers/application/push_script.rb index a0dc86298a..ab2a649af2 100644 --- a/lib/project_types/script/layers/application/push_script.rb +++ b/lib/project_types/script/layers/application/push_script.rb @@ -9,19 +9,11 @@ def call(ctx:, force:) script_project = ScriptProject.current task_runner = Infrastructure::TaskRunner.for(ctx, script_project.language, script_project.script_name) ProjectDependencies.install(ctx: ctx, task_runner: task_runner) - BuildScript.call( - ctx: ctx, - task_runner: task_runner, - extension_point_type: script_project.extension_point_type, - script_name: script_project.script_name, - description: script_project.description - ) + BuildScript.call(ctx: ctx, task_runner: task_runner, script_project: script_project) UI::PrintingSpinner.spin(ctx, ctx.message("script.application.pushing")) do |p_ctx, spinner| package = Infrastructure::PushPackageRepository.new(ctx: p_ctx).get_push_package( - extension_point_type: script_project.extension_point_type, - script_name: script_project.script_name, - description: script_project.description, + script_project: script_project, compiled_type: task_runner.compiled_type, metadata: task_runner.metadata ) diff --git a/lib/project_types/script/layers/domain/push_package.rb b/lib/project_types/script/layers/domain/push_package.rb index 725d72c3ca..5a11e2ca29 100644 --- a/lib/project_types/script/layers/domain/push_package.rb +++ b/lib/project_types/script/layers/domain/push_package.rb @@ -8,6 +8,7 @@ class PushPackage :extension_point_type, :script_name, :description, + :config_ui, :script_content, :compiled_type, :metadata @@ -19,7 +20,8 @@ def initialize( description:, script_content:, compiled_type:, - metadata: + metadata:, + config_ui: ) @id = id @extension_point_type = extension_point_type @@ -28,6 +30,7 @@ def initialize( @script_content = script_content @compiled_type = compiled_type @metadata = metadata + @config_ui = config_ui end def push(script_service, api_key, force) @@ -40,6 +43,7 @@ def push(script_service, api_key, force) api_key: api_key, force: force, metadata: @metadata, + config_ui: @config_ui, ) end end diff --git a/lib/project_types/script/layers/infrastructure/push_package_repository.rb b/lib/project_types/script/layers/infrastructure/push_package_repository.rb index 8bce255cee..4c32986d3c 100644 --- a/lib/project_types/script/layers/infrastructure/push_package_repository.rb +++ b/lib/project_types/script/layers/infrastructure/push_package_repository.rb @@ -7,42 +7,37 @@ class PushPackageRepository include SmartProperties property! :ctx, accepts: ShopifyCli::Context - def create_push_package( - extension_point_type:, - script_name:, - description:, - script_content:, - compiled_type:, - metadata: - ) - build_file_path = file_path(script_name, compiled_type) + def create_push_package(script_project:, script_content:, compiled_type:, metadata:) + build_file_path = file_path(script_project.script_name, compiled_type) write_to_path(build_file_path, script_content) Domain::PushPackage.new( id: build_file_path, - extension_point_type: extension_point_type, - script_name: script_name, - description: description, + extension_point_type: script_project.extension_point_type, + script_name: script_project.script_name, + description: script_project.description, script_content: script_content, compiled_type: compiled_type, metadata: metadata, + config_ui: script_project.config_ui, ) end - def get_push_package(extension_point_type:, script_name:, description:, compiled_type:, metadata:) - build_file_path = file_path(script_name, compiled_type) + def get_push_package(script_project:, compiled_type:, metadata:) + build_file_path = file_path(script_project.script_name, compiled_type) raise Domain::PushPackageNotFoundError unless ctx.file_exist?(build_file_path) script_content = File.read(build_file_path) Domain::PushPackage.new( id: build_file_path, - extension_point_type: extension_point_type, - script_name: script_name, - description: description, + extension_point_type: script_project.extension_point_type, + script_name: script_project.script_name, + description: script_project.description, script_content: script_content, compiled_type: compiled_type, metadata: metadata, + config_ui: script_project.config_ui, ) end diff --git a/lib/project_types/script/layers/infrastructure/script_service.rb b/lib/project_types/script/layers/infrastructure/script_service.rb index 9c2662b4f4..fd59f67a5c 100644 --- a/lib/project_types/script/layers/infrastructure/script_service.rb +++ b/lib/project_types/script/layers/infrastructure/script_service.rb @@ -18,13 +18,15 @@ def push( description: nil, api_key: nil, force: false, - metadata: + metadata:, + config_ui: ) query_name = "app_script_update_or_create" variables = { extensionPointName: extension_point_type.upcase, title: script_name, description: description, + configUi: config_ui, sourceCode: Base64.encode64(script_content), language: compiled_type, force: force, diff --git a/lib/project_types/script/messages/messages.rb b/lib/project_types/script/messages/messages.rb index 272e7e9afb..b20bc64d62 100644 --- a/lib/project_types/script/messages/messages.rb +++ b/lib/project_types/script/messages/messages.rb @@ -51,6 +51,12 @@ module Messages invalid_config: "Can't change the configuration values because %1$s is missing or "\ "it is not formatted properly.", + invalid_config_ui_definition_cause: "The UI configuration file %s contains invalid YAML.", + invalid_config_ui_definition_help: "Fix the errors and try again.", + + missing_config_ui_definition_cause: "You are missing the UI configuration file %s.", + missing_config_ui_definition_help: "Create this file and try again.", + script_not_found_cause: "Couldn't find script %s for extension point %s", service_failure_cause: "Internal service error.", diff --git a/lib/project_types/script/script_project.rb b/lib/project_types/script/script_project.rb index c348e561a7..6068277de7 100644 --- a/lib/project_types/script/script_project.rb +++ b/lib/project_types/script/script_project.rb @@ -2,7 +2,7 @@ module Script class ScriptProject < ShopifyCli::Project - attr_reader :extension_point_type, :script_name, :language, :description + attr_reader :extension_point_type, :script_name, :language, :description, :config_ui def initialize(*args) super @@ -10,6 +10,7 @@ def initialize(*args) raise Errors::DeprecatedEPError, @extension_point_type if deprecated?(@extension_point_type) @script_name = lookup_config!("script_name") @description = lookup_config("description") + @config_ui = lookup_config_ui @language = lookup_language ShopifyCli::Core::Monorail.metadata = { "script_name" => @script_name, @@ -38,6 +39,19 @@ def lookup_config!(key) config[key] end + def lookup_config_ui + filename = lookup_config("config_ui_file") + return nil unless filename + + path = File.join(directory, filename) + raise Errors::MissingSpecifiedConfigUiDefinitionError, filename unless File.exist?(path) + + contents = File.read(path) + raise Errors::InvalidConfigUiDefinitionError, filename unless valid_config_ui?(contents) + + contents + end + def lookup_language lang = lookup_config("language")&.downcase || Layers::Domain::ExtensionPointAssemblyScriptSDK.language if Layers::Application::ExtensionPoints.supported_language?(type: extension_point_type, language: lang) @@ -47,6 +61,14 @@ def lookup_language end end + def valid_config_ui?(raw_yaml) + require "yaml" # takes 20ms, so deferred as late as possible. + YAML.safe_load(raw_yaml) + true + rescue Psych::SyntaxError + false + end + class << self def create(ctx, dir) raise Errors::ScriptProjectAlreadyExistsError, dir if ctx.dir_exist?(dir) diff --git a/lib/project_types/script/ui/error_handler.rb b/lib/project_types/script/ui/error_handler.rb index 6b8f531263..56e634756e 100644 --- a/lib/project_types/script/ui/error_handler.rb +++ b/lib/project_types/script/ui/error_handler.rb @@ -61,6 +61,18 @@ def self.error_messages(e) Script::Layers::Application::ExtensionPoints.languages(type: e.extension_point_type).join(", ") ), } + when Errors::InvalidConfigUiDefinitionError + { + cause_of_error: ShopifyCli::Context + .message("script.error.invalid_config_ui_definition_cause", e.filename), + help_suggestion: ShopifyCli::Context.message("script.error.invalid_config_ui_definition_help"), + } + when Errors::MissingSpecifiedConfigUiDefinitionError + { + cause_of_error: ShopifyCli::Context + .message("script.error.missing_config_ui_definition_cause", e.filename), + help_suggestion: ShopifyCli::Context.message("script.error.missing_config_ui_definition_help"), + } when Errors::InvalidScriptNameError { cause_of_error: ShopifyCli::Context.message("script.error.invalid_script_name_cause"), diff --git a/test/project_types/script/layers/application/build_script_test.rb b/test/project_types/script/layers/application/build_script_test.rb index cc12a924e3..0de76c41fd 100644 --- a/test/project_types/script/layers/application/build_script_test.rb +++ b/test/project_types/script/layers/application/build_script_test.rb @@ -14,14 +14,13 @@ let(:compiled_type) { "wasm" } let(:metadata) { Script::Layers::Domain::Metadata.new("1", "0", false) } let(:task_runner) { stub(compiled_type: compiled_type, metadata: metadata) } + let(:script_project) { stub } subject do Script::Layers::Application::BuildScript.call( ctx: @context, task_runner: task_runner, - script_name: script_name, - extension_point_type: extension_point_type, - description: description + script_project: script_project ) end @@ -30,9 +29,7 @@ CLI::UI::Frame.expects(:with_frame_color_override).never task_runner.expects(:build).returns(content) Script::Layers::Infrastructure::PushPackageRepository.any_instance.expects(:create_push_package).with( - extension_point_type: extension_point_type, - script_name: script_name, - description: description, + script_project: script_project, script_content: content, compiled_type: "wasm", metadata: metadata diff --git a/test/project_types/script/layers/application/push_script_test.rb b/test/project_types/script/layers/application/push_script_test.rb index 3962ec5519..143126d872 100644 --- a/test/project_types/script/layers/application/push_script_test.rb +++ b/test/project_types/script/layers/application/push_script_test.rb @@ -23,6 +23,7 @@ extension_point_type: extension_point_type, script_name: script_name, description: description, + config_ui: "", env: { api_key: api_key } ) end @@ -41,9 +42,7 @@ Script::ScriptProject.stubs(:current).returns(project) extension_point_repository.create_extension_point(extension_point_type) push_package_repository.create_push_package( - extension_point_type: extension_point_type, - script_name: script_name, - description: description, + script_project: project, script_content: "content", compiled_type: compiled_type, metadata: metadata @@ -60,9 +59,7 @@ Script::Layers::Application::BuildScript.expects(:call).with( ctx: @context, task_runner: task_runner, - extension_point_type: extension_point_type, - script_name: script_name, - description: description + script_project: project ) Script::Layers::Infrastructure::ScriptService .expects(:new).returns(script_service_instance) diff --git a/test/project_types/script/layers/domain/push_package_test.rb b/test/project_types/script/layers/domain/push_package_test.rb index d302362cad..f878ba397d 100644 --- a/test/project_types/script/layers/domain/push_package_test.rb +++ b/test/project_types/script/layers/domain/push_package_test.rb @@ -7,6 +7,7 @@ let(:script_id) { "id" } let(:script_name) { "foo_script" } let(:description) { "my description" } + let(:config_ui) { "---\nversion: 1\n" } let(:api_key) { "fake_key" } let(:force) { false } let(:script_content) { "(module)" } @@ -18,6 +19,7 @@ extension_point_type: extension_point_type, script_name: script_name, description: description, + config_ui: config_ui, script_content: script_content, compiled_type: compiled_type, metadata: metadata diff --git a/test/project_types/script/layers/infrastructure/fake_push_package_repository.rb b/test/project_types/script/layers/infrastructure/fake_push_package_repository.rb index a69348d783..c15076ca41 100644 --- a/test/project_types/script/layers/infrastructure/fake_push_package_repository.rb +++ b/test/project_types/script/layers/infrastructure/fake_push_package_repository.rb @@ -9,28 +9,27 @@ def initialize end def create_push_package( - extension_point_type:, - script_name:, - description:, + script_project:, script_content:, compiled_type:, metadata: ) - id = id(script_name, compiled_type) + id = id(script_project.script_name, compiled_type) @cache[id] = Domain::PushPackage.new( id: id, - extension_point_type: extension_point_type, - script_name: script_name, - description: description, + extension_point_type: script_project.extension_point_type, + script_name: script_project.script_name, + description: script_project.description, script_content: script_content, compiled_type: compiled_type, - metadata: metadata + metadata: metadata, + config_ui: script_project.config_ui, ) end - def get_push_package(extension_point_type:, script_name:, description:, compiled_type:, metadata:) - _ = extension_point_type, description, metadata - id = id(script_name, compiled_type) + def get_push_package(script_project:, compiled_type:, metadata:) + _ = metadata + id = id(script_project.script_name, compiled_type) if @cache.key?(id) @cache[id] else diff --git a/test/project_types/script/layers/infrastructure/script_service_test.rb b/test/project_types/script/layers/infrastructure/script_service_test.rb index 93cab6485a..ed200e38b6 100644 --- a/test/project_types/script/layers/infrastructure/script_service_test.rb +++ b/test/project_types/script/layers/infrastructure/script_service_test.rb @@ -12,6 +12,7 @@ let(:schema_major_version) { "1" } let(:schema_minor_version) { "0" } let(:use_msgpack) { true } + let(:config_ui) { "---\nversion:1\n" } let(:script_service_proxy) do <<~HERE query ProxyRequest($api_key: String, $shop_domain: String, $query: String!, $variables: String) { @@ -36,6 +37,7 @@ $extensionPointName: ExtensionPointName!, $title: String, $description: String, + $configUi: String, $sourceCode: String, $language: String, $schemaMajorVersion: String, @@ -46,6 +48,7 @@ extensionPointName: $extensionPointName title: $title description: $description + configUi: $configUi sourceCode: $sourceCode language: $language schemaMajorVersion: $schemaMajorVersion @@ -79,6 +82,7 @@ extensionPointName: extension_point_type, title: script_name, description: description, + configUi: config_ui, sourceCode: Base64.encode64(script_content), language: "AssemblyScript", force: false, @@ -104,6 +108,7 @@ script_content: script_content, compiled_type: "AssemblyScript", description: description, + config_ui: config_ui, api_key: api_key, ) end diff --git a/test/project_types/script/script_project_test.rb b/test/project_types/script/script_project_test.rb index ce28d70bba..1a0b886a5c 100644 --- a/test/project_types/script/script_project_test.rb +++ b/test/project_types/script/script_project_test.rb @@ -6,7 +6,7 @@ module Script class ScriptProjectTest < MiniTest::Test def setup super - @context = TestHelpers::FakeContext.new + @context = TestHelpers::FakeContext.new(root: Dir.mktmpdir) @script_name = "name" @extension_point_type = "ep_type" end @@ -22,7 +22,7 @@ def test_initialize }) Script::Layers::Application::ExtensionPoints.expects(:supported_language?).returns(true) - ScriptProject.new(directory: "testdir") + ScriptProject.new(directory: @context.root) assert_equal({ "script_name" => @script_name, @@ -44,7 +44,7 @@ def test_initialize_with_deprecated_ep Script::Layers::Application::ExtensionPoints.stubs(:deprecated_types).returns([@extension_point_type]) assert_raises Errors::DeprecatedEPError do - ScriptProject.new(directory: "testdir") + ScriptProject.new(directory: @context.root) end end @@ -127,7 +127,7 @@ def test_reads_language_from_config_file "language" => language, }) - script = ScriptProject.new(directory: "testdir") + script = ScriptProject.new(directory: @context.root) assert_equal language.downcase, script.language end @@ -142,7 +142,7 @@ def test_fallsback_to_assemblyscript_if_config_doesnt_specify_language "script_name" => @script_name, }) - script = ScriptProject.new(directory: "testdir") + script = ScriptProject.new(directory: @context.root) assert_equal "assemblyscript", script.language end @@ -160,7 +160,77 @@ def test_unsupported_language_in_config_will_raise }) assert_raises(Script::Errors::InvalidLanguageError) do - ScriptProject.new(directory: "testdir") + ScriptProject.new(directory: @context.root) + end + end + + describe "#config_ui" do + let(:config_ui_file) { "ui-config.yml" } + let(:language) { "assemblyscript" } + + before do + @context.root = Dir.mktmpdir + Dir.stubs(:pwd).returns(@context.root) + Script::Layers::Application::ExtensionPoints.stubs(:deprecated_types).returns([]) + Script::Layers::Application::ExtensionPoints.stubs(:languages).returns(%(assemblyscript rust)) + end + + subject do + Script::ScriptProject.current.config_ui + end + + describe "when ui_configuration_file field exists in config" do + before do + Script::ScriptProject.write( + @context, + project_type: :script, + organization_id: nil, + extension_point_type: @extension_point_type, + script_name: @script_name, + language: language, + config_ui_file: config_ui_file + ) + end + + describe "when file exists" do + describe "when YAML is valid" do + let(:config_ui) { "---\nversion: 1" } + + it "returns the raw contents" do + @context.write(config_ui_file, config_ui) + assert_equal config_ui, subject + end + end + + describe "when YAML is invalid" do + let(:config_ui) { "*" } + + it "raises InvalidConfigUiDefinitionError" do + @context.write(config_ui_file, config_ui) + assert_raises(Script::Errors::InvalidConfigUiDefinitionError) { subject } + end + end + end + + describe "when file does not exist" do + it "raises MissingSpecifiedConfigUiDefinitionError" do + assert_raises(Script::Errors::MissingSpecifiedConfigUiDefinitionError) { subject } + end + end + end + + describe "when ui_configuration_file field does not exist in config" do + it "returns nil" do + Script::ScriptProject.write( + @context, + project_type: :script, + organization_id: nil, + extension_point_type: @extension_point_type, + script_name: @script_name, + language: language, + ) + assert_nil subject + end end end end diff --git a/test/project_types/script/test_helpers/fake_script_project.rb b/test/project_types/script/test_helpers/fake_script_project.rb index 86635ecbed..9d1dd3f234 100644 --- a/test/project_types/script/test_helpers/fake_script_project.rb +++ b/test/project_types/script/test_helpers/fake_script_project.rb @@ -4,6 +4,7 @@ class FakeScriptProject < FakeProject property :script_name property :language property :description + property :config_ui property :env def api_key diff --git a/test/project_types/script/ui/error_handler_test.rb b/test/project_types/script/ui/error_handler_test.rb index a506503a83..7bcffdec68 100644 --- a/test/project_types/script/ui/error_handler_test.rb +++ b/test/project_types/script/ui/error_handler_test.rb @@ -147,7 +147,21 @@ def should_call_display_and_raise end describe "when InvalidLanguageError" do - let(:err) { Script::Layers::Domain::Errors::InvalidExtensionPointError.new("ruby") } + let(:err) { Script::Errors::InvalidLanguageError.new("ruby", "discount") } + it "should call display_and_raise" do + should_call_display_and_raise + end + end + + describe "when InvalidConfigUiDefinitionError" do + let(:err) { Script::Errors::InvalidConfigUiDefinitionError.new("filename") } + it "should call display_and_raise" do + should_call_display_and_raise + end + end + + describe "when MissingSpecifiedConfigUiDefinitionError" do + let(:err) { Script::Errors::MissingSpecifiedConfigUiDefinitionError.new("filename") } it "should call display_and_raise" do should_call_display_and_raise end