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

tenantId key does not always exist. #7

Closed
wants to merge 1 commit into from

Conversation

Rigby90
Copy link
Contributor

@Rigby90 Rigby90 commented May 14, 2020

I believe I'm correct in my thinking here, this error started cropping up which at first I thought was an issue with my app but after further testing, I believe it relates to the lines mentioned in this PR.

This key-value pair is not always set within the payload. This occurs if either the tenant is not set when dispatching the job (line 31-33), or the job is a Tenant Unaware job (line 27-39).

However, this PR breaks the following tests -

Spatie\Multitenancy\Tests\Feature\TenantAwareJobs\QueueIsTenantAwareByDefaultTest::it_will_not_break_when_no_tenant_is_set

Spatie\Multitenancy\Tests\Feature\TenantAwareJobs\QueueIsTenantAwareByDefaultTest::it_will_not_make_jobs_tenant_aware_if_the_config_setting_is_set_to_false

Spatie\Multitenancy\Tests\Feature\TenantAwareJobs\QueueIsTenantAwareByDefaultTest::it_will_not_make_a_job_tenant_aware_if_it_implement_NotTenantAware

With regards to the it_will_not_break_when_no_tenant_is_set test, I believe what is happening here is you're actually triggering the same error I am during your testing, but instead of it being outputted it's causing the Job to fail and therefore returning a null response to the test, which is what it's expecting.

If I add $this->doesntExpectEvents(JobFailed::class); to the top of that test, it then starts to fail. As the job is actually failing and triggering a JobFailed event.

Admittedly I've not looked into the other two tests as I want to be sure I've understood the logic and how the test I have looked into should work first.

This key-value pair is not always set within the payload. This occurs if either the tenant is not set when dispatching the job (line 31-33), or the job is a Tenant Unaware job (line 27-39).
@freekmurze
Copy link
Member

Thank you for reporting this subtle bug.

Meanwhile, I've fixed in on master and also addressed the tests. Everything passes again.

Could you try the latest version and let me know if the issue has been fixed properly for you?

@freekmurze freekmurze closed this May 14, 2020
@Rigby90
Copy link
Contributor Author

Rigby90 commented May 14, 2020

Perfect. It does indeed, thank you! Had a feeling there would be a better way of handling it.

On an unrelated note, I can raise an issue or submit a PR but it may be easier to quickly mention it here. The only other issue I've noticed so far is I receive the following error when navigating to a tenant based route.

Unresolvable dependency resolving [Parameter #0 [ <required> $taskClassNames ]] in class Spatie\Multitenancy\Tasks\TasksCollection

It appears to be setting up the Tenant prior to having all the tasks setup and TasksCollection registered within the container.

Changing the ordering of the below resolves the error for myself -

$this
->configureRequests()
->configureQueue()
->registerTasksCollection();

To -

$this
->registerTasksCollection()
->configureRequests()
->configureQueue();

Once again, thank you for your work!

@freekmurze
Copy link
Member

Though I cannot reproduce that exception in the tests suite, I've made your suggested code change on dev-master.

Thanks for testing out the package.

@Rigby90
Copy link
Contributor Author

Rigby90 commented May 14, 2020

I tried to reproduce it in the tests but wasn't able to, however I'm not all too familiar with orchestral/testbench, so will have to dabble a bit more in it and learn it myself.

No problem at all, looking forward to v1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants