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

dev/financial#79 minimal deprecation of Contribution.transact. #15564

Merged
merged 1 commit into from
Oct 23, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Mark unsupported, flawed Contribution.transact api as deprecated in api explorer

Before

No messaging to tell people not to use this api

After

Screen Shot 2019-10-21 at 4 38 56 PM

Technical Details

It seems urgent that we so some deprecation of this api - this is the absolute minimum.

More would involve creating some noise when people use it but there
is a separate issue for that. This is just to get it out of being 'promoted'
by the api explorer

Comments

@civibot
Copy link

civibot bot commented Oct 21, 2019

(Standard links)

@civibot civibot bot added the master label Oct 21, 2019
@mattwire
Copy link
Contributor

I'd like to see this merged in conjunction with #15504 which helps sites manage their transition away from this API

@eileenmcnaughton
Copy link
Contributor Author

@mattwire the way for them to ease their transition is to read the docs I wrote & implement it civicrm/civicrm-dev-docs#705 - I didn't go further with the deprecation (as I believe we should) to ease the transition

@mattwire
Copy link
Contributor

@mattwire the way for them to ease their transition is to read the docs I wrote & implement it civicrm/civicrm-dev-docs#705 - I didn't go further with the deprecation (as I believe we should) to ease the transition

It's great that we've now got docs and we're making progress on this. But we still need the outreach and it's still going catch some sites by surprise. The result of that surprise is potentially a loss of lot's of money for the organisation when payments fail and a need to do dev work to get it working again. Yes, they should plan their upgrades better, but we all know that doesn't happen!

My position here is that either we merge both #15504 and #15564 or neither. Because I feel strongly that we need to allow time for admins to transition away from this (broken) API.

@eileenmcnaughton
Copy link
Contributor Author

eileenmcnaughton commented Oct 21, 2019

@mattwire right stopping advertising the api in the explorer is the first step towards discouraging people from using / adopting it! ie this is part of the outreach

It should never have been advertised in api explorer & this rectifies that. Really I'd rather add the noisy notices.

I would note I've been telling people publicly for over a year not to use it (as documented) - so noisy notices would probably be better. But I agree we should make not changes that make it work less than it currently does - deprecation doesn't break it but fully removing it would!

* Array of deprecated actions
*/
function _civicrm_api3_contribution_deprecation() {
return ['transact' => 'Contribute.transact is ureliable & unsupported - see https://github.com/civicrm/civicrm-dev-docs/pull/705 for how to move on'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return ['transact' => 'Contribute.transact is ureliable & unsupported - see https://github.com/civicrm/civicrm-dev-docs/pull/705 for how to move on'];
return ['transact' => 'Contribute.transact is ureliable & unsupported - see https://docs.civicrm.org/dev/en/latest/financial/OrderAPI/#transitioning-from-contributiontransact-api-to-order-api'];

@colemanw
Copy link
Member

I've merged the docs PR so the link here can reference something more permanent. See suggestion above @eileenmcnaughton

In general I think this is fine to merge as it's a NFC basically just changes how the api is documented.

@eileenmcnaughton
Copy link
Contributor Author

@colemanw this is NFC but since we've merged a PR that I was strongly opposed to I think we had better merge this ASAP AND merge a PR that noisily deprecates it

It seems urgent that we so some deprecation of this api - this is the absolute minimum.

More would involve creating some noise when people use it but there
is a separate issue for that. This is just to geet it out of being 'promoted'
by the api explorer
@eileenmcnaughton
Copy link
Contributor Author

@colemanw updated - also merge #15591 please

@seamuslee001
Copy link
Contributor

Test fail unrelated merging

@seamuslee001 seamuslee001 merged commit 2f08eb6 into civicrm:master Oct 23, 2019
@seamuslee001 seamuslee001 deleted the deprectate branch October 23, 2019 21:33
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