learnweb / moodle-tool_lifecycle

:recycle: Extensible Moodle plugin for managing course life cycles, e.g., deprovisioning
GNU General Public License v3.0
21 stars 32 forks source link

Improving sub plugins design #178

Open dmitriim opened 1 year ago

dmitriim commented 1 year ago

Rather than having sub-plugins (trigger, steps) committed to the plugin, I'd like to propose changing the design of managing subplugins so other plugins can add triggers, steps without actually code being committed to this plugin.

This can be implemented by using callbacks or relying on a class names. So the main plugin becomes more like API for others to extend it. This doesn't limit to have bunch of useful "subplugins" inside the main plugin and follow the same design. E.g. see that example https://github.com/catalyst/moodle-tool_dataflows/blob/MOODLE_35_STABLE/classes/manager.php#L36

Basically nothing should be changed for the end users and existing functionality should work exactly as it does now.

Pros:

Cons:

NinaHerrmann commented 1 year ago

I don´t really see the improvement, triggers and steps can have subplugins - if you want to implement it. What do you want to implement? :thinking:

dmitriim commented 1 year ago

sorry @NinaHerrmann I should be more specific.

Currently triggers and steps must be in the folders defined in db/subplugins.json https://github.com/learnweb/moodle-tool_lifecycle/blob/master/db/subplugins.json#L3 This is ok and this is how Moodle designed suplugins. However, this approach got few disadvantages. In particular when you want to have a functionality very specific for being part of that plugin (will never be accepted as a pull request). If that happens developers have to fork lifecycle plugin, implement their changes in the fork and then support forked version rather than spend the same effort on supporting main lifecycle plugin.

There is a way to avoid this. It's getting commonly used recently across different plugins (a callback workaround is in the initial description).

So the improvement here would be changing a way how tool_lifecycle works with subplugins. In particular how subplugins are getting discoverer. After the change, there will be no need for suplugins to be in the paths described in db/subplugins.json, but any plugin will be able to implement a trigger or a step functionality by implementing a callback and returning a list of expected classes.

That will help keeping very specific (not suitable for merging) functionality outside of the plugin.

The initial intention for me proposing this change is trying to avoid having multiple plugins doing the same thing. Currently we have few big clients looking at the archiving functionality for their fat moodle instances. We could start a dev work from scratch and implement a nice new plugin that would do everything that tool_lifecycle does. But we strongly believe that a better approach would be investing in the existing tool_lifecycle that does 80% of what's we require, rather than have multiple clones of the same functionality. However, there is a blocker in the current design of tool_lifecycle that could slow us down, so we are proposing this change.

Hope that all make sense.

dmitriim commented 1 year ago

What do you want to implement?

Unfortunately, I don't have all requirements at the moment as it's early stages of scoping. But from the experience I can tell that 100% there will be something that will never be accepted as PR due to being very specific. Also you may not want to have all that new stuff in the plugin as you will need to support all that code.

NinaHerrmann commented 1 year ago

Hey @dmitriim,

I guess you can not say what specific logic you want to implement but you have to be more precise to explain why this functionality can not be expressed as a step or trigger. Is it some recursive call? Is it some if/else logic? If you do not want to make that public we can make a short call.

dmitriim commented 1 year ago

I think to continue with this we actually should draft a PR to demonstrate what we mean with this change. Will see if we could get one done in the next few weeks.

NinaHerrmann commented 1 year ago

Yes and no, we want to go into the use cases before we merge pull requests.

tuanngocnguyen commented 1 year ago

Hi @dmitriim @NinaHerrmann ,

I have created a PR for this issue: https://github.com/learnweb/moodle-tool_lifecycle/pull/189/files Please let me know your opinion on the patch.

tuanngocnguyen commented 1 year ago

Hi @NinaHerrmann

An use case I can think of:

Client has an external system (beside Moodle), that determines when a course should be backed up/deleted. This can be solved by creating new trigger plugin. However, the implementation is specific to that client, and they don't want to publish the plugin. We can add this plugin in subplugin folder ('trigger'), but since we are using 'git' to manage our code, we cannot add new trigger as a 'git submodule' without changing lifecycle code base. With the patch, we can do so, hence easier to maintain.

Regards

brendanheywood commented 1 month ago

hi @NinaHerrmann I've tried to flesh this out into a typical complex scenario to illustrate why we feel this is a better architecture that we've used with success in a few other plugins. Lets say we have a large uni client who has bespoke needs and they require the implementation of 2 custom triggers (lets say fullmoon, easter) and 2 custom steps (sing, dance). These will always be private bits of code whether its due to licensing or just because the code doesn't have general merit and so isn't worth open sourcing. Even if we did open source it lets assume that it's not going to land in tool_lifecycle itself.

Under the current architecture you would need to do is add 4 sub plugins like this:

classDiagram
Moodle <-- tool_lifecycle
tool_lifecycle <-- lifecycletrigger_fullmoon
tool_lifecycle <-- lifecycletrigger_easter
tool_lifecycle <-- lifecyclestep_sing
tool_lifecycle <-- lifecyclestep_dance

And lets say that those new steps are intrinsically part of an integration with some other system which has an existing moodle plugins, eg local_mystudentsystem. So now we have 1 existing plugin, plus 4 new plugins which exist solely to extend tool_lifecycle. The 4 step and trigger plugins typically don't do much and are just thin wrappers around code which exists in local_mystudentsystem. All of these plugins have a lot of boilerplate files and typically would end up in their own git repo if the general moodle conventions are followed.

classDiagram
Moodle <-- tool_lifecycle
tool_lifecycle <-- lifecycletrigger_fullmoon
tool_lifecycle <-- lifecycletrigger_easter
tool_lifecycle <-- lifecyclestep_sing
tool_lifecycle <-- lifecyclestep_dance
Moodle <-- local_mystudentsystem
lifecycletrigger_fullmoon ..> local_mystudentsystem
lifecycletrigger_easter ..> local_mystudentsystem
lifecyclestep_sing ..> local_mystudentsystem
lifecyclestep_dance ..> local_mystudentsystem

What we are proposing instead is that any plugin at all can implement a step or trigger class, or multiple new classes and dynamically register them with tool_lifecycle. This way all of that custom logic can live inside a single plugin which dramatically simplifies the way the code is written and keeps it all in one place. It also reduces the boilerplate for adding another step.

classDiagram
Moodle <-- tool_lifecycle
Moodle <-- local_mystudentsystem
local_mystudentsystem ..> tool_lifecycle
class local_mystudentsystem {
  trigger fullmoon
  trigger easter
  step sing
  step dance
}

Probably the best example is our https://github.com/catalyst/moodle-tool_dataflows/ which is a general purpose work flow engine and conceptually has a lot in common with tool_lifecycle. Any 3rd party plugin can extend a dataflow with a trigger, or a step. Here are the built in steps:

https://github.com/catalyst/moodle-tool_dataflows/tree/MOODLE_401_STABLE/classes/local/step

and then a 3rd party plugin can just write 1 or more classes for custom, and register them and you don't need the extra plugins.

For example dataflows has a built s3 copy step, which has a dependency on moodle-local_aws, so this step could have been implemented inside https://github.com/catalyst/moodle-local_aws/ instead, and the only reason it wasn't is because the history of how the plugins were implemented.

https://github.com/catalyst/moodle-local_aws/

dmitriim commented 1 month ago

Thanks @brendanheywood ! Another example of a similar approach is Dynamic cohorts plugin https://github.com/catalyst/moodle-tool_dynamic_cohorts?tab=readme-ov-file#technical-details Where conditions can be implemented by any other plugin as long as they follow requirements.