core/384 CiviSMS does not fall back to non-primary mobile number #12890
Conversation
|
(Standard links)
|
|
Test failed as expected. Adding a commit to fix it soon. |
|
Further using testing required. Do not merge yet. |
|
@dereklewis123 @kenwest I think you have both been working on this - I think the one line change in this PR may be part of the problem too @dereklewis123 the 7 test fails above are unrelated -due to a current server issue |
|
Looks good to me |
|
Merging - change has been subject to a bit of discussion & seems agreed. Most of the patch is unit tests which we like |
| ); | ||
|
|
||
| // Prepare expected results | ||
| $checkPhoneIDs = array( | ||
| $contactID1 => $contactIDPhoneRecords[$contactID1]['primary_phone_id'], | ||
| $contactID2 => $contactIDPhoneRecords[$contactID2]['primary_phone_id'], | ||
| $contactID3 => $contactIDPhoneRecords[$contactID3]['other_phone_id'], |
There was a problem hiding this comment.
@JKingsnorth is this line wrong? ie if we fix order by it will be the primary phone id?
There was a problem hiding this comment.
@eileenmcnaughton - no this test is correct, we want to pick up the 'other' in this case, because the primary phone number is not of the type 'mobile'.
There was a problem hiding this comment.
More like 'aaarrghh?!'
|
As discussed in #12765 there are some issues with this PR/merge which will be tackled in a future PR. We will link to it when it is created. |
|
Issues resolved, this PR is fine just as it is, and has been merged. Rejoice. |
Overview
Fix CiviSMS to message non primary mobiles.
Before
SMS will not be sent to a contact if their mobile is not the primary phone
After
Non primary mobiles can receive CiviSMS
TESTS
Technical Details
This allows the non-primary mobile to get it & adds a test. However, it seems a fix on the order by is needed as well in order to prioritise them
Comments
@kenwest @dereklewis123 - I closed this #12765 but it seems it's probably still needed