dev/core#534 fix failure of print invoice to display on settings page #13137
Conversation
… invoicing, remove from form
|
(Standard links)
|
3a6a951 to
4a88526
Compare
| @@ -795,6 +795,7 @@ public static function recurringContribution(&$form) { | |||
| 'return' => ["id", "name", 'is_test'], | |||
| ]; | |||
| $paymentProcessors = civicrm_api3('PaymentProcessor', 'get', $paymentProcessorParams); | |||
| $paymentProcessorOpts = []; | |||
There was a problem hiding this comment.
enotice fix for test to pass
|
@eileenmcnaughton Installed via Buildkit, flushed caches. Works as described. Print Invoice button does work. We are still missing the 'Pay Now' option for pending payments. I've taken a look and not sure what change caused that to change from 5.7 to 5.8 |
|
@kcristiano OK - I'll merge this & then take a look at that as it will build on this |
… historical weirdness
f8b0413 to
ff61c74
Compare
ff61c74 to
e0001b1
Compare
It is causing a test fail and is only saving us from calling a cached function....
|
@eileenmcnaughton Wow! I'm wondering if it makes more sense to just add an upgrader function to split out the invoicing "settings" into separate settings so we don't need all this custom code to support them? |
|
@kcristiano I just added in the Pay Now link. I had to disable one of the tests for now as it passes locally but not when running on jenkins :-( I'd rather debug the test issue on master |
68d5841 to
b6cea87
Compare
| * | ||
| * We check both here. But will deprecate the latter in time. | ||
| */ | ||
| public static function isInvoicingEnabled() { |
There was a problem hiding this comment.
@mattwire it's really only the second part of these 2 functions that wouldn't be needed without the historical weirdness - but I'm inclined to think a Utils class for invoicing is a good thing to have - part of the dream of one day moving invoicing back out of core :-)
|
@mattwire If might be nice to convert them because then we could do away with that preferences form - but I worry a little if extensions are copying this core weird effort to access those settings |
b6cea87 to
f885761
Compare
|
@eileenmcnaughton I've applied the patch locally to D7 and WP. Tested and both Pay Now and Pay Later appear. Both buttons function properly and I was able to make payment and download an invoice. |
|
@eileenmcnaughton @kcristiano As this is an RC fix, assuming it works and is reviewed I'm happy for it to be merged. I do think we should look at properly migrating the invoice settings to be more standard - but that's for a future PR in master. |
|
@mattwire I've been thinking more about this - I think we should migrate the way Invoice settings are added so it's more like an internal page run hook - and one day it might move to an extension - but the more I think about it the default_invoice_page setting is not really an invoice thing & should be renamed & separated from the invoices - anyway - that can happen in master |
Overview
Adds tests for display of print invoice on user dashboard when invoicing is enabled. Fixes mis-setting of setting (unreleased regression)
Before
Turning on invoicing does not always turn on display of invoices on user dashboard page
After
Related setting managed through a toggle function - which works off the setting page and via the api
Technical Details
Our setting standards are for one setting per ... setting. The invoicing settings use a nested structure that doesn't comply & requires a lot of hacky handling to support it. I also added a function to start to wrap this stuff in utility function.
I tried - but so far failed - to add html validation to the test
Comments
Caches will need to be flushed to test this
@kcristiano