Skip to content
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

Extract post authenticate tasks from CallbackController #1819

Merged
merged 22 commits into from
Apr 2, 2024
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
Unreleased
----------
* Extracted class - `PostAuthenticateTasks` to handle post authenticate tasks. To learn more, see [post authenticate tasks](https://github.com/Shopify/shopify_app/blob/main/docs/shopify_app/authentication.md#post-authenticate-tasks). [1819](https://github.com/Shopify/shopify_app/pull/1819)

22.0.1 (March 12, 2024)
----------
Expand Down
11 changes: 10 additions & 1 deletion app/controllers/shopify_app/callback_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,16 @@ def callback

return respond_with_user_token_flow if start_user_token_flow?(api_session)

perform_post_authenticate_jobs(api_session)
if ShopifyApp::VERSION < "23.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice that we can just compare it this way, makes this very easy to understand :)

# deprecated in 23.0
if ShopifyApp.configuration.custom_post_authenticate_tasks.present?
ShopifyApp.configuration.post_authenticate_tasks.perform(api_session)
else
perform_post_authenticate_jobs(api_session)
end
else
ShopifyApp.configuration.post_authenticate_tasks.perform(api_session)
end
redirect_to_app if check_billing(api_session)
end

Expand Down
11 changes: 11 additions & 0 deletions docs/Upgrading.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ We also recommend the use of a staging site which matches your production enviro

If you do run into issues, we recommend looking at our [debugging tips.](https://github.com/Shopify/shopify_app/blob/main/docs/Troubleshooting.md#debugging-tips)

## Unreleased
#### (v23.0.0) - Deprecated methods in CallbackController
The following methods from `ShopifyApp::CallbackController` have been deprecated in `v23.0.0`
- `perform_after_authenticate_job`
- `install_webhooks`
- `perform_post_authenticate_jobs`

If you have overwritten these methods in your callback controller to modify the behavior of the inherited `CallbackController`, you will need to
update your app to use configurable option `config.custom_post_authenticate_tasks` instead. See [post authenticate tasks](https://github.com/Shopify/shopify_app/blob/main/docs/shopify_app/authentication.md#post-authenticate-tasks)
zzooeeyy marked this conversation as resolved.
Show resolved Hide resolved
for more information.

## Upgrading to `v22.0.0`
#### Dropped support for Ruby 2.x
Support for Ruby 2.x has been dropped as it is no longer supported. You'll need to upgrade to 3.x.x
Expand Down
50 changes: 36 additions & 14 deletions docs/shopify_app/authentication.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,27 +26,21 @@ The default callback controller [`ShopifyApp::CallbackController`](../../app/con

1. Logging into the shop and resetting the session
2. Storing the session to the `SessionRepository`
3. [Installing Webhooks](/docs/shopify_app/webhooks.md)
4. [Setting Scripttags](/docs/shopify_app/script-tags.md)
5. [Run jobs after the OAuth flow](#run-jobs-after-the-oauth-flow)
6. Redirecting to the return address

3. [Post authenticate tasks](#post-authenticate-tasks)
4. Redirecting to the return address

#### Customizing callback controller
If the app needs to do some extra work, it can define and configure the route to a custom callback controller.
Inheriting from `ShopifyApp::CallbackController` and hook into or override any of the defined helper methods.
If you need to define a custom callback controller to handle your app's use case, you can configure the callback route to your controller.

Example:

1. Create the new custom callback controller
```ruby
# web/app/controllers/my_custom_callback_controller.rb

class MyCustomCallbackController < ShopifyApp::CallbackController
private

def install_webhooks(session)
# My custom override/definition to install webhooks
class MyCustomCallbackController
def callback
# My custom callback logic
end
end
```
Expand All @@ -69,11 +63,39 @@ Rails.application.routes.draw do
end
```

### Run jobs after the OAuth flow
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this breaks the ToC at the top of this file :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

### Post Authenticate tasks
After authentication is complete, a few tasks are run by default by PostAuthenticateTasks:
1. [Installing Webhooks](/docs/shopify_app/webhooks.md)
2. [Run configured after_authenticate_job](#after_authenticate_job)

The [PostAuthenticateTasks](https://github.com/Shopify/shopify_app/blob/main/lib/shopify_app/auth/post_authenticate_tasks.rb)
class is responsible for triggering the webhooks manager for webhooks registration, and enqueue jobs from [after_authenticate_job](#after_authenticate_job).

If you simply need to enqueue more jobs to run after authenticate, use [after_authenticate_job](#after_authenticate_job) to define these jobs.

If your post authentication tasks is more complex and is different than just installing webhooks and enqueuing jobs,
you can customize the post authenticate tasks by creating a new class that has a `self.perform(session)` method,
and configuring `custom_post_authenticate_tasks` in the initializer.
Comment on lines +76 to +78
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a bit late in the game, but isn't this essentially the same as enqueueing a special job that does these tasks?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is very similar, but the after_authenticate_job only accepts a shop domain as the parameter, in which case it might not be enough for task customization?? If devs want to use the user session for whatever post auth task for their app, they wouldn't be able to since the task would only know the shop domain?? And since that's also setup in the configuration file, we'll need to somehow magically add this new special job to existing apps, and ensure there isn't conflict if they have already declared jobs?


```ruby
# my_custom_post_authenticate_task.rb
class MyCustomPostAuthenticateTask
def self.perform(session)
# This will be triggered after OAuth callback and token exchange completion
end
end

# config/initializers/shopify_app.rb
ShopifyApp.configure do |config|
config.custom_post_authenticate_tasks = "MyCustomPostAuthenticateTask"
end
```

#### after_authenticate_job

See [`ShopifyApp::AfterAuthenticateJob`](/lib/generators/shopify_app/add_after_authenticate_job/templates/after_authenticate_job.rb).

If your app needs to perform specific actions after the user is authenticated successfully (i.e. every time a new session is created), ShopifyApp can queue or run a job of your choosing (note that we already provide support for automatically creating Webhooks and Scripttags). To configure the after authenticate job, update your initializer as follows:
If your app needs to perform specific actions after the user is authenticated successfully (i.e. every time a new session is created), ShopifyApp can queue or run a job of your choosing. To configure the after authenticate job, update your initializer as follows:

```ruby
ShopifyApp.configure do |config|
Expand Down
2 changes: 1 addition & 1 deletion docs/shopify_app/webhooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ ShopifyApp.configure do |config|
end
```

When the [OAuth callback](/docs/shopify_app/authentication.md#oauth-callback) is completed successfully, ShopifyApp will queue a background job which will ensure all the specified webhooks exist for that shop. Because this runs on every OAuth callback, it means your app will always have the webhooks it needs even if the user uninstalls and re-installs the app.
When the [OAuth callback](/docs/shopify_app/authentication.md#oauth-callback) or token exchange is completed successfully, ShopifyApp will queue a background job which will ensure all the specified webhooks exist for that shop. Because this runs on every OAuth callback, it means your app will always have the webhooks it needs even if the user uninstalls and re-installs the app.

ShopifyApp also provides a [WebhooksController](/app/controllers/shopify_app/webhooks_controller.rb) that receives webhooks and queues a job based on the received topic. For example, if you register the webhook from above, then all you need to do is create a job called `CartsUpdateJob`. The job will be queued with 2 params: `shop_domain` and `webhook` (which is the webhook body).

Expand Down
3 changes: 3 additions & 0 deletions lib/shopify_app.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ def self.use_webpacker?
require "shopify_app/controller_concerns/webhook_verification"
require "shopify_app/controller_concerns/token_exchange"

# Auth helpers
require "shopify_app/auth/post_authenticate_tasks"

# jobs
require "shopify_app/jobs/webhooks_manager_job"

Expand Down
48 changes: 48 additions & 0 deletions lib/shopify_app/auth/post_authenticate_tasks.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
# frozen_string_literal: true

module ShopifyApp
module Auth
class PostAuthenticateTasks
class << self
def perform(session)
ShopifyApp::Logger.debug("Performing post authenticate tasks")
# Ensure we use the shop session to install webhooks
session_for_shop = session.online? ? shop_session(session) : session

install_webhooks(session_for_shop)

perform_after_authenticate_job(session)
end

private

def shop_session(session)
ShopifyApp::SessionRepository.retrieve_shop_session_by_shopify_domain(session.shop)
end

def install_webhooks(session)
ShopifyApp::Logger.debug("PostAuthenticateTasks: Installing webhooks")
return unless ShopifyApp.configuration.has_webhooks?

WebhooksManager.queue(session.shop, session.access_token)
end

def perform_after_authenticate_job(session)
ShopifyApp::Logger.debug("PostAuthenticateTasks: Performing after_authenticate_job")
config = ShopifyApp.configuration.after_authenticate_job

return unless config && config[:job].present?

job = config[:job]
job = job.constantize if job.is_a?(String)

if config[:inline] == true
job.perform_now(shop_domain: session.shop)
else
job.perform_later(shop_domain: session.shop)
end
Comment on lines +7 to +43
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this is copied from CallbackController

def perform_post_authenticate_jobs(session)
# Ensure we use the shop session to install webhooks
session_for_shop = session.online? ? shop_session : session
install_webhooks(session_for_shop)
perform_after_authenticate_job(session)
end
def install_webhooks(session)
return unless ShopifyApp.configuration.has_webhooks?
WebhooksManager.queue(session.shop, session.access_token)
end
def perform_after_authenticate_job(session)
config = ShopifyApp.configuration.after_authenticate_job
return unless config && config[:job].present?
job = config[:job]
job = job.constantize if job.is_a?(String)
if config[:inline] == true
job.perform_now(shop_domain: session.shop)
else
job.perform_later(shop_domain: session.shop)
end

end
end
end
end
end
47 changes: 47 additions & 0 deletions lib/shopify_app/configuration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ class Configuration
attr_writer :login_callback_url
attr_accessor :embedded_redirect_url

# customize post authenticate tasks
attr_accessor :custom_post_authenticate_tasks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: would it make more sense to have a post_authenticate_tasks setting that defaults to the new class rather than a custom setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yess that does make sense, but in this pre-deprecation case we don't want to use the post_authenticate_tasks with a default if it's not manually set since we still want to call the old methods. I think it does make sense to have a single config with a default once we roll out the deprecation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah, fair. It wouldn't be ideal to change the setting name in the future though - so I think we should stick with what we add in this PR, whichever way we prefer to do it.


# customise ActiveJob queue names
attr_accessor :scripttags_manager_queue_name
attr_accessor :webhooks_manager_queue_name
Expand All @@ -54,6 +57,8 @@ def initialize
@scripttags_manager_queue_name = Rails.application.config.active_job.queue_name
@webhooks_manager_queue_name = Rails.application.config.active_job.queue_name
@disable_webpacker = ENV["SHOPIFY_APP_DISABLE_WEBPACKER"].present?

log_callback_controller_method_deprecation
end

def login_url
Expand Down Expand Up @@ -130,6 +135,48 @@ def use_new_embedded_auth_strategy?
def online_token_configured?
!ShopifyApp.configuration.user_session_repository.blank? && ShopifyApp::SessionRepository.user_storage.present?
end

def post_authenticate_tasks
@post_authenticate_tasks || begin
if custom_post_authenticate_tasks
custom_class = if custom_post_authenticate_tasks.respond_to?(:safe_constantize)
custom_post_authenticate_tasks.safe_constantize
else
custom_post_authenticate_tasks
end
end

task_class = custom_class || ShopifyApp::Auth::PostAuthenticateTasks

[
:perform,
].each do |method|
raise(
::ShopifyApp::ConfigurationError,
"Missing method - '#{method}' for custom_post_authenticate_tasks",
) unless task_class.respond_to?(method)
end

task_class
end
end

private

def log_callback_controller_method_deprecation
return unless Rails.env.development?

# TODO: Remove this before releasing v23.0.0
message = <<~EOS
================================================
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this as a setup for logging general duplications - we might want to do that too when we go ahead with token exchange, so there might be an opportunity to make this a little bit more generic (like given an array of messages, log if it isn't empty).

Also, can we add some conditions to logging this? Is there any way we can detect that there is a controller that responds to the old methods we want to hide? We can probably build it back from the route path. It's more work but I think it would be worth it to not log every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree we should make it more generic, but I think that could be done when we know we'll have another need for it? It's hard to know what specs to build for until we know how the new messages might look? e.g. is it generic messages or is it specific to deprecations...

and also, I disagree on conditionally logging this by checking for controllers in route.. I don't think that's worth the effort since it'll be deprecated in next release anyways, plus this will only be logged when you first start the server, so it's not tooo noisy. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair. I think this comment is big enough that most folks would see it.

When I suggested we log this conditionally, I meant that ideally we wouldn't log it if there is no usage of the deprecated setup, i.e. if the app is configured in the "new" way, we just skip this to avoid confusion.

=> Upcoming deprecation in v23.0:
* 'CallbackController::perform_after_authenticate_job' and related methods 'install_webhooks', 'perform_after_authenticate_job'
* will be deprecated from CallbackController in the next major release. If you need to customize
* post authentication tasks, see https://github.com/Shopify/shopify_app/blob/main/docs/shopify_app/authentication.md#post-authenticate-tasks
================================================
EOS
puts message
end
end

class BillingConfiguration
Expand Down
Loading
Loading