Add after authenticate job option #432

alanhill · 2017-07-12T01:22:47Z

What

Adds the ability to generate an after authenticate job which can perform async or inline once the shop has authenticated with the app.

Branch renamed from: #428 to better reflect the changes.

@Hammadk @kevinhughes27 ready for review again - made the config a hash accepting job and inline.

kevinhughes27 · 2017-07-12T14:07:05Z

lib/generators/shopify_app/add_after_authenticate_job/add_after_authenticate_job_generator.rb

+
+      def init_after_authenticate_config
+        initializer = load_initializer
+        return if initializer.include?("config.webhooks")


why do you need to return if they have have webhooks configured?

🤦‍♂️ typo

kevinhughes27 · 2017-07-12T14:07:34Z

lib/generators/shopify_app/add_after_authenticate_job/add_after_authenticate_job_generator.rb

+
+        inject_into_file(
+          'config/initializers/shopify_app.rb',
+          "  config.after_authenticate_job = { job: Shopify::AfterAuthenticateJob, inline: false }\n",


I would generate this without the inline arg since most people should use it as a job

oh I see - its not really a default argument the way I was thinking of it. Maybe it is better to have here since otherwise it would only live in documentation. What are your thoughts @Hammadk

I agree that we should keep inline: false around for documentation purposes

Hammadk · 2017-07-12T16:30:39Z

lib/shopify_app/sessions_concern.rb

+    def perform_after_authenticate_job
+      return unless ShopifyApp.configuration.after_authenticate_job
+
+      job = ShopifyApp.configuration.after_authenticate_job[:job]


Is it possible for ShopifyApp.configuration.after_authenticate_job to be truthy, without a job set in ShopifyApp.configuration.after_authenticate_job[:job] ?

Hammadk

LGTM other than the minor comment

alanhill · 2017-07-12T19:11:21Z

Is it possible for ShopifyApp.configuration.after_authenticate_job to be truthy, without a job set in ShopifyApp.configuration.after_authenticate_job[:job]

Yes, I switched it up to check for the presence of the config in general, then [:job] and [:inline] otherwise it would fail on a nil class.

Also removed my typo and added the error handler like I originally intended.

Ready for another quick round of 👀

Hammadk · 2017-07-13T17:43:53Z

lib/shopify_app/sessions_concern.rb

+
+      return unless config && config[:job].present? && config[:inline].present?
+
+      if config[:inline]


We should add tests for this conditional.

I was pondering on this for a bit. There doesn't seem to be anything testing the authentication flow of the app - installing script tags and webhooks got lucky because they are contained in their own class instead of a job firing from within the app itself and not the gem, therefore you're able to unit test vs. integration test.

I think the only way to properly tests this is to have an integration test which goes through the entire authentication flow, unless there's a simpler way that I'm missing here?

If that's the case I could attempt to address it in another PR before merging this and can put this on hold for a bit until I can get to it.

Open to other suggestions - is there a way I can test this without essentially writing an entire integration test for the auth flow?

Have a look at the SessionControllerTests. One way to do this is by testing the #callback method.

💡 ahhh that's what I was missing. TY.

alanhill · 2017-07-18T00:44:36Z

Nice call on the test 😉 totally missed that one.

Should be good for review again!

Todo before :

Add to README
Add to Changelog
Bump version (minor I assume?)

kevinhughes27 · 2017-07-18T15:24:18Z

.byebug_history

+config[:inline].present?
+config[:job].present?
+config
+next


🤦‍♂️

Hammadk · 2017-07-18T17:35:21Z

lib/shopify_app/sessions_concern.rb

+
+      return unless config && config[:job].present? && config[:inline].in?([true, false])
+
+      if config[:inline]


Do we have a default value for config[:inline] ? If not, we won't get to this statement because most people won't set config[:inline]. We should change this to:

return unless config && config[:job].present? if config[:inline] == true ...

By default we add inline: false when you generate the job, but I see your point. So should we just consider anything but true to mean you want to perform asynchronously ?

Yeah, config[:inline] == true is good because it's self documenting code. If someone thinking about running an inline job, reading this code would tell them that they need to set that value to true.

Hammadk · 2017-07-18T17:37:11Z

test/controllers/sessions_controller_test.rb

+
+        mock_shopify_omniauth
+        get :callback, params: { shop: 'shop' }
+      end


Indentation is off here.

Hammadk

Great work. This will make the Shopify app gem much more useful. Looking forward to the updates.

alanhill · 2017-07-18T20:43:30Z

This should be ready for review - comments are fixed up, README and Changelog added, version bumped a minor version.

kevinhughes27 · 2017-07-18T20:58:34Z

README.md

@@ -23,6 +23,7 @@ Table of Contents
 * [Home Controller Generator](#home-controller-generator)
 * [App Proxy Controller Generator](#app-proxy-controller-generator)
 * [Controllers, Routes and Views](#controllers-routes-and-views)
+ * [After Authenticate Job and Generator](#after-authenticate-job-an-generator)


I would probably drop Generator from the section heading

Also I think this should go after the Webhooks and ScriptTags manager sections

Missed this one, updated!

kevinhughes27 · 2017-07-18T21:03:35Z

README.md

+
+You can generate an after authenticate job by running `shopify_app:add_after_authenticate_job` which will add a line in `shopify_app.rb` giving the option
+to run a job of your choosing either inline or asynchronously after the shop has authenticated. By default the job will run asynchronously unless `inline: false`
+is specified.


I would write this more like this:

If your app needs to perform specific actions after it is installed 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:

ShopifyApp.configure do |config| config.add_after_authenticate_job = { job: Shopify::AfterAuthenticateJob } end

If you need the job to run synchronously add the inline flag:

ShopifyApp.configure do |config| config.add_after_authenticate_job = { job: Shopify::AfterAuthenticateJob, inline: true } end

We've also provided a generator which creates a skeleton job and updates the initializer for you:

bin/rails g shopify_app:add_after_authenticate_job

👍 Thanks for that, I'm awful with words.

alanhill · 2017-07-18T21:30:16Z

README is updated with gooder ( 😉 ) words!

kevinhughes27 · 2017-07-19T14:19:03Z

CHANGELOG.md

@@ -1,3 +1,7 @@
+7.3.1


I'd make this 7.4 since we added a feature

👍 bumped.

kevinhughes27 · 2017-07-19T15:53:53Z

🚢

kevinhughes27 reviewed Jul 12, 2017

View reviewed changes

Hammadk reviewed Jul 12, 2017

View reviewed changes

Hammadk approved these changes Jul 12, 2017

View reviewed changes

alanhill force-pushed the add_after_authenticate_job_option branch 2 times, most recently from 30e37a3 to 5b60e70 Compare July 12, 2017 17:46

alanhill force-pushed the add_after_authenticate_job_option branch from 5b60e70 to 79bbb27 Compare July 12, 2017 19:12

Hammadk reviewed Jul 13, 2017

View reviewed changes

alanhill force-pushed the add_after_authenticate_job_option branch from 44e3d7e to 6b6152e Compare July 18, 2017 00:43

kevinhughes27 approved these changes Jul 18, 2017

View reviewed changes

Hammadk reviewed Jul 18, 2017

View reviewed changes

Hammadk requested changes Jul 18, 2017

View reviewed changes

alanhill force-pushed the add_after_authenticate_job_option branch 3 times, most recently from 7cf036f to debcd11 Compare July 18, 2017 20:40

kevinhughes27 reviewed Jul 18, 2017

View reviewed changes

Hammadk approved these changes Jul 18, 2017

View reviewed changes

alanhill force-pushed the add_after_authenticate_job_option branch from debcd11 to 6f1197f Compare July 18, 2017 21:29

alanhill force-pushed the add_after_authenticate_job_option branch from 6f1197f to 4da222e Compare July 18, 2017 21:33

kevinhughes27 approved these changes Jul 19, 2017

View reviewed changes

Add after_install hook and generator

3939188

alanhill force-pushed the add_after_authenticate_job_option branch from 4da222e to 3939188 Compare July 19, 2017 15:50

alanhill merged commit 1bb51d2 into master Jul 19, 2017

alanhill deleted the add_after_authenticate_job_option branch July 19, 2017 16:59

alanhill deployed to rubygems August 10, 2017 14:15 Active

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add after authenticate job option #432

Add after authenticate job option #432

alanhill commented Jul 12, 2017

kevinhughes27 Jul 12, 2017

alanhill Jul 12, 2017

kevinhughes27 Jul 12, 2017

kevinhughes27 Jul 12, 2017

Hammadk Jul 12, 2017 •

edited

Loading

Hammadk Jul 12, 2017 •

edited

Loading

Hammadk left a comment

alanhill commented Jul 12, 2017

Hammadk Jul 13, 2017

alanhill Jul 13, 2017 •

edited

Loading

Hammadk Jul 13, 2017

alanhill Jul 13, 2017

alanhill commented Jul 18, 2017 •

edited

Loading

kevinhughes27 Jul 18, 2017

alanhill Jul 18, 2017

Hammadk Jul 18, 2017 •

edited

Loading

alanhill Jul 18, 2017

Hammadk Jul 18, 2017

Hammadk Jul 18, 2017

Hammadk left a comment

alanhill commented Jul 18, 2017

kevinhughes27 Jul 18, 2017

kevinhughes27 Jul 18, 2017

alanhill Jul 18, 2017

kevinhughes27 Jul 18, 2017

alanhill Jul 18, 2017

alanhill commented Jul 18, 2017

kevinhughes27 Jul 19, 2017

alanhill Jul 19, 2017

kevinhughes27 commented Jul 19, 2017


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

		if config[:inline]

Add after authenticate job option #432

Add after authenticate job option #432

Conversation

alanhill commented Jul 12, 2017

What

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

Hammadk Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Hammadk Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

Hammadk left a comment

Choose a reason for hiding this comment

alanhill commented Jul 12, 2017

Choose a reason for hiding this comment

alanhill Jul 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanhill commented Jul 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hammadk Jul 18, 2017 • edited Loading

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

Hammadk left a comment

Choose a reason for hiding this comment

alanhill commented Jul 18, 2017

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

alanhill commented Jul 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinhughes27 commented Jul 19, 2017

Hammadk Jul 12, 2017 •

edited

Loading

Hammadk Jul 12, 2017 •

edited

Loading

alanhill Jul 13, 2017 •

edited

Loading

alanhill commented Jul 18, 2017 •

edited

Loading

Hammadk Jul 18, 2017 •

edited

Loading