CRM-20387 - Add invoice_number field for human readable contribution invoice

[colemanw]

• CRM-20387: Sales Tax and Invoicing code overwrites existing CiviCRM invoice ID

[colemanw]

@guanhuan can you please assign this for QA?

[colemanw]

@eileenmcnaughton do you understand that failing test?

[eileenmcnaughton]

hmm - it probably ensures that the invoice_id is returned to advanced_search?

[adixon]

Thanks for this, it looks excellent. Three questions:

1. I'm not convinced about the label Invoice Hash. For anyone who doesn't understand about hash functions (i.e. the majority of people using CiviCRM), this is a bit of a weird label that conjures up illicit substances. Does Unique Invoice ID work? Or Invoice UID? or Invoice Code? After all, let's give it a label that tells you what it's designed for instead of how it was generated. I promise not to talk about the colour of the bike shed.

2. Do you want some human testing?

3. I wonder if it makes sense to selectively populate the invoice_number field with the invoice_id field for backwards compatibility with people already using this feature? Maybe limit it to contributions where the invoice_id value has the right prefix?

[colemanw]

@adixon - I think this covers your point # 3.

[adixon]

Ah, missed that, perfect, better than I was thinking.

[colemanw]

@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 testGetContributionReturnFunctionality is failing. I'm a bit stumped.

[adixon]

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"?

[colemanw]

"Invoice Reference" sounds fine to me.

I thought I had added it to the View Contribution screen in this PR. No?

[adixon]

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.

[colemanw]

@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.

[monishdeb]

Jenkin test this please

[eileenmcnaughton]

test this please