-
Notifications
You must be signed in to change notification settings - Fork 683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add GDPR and Uninstall Jobs to Generator #1597
Changes from 5 commits
af0b2a9
c772229
04a9be8
aa750d6
0da864d
62fb6c3
2458306
f8196b5
683b9f4
1a293c3
67de851
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
# frozen_string_literal: true | ||
|
||
require "rails/generators/base" | ||
|
||
module ShopifyApp | ||
module Generators | ||
class AddAppUninstalledJobGenerator < Rails::Generators::Base | ||
source_root File.expand_path("../templates", __FILE__) | ||
|
||
def create_job | ||
template("app_uninstalled_job.rb", "app/jobs/app_uninstalled_job.rb") | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
class AppUninstalledJob < ActiveJob::Base | ||
extend ShopifyAPI::Webhooks::Handler | ||
|
||
class << self | ||
def handle(topic:, shop:, body:) | ||
perform_later(topic: topic, shop_domain: shop, webhook: body) | ||
end | ||
end | ||
|
||
def perform(topic:, shop_domain:, webhook:) | ||
shop = Shop.find_by(shopify_domain: shop_domain) | ||
|
||
if shop.nil? | ||
logger.error("#{self.class} failed: cannot find shop with domain '#{shop_domain}'") | ||
return | ||
end | ||
|
||
logger.info("#{self.class} started for shop '#{shop_domain}'") | ||
shop.destroy | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
# frozen_string_literal: true | ||
|
||
require "rails/generators/base" | ||
|
||
module ShopifyApp | ||
module Generators | ||
class AddGdprJobsGenerator < Rails::Generators::Base | ||
source_root File.expand_path("../templates", __FILE__) | ||
|
||
def add_customer_data_request_job | ||
template("customer_data_request_job.rb", "app/jobs/customer_data_request_job.rb") | ||
end | ||
|
||
def add_shop_redact_job | ||
template("shop_redact_job.rb", "app/jobs/shop_redact_job.rb") | ||
end | ||
|
||
def add_customer_redact_job | ||
template("customer_redact_job.rb", "app/jobs/customer_redact_job.rb") | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
class CustomersDataRequestJob < ActiveJob::Base | ||
extend ShopifyAPI::Webhooks::Handler | ||
|
||
class << self | ||
def handle(topic:, shop:, body:) | ||
perform_later(topic: topic, shop_domain: shop, webhook: body) | ||
end | ||
end | ||
|
||
def perform(topic:, shop_domain:, webhook:) | ||
shop = Shop.find_by(shopify_domain: shop_domain) | ||
|
||
if shop.nil? | ||
logger.error("#{self.class} failed: cannot find shop with domain '#{shop_domain}'") | ||
return | ||
end | ||
|
||
shop.with_shopify_session do | ||
logger.info("#{self.class} started for shop '#{shop_domain}'") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Im not sure if anyone has any context on why this log message is here but not in the others? (In case you missed it in the description this was ripped right out of the ruby template) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My guess is oversight. Can we use our new shopify app logger for all of these? We should be logging all of these and allowing users to turn log levels down if they are too loud. |
||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
class CustomersRedactJob < ActiveJob::Base | ||
extend ShopifyAPI::Webhooks::Handler | ||
|
||
class << self | ||
def handle(topic:, shop:, body:) | ||
perform_later(topic: topic, shop_domain: shop, webhook: body) | ||
end | ||
end | ||
|
||
def perform(topic:, shop_domain:, webhook:) | ||
shop = Shop.find_by(shopify_domain: shop_domain) | ||
|
||
if shop.nil? | ||
logger.error("#{self.class} failed: cannot find shop with domain '#{shop_domain}'") | ||
return | ||
end | ||
|
||
shop.with_shopify_session do | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
class ShopRedactJob < ActiveJob::Base | ||
extend ShopifyAPI::Webhooks::Handler | ||
|
||
class << self | ||
def handle(topic:, shop:, body:) | ||
perform_later(topic: topic, shop_domain: shop, webhook: body) | ||
end | ||
end | ||
|
||
def perform(topic:, shop_domain:, webhook:) | ||
shop = Shop.find_by(shopify_domain: shop_domain) | ||
|
||
if shop.nil? | ||
logger.error("#{self.class} failed: cannot find shop with domain '#{shop_domain}'") | ||
return | ||
end | ||
|
||
shop.with_shopify_session do | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# frozen_string_literal: true | ||
|
||
require "test_helper" | ||
require "generators/shopify_app/add_app_uninstalled_job/add_app_uninstalled_job_generator" | ||
|
||
class AddAppUninstalledJobGeneratorTest < Rails::Generators::TestCase | ||
tests ShopifyApp::Generators::AddAppUninstalledJobGenerator | ||
destination File.expand_path("../tmp", File.dirname(__FILE__)) | ||
|
||
setup do | ||
ShopifyApp.configure do |config| | ||
config.embedded_app = true | ||
end | ||
|
||
prepare_destination | ||
provide_existing_application_file | ||
provide_existing_routes_file | ||
provide_existing_application_controller | ||
end | ||
|
||
test "creates app uninstalled job file" do | ||
run_generator | ||
|
||
assert_file "app/jobs/app_uninstalled_job.rb" | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
# frozen_string_literal: true | ||
|
||
require "test_helper" | ||
require "generators/shopify_app/add_gdpr_jobs/add_gdpr_jobs_generator" | ||
|
||
class AddGdprJobsGeneratorJobTest < Rails::Generators::TestCase | ||
tests ShopifyApp::Generators::AddGdprJobsGenerator | ||
destination File.expand_path("../tmp", File.dirname(__FILE__)) | ||
|
||
setup do | ||
ShopifyApp.configure do |config| | ||
config.embedded_app = true | ||
end | ||
|
||
prepare_destination | ||
provide_existing_application_file | ||
provide_existing_routes_file | ||
provide_existing_application_controller | ||
end | ||
|
||
test "creates app uninstalled job file" do | ||
run_generator | ||
|
||
klenotiw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
assert_file "app/jobs/customer_data_request_job.rb" | ||
assert_file "app/jobs/shop_redact_job.rb" | ||
assert_file "app/jobs/customer_redact_job.rb" | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should be raising an error and failing jobs if we can't find their shop domain 🤔
Sidekiq and Active Job have cool tooling for jobs that fail. If devs aren't watching logs, they might be missing out on key information that jobs are failing.
Raising an error will be available with exception handing software (bugsnag, airbake, rollbar, etc) as well as job monitoring dashboards like sidekiq's.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pattern seems to be in other jobs as well. I'd recommend logging in addition to failing the job. Failing a job means the job raised an exception during its run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize you are probably boosting these from templates, but this is a good opportunity for improvement 😄 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I just added a raise
StandardError
I thought I remember you saying to raiseNotFound
error? I couldn't find it anywhere maybe you said a different error, my bad for forgetting.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://api.rubyonrails.org/classes/ActiveRecord/RecordNotFound.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, you could use find_by! instead of find_by whiches raise an active record not found error automagically by the power of rails. We'd lose the logging with that approach though