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

Deprecate crazy BAO handling of preferred_communication_method #23623

Merged
merged 1 commit into from
May 31, 2022

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented May 29, 2022

Overview

Deprecate crazy BAO handling of preferred_communication_method

Before

Lots of duplicate handling for imploding preferred_communication_method

After

Deprecated in the BAO, duplicate methods removed in the Import

Technical Details

In the context of import this is tested in
testImportFieldsWithVariousOptions

  • for the api it is tested via testPseudoFields - although in
    api context it is just a deprecation

Comments

one of the tests was buggy - passing [1,4] needed to fail for the test to pass - even though the checks made it clear that wasn't the expecation. It works now in import

Also - some tests were passing this in - but it's not clear they reflect 'real usage' - if there IS real usage it will cause deprecation notices but it will still work

@civibot
Copy link

civibot bot commented May 29, 2022

(Standard links)

@civibot civibot bot added the master label May 29, 2022
@eileenmcnaughton eileenmcnaughton force-pushed the pcm branch 3 times, most recently from baa1b04 to 5e7576d Compare May 30, 2022 01:47
In the context of import this is tested in
testImportFieldsWithVariousOptions

- for the api it is tested via testPseudoFields - although in
api context it is just a deprecation
@colemanw
Copy link
Member

@eileenmcnaughton it looks like this is changing the expected input format from an associative array of boolean checkbox values keyed by id, e.g. [6 => 1, 22 => 0, 31 =>1] to a non-associative array of ids e.g. [6, 31].
Does this change the API contract?

@eileenmcnaughton
Copy link
Contributor Author

@colemanw no - the api contract already accepted both - it still does - but with a deprecation notice. The tests had to be changed to avoid the notice

@colemanw
Copy link
Member

OK.

@colemanw colemanw merged commit e3c7486 into civicrm:master May 31, 2022
@colemanw colemanw deleted the pcm branch May 31, 2022 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants