Add GDPR and Uninstall Jobs to Generator #1597
Conversation
klenotiw
changed the title
klenotiw
changed the title
| end | ||
|
|
||
| shop.with_shopify_session do | ||
| logger.info("#{self.class} started for shop '#{shop_domain}'") |
There was a problem hiding this comment.
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.
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.
| shop = Shop.find_by(shopify_domain: shop_domain) | ||
|
|
||
| if shop.nil? | ||
| logger.error("#{self.class} failed: cannot find shop with domain '#{shop_domain}'") |
There was a problem hiding this comment.
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.
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.
I realize you are probably boosting these from templates, but this is a good opportunity for improvement 😄 😬
There was a problem hiding this comment.
Ok I just added a raise StandardError I thought I remember you saying to raise NotFound error? I couldn't find it anywhere maybe you said a different error, my bad for forgetting.
There was a problem hiding this comment.
There was a problem hiding this comment.
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
| end | ||
|
|
||
| shop.with_shopify_session do | ||
| logger.info("#{self.class} started for shop '#{shop_domain}'") |
There was a problem hiding this comment.
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.
|
Thanks for adding these generators! Super helpful to setup users for success as they go to approval and see what is needed! 🎉 💪 |
There was a problem hiding this comment.
🎉 Style wise, I'd prefer we use the RecordNotFoundError but this is great functionality you've added here!
|
🎩 'd your branch, everything appears to be working well! |
Shipping now, had to fix the webhook names or it wasn't find it on |
What this PR does
Out of the box our
shopify_appgenerator doesn't handle uninstall properly. It also doesn't provide any scaffolding for GDPR jobs. So this PR adds an uninstall webhook and creates a template for the three GDPR jobs we are expecting in the app review process.Since this adds an uninstall webhook an app should pass our automated review process by simply running the
rails generate shopify_appcommand.Reviewer's guide to testing
Please test this out yourself, and run it against our automated tests. You can also load up a new app and see if the database is removing the shop information on uninstall.
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