Add first unit tests for scm_request_processor

Currently process, process_ticket and accepts_topic are covered.

Signed-off-by: Michal Konečný <mkonecny@redhat.com>

tests/plugins/test_scm_request_processor.py

@@ -0,0 +1,350 @@
+"""
+Unit tests for `toddlers.plugins.scm_request_processor`
+"""
+import json
+from unittest.mock import call, patch, Mock
+import logging
+
+from pagure_messages.issue_schema import IssueNewV1
+import pytest
+
+import toddlers.plugins.scm_request_processor as scm_request_processor
+from toddlers.exceptions import ValidationError
+
+class TestAcceptsTopic:
+    """
+    Test class for `toddlers.plugins.scm_request_processor.SCMRequestProcessor.accepts_topic` method.
+    """
+    toddler_cls = scm_request_processor.SCMRequestProcessor
+
+    def test_accetps_topic_invalid(self, toddler):
+        """
+        Assert that invalid topic is not accepted.
+        """
+        assert toddler.accepts_topic("foo.bar") is False
+
+    @pytest.mark.parametrize(
+        "topic",
+        [
+            "org.fedoraproject.*.pagure.issue.new",
+            "org.fedoraproject.*.pagure.issue.edit",
+            "org.fedoraproject.stg.pagure.issue.new",
+            "org.fedoraproject.stg.pagure.issue.edit",
+            "org.fedoraproject.prod.pagure.issue.new",
+            "org.fedoraproject.prod.pagure.issue.edit",
+        ]
+    )
+    def test_accetps_topic_valid(self, topic, toddler):
+        """
+        Assert that valid topics are accepted.
+        """
+        assert toddler.accepts_topic(topic)
+
+
+class TestProcess:
+    """
+    Test class for `toddlers.plugins.scm_request_processor.SCMRequestProcessor.process` method.
+    """
+    toddler_cls = scm_request_processor.SCMRequestProcessor
+
+    def test_process_invalid_project(self, caplog, toddler):
+        """
+        Assert that messages from other projects than fedora_scm_requests will be skipped.
+        """
+        caplog.set_level(logging.INFO)
+
+        msg = IssueNewV1()
+        msg.body = {
+            "project": {
+                "fullname": "foo/bar"
+            }
+        }
+
+        with patch(
+                "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_ticket"
+        ) as mock_process_ticket:
+            toddler.process({}, msg)
+
+            mock_process_ticket.assert_not_called()
+
+        assert (
+            caplog.records[-1].message ==
+            "The message doesn't belong to project releng/fedora-scm-requests. Skipping message."
+        )
+
+    def test_process_issue_not_open(self, caplog, toddler):
+        """
+        Assert that messages with closed issues will be skipped.
+        """
+        caplog.set_level(logging.INFO)
+
+        msg = IssueNewV1()
+        msg.body = {
+            "project": {
+                "fullname": scm_request_processor.PROJECT_NAMESPACE
+            },
+            "issue": {
+                "id": 100,
+                "close_status": "Closed"
+            }
+        }
+
+        with patch(
+                "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_ticket"
+        ) as mock_process_ticket:
+            toddler.process({}, msg)
+
+            mock_process_ticket.assert_not_called()
+
+        assert (
+            caplog.records[-1].message ==
+            "The issue 100 is not open. Skipping message."
+        )
+
+    @patch("toddlers.utils.pdc.set_pdc")
+    @patch("toddlers.utils.pagure.set_pagure")
+    @patch("toddlers.utils.fedora_account.set_fasjson")
+    @patch("toddlers.utils.bugzilla_system.set_bz")
+    def test_process(self, mock_bugzilla, mock_fasjson, mock_pagure, mock_pdc, toddler):
+        """
+        Assert that message toddler will be initialized correctly, if message passes
+        initial processing.
+        """
+        msg = IssueNewV1()
+        issue = {
+            "id": 100,
+            "close_status": "Open"
+        }
+        msg.body = {
+            "project": {
+                "fullname": scm_request_processor.PROJECT_NAMESPACE
+            },
+            "issue": issue
+        }
+        config = {
+            "branch_slas": {},
+            "monitoring_choices": [],
+            "pagure_namespace_to_component": {},
+            "pagure_namespace_to_product": {},
+            "temp_dir": "",
+            "dist_git_url": "https://src.fedoraproject.org",
+            "dist_git_token": "Private API Key"
+        }
+
+        with patch(
+                "toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_ticket"
+        ) as mock_process_ticket:
+            toddler.process(config, msg)
+
+            mock_process_ticket.assert_called_with(issue)
+
+        mock_pdc.assert_called_with(config)
+        mock_pagure.assert_has_calls([
+            call(config),
+            call({
+                "pagure_url": "https://src.fedoraproject.org",
+                "pagure_api_key": "Private API Key"
+            })
+        ])
+        mock_fasjson.assert_called_with(config)
+        mock_bugzilla.assert_called_with(config)
+
+
+class TestProcessTicket:
+    """
+    Test class for `toddlers.plugins.scm_request_processor.SCMRequestProcessor.process_ticket` method.
+    """
+
+    def setup(self):
+        """
+        Initialize toddler.
+        """
+        self.toddler = scm_request_processor.SCMRequestProcessor()
+        self.toddler.pagure_io = Mock()
+        self.toddler.dist_git = Mock()
+
+    def test_process_ticket_invalid_json(self):
+        """
+        Assert that invalid json in issue will end the processing.
+        """
+        issue = {
+            "id": 100,
+            "content": "invalid JSON",
+            "full_url": "https://blacklibrary.wh40k"
+        }
+        self.toddler.process_ticket(issue)
+
+        self.toddler.pagure_io.close_issue.assert_called_with(
+            100, namespace=scm_request_processor.PROJECT_NAMESPACE,
+            message="Invalid JSON provided",
+            reason="Invalid"
+        )
+
+    def test_process_ticket_invalid_slas(self):
+        """
+        Assert that invalid SLAs in issue will end the processing.
+        """
+        content = {
+            "sls": {},
+            "branch": "branch",
+        }
+        issue = {
+            "id": 100,
+            "content": json.dumps(content),
+            "full_url": "https://blacklibrary.wh40k"
+        }
+        with patch(
+                "toddlers.plugins.scm_request_processor.SCMRequestProcessor.verify_slas"
+        ) as mock_verify_slas:
+            mock_verify_slas.side_effect = ValidationError("error")
+            self.toddler.process_ticket(issue)
+
+            mock_verify_slas.assert_called_with("branch", {})
+
+        self.toddler.pagure_io.close_issue.assert_called_with(
+            100, namespace=scm_request_processor.PROJECT_NAMESPACE,
+            message="error",
+            reason="Invalid"
+        )
+
+    def test_process_ticket_missing_action(self):
+        """
+        Assert that missing action in issue will end the processing.
+        """
+        content = {
+            "sls": {},
+            "branch": "branch",
+        }
+        issue = {
+            "id": 100,
+            "content": json.dumps(content),
+            "full_url": "https://blacklibrary.wh40k"
+        }
+        self.toddler.process_ticket(issue)
+
+        self.toddler.pagure_io.close_issue.assert_called_with(
+            100, namespace=scm_request_processor.PROJECT_NAMESPACE,
+            message="Invalid or missing action field",
+            reason="Invalid"
+        )
+
+    def test_process_ticket_missing_sla(self):
+        """
+        Assert that missing SLA for branch will end the processing.
+        """
+        content = {
+            "branch": "branch",
+            "action": "new_repo"
+        }
+        issue = {
+            "id": 100,
+            "content": json.dumps(content),
+            "full_url": "https://blacklibrary.wh40k"
+        }
+        self.toddler.process_ticket(issue)
+
+        self.toddler.pagure_io.close_issue.assert_called_with(
+            100, namespace=scm_request_processor.PROJECT_NAMESPACE,
+            message="Couldn't find standard SLA for branch 'branch'",
+            reason="Invalid"
+        )
+
+    def test_process_ticket_invalid_branch_name(self):
+        """
+        Assert that invalid name for branch in specific namespace will end the processing.
+        """
+        content = {
+            "branch": "branch/",
+            "action": "new_repo",
+            "namespace": "flatpaks"
+        }
+        issue = {
+            "id": 100,
+            "content": json.dumps(content),
+            "full_url": "https://blacklibrary.wh40k"
+        }
+        self.toddler.branch_slas = {"branch/": "SLA"}
+
+        self.toddler.process_ticket(issue)
+
+        self.toddler.pagure_io.close_issue.assert_called_with(
+            100, namespace=scm_request_processor.PROJECT_NAMESPACE,
+            message=("Only characters, numbers, periods, dashes, underscores, "
+                     "and pluses are allowed in flatpak branch names"),
+            reason="Invalid"
+        )
+
+    def test_process_ticket_invalid_monitoring_setting(self):
+        """
+        Assert that invalid monitoring setting for repo will end the processing.
+        """
+        content = {
+            "branch": "branch",
+            "action": "new_repo",
+            "namespace": "flatpaks",
+            "monitor": "monitor"
+        }
+        issue = {
+            "id": 100,
+            "content": json.dumps(content),
+            "full_url": "https://blacklibrary.wh40k"
+        }
+        self.toddler.branch_slas = {"branch": "SLA"}
+
+        self.toddler.process_ticket(issue)
+
+        self.toddler.pagure_io.close_issue.assert_called_with(
+            100, namespace=scm_request_processor.PROJECT_NAMESPACE,
+            message='The monitor choice of "monitor" is invalid',
+            reason="Invalid"
+        )
+
+    def test_process_ticket_action_new_repo(self):
+        """
+        Assert that action new_repo is correctly processed.
+        """
+        content = {
+            "branch": "branch",
+            "action": "new_repo",
+        }
+        issue = {
+            "id": 100,
+            "content": json.dumps(content),
+            "full_url": "https://blacklibrary.wh40k"
+        }
+        self.toddler.branch_slas = {"branch": "SLA"}
+
+        with patch(
+                "toddlers.plugins.scm_request_processor.SCMRequestProcessor.create_new_repo"
+        ) as mock_new_repo:
+            self.toddler.process_ticket(issue)
+
+            content["sls"] = "SLA"
+            mock_new_repo.assert_called_with(
+                issue, content, initial_commit=True,
+            )
+
+    def test_process_ticket_action_new_branch(self):
+        """
+        Assert that action new_branch is correctly processed.
+        """
+        content = {
+            "branch": "branch",
+            "action": "new_branch",
+        }
+        issue = {
+            "id": 100,
+            "content": json.dumps(content),
+            "full_url": "https://blacklibrary.wh40k"
+        }
+        self.toddler.branch_slas = {"branch": "SLA"}
+
+        with patch(
+                "toddlers.plugins.scm_request_processor.SCMRequestProcessor.create_new_branch"
+        ) as mock_new_branch:
+            self.toddler.process_ticket(issue)
+
+            content["sls"] = "SLA"
+            mock_new_branch.assert_called_with(
+                issue, content,
+            )

