[minor] Scrip tags manager #1948
Conversation
lizkenyon
force-pushed
the
liz/add-scriptags-manager
branch
2 times, most recently
from
a9aefb3 to
f09e915
Compare
Add check for app blocks Add docs Update naming rubo cop rubo cop
lizkenyon
force-pushed
the
liz/add-scriptags-manager
branch
from
7ef47f6 to
7aafea2
Compare
| @@ -15,7 +15,7 @@ class Configuration | |||
| attr_accessor :embedded_app | |||
| alias_method :embedded_app?, :embedded_app | |||
| attr_accessor :webhooks | |||
| attr_accessor :scripttags | |||
| attr_accessor :script_tags | |||
| if template_types_to_check.all? do |template_type| | ||
| template_supports_app_blocks?(active_theme["id"], template_type) | ||
| end |
There was a problem hiding this comment.
can we split this into a variable?
theme_support_app_blocks = template_types_to_check.all? do |template_type|
template_supports_app_blocks?(active_theme["id"], template_type)
endwe also have this method theme_supports_app_blocks?, can we use it here?
| begin | ||
| response = client.query(query: query) | ||
|
|
||
| # Check for errors in the response | ||
| if response.body["errors"].present? | ||
| error_message = response.body["errors"].map { |e| e["message"] }.join(", ") | ||
| raise "GraphQL error: #{error_message}" | ||
| end | ||
|
|
||
| # Check if the response has the expected structure | ||
| unless response.body["data"] && response.body["data"]["themes"] && response.body["data"]["themes"]["nodes"] | ||
| raise "Invalid response structure" | ||
| end | ||
|
|
||
| themes = response.body["data"]["themes"]["nodes"] | ||
| return nil if themes.empty? | ||
|
|
||
| themes.first | ||
| rescue => e | ||
| ShopifyApp::Logger.warn("Failed to fetch active theme: #{e.message}") | ||
| nil | ||
| end | ||
| end |
There was a problem hiding this comment.
i don't think we need the begin..rescue..end block here, can we rescue on the method instead?
def fetch_active_theme
rescue
end
| begin | ||
| response = client.query(query: query) | ||
|
|
||
| # Check for errors in the response | ||
| if response.body["errors"].present? | ||
| error_message = response.body["errors"].map { |e| e["message"] }.join(", ") | ||
| raise "GraphQL error: #{error_message}" | ||
| end | ||
|
|
||
| # Check if the response has the expected structure | ||
| unless response.body["data"] && response.body["data"]["themes"] && response.body["data"]["themes"]["nodes"] | ||
| raise "Invalid response structure" | ||
| end | ||
|
|
||
| themes = response.body["data"]["themes"]["nodes"] | ||
| return nil if themes.empty? | ||
|
|
||
| themes.first | ||
| rescue => e | ||
| ShopifyApp::Logger.warn("Failed to fetch active theme: #{e.message}") | ||
| nil | ||
| end | ||
| end |
There was a problem hiding this comment.
can we remove the nested begin...rescue...end and use the def...rescue...end pattern?
def fetch_active_theme
rescue => e
end| unless response.body["data"] && response.body["data"]["themes"] && response.body["data"]["themes"]["nodes"] | ||
| raise "Invalid response structure" | ||
| end |
There was a problem hiding this comment.
do we need this check? It should be guaranteed by the graphql query we send right?
There was a problem hiding this comment.
i think we should handle userErrors response in addition to the previous errors response
| end | ||
| end | ||
|
|
||
| def template_supports_app_blocks?(theme_id, template_type) |
There was a problem hiding this comment.
can we break down this method to smaller ones and handle exceptions in each method instead of multiple calls here?
| if response.body["data"]["scriptTagCreate"]["userErrors"].any? | ||
| errors = response.body["data"]["scriptTagCreate"]["userErrors"] | ||
| error_messages = errors.map { |e| "#{e["field"]}: #{e["message"]}" }.join(", ") | ||
| raise ::ShopifyApp::CreationFailed, "ScriptTag creation failed: #{error_messages}" |
There was a problem hiding this comment.
some of the exception we raise above are not wrapped around a class like this one. Should we wrap them in error classes like this so it's more explicit?
| ShopifyApp::Logger.warn("GraphQL error fetching script tags: #{response.body["errors"].map do |e| | ||
| e["message"] | ||
| end.join(", ")}") |
There was a problem hiding this comment.
this formatting is kinda strange 🤔
| return [] unless response.body["data"] && | ||
| response.body["data"]["scriptTags"] && | ||
| response.body["data"]["scriptTags"]["edges"] |
There was a problem hiding this comment.
shouldn't the structure be guaranteed here as well?
There was a problem hiding this comment.
also we should handle userErrors response
| @@ -13,7 +13,7 @@ def self.call | |||
| config.myshopify_domain = "myshopify.com" | |||
| config.api_version = ShopifyAPI::LATEST_SUPPORTED_ADMIN_VERSION | |||
| config.billing = nil | |||
| config.scripttags = nil | |||
| config.script_tags = nil | |||
There was a problem hiding this comment.
wondering if this is gonna make this incompatible with previous versions 🤔
What this PR does
This PR adds the a script tag manager to the
shopify_appgem.This functionality was removed in version 22 of this package.
Script tags are still a valid way for modify the content of vintage themes.
This PR has several differences between the previous script tag manager
scripttag_managertoscript_tag_managerscript_tag-720.mov
Reviewer's guide to testing
Things to focus on
Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.mdif the changes would impact usersREADME.md, if appropriate./docs, if necessary