Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Allow for multiple after auth jobs #106

Merged
merged 8 commits into from
Sep 24, 2018
Merged

Allow for multiple after auth jobs #106

merged 8 commits into from
Sep 24, 2018

Conversation

amosmos
Copy link
Contributor

@amosmos amosmos commented Sep 23, 2018

No description provided.

@tobiasdalhof
Copy link

tobiasdalhof commented Sep 23, 2018

It would be a breaking change if config('shopify-app.after_authenticate_job') only accepts an array now.

In my opinion this should be changed like this:

  • Check if config('shopify-app.after_authenticate_job')[0] is array or string
  • String => Run single job
  • Array => Run each job

@amosmos
Copy link
Contributor Author

amosmos commented Sep 23, 2018

@tobiasdalhof - I agree - that's a great idea. Will try to make it.

@amosmos
Copy link
Contributor Author

amosmos commented Sep 23, 2018

@tobiasdalhof - actually - even if the parameter is only 1 job and not an array, it is still a php array.

Any idea how can this be checked?

@tobiasdalhof
Copy link

tobiasdalhof commented Sep 23, 2018

Something like this comes to my mind

$afterAuthJobConfig = config('shopify-app.after_authenticate_job');

if (!empty($afterAuthJobConfig['job'])) {
    // Run job
    return;
}

foreach ($afterAuthJobConfig as $job) {
    // Run job
}

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0006%) to 99.643% when pulling 3e7d4f3 on amosmos:master into 6c569bf on ohmybrew:master.

@amosmos
Copy link
Contributor Author

amosmos commented Sep 23, 2018

@tobiasdalhof Great idea, I added a similar solution.

@amosmos
Copy link
Contributor Author

amosmos commented Sep 23, 2018

The failed travis test is is now not necessary as the jobs are now installed with a foreach loop, which will not run if there are no jobs to run.

@gnikyt
Copy link
Owner

gnikyt commented Sep 23, 2018

Thanks for this. Will review after my flights but from the phone looks all good :) the failed test, wifi here is bad, but it's probably the billing controller. There's a test in there which I haven't figured out how to solve, it fails only sometimes due to an order of operations thing, usually restart of the Travis test makes it pass. Either way I'll check it out!

@amosmos
Copy link
Contributor Author

amosmos commented Sep 24, 2018

Thanks! Happy to help with this great project!

This is the failed test:
OhMyBrew\ShopifyApp\Test\Controllers\AuthControllerTest::testAfterAuthenticateDoesNotFireForNoConfig Failed asserting that true is false.

And it's ok that it is failing because indeed now the afterauth method does not return false if the parameter is empty, and that is because it runs in a foreach loop which will simply not run if the array is empty.

Thanks again!
Amos

@gnikyt gnikyt self-assigned this Sep 24, 2018
@gnikyt gnikyt added the feature Enhancement to the code label Sep 24, 2018
@gnikyt
Copy link
Owner

gnikyt commented Sep 24, 2018

Looks fine to me. Will merge in and kill that test and should be fine since its a loop anyways.

@gnikyt gnikyt merged commit 0a747de into gnikyt:master Sep 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature Enhancement to the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants