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

dev/core#1703: Created a system extension tab on the manage extension screen #17059

Closed
wants to merge 1 commit into from

Conversation

jaapjansma
Copy link
Contributor

@jaapjansma jaapjansma commented Apr 12, 2020

Overview

Since CiviCRM 5.24 hidden extensions are shipped with CiviCRM, such as Sequential Credit Notes.
Those extension do not show up in manage extensions screen.

Before

Hidden extensions are not shown to a system administrator on the manage system extensions page.

Screenshot_20200412_213642

After

The manage extensions page also contain a tab for System Extensions showing the hidden extensions.

Screenshot_20200412_214828

Comments

I am open for any better texts for the warning of a system administrator.

Also the documentation should be updated after this PR got merged.

See also: https://lab.civicrm.org/dev/core/-/issues/1703

@civibot
Copy link

civibot bot commented Apr 12, 2020

(Standard links)

@civibot civibot bot added the master label Apr 12, 2020
@jaapjansma jaapjansma changed the title Created a system extension tab on the manage extension screen dev/core#1703: Created a system extension tab on the manage extension screen Apr 12, 2020
@eileenmcnaughton
Copy link
Contributor

@jaapjansma I would rather let the concept of core extensions mature before exposing them in any way. The current position is that disabling them is not a supported configuration - see docs

https://github.com/civicrm/civicrm-dev-docs/pull/767/files

The hope is to start to more logically organise parts of core code - but I don't want to rush into exposing that to non-developers

@colemanw
Copy link
Member

I'm in favor of giving system extensions visibility eventually, when it's matured.
In terms of this implementation, I notice it duplicates a lot of .tpl code. Perhaps that can be streamlined.

@eileenmcnaughton
Copy link
Contributor

@colemanw implementation-wise - I am of the impression that at some point we will replace this whole screen - possibly around the point where the core extensions are mature

@totten
Copy link
Member

totten commented Apr 13, 2020

I also agree that the approach in this PR - making it visible, but flagging it with a disclaimer - is better than the status quo.

The current position is that disabling [sequentialcreditnotes] is not a supported configuration... The hope is to start to more logically organise parts of core code - but I don't want to rush into exposing that to non-developers

Agree it's fair to try to hold some space so that we can iterate on the scoping/support/design while this stuff is extracted/refactored. I think that what you're driving at here (starting to massage the code into modular form without making any guarantees about the interim technical contracts) is pretty compelling -- ie as a way to relieve some of the short/mid-term pressure around how to get this right.

But... let me read between the lines in dev/core#1703 and offer a counter-point. For many people, the "Manage Extensions" screen is the primary way they conceive of the state-of-play of extensions. If there's an active extension that doesn't show up there, then that would feel like obscure magic which undermines their understanding of the state-of-play. It is important for us to communicate effectively with contributors/developers/sysadmins about that state-of-play.

For sequentialcreditnotes (at time of writing), what is the state-of-play? Sure, the TLDR is "You need to keep this enabled". The complex explanation is everything we've talked about (it exists; it's being extracted from existing functionality; it may change technically; there are some dev-docs and Gitlab issues; it can be disabled via API/CLI; one or two people may be disabling it; disable at your own risk; etc).

I think that hiding the extension is fine as a quick/low-cost/defensive maneuver. But I think what Jaap's pointing to is higher-value than the status quo (even in the near-term, even if sequentialcreditnotes is going to evolve). The essential elements of the higher-value approach are: (a) show that the extension exists and (b) clearly describe the status/limits/support/scope.

That's my main rant. A little bit of a braindump on random bits:

  • Next to any "special" extensions, there could be a highlighted link to a docs-page/wiki-page which discusses its special status/lifecycle in more detail. That makes it easy to fix-up the communications in response to any post-release issues/feedback. This would be better than trying to embed pointed advice in the Smarty templates.
  • If the extension is not hidden in the UI, then don't use the tagmgmt:hidden ("the item is not displayed"). Maybe implement a tag like mgmt:mandatory ("the 'Disable' button is unavailable") or mgmt:system ("shows in a separate 'System' tab").
  • If there's a concern that the code for the separate tab is too duplicative, then one could keep the UI as one tab... just add an extra CSS class crm-extension-hidden for any relevant rows, and then provide a checkbox like [_] Show hidden extensions.

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Apr 13, 2020

The intention was that other pieces of functionality would be moved to core extensions and that this might have to happen over a few releases as it would be difficult. However, by not exposing them & stating that they are still part of core and disabling them is not supported we give ourselves the space to do that. In particular there needs to be no implicit guarantee that they can be disabled without causing fatal errors or whitescreens.

Exposing the core-extensions at this stage would probably mean we need to find a new way to start disentangling core functionality as core extensions would not make sense until a piece of functionality was fully dis-entangled. Maybe we could have core-extensions and core-not-really-extensions-extensions? Then sequenialcreditnotes could be exposed but not any future ones ...

@mlutfy
Copy link
Member

mlutfy commented Apr 14, 2020

It's more about transparency, than about being able to disable it?

It seems OK to have extensions that cannot be disabled (considering that this is core code that's being moved into extensions, and where we haven't yet tested what happens if the extension is disabled; and not some malicious intent)

What if other extensions start abusing this tag (potential malware)?

@jaapjansma
Copy link
Contributor Author

For me it is indeed about transparency and the risk of disabling system extension is upon the administrator who click that button. Maybe the caution message could be more clearly worded and positioned. Basically we want to warn the system administrator that disabling a system extension might break the whole system.

@eileenmcnaughton
Copy link
Contributor

eileenmcnaughton commented Apr 14, 2020

OK -so coming back to the original goal - re-organising core code into extensions in a way that disentangles them & allows us to consider making them optional at some point in the future. My expectation is that we need to expect missing classes causing instant UI-unrecoverable white-screens if these extensions are disabled.

We decided to go with core-extensions to re-use the existing framework. sequentilacreditnotes was the experimental version. If we expose these via the UI then it pretty much ends the usefulness of this path for the purpose of re-organising core code.

If the hidden tag is a risk factor (how would they get enabled?) we could consider enabling sequentialcreditnotes just using hard-coding & undoing the 'hidden' tag - this would be the end of the effort to disentangle functionality from core - 'diet-civi' would then be about restricting what enters core but there would be no further effort to disentangle existing functionality from core if we can't do a dev-code-re-organization without it being visible to users.

@eileenmcnaughton
Copy link
Contributor

I'm OK revisiting making some core-extensions disablable in 6-9 months but if we do it now I think we are basically ending the experiment / initiative in finding a path towards moving stuff out of core (I'm certainly out & I was the person who was likely to be doing it)

@colemanw
Copy link
Member

I don't think we have consensus to merge this yet and should continue this discussion on GitLab.
See https://lab.civicrm.org/dev/core/-/issues/1703

I know this process can be frustrating sometimes but let's keep in mind that we all want the same thing - a more useful, modular & flexible CRM. And @jaapjansma and @eileenmcnaughton are both making valuable contributions to move us in that direction.

@colemanw colemanw closed this Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants