kolibri icon indicating copy to clipboard operation
kolibri copied to clipboard

By default don't initialize hooks when enabling and disabling plugins programmatically

Open rtibbles opened this issue 3 years ago • 1 comments

Summary

  • To do a full disable or enabling of a plugin, we also register all the hooks.
  • This is appropriate from the CLI as it gives us warning of whether a hook registration does forbidden calls and initialization
  • When enabling and disabling plugins programmatically, it can prematurely register a hook within the life cycle of an application using Kolibri
  • Adds an extra argument to toggle hook initialization, which defaults to True
  • Turns it off by default for the enable_plugin and disable_plugin functions
  • Turns it on by default for the CLI invocation of enable_plugin

References

Fixes https://github.com/learningequality/kolibri/issues/9696

Reviewer guidance

Open a python shell and do the following:

from kolibri.core.hooks import RoleBasedRedirectHook
print(list(RoleBasedRedirectHook.registered_hooks))

from kolibri.main import disable_plugin
disable_plugin("kolibri.plugins.learn")

print(list(RoleBasedRedirectHook.registered_hooks))

With this PR, both print statements will produce empty lists. Without it, the second will produce a list like this: [<kolibri.plugins.learn.kolibri_plugin.LearnRedirect at 0x7feaa74dd8d0>]


Testing checklist

  • [x] Contributor has fully tested the PR manually
  • [ ] If there are any front-end changes, before/after screenshots are included
  • [ ] Critical user journeys are covered by Gherkin stories
  • [ ] Critical and brittle code paths are covered by unit tests

PR process

  • [x] PR has the correct target branch and milestone
  • [x] PR has 'needs review' or 'work-in-progress' label
  • [x] If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • [ ] If this is an important user-facing change, PR or related issue has a 'changelog' label
  • [ ] If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

rtibbles avatar Sep 23 '22 15:09 rtibbles