diff --git a/.config/pycodestyle b/.config/pycodestyle new file mode 100644 index 0000000..9d54e0f --- /dev/null +++ b/.config/pycodestyle @@ -0,0 +1,2 @@ +[pycodestyle] +max_line_length = 120 diff --git a/.gitignore b/.gitignore index b9ee905..545e413 100644 --- a/.gitignore +++ b/.gitignore @@ -58,3 +58,6 @@ target/ # Where to store your personal API token *_token.txt + +# VS Code configs +.vscode/ diff --git a/.travis.yml b/.travis.yml index 7ce7337..cdb2170 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,7 +6,6 @@ services: language: python python: - - 2.7 - 3.6 cache: pip diff --git a/bin/install b/bin/install index 0091a7e..6e6c374 100755 --- a/bin/install +++ b/bin/install @@ -1,5 +1,16 @@ #!/bin/sh set -eux -pip install -r build-requirements.txt -pip install -r requirements.txt +if [ ${TRAVIS:-false} = 'true' ]; then + pip install -r build-requirements.txt + pip install -r requirements.txt +else + if command -v pip3 &> /dev/null + then + pip3 install -r build-requirements.txt + pip3 install -r requirements.txt + else + pip install -r build-requirements.txt + pip install -r requirements.txt + fi +fi diff --git a/bin/test b/bin/test index f018849..866b32c 100755 --- a/bin/test +++ b/bin/test @@ -3,10 +3,8 @@ set -eux flake8 --ignore=E501 -# Skip tests/test_destalinator.py due to heavy mocking. See https://github.com/jendrikseipp/vulture/issues/95 -# Skip utils/slack_logging.py due to implementing Handler#emit but never calling directly -# Skip scheduler.py due to unused lambda entrypoint handler -vulture . --exclude=tests/test_destalinator.py,utils/slack_logging.py,scheduler.py +# Skip uncalled lambda entry point +vulture . --ignore-names=destalinate_lambda coverage run --branch --source=. -m unittest discover -f coverage report -m --skip-covered --fail-under=60 diff --git a/build-requirements.txt b/build-requirements.txt index e4afa46..441e487 100644 --- a/build-requirements.txt +++ b/build-requirements.txt @@ -1,6 +1,5 @@ -mock==2.0.0 -six==1.10.0 -coverage==4.4.1 -coveralls==1.2.0 -flake8==3.4.1 -vulture==0.26 +coverage>=5.4 +coveralls>=3.0.0 +flake8>=3.8.4 +mock>=4.0.3 +vulture>=2.3 diff --git a/destalinator.py b/destalinator.py index 4d8890f..2a029d4 100755 --- a/destalinator.py +++ b/destalinator.py @@ -21,9 +21,10 @@ class Destalinator(WithLogger, WithConfig): def __init__(self, slacker, slackbot, activated): """ - slacker is a Slacker() object - slackbot should be an initialized slackbot.Slackbot() object - activated is a boolean indicating whether destalinator should do dry runs or real runs + `slacker` is a Slacker() object + `slackbot` should be an initialized slackbot.Slackbot() object + `activated` is a boolean indicating whether destalinator should do dry + runs or real runs """ self.closure_text = utils.get_local_file_content(self.closure_text_fname) self.warning_text = utils.get_local_file_content(self.warning_text_fname) @@ -67,8 +68,7 @@ def flush_channel_cache(self, channel_name): def get_earliest_archive_date(self): """Return a datetime.date object representing the earliest archive date.""" - date_string = self.config.earliest_archive_date \ - or PAST_DATE_STRING + date_string = self.config.earliest_archive_date or PAST_DATE_STRING return datetime.strptime(date_string, "%Y-%m-%d").date() def get_messages(self, channel_name, days): @@ -77,14 +77,17 @@ def get_messages(self, channel_name, days): cid = self.slacker.get_channelid(channel_name) if oldest in self.cache.get(cid, {}): - self.logger.debug("Returning %s cached messages for #%s over %s days", len(self.cache[cid][oldest]), channel_name, days) + self.logger.debug("Returning %s cached messages for #%s over %s days", len( + self.cache[cid][oldest]), channel_name, days) return self.cache[cid][oldest] messages = self.slacker.get_messages_in_time_range(oldest, cid) self.logger.debug("Fetched %s messages for #%s over %s days", len(messages), channel_name, days) - messages = [x for x in messages if x.get("subtype") is None or x.get("subtype") in self.config.included_subtypes] - self.logger.debug("Filtered down to %s messages based on included_subtypes: %s", len(messages), ", ".join(self.config.included_subtypes)) + messages = [x for x in messages if x.get("subtype") is None or x.get( + "subtype") in self.config.included_subtypes] + self.logger.debug("Filtered down to %s messages based on included_subtypes: %s", len( + messages), ", ".join(self.config.included_subtypes)) if cid not in self.cache: self.cache[cid] = {} @@ -107,7 +110,8 @@ def post_marked_up_message(self, channel_name, message, **kwargs): def stale(self, channel_name, days): """ Return True if channel represented by `channel_name` is stale. - Definition of stale is: no messages in the last `days` which are not from config.ignore_users. + Definition of stale is: no messages in the last `days` which are not + from config.ignore_users. """ if not self.channel_minimum_age(channel_name, days): return False @@ -158,7 +162,8 @@ def archive(self, channel_name): self.logger.info("Archived %s", channel_name) else: error = payload.get('error', '!! No error found in payload %s !!' % payload) - self.logger.error("Failed to archive %s: %s. See https://api.slack.com/methods/conversations.archive for more context.", channel_name, error) + url = "https://api.slack.com/methods/conversations.archive" + self.logger.error("Failed to archive %s: %s. See " + url + " for more context.", channel_name, error) return payload @@ -195,21 +200,13 @@ def warn(self, channel_name, days, force_warn=False): Using `force_warn=True` will warn even if a previous warning exists. Return True if we actually warned, otherwise False. """ - # Might not need to do this since we now do this in `stale` - if self.slacker.channel_has_only_restricted_members(channel_name): - self.logger.debug("Would have warned #%s but it contains only restricted users", channel_name) - return False - - # Might not need to do this since we now do this in `stale` - if self.ignore_channel(channel_name): - self.logger.debug("Not warning #%s because it's in ignore_channels", channel_name) - return False messages = self.get_messages(channel_name, days) texts = [x.get("text").strip() for x in messages if x.get("text")] - if (not force_warn and - (self.add_slack_channel_markup(self.warning_text) in texts or - any(any(a.get('fallback') == 'channel_warning' for a in m.get('attachments', [])) for m in messages))): + warned_in_message = self.add_slack_channel_markup(self.warning_text) in texts + warned_in_attachment = any(any(a.get('fallback') == 'channel_warning' for a in m.get( + 'attachments', [])) for m in messages) + if (not force_warn and (warned_in_message or warned_in_attachment)): self.logger.debug("Not warning #%s because we found a prior warning", channel_name) return False @@ -228,21 +225,16 @@ def warn_all(self, days, force_warn=False): stale = [] for channel in sorted(self.slacker.channels_by_name.keys()): - if self.ignore_channel(channel): - self.logger.debug("Not warning #%s because it's in ignore_channels", channel) - continue if self.stale(channel, days): if self.warn(channel, days, force_warn): stale.append(channel) self.flush_channel_cache(channel) - if stale and self.config.general_message_channel: + if len(stale) > 0 and self.config.general_message_channel: self.logger.debug("Notifying #%s of warned channels", self.config.general_message_channel) self.warn_in_general(stale) def warn_in_general(self, stale_channels): - if not stale_channels: - return if len(stale_channels) > 1: channel = "channels" being = "are" diff --git a/flagger.py b/flagger.py index d8fa629..4e3008d 100755 --- a/flagger.py +++ b/flagger.py @@ -26,7 +26,7 @@ class Flagger(executor.Executor): def __init__(self, *args, **kwargs): self.debug = kwargs.pop('debug', False) - super(self.__class__, self).__init__(*args, **kwargs) + super(Flagger, self).__init__(*args, **kwargs) self.now = int(time.time()) def extract_threshold(self, token): @@ -40,8 +40,8 @@ def extract_threshold(self, token): ]", "", tokens[6]) if output_channel_id.find("|") != -1: - cid, cname = output_channel_id.split("|") + cid, _ = output_channel_id.split("|") output_channel_id = cid output_channel_name = self.slacker.replace_id(output_channel_id) control[uuid] = {'threshold': threshold, "comparator": comparator, @@ -209,7 +209,7 @@ def announce_interesting_messages(self): if not self.debug and self.config.activated: # TODO: rename debug to dry run? self.slackbot.say(output_channel["output"], m) else: - self.logger.warning("Attempted to announce in %s because of rule :%s:%s%s, but channel does not exist.".format( + self.logger.warning("Attempted to announce in {} because of rule :{}:{}{}, but channel does not exist.".format( output_channel["output"], output_channel["emoji"], output_channel["comparator"], diff --git a/requirements.txt b/requirements.txt index 8a8f286..424973c 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,4 @@ APScheduler>=3.0.5 raven>=6.1.0 requests>=2.20.0 -ruamel.yaml \ No newline at end of file +ruamel.yaml>=0.6.12 diff --git a/scheduler.py b/scheduler.py index f17f504..ed4c141 100644 --- a/scheduler.py +++ b/scheduler.py @@ -22,7 +22,7 @@ def schedule_job(): sched.start() -def destalinate_lambda(event, context): +def destalinate_lambda(_event, _context): destalinate_job() @@ -31,10 +31,8 @@ def destalinate_job(): logging.info("Destalinating") if not get_config().sb_token or not get_config().api_token: - logging.error( - "Missing at least one required Slack environment variable.\n" - "Make sure to set DESTALINATOR_SB_TOKEN and DESTALINATOR_API_TOKEN." - ) + logging.error("Missing at least one required Slack environment variable.\n" + "Make sure to set DESTALINATOR_SB_TOKEN and DESTALINATOR_API_TOKEN.") else: try: archiver.Archiver().archive() diff --git a/tests/test_destalinator.py b/tests/test_destalinator.py index 353a9f1..5188040 100644 --- a/tests/test_destalinator.py +++ b/tests/test_destalinator.py @@ -76,18 +76,18 @@ } ] -sample_warning_messages = [ - { - "user": "U023BCDA1", - "text": "This is a channel warning! Put on your helmets!", - "username": "bot", - "bot_id": "B0T8EDVLY", - "attachments": [{"fallback": "channel_warning", "id": 1}], - "type": "message", - "subtype": "bot_message", - "ts": "1496855882.185855" - } -] +# sample_warning_messages = [ +# { +# "user": "U023BCDA1", +# "text": "This is a channel warning! Put on your helmets!", +# "username": "bot", +# "bot_id": "B0T8EDVLY", +# "attachments": [{"fallback": "channel_warning", "id": 1}], +# "type": "message", +# "subtype": "bot_message", +# "ts": "1496855882.185855" +# } +# ] class MockValidator(object): @@ -329,6 +329,20 @@ def test_with_all_sample_messages(self, mock_slacker): self.destalinator.get_messages = mock.MagicMock(return_value=sample_slack_messages) self.assertFalse(self.destalinator.stale('stalinists', 30)) + @mock.patch('tests.test_destalinator.SlackerMock') + def test_with_minimum_channel_age_not_met(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.get_channel_info.return_value = {'age': 60 * 86400} + self.destalinator.channel_minimum_age = mock.MagicMock(return_value=False) + self.assertFalse(self.destalinator.stale('stalinists', 30)) + + @mock.patch('tests.test_destalinator.SlackerMock') + def test_with_channel_ignored(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.get_channel_info.return_value = {'age': 60 * 86400} + self.destalinator.ignore_channel = mock.MagicMock(return_value=True) + self.assertFalse(self.destalinator.stale('stalinists', 30)) + @mock.patch.object(get_config(), 'ignore_users', [m['user'] for m in sample_slack_messages if m.get('user')]) @mock.patch('tests.test_destalinator.SlackerMock') def test_with_all_users_ignored(self, mock_slacker): @@ -417,6 +431,14 @@ def test_calls_archive_method(self, mock_slacker): self.destalinator.archive("stalinists") mock_slacker.archive.assert_called_once_with('stalinists') + @mock.patch('tests.test_destalinator.SlackerMock') + def test_handles_a_bad_archive_api_response(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.post_message.return_value = {} + mock_slacker.archive.return_value = {'ok': False, 'error': 'yup'} + self.destalinator.archive("stalinists") + mock_slacker.archive.assert_called_once_with('stalinists') + class DestalinatorSafeArchiveTestCase(unittest.TestCase): def setUp(self): @@ -459,9 +481,22 @@ def setUp(self): self.slackbot = slackbot.Slackbot("testing", "token") @mock.patch('tests.test_destalinator.SlackerMock') - def test_calls_stale_once_for_each_channel(self, mock_slacker): + def test_calls_stale_once_for_each_channel_with_cache(self, mock_slacker): self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.get_channelid.return_value = 'C012839' + days_ago = self.destalinator.now - (86400 * 30) + self.destalinator.cache = {'C012839': {days_ago: sample_slack_messages}} + self.destalinator.stale = mock.MagicMock(return_value=False) + days = self.destalinator.config.archive_threshold + self.destalinator.safe_archive_all(days) + self.assertEqual(self.destalinator.stale.mock_calls, [mock.call('leninists', days), mock.call('stalinists', days)]) + + @mock.patch('tests.test_destalinator.SlackerMock') + def test_calls_stale_once_for_each_channel_without_cache(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.get_channelid.return_value = 'C012839' self.destalinator.stale = mock.MagicMock(return_value=False) days = self.destalinator.config.archive_threshold self.destalinator.safe_archive_all(days) @@ -512,6 +547,18 @@ def test_warns_by_posting_message(self, mock_slacker): self.destalinator.warning_text, message_type='channel_warning') + @mock.patch('tests.test_destalinator.SlackerMock') + def test_warns_with_cached_messages(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + days_ago = self.destalinator.now - (86400 * 30) + self.destalinator.cache = {'C012839': {days_ago: sample_slack_messages}} + mock_slacker.get_channelid.return_value = 'C012839' + mock_slacker.channel_has_only_restricted_members.return_value = False + self.destalinator.warn("stalinists", 30) + mock_slacker.post_message.assert_called_with("stalinists", + self.destalinator.warning_text, + message_type='channel_warning') + def test_warns_by_posting_message_with_channel_names(self): self.destalinator = destalinator.Destalinator(self.slacker, self.slackbot, activated=True) warning_text = self.destalinator.warning_text + " #leninists" @@ -552,3 +599,110 @@ def test_does_not_warn_when_previous_warning_with_changed_text_found(self, mock_ ] self.destalinator.warn("stalinists", 30) self.assertFalse(mock_slacker.post_message.called) + + +class DestalinatorWarnAllTestCase(unittest.TestCase): + def setUp(self): + self.slacker = SlackerMock("testing", "token") + self.slackbot = slackbot.Slackbot("testing", "token") + + @mock.patch('tests.test_destalinator.SlackerMock') + def test_posts_a_warning_for_a_stale_channel(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_add_channel_markup(channel: str): + return "<#{}|C012839>".format(channel) + + mock_slacker.add_channel_markup = mock.MagicMock(side_effect=fake_add_channel_markup) + + def fake_stale(channel, days): + return {'leninists': True, 'stalinists': False}[channel] + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertTrue(mock_slacker.post_message.called) + + @mock.patch('tests.test_destalinator.SlackerMock') + def test_posts_no_warning_during_dry_run(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=False) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_stale(channel, days): + return {'leninists': True, 'stalinists': False}[channel] + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertFalse(mock_slacker.post_message.called) + + @mock.patch.object(get_config(), 'general_message_channel', False) + @mock.patch('tests.test_destalinator.SlackerMock') + def test_does_not_warn_without_general_channel_configured(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_stale(channel, days): + return {'leninists': True, 'stalinists': False}[channel] + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_in_general = mock.MagicMock() + + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertFalse(self.destalinator.warn_in_general.called) + + @mock.patch.object(get_config(), 'general_message_channel', 'stalinists') + @mock.patch('tests.test_destalinator.SlackerMock') + def test_does_not_warn_in_general_channel_with_no_stale_channels(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_stale(channel, days): + return False + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_in_general = mock.MagicMock(return_value=True) + + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertFalse(self.destalinator.warn_in_general.called) + + @mock.patch.object(get_config(), 'general_message_channel', 'stalinists') + @mock.patch('tests.test_destalinator.SlackerMock') + def test_warns_in_general_channel_with_one_stale_channel(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_add_channel_markup(channel: str): + return "<#{}|C012839>".format(channel) + + mock_slacker.add_channel_markup = mock.MagicMock(side_effect=fake_add_channel_markup) + + def fake_stale(channel, days): + return {'leninists': True, 'stalinists': False}[channel] + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertTrue(mock_slacker.post_message.called) + + @mock.patch.object(get_config(), 'general_message_channel', 'stalinists') + @mock.patch('tests.test_destalinator.SlackerMock') + def test_warns_in_general_channel_with_two_stale_channels(self, mock_slacker): + self.destalinator = destalinator.Destalinator(mock_slacker, self.slackbot, activated=True) + mock_slacker.channels_by_name = {'leninists': 'C012839', 'stalinists': 'C102843'} + mock_slacker.channel_has_only_restricted_members.return_value = False + + def fake_add_channel_markup(channel: str): + return "<#{}|C012839>".format(channel) + + mock_slacker.add_channel_markup = mock.MagicMock(side_effect=fake_add_channel_markup) + + def fake_stale(channel, days): + return True + + self.destalinator.stale = mock.MagicMock(side_effect=fake_stale) + self.destalinator.warn_all(self.destalinator.config.archive_threshold) + self.assertTrue(mock_slacker.post_message.called)