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

issue#4 fix #5

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

issue#4 fix #5

wants to merge 2 commits into from

Conversation

monishdeb
Copy link
Member

See #4

@MegaphoneJon
Copy link
Contributor

Hi @monishdeb - I tested this fix, and it seems to work. Additionally, it fixes a bug I hadn't realized until yesterday was tied to this extension - the length of time it takes to load the search results on this page usually causes a timeout.

Note, however, that I don't have any grant entries to test currently.

@MegaphoneJon
Copy link
Contributor

I also think this improves performance - I believe there was a performance issue in the existing function, but I'm not going to try to benchmark it. While I can't speak to the grant side of things, this is all around better than previously.

@MegaphoneJon
Copy link
Contributor

I've tested further and noticed two issues:

  • Clicking View next to a transaction doesn't bring it up as a snippet.
  • I'm having an issue that I can't quite figure out that differs between my live and dev environments. On my live site, the search doesn't work at all, with calls to civicrm/getfinancialtransaction which is in the templates/CRM/Financial/Form/BatchTransaction.tpl, but of course has been removed from the PHP. I'll investigate further.

@MegaphoneJon
Copy link
Contributor

OK, the issue was some bad caching on my dev site. I think you'll also need to remove templates/CRM/Financial/Form/BatchTransaction.tpl.

@monishdeb
Copy link
Member Author

@MegaphoneJon Sorry for the delay. Yes, that was a miss on my part and I have updated the PR. Please check now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants