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

Enable the "sequentialcreditnotes" extension on new installations #16598

Merged
merged 3 commits into from
Feb 24, 2020

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Adds install for our new prototype Core Extension

Before

New Core Extension sequenttialcreditnotes added on upgrade but not on install

After

Added on install

Technical Details

Per @totten there are some good future-thinking ways to address this issue of how to
install extensions on install. For now however, this should work & achieve our short-term goal

Comments

@civibot
Copy link

civibot bot commented Feb 19, 2020

(Standard links)

@civibot civibot bot added the master label Feb 19, 2020
@totten totten changed the title Add new core-extension to install sql Enable the "sequentialcreditnotes" extension on new installations Feb 20, 2020
@@ -1775,3 +1775,5 @@ VALUES
(@option_group_id_soft_credit_type , {localize}'{ts escape="sql"}Matched Gift{/ts}'{/localize}, 9, 'matched_gift', 9, 0, 1, 0),
(@option_group_id_soft_credit_type , {localize}'{ts escape="sql"}Personal Campaign Page{/ts}'{/localize}, 10, 'pcp', 10, 0, 1, 1),
(@option_group_id_soft_credit_type , {localize}'{ts escape="sql"}Gift{/ts}'{/localize}, 11, 'gift', 11, 0, 1, 1);

INSERT INTO civicrm_extension (type, full_name, name, label, file, is_active) VALUES ('module', 'sequentialcreditnotes', 'Sequential credit notes', 'Sequential credit notes', 'sequentialcreditnotes', 1);
Copy link
Member

@totten totten Feb 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Probably need to run regen.sh so that dev/test sites get an updated civicrm_generated.mysql.

  2. I think this is a fairly clever work-around and sounds pretty plausible for the sequentialcreditnotes scenario.

An observation (probably obvious for you and me - but maybe not for all readers)... this bypasses the normal lifecycle functions (install/enable/upgrade) of an extension. As it stands, for sequentialcreditnotes, that's OK - because this extension doesn't currently use those. And I see the appeal in un-blocking this aspect of sequentialcreditnotes.

There was a voice in the back of my head that worried about this as a precedent / example - e.g it doesn't work for the General Case of activating an extension during install, but it will be the only example on record, so Future Code Reader will use it as a reference-point. They'll think, "this stuff is working and I can see how!", start on a path to adding another extension, imitate the example, and be surprised when it doesn't work for their (qualitatively-different) extension - at which point their easiest option will be to ignore the extension lifecycle and somehow hotwire the core lifecycle with the extension lifecycle. God forbid, they might even rationalize it as a feature. ("Well of course a core-extension gets a different lifecycle!...") And then we won't be able to reason about "extensions"... we'll have to reason about "regular-extensions" vs "core-extensions". Let's please keep the contracts for "extensions" consistent. 😅

But I digress.

Perhaps we can head this off with some comments in-situ? e.g.

-- Auto-install the special extension "sequentialcreditnotes". 
-- This technique only works because sequentialcreditnotes is relatively small and does not require any installation logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten I would hope given all the docs we have on extensions people would follow those rather than copy what is in a core folder - if not there is only so much we can do to save people from themselves. Once the tags thing is resolved I'll add a note to the docs clarifying that coreextensions are about being able to separate add-on-core functionality into separate features for code management & are not the template for extensions

@eileenmcnaughton eileenmcnaughton force-pushed the setup branch 2 times, most recently from d6199c5 to c5f7499 Compare February 21, 2020 04:45
@totten
Copy link
Member

totten commented Feb 22, 2020

@eileenmcnaughton So I gave this a light r-run locally to see if the extension was activated, and it wasn't - and it was still an issue with civicrm_generated.mysql.

What was up with that? The issue is inregen.sh, which has a list of certain tables which should not be based on the local-data at time of execution (incl civicrm_cache, civicrm_setting, civicrm_extension). IIRC, these are based in historical incidents wherein someone ran regen.sh locally and wherein local settings inadvertently leaked into other dev sites.

e98ed87 carves a narrow allowance for sequentialcreditnotes.

Now, when I do a civibuild reinstall dmaster, it does appear to end with the ext active:


[bknix-max:~/bknix/build/dmaster/web/sites/all/modules/civicrm] cv ext:list /credit/
Using extension feed "https://civicrm.org/extdir/ver=5.24.alpha1|uf=Drupal|status=stable|ready=ready"
+----------+-----------------------+-----------------------+---------+-----------+-------------+
| location | key                   | name                  | version | status    | downloadUrl |
+----------+-----------------------+-----------------------+---------+-----------+-------------+
| local    | sequentialcreditnotes | sequentialcreditnotes | 1.0     | installed |             |
+----------+-----------------------+-----------------------+---------+-----------+-------------+

@totten
Copy link
Member

totten commented Feb 23, 2020

Regarding tests... it might help to state the intended behavior. My guess from the file-structure is that one would want:

  • For the core headless suites in civicrm/tests/phpunit/{api,CRM,Civi}, the sequentialcreditnotes extension should not be active.
  • For any tests in civicrm/ext/sequentialcreditnotes/tests, the extension sequentialcreditnotes should be active.

The CI reports a failure in api_v3_PaymentTest.testRefundPayment. Is this because the extension is (contrary to desire) active during the execution of tests/phpunit/api?

@eileenmcnaughton
Copy link
Contributor Author

@totten no the expectation is this extension would be active for all tests - core extensions are still core (they can be disabled but that is not part of what we are testing).

I'm not sure why the test is failing - I will rebuild locally with your change & test

eileenmcnaughton and others added 3 commits February 24, 2020 12:45
Per @totten there are some good future-thinking ways to address this issue of how to
install extensions on install. For now however, this should work & achieve our short-term goal
@eileenmcnaughton
Copy link
Contributor Author

OK - the bug was because I switched to api v4 & missed the setPermissions(FALSE) but looks good now

@eileenmcnaughton
Copy link
Contributor Author

@totten passing now

@seamuslee001
Copy link
Contributor

This looks fine to me now @totten @eileenmcnaughton

@totten totten merged commit 63214e6 into civicrm:master Feb 24, 2020
@eileenmcnaughton eileenmcnaughton deleted the setup branch February 24, 2020 20:50
@eileenmcnaughton
Copy link
Contributor Author

cool

@eileenmcnaughton
Copy link
Contributor Author

did we get the tests running on it @totten @seamuslee001 ?

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

Successfully merging this pull request may close these issues.

3 participants