Clarify the implementation of "Site" plugins

[ctgraham]

In OJS master (with a nod to OMP master as well):

Currently we have a hard distinction between "Site Plugins" and "not Site Plugins" via Plugin::isSitePlugin() but this could be enhanced:

• (from @ctgraham) I want to provide site-level plugin settings which could be overridden by a Manager at the context-specific level. The plugin settings page would be addressable from both the Site level and the Context level. For Site-and-Context Plugins, when checking for settings, the Plugin would check the specific context first, then fall back on the site’s context.
• (from @jmacgreg) if a plugin is site-level only, don’t display it on the journal plugin listing; and if a plugin is both site- and journal-level, just treat the site level as another “journal”, in this case journal 0. So for example, the Site Admin could create custom blocks for the site index, and those plugin settings would be stored in the plugin_settings table for journal_id = 0.
• (from @ctgraham) When viewing plugins at the site level, the "enabled" checkbox doesn't do anything for context-dependent plugins. This should instead enable or disable the plugin as follows:
  • if the plugin is a Site-only plugin, enable or disable the plugin
  • if the plugin is a Site-and-Context plugin, enable the plugin for configuration by the Site and/or Contexts
  • if the plugin is a Context-only plugin, enable or disable the configuration of the plugin in the Contexts

[asmecher]

(Throwing at 3.0.2 for no particular reason)

[ctgraham]

Right now, I'm picturing the inheritance of plugin settings happening as follows:

• The plugin code via Plugin::isSitePlugin() defines whether it is locked to the context-specific level (false), locked to the site level (true), or whether it can be scoped to the site level and the context-specific level ( integer constant evaluating to true ).
• In the plugin's site-level settings, a new boolean setting value will indicate whether a site administrator permits the managers to customize the settings at the context-specific level.
• In the plugin's context-level settings, if the plugin is enabled at the site level and if the site administrator has allowed plugin inheritance to the context level, a new boolean setting value will indicate whether the manager wishes to override the site level settings. This might be the same setting_name as the prior new setting?

What I'm imagining might pose UI/UX confusion. From a development perspective, it is easy to reuse the name of the new setting for both the site level and context-specific level settings, even though the meaning of the setting is a bit conflated:

plugin_name	context	setting_name	setting_value
ldap	0	enable_scoping	true
ldap	1	enable_scoping	false
ldap	3	enable_scoping	true

This would mean the site admin has allowed inheritance, and that contexts 1 and 2 use the site settings, and that context 3 has overridden the site settings.

The UI might similarly be easily structured where a checkbox for this new setting exists in both the site level settings, and in the context-specific level settings, but the meaning of the checkbox would vary.

At the site index, checking this means "allow this plugin to be inherited", at the context-specific level, this means "I'm entering settings to override the site settings".

Please comment with any design suggestions.

[ctgraham]

Where I'm currently headed with this is a new plugin setting "enable_site_config" with a English meaning of "Use Site Settings". In the context 0 (site context), if set this means that the plugin will settings at the context-specific level will be disallowed/ignored. If unset at the site context, this checkbox can be used in the journal/press/conference context to indicate that the plugin will use the setting from the site context instead of any context-specific settings.

plugin_name	context	setting_name	setting_value
ldap	0	enable_site_config	true
ldap	1	enable_site_config	false
ldap	3	enable_site_config	true

In this case, the LDAP plugin will only ever use site level settings. (Site config is set at the site level nothing can override it).

plugin_name	context	setting_name	setting_value
ldap	0	enable_site_config	false
ldap	1	enable_site_config	false
ldap	3	enable_site_config	true

In this case, the LDAP plugin will use site configuration settings at the site level (implicit), and from context 3 (by site config setting), but context 1 will use its own settings.

Thoughts, concerns?

[ctgraham]

@asmecher, why is getCurrentContextId() implemented only in LazyLoadPlugin and GatewayPlugin instead of in Plugin? Is there an architectural consideration why some subclasses of plugins might no have a context?

I'm tempted to migrate this function to Plugin, but unclear on the implications.

[asmecher]

@ctgraham, hmm, I don't really like that function as it's currently written -- it makes static use of the PKPRequest object, which is deprecated behavior. I would rather see getEnabled take a $contextId parameter or something, which would uncouple the request handling from the plugin infrastructure, though that would probably take some refactoring.

[ctgraham]

This is where I get a little lost in the new framework. How does a GridCellProvider know its current context for a call to something like $plugin->getEnabled()?

pkp-lib/controllers/grid/plugins/PluginGridCellProvider.inc.php

Line 50 in 0289101

$isEnabled = $plugin->getEnabled();

[asmecher]

This is where some refactoring might get persnickety. In my opinion we should be providing the $request object more consistently to the controller classes, including GridCellProviders etc., perhaps by passing them in right when those classes are instantiated and having them save a reference. Right now access by those objects to the instantiated $request is spotty.

[ctgraham]

Well, @asmecher, I'm committed to adding new functionality for plugins to understand their instantiated context better. I'm not going to refactor the whole controller framework, but could I start refactoring the pieces I touch (like the GridCellProvider). Do you have a preference if I start into this piecemeal vs. someone else actually approaching this from a full-picture perspective? If I left this to a comprehensive refactoring, I would continue to leverage the existing static use of PKPRequest.

[ctgraham]

@asmecher, can you comment on the direction of these pulls:
#2283
pkp/ojs#1251
?

[asmecher]

Rather than adding the $request object to so many calls, I was thinking it would be better to add $request as a parameter to the constructor of many of the component (grid) related classes. Since components are only used in responding to requests, I can't think of a case where that's not logical, and it would massively reduce the amount of parameter-chucking.

[ctgraham]

Ok. There are a ton of empty constructors:

function __construct() {
    parent::__construct();
}

If I refactor this, I'm inclined to delete them rather than change every one to:

function __construct($request) {
    parent::__construct($request);
}

Let inheritance do its job, and only override the constructor when needed, as:

	function __construct($request, $submission) {
		parent::__construct($request);
		$this->_submission = $submission;
	}

Thoughts?

[ctgraham]

@asmecher, let me know what you think of these commits:
ulsdevteam@64e090d
ulsdevteam/ojs@237da01

This drafts adding the PKPRequest to the constructor of GridCellProvider and subclasses. Identifying how deep to go into GridHandler and CellProvider is TBD.

Particular questions/concerns would include:

• This not backwards compatible. The $request parameter could be added as an optional final parameter instead, but that leaves an truly ugly legacy. This means that changes would need to be coordinated within plugins as well. A backwards compatible strategy could politely throw warnings rather than failing hard.
• Repeat of the question above on removing useless constructors.
• I replaced any duplicated documentation I came across with @copydoc references to GridCellProvider. Perhaps this should have been a @copydoc reference to the parent instead, depending on personal preference.

[ctgraham]

Bump: @asmecher

[asmecher]

I think breaking backwards compatibility is justified in this case, nasty as it is. An attempt to make this accept either behavior gracefully would be a cure worse than the disease.

Your take on @copydoc is fine.

Please nuke the unnecessary constructors, thanks -- they're worse than useless!

[asmecher]

@ctgraham, just FYI: with OMP 3.1 out and OJS 3.1 a few months out, I'm making some related compatibility-breaking changes -- see e.g. #2336. Shall we see if we can get this bashed into shape and merged sometime in the next week? Let me know where it's at and what your time looks like, and I'll fill in the rest.

[bozana]

@ctgraham, would it be maybe also possible to consider just some of the plugin settings on context-level in the similar way? -- For example, the statistics display on the article page. -- The usage statistics plugin is site-wide and it is good that most of the settings (e.g. the statistics log format) are site-wide, but it would be nice for journals to configure the display/chart on their own.