toddlers/plugins/scm_request_processor.py

@@ -7,15 +7,17 @@ Authors:    Michal Konecny <mkonecny@redhat.com>
 
 """
 import fnmatch
+import json
 import logging
 import re
 from tempfile import TemporaryDirectory
+from typing import Optional
 
 import arrow
 from pagure_messages.issue_schema import IssueNewV1
 
 from toddlers.base import ToddlerBase
-from toddlers.utils import bugzilla_system, fedora_account, pagure, pdc, git
+from toddlers.utils import bugzilla_system, fedora_account, git, pagure, pdc, requests
 from toddlers.exceptions import ValidationError
 
 # Regex for branch name validation
@@ -72,6 +74,9 @@ class SCMRequestProcessor(ToddlerBase):
     # Path to temporary dir
     temp_dir = None
 
+    # Requests session
+    requests_session = None
+
     def accepts_topic(self, topic: str) -> bool:
         """Returns a boolean whether this toddler is interested in messages
         from this specific topic.
@@ -98,11 +103,11 @@ class SCMRequestProcessor(ToddlerBase):
         :arg config: Toddlers configuration
         :arg message: Message to process
         """
-        project_name = message.body["project"]["name"]
+        project_name = message.body["project"]["fullname"]
 
-        if project_name is not PROJECT_NAME:
+        if project_name is not PROJECT_NAMESPACE:
             _log.info(
-                "The message doesn't belong to project {0}. Skipping message.".format(PROJECT_NAME)
+                "The message doesn't belong to project {0}. Skipping message.".format(PROJECT_NAMESPACE)
             )
             return
 
@@ -110,7 +115,7 @@ class SCMRequestProcessor(ToddlerBase):
 
         if issue["close_status"] != "Open":
             _log.info(
-                "The issue {0} is closed. Skipping message.".format(issue["id"])
+                "The issue {0} is not open. Skipping message.".format(issue["id"])
             )
             return
 
@@ -119,6 +124,7 @@ class SCMRequestProcessor(ToddlerBase):
         self.pagure_namespace_to_component = config.get("pagure_namespace_to_component")
         self.pagure_namespace_to_product = config.get("pagure_namespace_to_product")
         self.temp_dir = config.get("temp_dir")
+        self.requests_session = requests.make_session()
 
         _log.info("Setting up PDC client")
         pdc.set_pdc(config)
@@ -160,11 +166,12 @@ class SCMRequestProcessor(ToddlerBase):
                 message="Invalid JSON provided",
                 reason="Invalid"
             )
+            return
 
         if 'sls' in issue_body:
             try:
                 # If a ValueError is raised, that means they aren't valid SLAs
-                log.info('- Verifying service-levels from the ticket.')
+                _log.info('- Verifying service-levels from the ticket.')
                 self.verify_slas(issue_body.get('branch', None), issue_body['sls'])
             except ValidationError as error:
                 _log.info("Couldn't verify SLAs. Closing '{0}'".format(issue["full_url"]))
@@ -174,6 +181,7 @@ class SCMRequestProcessor(ToddlerBase):
                     message=str(error),
                     reason="Invalid"
                 )
+                return
         else:
             branch = issue_body.get('branch', '').strip()
             if issue_body.get('action') in ['new_repo', 'new_branch'] and branch:
@@ -191,6 +199,7 @@ class SCMRequestProcessor(ToddlerBase):
                         message=error,
                         reason="Invalid"
                     )
+                    return
 
         if 'branch' in issue_body and 'namespace' in issue_body:
             branch = issue_body['branch']
@@ -219,6 +228,7 @@ class SCMRequestProcessor(ToddlerBase):
                 message=error_msg,
                 reason="Invalid"
             )
+            return
 
         if issue_body.get('action') == 'new_repo':
             self.create_new_repo(issue, issue_body,
@@ -314,12 +324,12 @@ class SCMRequestProcessor(ToddlerBase):
                     'characters long with only letters, numbers, hyphens, '
                     'underscores, plus signs, and/or periods. Please note that '
                     'the project cannot start with a period or a plus sign.')
-                self.pagure_io.close_issue(
+            self.pagure_io.close_issue(
-                    issue["id"],
+                issue["id"],
-                    namespace=PROJECT_NAMESPACE,
+                namespace=PROJECT_NAMESPACE,
-                    message=error,
+                message=error,
-                    reason="Invalid"
+                reason="Invalid"
-                )
+            )
             return
 
         if exception is True or namespace in ('modules', 'flatpaks', 'tests', 'container'):
@@ -632,6 +642,8 @@ class SCMRequestProcessor(ToddlerBase):
             message=new_branch_comment,
             reason="Processed"
         )
+        if bug_id:
+            bugzilla_system.comment_on_bug(bug_id, new_repo_comment)
 
     def validate_review_bug(self, bug_id: str, pkg: str, branch: str,
                             require_auth: bool = False, check_fas: bool = False,
@@ -768,69 +780,29 @@ class SCMRequestProcessor(ToddlerBase):
                      'the one provided "{1}"'.format(pkg_in_bug, pkg))
             raise ValidationError(error)
 
-    def comment_and_close_ticket(issue_id, bug_id, comment=None,
+    def valid_epel_package(self, name: str, branch: str) -> bool:
-                                close_status='Processed',
-                                prompt_for_comment=True):
-        """
-        A helper function that adds a comment, and then prompts the user if they'd
-        like to add one
-        :param issue_id: a string or int of the id of the issue to comment and
-        close
-        :param bug_id: a string or int of the id of the Bugzilla review bug.
-        None is an acceptable value if there isn't a bug.
-        :param comment: a string of the comment to add to the issue
-        :param close_status: a string of the status of the ticket when closed
-        :param prompt_for_comment: a boolean that determines the admin should be
-        prompted for an extra comment. This defaults to True.
-        :return: None
-        """
-        if comment is not None:
-            pagure.add_comment_to_issue(issue_id, comment)
-            if bug_id:
-                BUGZILLA_CLIENT.comment(bug_id, comment)
-            click.echo('The following comment was added to the issue "{0}"'
-                    .format(comment))
-        custom_comment = None
-        if prompt_for_comment:
-            if click.confirm('Would you like to add another comment?'):
-                custom_comment = click.prompt('Please enter a comment')
-
-        pagure.close_issue(issue_id, custom_comment, close_status)
-        if custom_comment and bug_id:
-            BUGZILLA_CLIENT.comment(bug_id, custom_comment)
-
-    def valid_project_name(project):
-        """
-        A helper function to determine if a project name meets naming standards
-        :param project: a string of the project name
-        :return: a boolean detremining if the project name is valid
-        """
-        return bool(re.match(r'^[a-zA-Z0-9_][a-zA-Z0-9-_.+]*$', project))
-
-
-    def valid_module_stream_name(stream):
-        """
-        A helper function to determine if a module's stream name meets naming
-        standards
-        :param stream: a string of the module stream name
-        :return: a boolean detremining if the module stream name is valid
-        """
-        return bool(re.match(r'^[a-zA-Z0-9.\-_+]+$', stream))
-
-
-    def valid_epel_package(self, name, branch):
         """
         Determines if the package is allowed to have an EPEL branch.
-        :param name: a string of the package name
+
-        :param branch: a string of the EPEL branch name (e.g. epel7)
+        Params:
-        :return: a boolean
+          name: a string of the package name
+          branch: a string of the EPEL branch name (e.g. epel7)
+
+        Returns:
+          If package is valid EPEL package.
+
+        Raises:
+          `ValidationError`: When we can't retrieve list of official EL packages.
         """
         # Extract any digits in the branch name to determine the EL version
         version = ''.join([i for i in branch if re.match(r'\d', i)])
         url = f'https://infrastructure.fedoraproject.org/repo/json/pkg_el{version}.json'
-        rv = requests_wrapper(
+        response = self.requests_session.get(url)
-            url, timeout=60, service_name='infrastructure.fedoraproject.org')
+
-        rv_json = get_request_json(rv, 'getting the list of official EL packages')
+        if response.status_code != 200:
+            raise ValidationError("Couldn't retrieve the list of official EL packages")
+        rv_json = response.json()
+
         # Remove noarch from this because noarch is treated specially
         all_arches = set(rv_json['arches']) - set(['noarch'])
         # On EL6, also remove ppc and i386 as many packages will
@@ -849,69 +821,3 @@ class SCMRequestProcessor(ToddlerBase):
                 if pkg_arches == set(['noarch']) or not (all_arches - pkg_arches):
                     return False
         return True
-
-
-    def bool_to_word(bool_value):
-        """
-        Converts a boolean to a "Yes" or "No"
-        :param bool_value: a boolean
-        :return: a string of the word representing the boolean
-        """
-        if bool_value is True:
-            return 'Yes'
-        else:
-            return 'No'
-
-
-    def assert_git_repo_initialized_remotely(namespace, repo):
-        """
-        Determines if the git repo is initialized remotely or not. If it isn't,
-        a ValidationError is raised.
-        :param namespace: a string of the namespace of the project
-        :param project: a string of the project name
-        :return: None or ValidationError
-        """
-        click.echo('- Verifying that the git repo is initialized')
-        git_url = pagure.get_project_git_url(
-            namespace, repo, url_type='git',
-            username=FAS_CLIENT.client.username)
-        git_obj = git.GitRepo(git_url)
-        if not git_obj.initialized_remotely(namespace, repo):
-            raise ValidationError('The git repository is not initialized. The git '
-                                'branch can\'t be created.')
-
-
-    def new_git_branch(namespace, repo, branch, use_default_branch=False):
-        """
-        Create a new branch in git using Pagure. This does some sanity checking
-        before sending off the API request.
-        :param namespace: a string of the namespace of the project
-        :param project: a string of the project name
-        :param branch: a string of the branch to create
-        :param use_default_branch: a boolean that determines whether to use the default
-        branch or the first commit of the default branch as the starting point for
-        the new branch
-        :return: None or ValidationError
-        """
-        if use_default_branch is True:
-            default_branch = get_project_default_branch(namespace, repo)
-            if default_branch:
-                pagure.new_branch(
-                    namespace, repo, branch, from_branch=default_branch)
-            else:
-                raise ValidationError('There is no default branch for {0}/{1}. A '
-                                    'git branch can\'t be created.'.format(namespace, repo))
-        else:
-            # Even though the branches are created using pagure api which dont
-            # require ssh, but the code supports adding package.cfg file.
-            # This should be pushed using ssh.
-            git_url = pagure.get_project_git_url(
-                namespace, repo, url_type='ssh',
-                username=FAS_CLIENT.client.username)
-            git_obj = git.GitRepo(git_url)
-            git_obj.clone_repo()
-            if not git_obj.initialized:
-                raise ValidationError('The git repository is not initialized. A '
-                                    'git branch can\'t be created.')
-            pagure.new_branch(
-                namespace, repo, branch, from_commit=git_obj.first_commit(namespace, repo))