-
-
Notifications
You must be signed in to change notification settings - Fork 814
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
CRM-20387 - Add invoice_number field for human readable contribution invoice #10298
Conversation
colemanw
commented
May 3, 2017
•
edited by civicrm-builder
Loading
edited by civicrm-builder
- CRM-20387: Sales Tax and Invoicing code overwrites existing CiviCRM invoice ID
@guanhuan can you please assign this for QA? |
d78b331
to
e3f07ac
Compare
@eileenmcnaughton do you understand that failing test? |
hmm - it probably ensures that the invoice_id is returned to advanced_search? |
Thanks for this, it looks excellent. Three questions:
|
{include file='../CRM/Upgrade/4.7.21.msg_template/civicrm_msg_template.tpl'} | ||
|
||
-- CRM-20387 | ||
UPDATE `civicrm_contribution` SET `invoice_number` = `invoice_id` WHERE `invoice_id` LIKE CONCAT('%', `id`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adixon - I think this covers your point # 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, missed that, perfect, better than I was thinking.
@adixon I don't want to belabor that point either but agree we could do better than "Invoice Hash". What is the field mostly used for? It's a token sent to the payment processor right? So maybe it should be "Payment Processor Code"? Human testing: YES PLEASE! In this case I have no moral objections to experimenting on humans. Speaking of testing. What would be incredibly helpful would be insight into why |
So far so good with the human testing, no harm has been recorded. I think there may be some updates on various screens that show the invoice_id and now should also show the invoice_number (e.g. the contribution view), but we can also add that as a subsequent PR, it'd be good to get the basic stuff in for now. re: label - definitely not "Payment processor code", that's too many overused words. Including the word invoice is appropriate since it may show up in the invoice field of the payment processor record. How about "Invoice Reference"? |
"Invoice Reference" sounds fine to me. I thought I had added it to the View Contribution screen in this PR. No? |
I applied your PR diff to an existing install, so that didn't do the db upgrade, I'll have to be more responsible to get a real test going. re: why the unit test is failing - I think it's failing because invoice_number is empty and it thinks it should be populated - i.e. by default it looks like all fields need to be non-empty. It's a guess, based on the fact that all the other fields seem to be populated with sample test values at the start of that test. So if you just give invoice_number a value at the beginnging, it should work - I think that test is just trying to verify that all the values in params are actually populating the contribution record, and that some aren't getting changed. |
a9c3320
to
8894c71
Compare
@adixon I've rebased and updated this PR with the changes we discussed ("Invoice Reference") as the field label. Just need to get that test passing and then we can merge this in before the RC goes out. |
Jenkin test this please |
test this please |
b079c0c
to
9983ddf
Compare