[ctgraham]

@asmecher, I will be focused early this week on fixing #2327 and working some data cleanup that accompanies it and http://forum.pkp.sfu.ca/t/keyword-indexing-for-word-documents-doc-docx/29040. I may get back to this refactoring Wednesday and/or Thursday, then will be out of office for a few days. I haven't done anything beyond searching for usages of GridCellProvider instances as per the above commits.

[ctgraham]

@bozana, interesting consideration. I wonder what practically this would look like. Individual setting overrides would be a UI/UX nightmare, I suspect; so perhaps groups of settings? But if we group settings and functionality together, doesn't that make for a separate plugin?

[asmecher]

Thanks, @ctgraham. Maybe with your permission I'll run with the $request stuff in constructors and merge it this week?

[ctgraham]

Permission happily granted.

[bozana]

@ctgraham, yes, I think you are right...

[ctgraham]

@asmecher, unless I'm misreading the current state of master, I don't think the work on adding the $request as a parameter to the GridCellProvider made it into one of your refactoring changes. I'm planning on opening a different issue and PR for this, as a blocker for this issue. Let me know if I've misread your comment from Mar 6.

[asmecher]

I don't think the work on adding...

Yes, thanks -- I fell into a parallel but distinct rabbit hole.

[ctgraham]

PluginAccessPolicy currently makes a hard distinction on the boolean of PKPPlugin::isSitePlugin():

pkp-lib/classes/security/authorization/PluginAccessPolicy.inc.php

Lines 45 to 74 in f8d959b

	if (isset($roleAssignments[ROLE_ID_MANAGER])) {
	if ($accessMode & ACCESS_MODE_MANAGE) {
	// Managers have edit settings access mode...
	$managerPluginAccessPolicy = new PolicySet(COMBINING_DENY_OVERRIDES);
	$managerPluginAccessPolicy->addPolicy(new RoleBasedHandlerOperationPolicy($request, ROLE_ID_MANAGER, $roleAssignments[ROLE_ID_MANAGER]));
	// ...only to context-level plugins.
	$managerPluginAccessPolicy->addPolicy(new PluginLevelRequiredPolicy($request, true));
	$pluginAccessPolicy->addPolicy($managerPluginAccessPolicy);
	}
	}
	//
	// Site administrator role
	//
	if (isset($roleAssignments[ROLE_ID_SITE_ADMIN])) {
	// Site admin have access to all plugins...
	$siteAdminPluginAccessPolicy = new PolicySet(COMBINING_DENY_OVERRIDES);
	$siteAdminPluginAccessPolicy->addPolicy(new RoleBasedHandlerOperationPolicy($request, ROLE_ID_SITE_ADMIN, $roleAssignments[ROLE_ID_SITE_ADMIN]));
	if ($accessMode & ACCESS_MODE_MANAGE) {
	// ...of site level only.
	$siteAdminPluginAccessPolicy->addPolicy(new PluginLevelRequiredPolicy($request, false));
	}
	$pluginAccessPolicy->addPolicy($siteAdminPluginAccessPolicy);
	}
	$this->addPolicy($pluginAccessPolicy);

based on PluginLevelRequiredPolicy:

pkp-lib/classes/security/authorization/internal/PluginLevelRequiredPolicy.inc.php

Lines 44 to 47 in f8d959b

	if (!$this->_contextPresent) { // Site context
	return $plugin->isSitePlugin()?AUTHORIZATION_PERMIT:AUTHORIZATION_DENY;
	}
	return $plugin->isSitePlugin()?AUTHORIZATION_DENY:AUTHORIZATION_PERMIT;

This policy framework will need to be reworked in order to implement site/context inheriting/overriding plugins (or even just plugins that work in both a site and context, like Google Analytics).

[NateWr]

The usage stats plugin does something like this with different settings available at context/site level. See #6351.