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

core#1840 - Can't add/change recipient on non-bulk SMS #17691

Merged
merged 1 commit into from
Jun 25, 2020

Conversation

MegaphoneJon
Copy link
Contributor

@MegaphoneJon MegaphoneJon commented Jun 24, 2020

https://lab.civicrm.org/dev/core/-/issues/1840

Overview

When sending a non-bulk SMS, you can't add recipients or change the recipient; only the default recipient works.

Before

Crash with an exception:

CRM_Core_Exception: One of parameters  (value: "Lastname) is not of the type Positive

After

Works.

Technical Details

The JS is making an AJAX request to CRM_Contact_Page_AJAX::getContactPhone(). In line 451, you can see that the results can be returned in one of two formats, depending on whether it was called with a true id parameter. This JS wasn't passing id, so it was getting back the results in an incorrect format.

@civibot
Copy link

civibot bot commented Jun 24, 2020

(Standard links)

@civibot civibot bot added the master label Jun 24, 2020
@demeritcowboy
Copy link
Contributor

  • General standards
    • [r-explain] PASS
    • [r-user] PASS
    • [r-doc] PASS
    • [r-run] PASS
      • Can confirm the problem and patch fixes it.
  • Developer standards
    • [r-tech] PASS
    • [r-code] PASS
    • [r-maint] Undecided
    • [r-test] PASS
      • Test fail is a known flaky test but just in case: jenkins test this please.

I'm surprised I haven't heard about this from users out in the field, but I tested with a real account with a real provider and it's a thing.

@seamuslee001 seamuslee001 merged commit d70a162 into civicrm:master Jun 25, 2020
@MegaphoneJon MegaphoneJon deleted the core-1840 branch June 25, 2020 12:36
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.

3 participants