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

Allow contribution search by recurring payment processor ID / Transaction ID #12818

Merged

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Sep 14, 2018

Overview

Allow to search by recurring contribution transaction ID / payment processor ID / processor ID

Before

Cannot search by recurring contribution transaction ID / payment processor ID / processor ID

After

Can search by recurring contribution transaction ID / payment processor ID / processor ID

recur_search

Technical Details

Just exposes three extra fields for searching.

@seamuslee001
Copy link
Contributor

@mattwire can i confirm that even tho the Payment Processor was added to the $form array as a text field it was never exposed through the template is that correct?

@mattwire
Copy link
Contributor Author

@seamuslee001 Both transaction ID and processor ID were exposed on the form but not on the template. Note that processor_id and payment_processor_id are not actually the same thing. If it makes sense we could also expose processor_id on the search template?

@seamuslee001
Copy link
Contributor

@mattwire i guess what i was going for was is there a possibility that smart groups perhaps could have been built on the field before hand because i note you are changing the processor_id from a text to a select field

@mattwire mattwire force-pushed the contributionsearch_byrecurtrxnprocessor branch from 3b58c1c to 3c10f05 Compare October 7, 2018 10:48
@kirk-jackson
Copy link
Contributor

I'm going to review this now.

@mattwire
Copy link
Contributor Author

@kirk-jackson I remember you having some comments on this, but they're not here?

@kirk-jackson
Copy link
Contributor

Hi @mattwire, sorry for the delay. I think this is a very useful improvement, but it's missing a couple of things. Specifically, when values are supplied for any of these fields:

  • the values need to be displayed in the search results summary (aka the 'QILL').
  • the Recurring Contributions search criteria pane needs to be kept open, otherwise the values will disappear on subsequent searches.

Furthermore, I think that there may be confusion about what the Processor ID and Transaction ID are for. To make it clear, I propose a couple of improvements:

  • Add help balloons for the Processor ID and Transaction ID fields.
  • Move the Processor ID field up so that it's directly below the Payment Processor field, as the two are related. (The Processor ID is supplied by the Payment Processor.)

I have attached a patch file to implement the above changes: civicrm-pr12818-patch.txt. What do you think?

@eileenmcnaughton
Copy link
Contributor

I took a look at the code - once @kirk-jackson is happy I think we should merge this

@kirk-jackson
Copy link
Contributor

  • Explain (r-explain)
    • PASS : The goal/problem/solution have been adequately explained in the PR.
  • Test results (r-test)
    • PASS: The test results are all-clear.
  • Code quality (r-code)
    • PASS: The functionality, purpose, and style of the code seems clear+sensible.
  • Documentation (r-doc)
    • PASS: The changes do not require documentation.
  • Maintainability (r-maint)
    • PASS: The change is trivial enough that it does not require tests.
  • Run it (r-run)
    • PASS: With sample recurring contribution records in the database, searches using a range of values in each of the three new search fields gave the expected results.
  • User impact (r-user)
    • PASS: The change would be intuitive for a majority of users who work with this feature.
  • Technical impact (r-tech)
    • PASS: The change preserves compatibility with existing callers/code/downstream.

Happy for this to be merged @eileenmcnaughton, @mattwire.

@seamuslee001
Copy link
Contributor

Merging as per @kirk-jackson 's review

@seamuslee001 seamuslee001 merged commit 7649c95 into civicrm:master Nov 2, 2018
@eileenmcnaughton
Copy link
Contributor

thanks @kirk-jackson @mattwire

@mattwire mattwire deleted the contributionsearch_byrecurtrxnprocessor branch March 1, 2019 11:25
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