MAINT: PR comments addressed

cloud-chatops/.coveragerc

@@ -1,3 +1,5 @@
 [run]
 omit =
-    /usr/lib/**
+    # Omitting this directory as sometimes it's included in the coverage.
+    # This is likely due to messy Python Paths and environments
+    /usr/lib/**

cloud-chatops/src/features/base_feature.py

@@ -3,6 +3,7 @@
 All features should inherit this class to reduce code duplication.
 """
 
+from abc import ABC
 from typing import Dict, List
 from datetime import datetime, timedelta
 from dataclasses import replace
@@ -21,8 +22,8 @@
 DEFAULT_REPO_OWNER = "stfc"  # Our repos are owned by "stfc"
 
 
-class BaseFeature:
-    """This base class provides universal methods for features."""
+class BaseFeature(ABC):
+    """This base abstract class provides universal methods for features."""
 
     # pylint: disable=R0903
     # This is a base feature and not intended to be used on its own
@@ -94,7 +95,7 @@ def _send_thread_react(self, pr: PrData, channel: str, thread_ts: str) -> None:
         if pr.old:
             react_with.append("alarm_clock")
         if pr.draft:
-            react_with.append("scroll")
+            react_with.append("building_construction")
         for react in react_with:
             react_response = self.client.reactions_add(
                 channel=channel, name=react, timestamp=thread_ts

cloud-chatops/src/read_data.py

@@ -10,6 +10,9 @@
     TokensNotGiven,
 )
 
+# Using dev secrets here for local testing as it runs the application
+# in a separate Slack Workspace than the production application.
+# This means the slash commands won't be picked up by the production application.
 PATH = "/usr/src/app/dev_cloud_chatops_secrets/"
 try:
     if sys.argv[1] == "local":

cloud-chatops/tests/test_base_feature.py

@@ -1,7 +1,4 @@
 """Tests for features.base_feature.BaseFeature"""
-
-# pylint: disable=W0212
-# Disabling this as we need to access protected methods to test them
 from unittest.mock import NonCallableMock, patch, MagicMock
 import pytest
 from slack_sdk.errors import SlackApiError
@@ -15,30 +12,62 @@
 @patch("features.base_feature.get_repos")
 @patch("features.base_feature.get_user_map")
 def instance_fixture(
-    mock_get_user_map,
-    mock_get_repos,
-    mock_get_token,
-    mock_get_github_prs,
-    mock_web_client,
+        mock_get_user_map,
+        mock_get_repos,
+        mock_get_token,
+        mock_get_github_prs,
+        mock_web_client,
 ):
     """This fixture provides a class instance for the tests"""
     mock_get_user_map.return_value = {"mock_github": "mock_slack"}
     mock_get_repos.return_value = ["mock_repo1", "mock_repo2"]
     mock_get_token.return_value = "mock_slack_token"
     mock_get_github_prs.run.return_value = []
     mock_web_client.return_value = NonCallableMock()
-    return BaseFeature()
+
+    class BaseFeatureWrapper(BaseFeature):
+        """A wrapper class to make the private methods accessible"""
+
+        def github_to_slack_username(self, user: str):
+            """Returns the private method"""
+            return self._github_to_slack_username(user)
+
+        def slack_to_human_username(self, slack_member_id):
+            """Returns the private method"""""
+            return self._slack_to_human_username(slack_member_id)
+
+        def post_reminder_message(self):
+            """Returns the private method"""""
+            return self._post_reminder_message()
+
+        def send_no_prs_found(self, thread_ts):
+            """Returns the private method"""""
+            return self._send_no_prs_found(thread_ts)
+
+        def send_thread(self, pr_data, thread_ts):
+            """Returns the private method"""""
+            return self._send_thread(pr_data, thread_ts)
+
+        def send_thread_react(self, pr, channel, thread_ts):
+            """Returns the private method"""""
+            return self._send_thread_react(pr, channel, thread_ts)
+
+        def format_prs(self, prs):
+            """Returns the private method"""""
+            return self._format_prs(prs)
+
+    return BaseFeatureWrapper()
 
 
 def test_github_to_slack_username_valid(instance):
     """Test the github to slack name translation works"""
-    res = instance._github_to_slack_username("mock_github")
+    res = instance.github_to_slack_username("mock_github")
     assert res == "mock_slack"
 
 
 def test_github_to_slack_username_invalid(instance):
     """Test the github to slack name translation works"""
-    res = instance._github_to_slack_username("mock_github_not_found")
+    res = instance.github_to_slack_username("mock_github_not_found")
     assert res == "mock_github_not_found"
 
 
@@ -47,15 +76,15 @@ def test_slack_to_human_username_valid(instance):
     instance.client.users_profile_get.return_value = {
         "profile": {"real_name": "mock_name"}
     }
-    res = instance._slack_to_human_username("mock_member_id")
+    res = instance.slack_to_human_username("mock_member_id")
     instance.client.users_profile_get.assert_called_once_with(user="mock_member_id")
     assert res == "mock_name"
 
 
 def test_slack_to_human_username_invalid(instance):
     """Test the slack member id to real name works"""
     instance.client.users_profile_get.side_effect = SlackApiError("", "")
-    res = instance._slack_to_human_username("mock_member_id")
+    res = instance.slack_to_human_username("mock_member_id")
     assert res == "mock_member_id"
 
 
@@ -65,7 +94,7 @@ def test_post_reminder_message(instance):
     return_obj.data = {"ts": 100}
     return_obj.__getitem__.side_effect = [True]
     instance.client.chat_postMessage.return_value = return_obj
-    res = instance._post_reminder_message()
+    res = instance.post_reminder_message()
     instance.client.chat_postMessage.assert_called_once_with(
         channel=instance.channel,
         text="Here are the outstanding PRs as of today:",
@@ -77,13 +106,13 @@ def test_post_reminder_message_fails(instance):
     """Test a message is not posted"""
     instance.client.chat_postMessage.return_value = {"ok": False}
     with pytest.raises(AssertionError):
-        instance._post_reminder_message()
+        instance.post_reminder_message()
 
 
 def test_send_no_prs_found(instance):
     """Test a message is posted"""
     instance.client.chat_postMessage.return_value = {"ok": True}
-    instance._send_no_prs_found(100)
+    instance.send_no_prs_found(100)
     instance.client.chat_postMessage.assert_called_once_with(
         channel=instance.channel,
         thread_ts=100,
@@ -96,7 +125,7 @@ def test_send_no_prs_found_fails(instance):
     """Test a message is not posted"""
     instance.client.chat_postMessage.return_value = {"ok": False}
     with pytest.raises(AssertionError):
-        instance._send_no_prs_found(100)
+        instance.send_no_prs_found(100)
 
 
 @patch("features.base_feature.PRMessageBuilder")
@@ -105,7 +134,7 @@ def test_send_thread(mock_pr_message_builder, instance):
     mock_pr_data = NonCallableMock()
     mock_pr_message_builder.return_value.make_message.return_value = "mock_message"
     instance.client.chat_postMessage.return_value = {"ok": True}
-    res = instance._send_thread(mock_pr_data, "100")
+    res = instance.send_thread(mock_pr_data, "100")
     mock_pr_message_builder.return_value.make_message.assert_called_once_with(
         mock_pr_data
     )
@@ -123,15 +152,15 @@ def test_send_thread_fails(_, instance):
     """Test a message is not sent"""
     instance.client.chat_postMessage.return_value = {"ok": False}
     with pytest.raises(AssertionError):
-        instance._send_thread(NonCallableMock(), "100")
+        instance.send_thread(NonCallableMock(), "100")
 
 
 def test_send_thread_react_no_reactions(instance):
     """Test no reactions are added"""
     mock_pr_data = NonCallableMock()
     mock_pr_data.old = False
     mock_pr_data.draft = False
-    instance._send_thread_react(mock_pr_data, "mock_channel", "100")
+    instance.send_thread_react(mock_pr_data, "mock_channel", "100")
     instance.client.reactions_add.assert_not_called()
 
 
@@ -142,7 +171,7 @@ def test_send_thread_react_fails_to_add(instance):
     mock_pr_data.draft = True
     instance.client.reactions_add.side_effect = [{"ok": True}, {"ok": False}]
     with pytest.raises(AssertionError):
-        instance._send_thread_react(mock_pr_data, "mock_channel", "100")
+        instance.send_thread_react(mock_pr_data, "mock_channel", "100")
 
 
 def test_send_thread_react_with_reactions(instance):
@@ -151,10 +180,10 @@ def test_send_thread_react_with_reactions(instance):
     mock_pr_data.old = True
     mock_pr_data.draft = True
     instance.client.reactions_add.return_value = {"ok": True}
-    instance._send_thread_react(mock_pr_data, "mock_channel", "100")
+    instance.send_thread_react(mock_pr_data, "mock_channel", "100")
     instance.client.reactions_add.assert_any_call(
         channel="mock_channel", name="alarm_clock", timestamp="100"
     )
     instance.client.reactions_add.assert_any_call(
-        channel="mock_channel", name="scroll", timestamp="100"
+        channel="mock_channel", name="building_construction", timestamp="100"
     )