From 370e678ae41da49693ad5cfe10c23aba371e87c1 Mon Sep 17 00:00:00 2001
From: Slavek Kabrda <bkabrda@redhat.com>
Date: Nov 05 2018 15:40:20 +0000
Subject: [PATCH 1/2] Make it possible to create hooks that don't have DB entries. Fixes #3972


---

diff --git a/pagure/hooks/__init__.py b/pagure/hooks/__init__.py
index 67212ae..cad4c94 100644
--- a/pagure/hooks/__init__.py
+++ b/pagure/hooks/__init__.py
@@ -145,6 +145,7 @@ class BaseHook(object):
     name = None
     form = None
     description = None
+    backref = None
     db_object = None
     # hook_type is not used in hooks that use a Runner class, as those can
     # implement run actions on whatever is useful to them.
@@ -259,6 +260,26 @@ class BaseHook(object):
         if not cls.runner:
             raise ValueError("BaseHook.remove called for runner-less hook")
 
+    @classmethod
+    def is_enabled_for(cls, project):
+        """ Determine if this hook should be run for given project.
+
+        On some Pagure instances, some hooks should be run on all projects
+        that fulfill certain criteria. It is therefore not necessary to keep
+        database objects for them.
+
+        If a hook's backref is set to None, this method is run to determine
+        whether the hook should be run or not. These hooks also won't show
+        up on settings page, since they can't be turned off.
+
+        :arg project: The project to inspect
+        :type project: pagure.lib.model.Project
+        :return: True if this hook should be run on the given project,
+            False otherwise
+
+        """
+        return False
+
 
 def run_project_hooks(
     session,
diff --git a/pagure/lib/plugins.py b/pagure/lib/plugins.py
index 09fb49c..6bd8944 100644
--- a/pagure/lib/plugins.py
+++ b/pagure/lib/plugins.py
@@ -15,8 +15,16 @@ from straight.plugin import load
 from pagure.lib.model_base import BASE
 
 
-def get_plugin_names(blacklist=None):
-    """ Return the list of plugins names. """
+def get_plugin_names(blacklist=None, without_backref=False):
+    """ Return the list of plugins names.
+
+    :arg blacklist: name or list of names to not return
+    :type blacklist: string or list of strings
+    :arg without_backref: whether or not to include hooks that
+        have backref "None"
+    :type without_backref: bool
+    :return: list of plugin names (strings)
+    """
     from pagure.hooks import BaseHook
 
     plugins = load("pagure.hooks", subclasses=BaseHook)
@@ -26,7 +34,8 @@ def get_plugin_names(blacklist=None):
         blacklist = [blacklist]
 
     output = [
-        plugin.name for plugin in plugins if plugin.name not in blacklist
+        plugin.name for plugin in plugins
+        if plugin.name not in blacklist and (plugin.backref or without_backref)
     ]
     # The default hook is not one we show
     if "default" in output:
@@ -69,9 +78,13 @@ def get_enabled_plugins(project, with_default=False):
     for plugin in load("pagure.hooks", subclasses=BaseHook):
         if plugin.name == "default":
             continue
-        plugin.db_object()
-        if hasattr(project, plugin.backref):
-            dbobj = getattr(project, plugin.backref)
-            if dbobj and dbobj.active:
-                enabled.append((plugin, dbobj))
+        if plugin.backref is None:
+            if plugin.is_enabled_for(project):
+                enabled.append((plugin, None))
+        else:
+            plugin.db_object()
+            if hasattr(project, plugin.backref):
+                dbobj = getattr(project, plugin.backref)
+                if dbobj and dbobj.active:
+                    enabled.append((plugin, dbobj))
     return enabled
diff --git a/tests/test_pagure_lib_plugins.py b/tests/test_pagure_lib_plugins.py
new file mode 100644
index 0000000..b6a174b
--- /dev/null
+++ b/tests/test_pagure_lib_plugins.py
@@ -0,0 +1,76 @@
+# coding=utf-8
+"""
+ (c) 2015-2018 - Copyright Red Hat Inc
+
+ Authors:
+   Slavek Kabrda <bkabrda@redhat.com>
+
+"""
+
+from __future__ import unicode_literals
+
+__requires__ = ['SQLAlchemy >= 0.8']
+import pkg_resources
+
+import os
+import sys
+
+from mock import patch
+
+sys.path.insert(0, os.path.join(os.path.dirname(
+    os.path.abspath(__file__)), '..'))
+
+import pagure.hooks
+import pagure.lib.plugins
+import tests
+
+
+class EnabledForAll(pagure.hooks.BaseHook):
+    name = "EnabledForAll"
+
+    @classmethod
+    def is_enabled_for(cls, project):
+        return True
+
+class DisabledForAll(pagure.hooks.BaseHook):
+    name = "DisabledForAll"
+    # disabled for all is the default
+
+class PagureLibtests_plugins(tests.Modeltests):
+    """
+    Test the pagure.lib.plugins module
+    """
+
+    maxDiff = None
+
+    @patch("pagure.lib.plugins.load")
+    def test_plugin_is_enabled_for(self, load):
+        """ Test the is_enabled_for method of plugins is properly
+        handled by pagure.lib.plugins.get_enabled_plugins.
+        """
+        tests.create_projects(self.session)
+        project = pagure.lib.query._get_project(self.session, "test")
+
+        load.return_value = [EnabledForAll]
+        self.assertEqual(
+            pagure.lib.plugins.get_enabled_plugins(project),
+            [(EnabledForAll, None)]
+        )
+
+        load.return_value = [DisabledForAll]
+        self.assertEqual(
+            pagure.lib.plugins.get_enabled_plugins(project),
+            []
+        )
+
+    @patch("pagure.lib.plugins.load")
+    def test_get_plugin_names(self, load):
+        """ Test the get_plugin_names method with plugins that don't
+        have backref.
+        """
+        load.return_value = [EnabledForAll]
+        self.assertEqual(pagure.lib.plugins.get_plugin_names(), [])
+        self.assertEqual(
+            pagure.lib.plugins.get_plugin_names(without_backref=True),
+            ['EnabledForAll']
+        )

From adbb5f9e45b1ba915222512a776dd77cae7b9c5f Mon Sep 17 00:00:00 2001
From: Slavek Kabrda <bkabrda@redhat.com>
Date: Nov 05 2018 15:56:07 +0000
Subject: [PATCH 2/2] Make the default hook be active all the time on all projects


---

diff --git a/pagure/hooks/__init__.py b/pagure/hooks/__init__.py
index cad4c94..4824074 100644
--- a/pagure/hooks/__init__.py
+++ b/pagure/hooks/__init__.py
@@ -373,7 +373,7 @@ def run_project_hooks(
 
     # Now we run the hooks for plugins
     haderrors = False
-    for plugin, _ in get_enabled_plugins(project, with_default=True):
+    for plugin, _ in get_enabled_plugins(project):
         if not plugin.runner:
             if debug:
                 print(
diff --git a/pagure/hooks/default.py b/pagure/hooks/default.py
index 14eb098..1623126 100644
--- a/pagure/hooks/default.py
+++ b/pagure/hooks/default.py
@@ -13,16 +13,7 @@ from __future__ import unicode_literals, print_function
 import logging
 
 import pygit2
-import sqlalchemy as sa
 import six
-import wtforms
-
-try:
-    from flask_wtf import FlaskForm
-except ImportError:
-    from flask_wtf import Form as FlaskForm
-from sqlalchemy.orm import relation
-from sqlalchemy.orm import backref
 
 import pagure.config
 import pagure.exceptions
@@ -31,43 +22,12 @@ import pagure.lib.tasks
 import pagure.lib.tasks_services
 import pagure.utils
 from pagure.hooks import BaseHook, BaseRunner
-from pagure.lib.model import BASE, Project
 
 
 _config = pagure.config.reload_config()
 _log = logging.getLogger(__name__)
 
 
-class DefaultTable(BASE):
-    """ Stores information about the default hook of a project.
-
-    Table -- hook_default
-    """
-
-    __tablename__ = "hook_default"
-
-    id = sa.Column(sa.Integer, primary_key=True)
-    project_id = sa.Column(
-        sa.Integer,
-        sa.ForeignKey("projects.id", onupdate="CASCADE", ondelete="CASCADE"),
-        nullable=False,
-        unique=True,
-        index=True,
-    )
-    active = sa.Column(sa.Boolean, nullable=False, default=False)
-
-    project = relation(
-        "Project",
-        remote_side=[Project.id],
-        backref=backref(
-            "default_hook",
-            cascade="delete, delete-orphan",
-            single_parent=True,
-            uselist=False,
-        ),
-    )
-
-
 def send_fedmsg_notifications(project, topic, msg):
     """ If the user asked for fedmsg notifications on commit, this will
     do it.
@@ -317,19 +277,6 @@ class DefaultRunner(BaseRunner):
         )
 
 
-class DefaultForm(FlaskForm):
-    """ Form to configure the default hook. """
-
-    active = wtforms.BooleanField("Active", [wtforms.validators.Optional()])
-
-    def __init__(self, *args, **kwargs):
-        """ Calls the default constructor with the normal argument but
-        uses the list of collection provided to fill the choices of the
-        drop-down list.
-        """
-        super(DefaultForm, self).__init__(*args, **kwargs)
-
-
 class Default(BaseHook):
     """ Default hooks. """
 
@@ -337,9 +284,8 @@ class Default(BaseHook):
     description = (
         "Default hooks that should be enabled for each and every project."
     )
-
-    form = DefaultForm
-    db_object = DefaultTable
-    backref = "default_hook"
-    form_fields = ["active"]
     runner = DefaultRunner
+
+    @classmethod
+    def is_enabled_for(cls, project):
+        return True
diff --git a/pagure/lib/plugins.py b/pagure/lib/plugins.py
index 6bd8944..f04f621 100644
--- a/pagure/lib/plugins.py
+++ b/pagure/lib/plugins.py
@@ -59,25 +59,18 @@ def get_plugin(plugin_name):
             return plugin
 
 
-def get_enabled_plugins(project, with_default=False):
+def get_enabled_plugins(project):
     """ Returns a list of plugins enabled for a specific project.
 
     Args:
         project (model.Project): The project to look for.
-        with_default (boolean): Wether or not the returned list should
-            include the default hook/plugin.
     Returns: (list): A  list of tuples (pluginclass, dbobj) with the plugin
         classess and dbobjects for plugins enabled for the project.
     """
     from pagure.hooks import BaseHook
-    from pagure.hooks.default import Default
 
     enabled = []
-    if with_default:
-        enabled = [(Default(), None)]
     for plugin in load("pagure.hooks", subclasses=BaseHook):
-        if plugin.name == "default":
-            continue
         if plugin.backref is None:
             if plugin.is_enabled_for(project):
                 enabled.append((plugin, None))
diff --git a/pagure/lib/tasks.py b/pagure/lib/tasks.py
index c8630c0..0977ca4 100644
--- a/pagure/lib/tasks.py
+++ b/pagure/lib/tasks.py
@@ -299,17 +299,6 @@ def create_project(
                 master_ref = temp_gitrepo.lookup_reference("HEAD").resolve()
                 tempclone.push("pagure", master_ref.name, internal="yes")
 
-            # Install the default hook
-            plugin = pagure.lib.plugins.get_plugin("default")
-            dbobj = plugin.db_object()
-            dbobj.active = True
-            dbobj.project_id = project.id
-            session.add(dbobj)
-            session.flush()
-            plugin.set_up(project)
-            plugin.install(project, dbobj)
-            session.commit()
-
     task = generate_gitolite_acls.delay(
         namespace=project.namespace,
         name=project.name,
diff --git a/tests/test_pagure_flask_ui_plugins_default_hook.py b/tests/test_pagure_flask_ui_plugins_default_hook.py
index 76c05a8..2c9eb6d 100644
--- a/tests/test_pagure_flask_ui_plugins_default_hook.py
+++ b/tests/test_pagure_flask_ui_plugins_default_hook.py
@@ -26,6 +26,8 @@ from mock import patch, MagicMock
 sys.path.insert(0, os.path.join(os.path.dirname(
     os.path.abspath(__file__)), '..'))
 
+import pagure.hooks.default
+import pagure.lib.plugins
 import pagure.lib.query
 import tests
 
@@ -33,116 +35,19 @@ import tests
 class PagureFlaskPluginDefaultHooktests(tests.Modeltests):
     """ Tests for default_hook plugin of pagure """
 
-    def test_plugin_default_ui(self):
-        """ Test the default hook plugin on/off endpoint. """
+    def test_plugin_default_active_on_project(self):
+        """ Test that the default hook is active on random project. """
 
         tests.create_projects(self.session)
-        tests.create_projects_git(os.path.join(self.path, 'repos'))
-
-        user = tests.FakeUser(username='pingou')
-        with tests.user_set(self.app.application, user):
-            output = self.app.get('/test/settings/default')
-            self.assertEqual(output.status_code, 403)
-
-    def test_plugin_default_install(self):
-        """ Check that the default plugin is correctly installed when a
-        project is created.
-        """
-
-        task = pagure.lib.query.new_project(
-            self.session,
-            user='pingou',
-            name='test',
-            repospanner_region=None,
-            blacklist=[],
-            allowed_prefix=[],
-            description=None,
-            url=None, avatar_email=None,
-            parent_id=None,
-            add_readme=False,
-            userobj=None,
-            prevent_40_chars=False,
-            namespace=None
+        test = pagure.lib.query.search_projects(self.session)[0]
+        self.assertIsNone(pagure.hooks.default.Default.backref)
+        self.assertTrue(
+            pagure.hooks.default.Default.is_enabled_for(test)
         )
-        self.assertEqual(task.get(),
-                         {'endpoint': 'ui_ns.view_repo',
-                          'repo': 'test',
-                          'namespace': None})
-
-        self.assertTrue(os.path.exists(os.path.join(
-            self.path, 'repos', 'test.git', 'hooks', 'post-receive')))
-
-    def test_plugin_default_remove(self):
-        """ Check that the default plugin can be correctly removed if
-        somehow managed.
-        """
-
-        task = pagure.lib.query.new_project(
-            self.session,
-            user='pingou',
-            name='test',
-            repospanner_region=None,
-            blacklist=[],
-            allowed_prefix=[],
-            description=None,
-            url=None, avatar_email=None,
-            parent_id=None,
-            add_readme=False,
-            userobj=None,
-            prevent_40_chars=False,
-            namespace=None
+        self.assertEqual(
+            [(pagure.hooks.default.Default, None)],
+            pagure.lib.plugins.get_enabled_plugins(test)
         )
-        self.assertEqual(task.get(),
-                         {'endpoint': 'ui_ns.view_repo',
-                          'repo': 'test',
-                          'namespace': None})
-
-        repo = pagure.lib.query.get_authorized_project(self.session, 'test')
-        plugin = pagure.lib.plugins.get_plugin('default')
-        dbobj = plugin.db_object()
-
-        plugin.remove(repo)
-
-        self.assertFalse(os.path.exists(os.path.join(
-            self.path, 'repos', 'test.git', 'hooks', 'post-receive.default')))
-        self.assertTrue(os.path.exists(os.path.join(
-            self.path, 'repos', 'test.git', 'hooks', 'post-receive')))
-
-    def test_plugin_default_form(self):
-        """ Check that the default plugin's form.
-        """
-        with self._app.test_request_context('/') as ctx:
-            flask.g.session = self.session
-            flask.g.fas_user = tests.FakeUser(username='foo')
-
-            task = pagure.lib.query.new_project(
-                self.session,
-                user='pingou',
-                name='test',
-                repospanner_region=None,
-                blacklist=[],
-                allowed_prefix=[],
-                description=None,
-                url=None, avatar_email=None,
-                parent_id=None,
-                add_readme=False,
-                userobj=None,
-                prevent_40_chars=False,
-                namespace=None
-            )
-            self.assertEqual(task.get(),
-                             {'endpoint': 'ui_ns.view_repo',
-                              'repo': 'test',
-                              'namespace': None})
-
-            repo = pagure.lib.query.get_authorized_project(self.session, 'test')
-            plugin = pagure.lib.plugins.get_plugin('default')
-            dbobj = plugin.db_object()
-            form = plugin.form(obj=dbobj)
-            self.assertEqual(
-                str(form.active),
-                '<input id="active" name="active" type="checkbox" value="y">'
-            )
 
 
 if __name__ == '__main__':

