Extract post authenticate tasks from CallbackController #1819

zzooeeyy · 2024-03-26T15:19:41Z

What this PR does

https://github.com/Shopify/develop-app-access/issues/235
Extract common logic to run post authenticate task from CallbackController so that it can be used after the token exchange flow.
Added PostAuthenticateTasks to run the authenticated task
This is configurable as config.custom_post_authenticate_tasks, if that's not set, the default config.post_authenticate_tasks will return PostAuthenticateTasks class
Updated docs on post authenticate tasks
Added deprecation warnings

Tophatting:

Deprecation warning in server start from configuration

Deprecation reminder in tests logs

When `config.custom_post_authenticate_tasks` is set, it will be used to handle post authenticate tasks

I also tested not setting the custom_post_authenticate_tasks to ensure the methods in CallbackController to perform post authenticate tasks are done.

Checklist

Before submitting the PR, please consider if any of the following are needed:

Update CHANGELOG.md if the changes would impact users
Update README.md, if appropriate.
Update any relevant pages in /docs, if necessary
For security fixes, the Disclosure Policy must be followed.

zzooeeyy · 2024-03-26T23:54:41Z

lib/shopify_app/auth/post_authenticate_tasks.rb

+        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


All of this is copied from CallbackController

shopify_app/app/controllers/shopify_app/callback_controller.rb

Lines 151 to 178 in 1f993c2

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

paulomarg

Had a few thoughts but most of them are just me trying to understand things better so take them with a grain of salt :)

paulomarg · 2024-03-27T13:47:54Z

app/controllers/shopify_app/callback_controller.rb

+      # TODO: Remove before releasing v23.0.0
+      # "perform_after_authenticate_job" and related methods (install_webhooks, perform_after_authenticate_job)
+      # will be deprecated in the next major release - 23.0.0
+      # Set `custom_post_authenticate_tasks`from configuration instead to handle special cases.
+      if ShopifyApp.configuration.custom_post_authenticate_tasks.present?
+        ShopifyApp.configuration.post_authenticate_tasks.perform(api_session)
+      else
+        perform_post_authenticate_jobs(api_session)
+      end
+      ########### When deprecating, replace the above block with:
+      # ShopifyApp.configuration.post_authenticate_tasks.perform(api_session)


Could we simplify this by adding a check for the version itself instead of adding the comments? We should still have a comment with the word deprecated in it because that makes it easier to search / replace when we're releasing a new major version.

paulomarg · 2024-03-27T14:03:30Z

lib/shopify_app/configuration.rb

@@ -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


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

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.

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.

paulomarg · 2024-03-27T14:06:27Z

lib/shopify_app/configuration.rb

+
+      # TODO: Remove this before releasing v23.0.0
+      message = <<~EOS
+        ================================================


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.

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. 🤔

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.

paulomarg · 2024-03-27T14:08:56Z

test/controllers/callback_controller_test.rb

@@ -174,6 +183,7 @@ class CallbackControllerTest < ActionController::TestCase
    end

    test "#callback doesn't run the WebhooksManager if no webhooks are configured" do
+      log_deprecation


Personal opinion, but I'm not sure I'd explicitly log these - just adding a comment saying they're deprecated (or better yet, adding actual deprecation logs) rather than puts-ing might be enough. The logger call would actually error out so it would create actual test failures when we get there, which is nice.

paulomarg · 2024-03-27T14:09:39Z

test/controllers/callback_controller_test.rb

@@ -333,6 +352,32 @@ class CallbackControllerTest < ActionController::TestCase
      assert_response 302
    end

+    test "#callback calls post_authenticate_tasks if custom_post_authenticate_tasks is set" do


Should we have a very simple test here to ensure that the default behaviour gets called (even if we have to stub it)?

paulomarg · 2024-03-27T14:11:28Z

test/shopify_app/auth/post_authenticate_tasks_test.rb

+    ShopifyApp.configure do |config|
+      config.webhooks = []
+    end
+    ShopifyApp::WebhooksManager.add_registrations


Nit: why do we have to call add_registrations here but not in the success tests? Makes me feel like there's a wrong check somewhere in the code :)

ooops that's my oversight, I copied straight from CallbackControllTest LOL I'll also remove it from that test case

https://github.com/Shopify/shopify_app/blob/main/test/controllers/callback_controller_test.rb#L180

paulomarg · 2024-03-27T14:13:04Z

test/shopify_app/configuration_test.rb

@@ -2,6 +2,18 @@

 require "test_helper"

+module Shopify
+  class CustomPostAuthenticateTasks


This is just a question for my own benefit: will any class that defines a perform method do here? Should we check if it extends a job class instead to standardize things a bit more?

I didn't put too much consideration there, because I didn't think the standardization is too necessary? This way it's more flexible, plus the configuration will raise a ConfigurationError if "perform" isn't a method.. What do you think?

I also don't think it matters too much, but I remember the documentation said it could "run this later", which I took to mean it would deal with a perform_later type of thing. But just for the sake of standardization, I don't think it's necessary.

…s is set in configuration

paulomarg · 2024-03-27T20:04:01Z

test/shopify_app/configuration_test.rb

@@ -2,6 +2,18 @@

 require "test_helper"

+module Shopify
+  class CustomPostAuthenticateTasks


I also don't think it matters too much, but I remember the documentation said it could "run this later", which I took to mean it would deal with a perform_later type of thing. But just for the sake of standardization, I don't think it's necessary.

paulomarg · 2024-03-27T20:05:12Z

app/controllers/shopify_app/callback_controller.rb

@@ -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"


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

docs/Upgrading.md

paulomarg · 2024-03-27T20:09:42Z

lib/shopify_app/configuration.rb

@@ -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


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.

paulomarg · 2024-03-27T20:11:24Z

