Clarify the implementation of "Site" plugins #8115

ctgraham · 2016-10-27T18:14:57Z

ctgraham
Oct 27, 2016

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 · 2016-10-28T22:12:53Z

asmecher
Oct 28, 2016
Maintainer

(Throwing at 3.0.2 for no particular reason)

0 replies

ctgraham · 2016-11-10T19:04:25Z

ctgraham
Nov 10, 2016
Author

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.

0 replies

ctgraham · 2016-11-15T17:43:05Z

ctgraham
Nov 15, 2016
Author

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?

0 replies

ctgraham · 2017-02-07T18:43:52Z

ctgraham
Feb 7, 2017
Author

@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.

0 replies

asmecher · 2017-02-07T18:50:51Z

asmecher
Feb 7, 2017
Maintainer

@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.

0 replies

ctgraham · 2017-02-07T18:58:15Z

ctgraham
Feb 7, 2017
Author

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();

0 replies

asmecher · 2017-02-08T21:45:33Z

asmecher
Feb 8, 2017
Maintainer

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.

0 replies

ctgraham · 2017-02-09T19:33:27Z

ctgraham
Feb 9, 2017
Author

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.

0 replies

ctgraham · 2017-02-21T17:50:23Z

ctgraham
Feb 21, 2017
Author

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

0 replies

asmecher · 2017-02-21T18:12:38Z

asmecher
Feb 21, 2017
Maintainer

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.

0 replies

ctgraham · 2017-02-21T21:06:36Z

ctgraham
Feb 21, 2017
Author

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?

0 replies

ctgraham · 2017-02-22T20:19:02Z

ctgraham
Feb 22, 2017
Author

@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.

0 replies

ctgraham · 2017-02-27T15:49:52Z

ctgraham
Feb 27, 2017
Author

Bump: @asmecher

0 replies

asmecher · 2017-02-27T21:45:35Z

asmecher
Feb 27, 2017
Maintainer

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!

0 replies

asmecher · 2017-03-04T00:58:42Z

asmecher
Mar 4, 2017
Maintainer

@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.

0 replies

bozana · 2017-03-06T13:00:33Z

bozana
Mar 6, 2017
Maintainer

@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.

0 replies

ctgraham · 2017-03-06T14:18:13Z

ctgraham
Mar 6, 2017
Author

@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.

0 replies

ctgraham · 2017-03-06T14:33:07Z

ctgraham
Mar 6, 2017
Author

@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?

0 replies

asmecher · 2017-03-06T16:51:43Z

asmecher
Mar 6, 2017
Maintainer

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

0 replies

ctgraham · 2017-03-06T17:04:12Z

ctgraham
Mar 6, 2017
Author

Permission happily granted.

0 replies

bozana · 2017-03-07T16:38:51Z

bozana
Mar 7, 2017
Maintainer

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

0 replies

ctgraham · 2017-04-14T14:37:08Z

ctgraham
10000 Apr 14, 2017
Author

@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.

0 replies

asmecher · 2017-04-21T18:12:16Z

asmecher
Apr 21, 2017
Maintainer

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

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

0 replies

ctgraham · 2019-02-11T21:04:11Z

ctgraham
Feb 11, 2019
Author

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).

0 replies

NateWr · 2020-11-27T13:10:24Z

NateWr
Nov 27, 2020

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

0 replies

Clarify the implementation of "Site" plugins #8115

Uh oh!

ctgraham Oct 27, 2016

Replies: 25 comments

Uh oh!

asmecher Oct 28, 2016 Maintainer

Uh oh!

ctgraham Nov 10, 2016 Author

Uh oh!

ctgraham Nov 15, 2016 Author

Uh oh!

Uh oh!

ctgraham Feb 7, 2017 Author

Uh oh!

asmecher Feb 7, 2017 Maintainer

Uh oh!

ctgraham Feb 7, 2017 Author

Uh oh!

asmecher Feb 8, 2017 Maintainer

Uh oh!

ctgraham Feb 9, 2017 Author

Uh oh!

ctgraham Feb 21, 2017 Author

Uh oh!

asmecher Feb 21, 2017 Maintainer

Uh oh!

ctgraham Feb 21, 2017 Author

Uh oh!

ctgraham Feb 22, 2017 Author

Uh oh!

ctgraham Feb 27, 2017 Author

Uh oh!

asmecher Feb 27, 2017 Maintainer

Uh oh!

asmecher Mar 4, 2017 Maintainer

Uh oh!

bozana Mar 6, 2017 Maintainer

Uh oh!

ctgraham Mar 6, 2017 Author

Uh oh!

ctgraham Mar 6, 2017 Author

Uh oh!

asmecher Mar 6, 2017 Maintainer

Uh oh!

ctgraham Mar 6, 2017 Author

Uh oh!

bozana Mar 7, 2017 Maintainer

Uh oh!

ctgraham 10000 Apr 14, 2017 Author

Uh oh!

asmecher Apr 21, 2017 Maintainer

Uh oh!

ctgraham Feb 11, 2019 Author

Uh oh!

NateWr Nov 27, 2020

ctgraham
Oct 27, 2016

asmecher
Oct 28, 2016
Maintainer

ctgraham
Nov 10, 2016
Author

ctgraham
Nov 15, 2016
Author

ctgraham
Feb 7, 2017
Author

asmecher
Feb 7, 2017
Maintainer

ctgraham
Feb 7, 2017
Author

asmecher
Feb 8, 2017
Maintainer

ctgraham
Feb 9, 2017
Author

ctgraham
Feb 21, 2017
Author

asmecher
Feb 21, 2017
Maintainer

ctgraham
Feb 21, 2017
Author

ctgraham
Feb 22, 2017
Author

ctgraham
Feb 27, 2017
Author

asmecher
Feb 27, 2017
Maintainer

asmecher
Mar 4, 2017
Maintainer

bozana
Mar 6, 2017
Maintainer

ctgraham
Mar 6, 2017
Author

ctgraham
Mar 6, 2017
Author

asmecher
Mar 6, 2017
Maintainer

ctgraham
Mar 6, 2017
Author

bozana
Mar 7, 2017
Maintainer

ctgraham
10000 Apr 14, 2017
Author

asmecher
Apr 21, 2017
Maintainer

ctgraham
Feb 11, 2019
Author

NateWr
Nov 27, 2020