[NFC] Use insert ignore for inserts into civicrm_extension to stop warnings on duplicate entry for sequential credit notes extension

[seamuslee001]

Overview

This resolved notices such as

/home/jenkins/bknix-dfl/build/core-16642-770zv/web/sites/all/modules/civicrm/Civi/Test.php:205:
bool(false)
/home/jenkins/bknix-dfl/build/core-16642-770zv/web/sites/all/modules/civicrm/Civi/Test.php:206:
array(3) {
  [0] =>
  string(5) "23000"
  [1] =>
  int(1062)
  [2] =>
  string(72) "Duplicate entry 'sequentialcreditnotes' for key 'UI_extension_full_name'"
}

occurring when running tests

Before

Notices show

After

No notices

Technical Details

The issue is that civicrm_extension table is not 'cleaned' out between test runs unlike other tables to ensure that we don't remove information on enabled extensions. This adds in some logic specific to the sequential credit notes extension as it is added via the civicrm_data.mysql file

ping @demeritcowboy @eileenmcnaughton @totten

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

@seamuslee001 what about truncating the table between runs? We've done things very specific to this extension because it's the first but we don't see it being the only

[eileenmcnaughton]

(presumably we would truncate in tearDown?)

[seamuslee001]

@eileenmcnaughton the table has been specifically excluded from the truncate https://github.com/civicrm/civicrm-core/blob/master/Civi/Test/Schema.php#L101 this i suspect is designed to help extension testing

[eileenmcnaughton]

Hmm - well I'm OK merging this as it fixes but going forwards we probably need another way to check

[demeritcowboy]

It works for me locally to fix it. I wonder if another way is to use INSERT ON DUPLICATE KEY UPDATE in the civicrm_data xml template, or might that then end up missing failures that should get caught?

[eileenmcnaughton]

@seamuslee001 ohh - I like @demeritcowboy's suggestion

[seamuslee001]

updated it now @eileenmcnaughton @demeritcowboy

[eileenmcnaughton]

Cool- next round we can get more generic

[demeritcowboy]

Insert Ignore is different than Duplicate Update and will silently skip errors on real sites for the initial insert since this file isn't just for tests. I guess as long as it doesn't stay like this permanently?

[seamuslee001]

so @demeritcowboy if someone had actually disabled the extension we shouldn't be going through and updating the civicrm_extension row hence an insert ignore

[demeritcowboy]

Ok I'm not following again but if you're good.

[totten]

I love the critical evaluation of the effect on real sites. In this case, I don't think it matters. civicrm_data.tpl is only used during the initial installation - for a normal site, it's one-off. Unit-tests are odd in that they periodically reset everything and have some narrow carve-outs. I can't think of any other scenario where the difference between INSERT / INSERT IGNORE (for this line) could matter.

Within the unit-testing world, I think I see/agree with @demeritcowboy's point about disabling. Suppose (hypothetically) you have complementary tests WithSequentialTest and WithoutSequentialTest. One test relies on a default (sequentialcreditnotes enabled), and the other test opts-out (disabling sequentialcreditnotes).

You would find that the tests were flaky/order-sensitive - because the INSERT IGNORE doesn't fully reset us back to baseline.

Generally INSERT ... ON DUPLICATE UPDATE does sound more correct. If for some reason the uniquess constraints aren't suitable, DELETE ... WHERE plus INSERT would probably have the desired effect too.

[eileenmcnaughton]

@totten at this stage the position is that core extensions are part of core & we only test with them enabled. We merged to docs to that effect https://docs.civicrm.org/dev/en/latest/extensions/

[eileenmcnaughton]

merging this - it solves the problem for now - this is an evolving area that we will hopefully make more generic as we move more code but for now this helps us get to the end of the beginning of the beginning

[totten]

Sure, if sequentialcreditnotes is hidden, that works for now.

But extracting this functionality in an extension signals a long-term intention to allow the system to work with the toggle going either way (on or off), right? That was the purpose of extracting it to an extension? Eventually you'll want some amount of test-coverage for both situations (where the toggle is on and off)

[eileenmcnaughton]

@totten the point of extracting stuff is more about making the code more readable & more modular. Yes it WILL work with sequentialcreditnotes off -(and yes I will disable it for my sites) but the problem with this &a bunch of other features are the way they are integrated into the code (hacks on the forms, spaghetti deep in the BAO) makes our code un-fixable.

To look at the order api - it's not where it should be and that's largely because of add-on-features (this was one) embedded deeply into code that we need to make sensible in order to make it ready to support afform.

[eileenmcnaughton]

"Eventually you'll want some amount of test-coverage for both situations (where the toggle is on and off)" - at the moment we have only figured out where to add coreextensions in to make them be tested & installed & added on upgrade - we need to make all of those things generic before that eventually happens so they all need to be revisited