lib/shopify_app/configuration.rb

+
+      # TODO: Remove this before releasing v23.0.0
+      message = <<~EOS
+        ================================================


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.

paulomarg · 2024-03-27T20:12:06Z

test/controllers/callback_controller_test.rb

@@ -47,6 +54,7 @@ class CallbackControllerTest < ActionController::TestCase
        "AppleWebKit/537.36 (KHTML, like Gecko)"\
        "Chrome/69.0.3497.100 Safari/537.36"
      ShopifyApp::SessionRepository.stubs(:store_session)
+      ShopifyApp.configuration.custom_post_authenticate_tasks = nil


Should this be the global test config value, so that other tests start with this as an assumption?

Not necessary I only did it to "reset" the configuration state, but I can move that to the teardown so it doesn't become and assumption for other tests!

Co-authored-by: Paulo Margarido <[email protected]>

paulomarg

Just a general question, but if this is the way, LGTM

paulomarg · 2024-04-02T15:18:42Z

docs/shopify_app/authentication.md

@@ -69,11 +63,39 @@ Rails.application.routes.draw do
 end
 ```

-### Run jobs after the OAuth flow


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

Good catch!

paulomarg · 2024-04-02T15:20:07Z

docs/shopify_app/authentication.md

+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.


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?

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?

zzooeeyy added the #gsd:35360 label Mar 26, 2024

zzooeeyy force-pushed the post-auth-tasks branch 11 times, most recently from 27a1a91 to 02d14ba Compare March 26, 2024 22:14

zzooeeyy commented Mar 26, 2024

View reviewed changes

zzooeeyy marked this pull request as ready for review March 26, 2024 23:58

zzooeeyy requested a review from a team as a code owner March 26, 2024 23:58

paulomarg reviewed Mar 27, 2024

View reviewed changes

zzooeeyy added 9 commits March 27, 2024 12:45

Extract post authenticate tasks into a class to be reused

31f7477

Extract post_authenticate_task class into config

81b7690

Use configuration.post_authenticate_tasks

ae6e171

Copy more test from callback_controller_test

e73611c

Only call new post_authenticate_task if custom_post_authenticate_task…

7b5237e

…s is set in configuration

Update docs

5ee1b12

Log method deprecation during configuration

d48ced0

Update changelog

67954b1

Update doc example

5e86247

zzooeeyy force-pushed the post-auth-tasks branch from 1f993c2 to 5e86247 Compare March 27, 2024 18:46

zzooeeyy added 4 commits March 27, 2024 12:52

Add missed 'end' from rebase

8c9ddfb

remove unnecssary test setup

74715ff

Replace comment with version check

bbd3450

Replace test log with comments

a70b0a5

zzooeeyy added 4 commits March 27, 2024 13:20

Add extra test case

93b355a

Add missed 'end' from rebase

c1468e9

Using correct version check

43311b2

Add upgrade note for unreleased version

da83fd8

zzooeeyy force-pushed the post-auth-tasks branch from 4c053fe to da83fd8 Compare March 27, 2024 19:50

Only log deprecation in development env

6d0499e

zzooeeyy requested a review from paulomarg March 27, 2024 19:56

paulomarg reviewed Mar 27, 2024

View reviewed changes

zzooeeyy and others added 3 commits March 27, 2024 16:14

Use relative link

597a87b

Co-authored-by: Paulo Margarido <[email protected]>

Use relative links

f21fe74

Move config reset to test teardown instead

33d8be4

zzooeeyy requested a review from paulomarg March 28, 2024 14:38

paulomarg approved these changes Apr 2, 2024

View reviewed changes

Fix markdown link

77de512

zzooeeyy merged commit 41bb033 into main Apr 2, 2024
7 checks passed

zzooeeyy deleted the post-auth-tasks branch April 2, 2024 16:36

shopify-shipit bot temporarily deployed to rubygems April 9, 2024 14:45 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract post authenticate tasks from CallbackController #1819

Extract post authenticate tasks from CallbackController #1819

zzooeeyy commented Mar 26, 2024 •

edited

Loading

zzooeeyy Mar 26, 2024

paulomarg left a comment

paulomarg Mar 27, 2024

zzooeeyy Mar 27, 2024

paulomarg Mar 27, 2024

zzooeeyy Mar 27, 2024

paulomarg Mar 27, 2024

paulomarg Mar 27, 2024

zzooeeyy Mar 27, 2024

paulomarg Mar 27, 2024

paulomarg Mar 27, 2024

paulomarg Mar 27, 2024

paulomarg Mar 27, 2024

zzooeeyy Mar 27, 2024

paulomarg Mar 27, 2024

zzooeeyy Mar 27, 2024

paulomarg Mar 27, 2024

paulomarg Mar 27, 2024

paulomarg Mar 27, 2024

paulomarg Mar 27, 2024

paulomarg Mar 27, 2024

paulomarg Mar 27, 2024

zzooeeyy Mar 27, 2024

paulomarg left a comment

paulomarg Apr 2, 2024

zzooeeyy Apr 2, 2024

paulomarg Apr 2, 2024

zzooeeyy Apr 2, 2024

	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

Extract post authenticate tasks from CallbackController #1819

Extract post authenticate tasks from CallbackController #1819

Conversation

zzooeeyy commented Mar 26, 2024 • edited Loading

What this PR does

Tophatting:

Deprecation warning in server start from configuration

Deprecation reminder in tests logs

When config.custom_post_authenticate_tasks is set, it will be used to handle post authenticate tasks

Checklist

Choose a reason for hiding this comment

paulomarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulomarg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zzooeeyy commented Mar 26, 2024 •

edited

Loading

When `config.custom_post_authenticate_tasks` is set, it will be used to handle post authenticate tasks