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

CRM-20989 Fix Active provider count to know about multisite #10792

Merged
merged 2 commits into from
Jul 31, 2017

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Jul 31, 2017

Overview

At Present CRM_SMS_BAO_Provider::getActiveProviders just checks to see if there is an active provider in the database and not if it is in available for the current domain

Before

This can cause scheduled reminders to ask the user to select an SMS provider even when none load due to access issues

After

Correctly only adds in SMS requirements to scheduled providers if there are providers available in the db for the domain

Technical Details

Swaps out max for count to get an accurate count and adds check on the domain_id column

ping @eileenmcnaughton


@eileenmcnaughton
Copy link
Contributor

Seems good - any test cover on this fn?

@seamuslee001
Copy link
Contributor Author

None previously and none added in with this PR, I'm not sure how to mock up say a 2nd domain in the unit tests

@eileenmcnaughton
Copy link
Contributor

Actually there IS a second domain in the test Db- but on this I'd be happy with a test that just called the function & demonstrated no sql error

@seamuslee001
Copy link
Contributor Author

@eileenmcnaughton added a unit test and got it to Mock correctly

@eileenmcnaughton
Copy link
Contributor

Damn you are quick!

@monishdeb
Copy link
Member

Merging as per tag

@monishdeb monishdeb merged commit a802699 into civicrm:master Jul 31, 2017
@eileenmcnaughton eileenmcnaughton deleted the CRM-20989 branch July 31, 2017 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants