From 1be31bdb229dcf8f515c2ca45739bc6b97451514 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sybren=20A=2E=20St=C3=BCvel?= Date: Thu, 19 Apr 2018 18:00:03 +0200 Subject: [PATCH] Fix issue with task shortcodes Part of the code assumed shortcodes were globally unique, and another part assumed the shortcodes are unique per project (the latter is correct). Now the project ID is taken from the URL the Subversion hook pushes to. --- attract/subversion/__init__.py | 6 +- attract/subversion/routes.py | 5 +- attract/tasks/__init__.py | 13 +- docs/docs/user_manual/subversion.md | 3 + tests/test_shortcode.py | 32 ++-- tests/test_subversion.py | 221 +++++++++++++++++++++------- 6 files changed, 200 insertions(+), 80 deletions(-) diff --git a/attract/subversion/__init__.py b/attract/subversion/__init__.py index cd77faa..f60fd46 100644 --- a/attract/subversion/__init__.py +++ b/attract/subversion/__init__.py @@ -17,10 +17,11 @@ signals = { 'T': task_logged, } -# Copy of namedtuple defined in svn.common.log_default(). +# Copy of namedtuple defined in svn.common.log_default(), +# extended with our project_id. LogEntry = collections.namedtuple( 'LogEntry', - ['date', 'msg', 'revision', 'author', 'changelist'] + ['date', 'msg', 'revision', 'author', 'changelist', 'project_id'] ) @@ -54,6 +55,7 @@ class CommitLogObserver(object): def fetch_and_observe(self): """Obtains task IDs from SVN logs.""" + # FIXME: this code is unaware of the fact that task markers are only unique per project. self._log.debug('%s: fetch_and_observe()', self) try: diff --git a/attract/subversion/routes.py b/attract/subversion/routes.py index 2529f92..36d3dde 100644 --- a/attract/subversion/routes.py +++ b/attract/subversion/routes.py @@ -87,12 +87,13 @@ def subversion_log(project_url): except KeyError: return 'Not set up for Attract', 400 - svn_server_url = attract_props['svn_url'] + svn_server_url = attract_props.get('svn_url', '-unknown-') log.debug('Receiving commit from SVN server %s', svn_server_url) log_entry = subversion.create_log_entry(revision=revision, msg=commit_message, author=commit_author, - date_text=commit_date) + date_text=commit_date, + project_id=project['_id']) observer = subversion.CommitLogObserver() log.debug('Processing %s via %s', log_entry, observer) observer.process_log(log_entry) diff --git a/attract/tasks/__init__.py b/attract/tasks/__init__.py index d453484..6610a55 100644 --- a/attract/tasks/__init__.py +++ b/attract/tasks/__init__.py @@ -4,6 +4,7 @@ import attr import flask import flask_login from dateutil import parser +import bson import pillarsdk from pillar import attrs_extra @@ -128,7 +129,7 @@ class TaskManager(object): }}, api=api) return tasks - def api_task_for_shortcode(self, shortcode): + def api_task_for_shortcode(self, project_id: bson.ObjectId, shortcode: str) -> dict: """Returns the task for the given shortcode. :returns: the task Node, or None if not found. @@ -137,6 +138,7 @@ class TaskManager(object): db = flask.current_app.db() task = db['nodes'].find_one({ 'properties.shortcode': shortcode, + 'project': project_id, 'node_type': node_type_task['name'], }) @@ -150,11 +152,12 @@ class TaskManager(object): :type log_entry: attract.subversion.LogEntry """ - self._log.info("Task '%s' logged in SVN by %s: %s...", - shortcode, log_entry.author, log_entry.msg[:30].replace('\n', ' // ')) + self._log.info("Project %s, task '%s' logged in SVN by %s: %s...", + log_entry.project_id, shortcode, + log_entry.author, log_entry.msg[:30].replace('\n', ' // ')) # Find the task - task = self.api_task_for_shortcode(shortcode) + task = self.api_task_for_shortcode(log_entry.project_id, shortcode) if not task: self._log.warning('Task %s not found, ignoring SVN commit.', shortcode) return @@ -162,7 +165,7 @@ class TaskManager(object): # Find the author db = flask.current_app.db() proj = db['projects'].find_one({'_id': task['project']}, - projection={'extension_props.attract': 1}) + projection={'extension_props.attract': 1}) if not proj: self._log.warning('Project %s for task %s not found, ignoring SVN commit.', task['project'], task['_id']) diff --git a/docs/docs/user_manual/subversion.md b/docs/docs/user_manual/subversion.md index bbb3b26..222ccaa 100644 --- a/docs/docs/user_manual/subversion.md +++ b/docs/docs/user_manual/subversion.md @@ -23,3 +23,6 @@ file is bundled with Attract's source code, and needs to be copied to the Subver } The authentication token must be created on the server using `manage.py attract create_svner_account {emailaddress} {project-url}` + +The project should have an `svn_url` extension property in the MongoDB database, which points to +the Subversion server URL. This is for logging use only. diff --git a/tests/test_shortcode.py b/tests/test_shortcode.py index ca3d754..257af54 100644 --- a/tests/test_shortcode.py +++ b/tests/test_shortcode.py @@ -1,18 +1,5 @@ -# -*- coding=utf-8 -*- +import bson -import collections -import datetime -import logging.config -import unittest - -from dateutil.tz import tzutc -import mock -import svn.common - -from pillar.tests import common_test_data as ctd - -import logging_config -from attract import subversion from abstract_attract_test import AbstractAttractTest SVN_SERVER_URL = 'svn://biserver/agent327' @@ -26,7 +13,6 @@ class ShortcodeTest(AbstractAttractTest): self.proj_id, self.project = self.ensure_project_exists() def test_increment_simple(self): - from attract import shortcodes with self.app.test_request_context(): @@ -36,3 +22,19 @@ class ShortcodeTest(AbstractAttractTest): with self.app.test_request_context(): code = shortcodes.generate_shortcode(self.proj_id, 'jemoeder', 'č') self.assertEqual('č2', code) + + def test_multiple_projects(self): + from attract import shortcodes + + proj_id2, project2 = self.ensure_project_exists(project_overrides={ + '_id': bson.ObjectId(24 * 'f'), + 'url': 'proj2', + }) + + with self.app.app_context(): + code1 = shortcodes.generate_shortcode(self.proj_id, 'jemoeder', 'č') + code2 = shortcodes.generate_shortcode(proj_id2, 'jemoeder', 'č') + code3 = shortcodes.generate_shortcode(proj_id2, 'jemoeder', 'č') + self.assertEqual('č1', code1) + self.assertEqual('č1', code2) + self.assertEqual('č2', code3) diff --git a/tests/test_subversion.py b/tests/test_subversion.py index c51fead..48661b7 100644 --- a/tests/test_subversion.py +++ b/tests/test_subversion.py @@ -22,51 +22,63 @@ SVN_SERVER_URL = 'svn://biserver/agent327' logging.config.dictConfig(logging_config.LOGGING) -# Unfortunately, the svn module doesn't use classes, but uses in-function-defined -# namedtuples instead. As a result, we can't import them, but have to recreate. -LogEntry = collections.namedtuple('LogEntry', ['date', 'msg', 'revision', 'author', 'changelist']) - -SVN_LOG_BATCH_1 = [ - LogEntry(date=datetime.datetime(2016, 4, 5, 10, 8, 5, 19211, tzinfo=tzutc()), - msg='Initial commit', revision=43, author='fsiddi', changelist=None), - LogEntry(date=datetime.datetime(2016, 4, 8, 13, 5, 39, 42537, tzinfo=tzutc()), - msg='Initial commit of layout files', revision=44, author='hjalti', changelist=None), - LogEntry(date=datetime.datetime(2016, 4, 8, 13, 6, 18, 947830, tzinfo=tzutc()), - msg=None, revision=45, author='andy', changelist=None), - LogEntry(date=datetime.datetime(2016, 4, 8, 14, 22, 24, 411916, tzinfo=tzutc()), - msg="Add the eye lattices to the main group\n\nOtherwise when you link the agent group, those two lattices would be\nlinked as regular objects, and you'd need to move both proxy+lattices\nindividually.\n\n\n", - revision=46, author='pablo', changelist=None), -] - -SVN_LOG_BATCH_2 = [ - LogEntry(date=datetime.datetime(2016, 4, 13, 17, 54, 50, 244305, tzinfo=tzutc()), - msg='first initial agent model rework.', revision=47, author='andy', changelist=None), - LogEntry(date=datetime.datetime(2016, 4, 14, 15, 57, 30, 951714, tzinfo=tzutc()), - msg='third day of puching verts around', revision=48, author='andy', changelist=None), - LogEntry(date=datetime.datetime(2016, 4, 21, 8, 21, 19, 390478, tzinfo=tzutc()), - msg='last weeks edit. a couple of facial expression tests.\nstarting to modify the agent head heavily... W A R N I N G', - revision=49, author='andy', changelist=None), - LogEntry(date=datetime.datetime(2016, 4, 25, 9, 18, 17, 23841, tzinfo=tzutc()), - msg='some expression tests.', revision=50, author='andy', changelist=None), - LogEntry(date=datetime.datetime(2016, 4, 25, 10, 12, 23, 233796, tzinfo=tzutc()), - msg='older version of the layout', revision=51, author='hjalti', changelist=None), -] - -SVN_LOG_BATCH_WITH_TASK_MARKERS = [ - LogEntry(date=datetime.datetime(2016, 4, 5, 10, 8, 5, 19211, tzinfo=tzutc()), - msg='Initial commit', revision=1, author='fsiddi', changelist=None), - LogEntry(date=datetime.datetime(2016, 4, 8, 13, 5, 39, 42537, tzinfo=tzutc()), - msg='[T1234] modeled Hendrik IJzerbroot', revision=2, author='andy', changelist=None), - LogEntry(date=datetime.datetime(2016, 4, 8, 13, 6, 18, 947830, tzinfo=tzutc()), - msg='[T4415] scene layout, which also closes [T4433]', revision=3, author='hjalti', - changelist=None), -] - class TestCommitLogObserver(unittest.TestCase): def setUp(self): from attract import subversion + self.pid1 = ObjectId(24 * 'a') + self.pid2 = ObjectId(24 * 'b') + + LogEntry = subversion.LogEntry + self.SVN_LOG_BATCH_1 = [ + LogEntry(date=datetime.datetime(2016, 4, 5, 10, 8, 5, 19211, tzinfo=tzutc()), + msg='Initial commit', revision=43, author='fsiddi', changelist=None, + project_id=self.pid1), + LogEntry(date=datetime.datetime(2016, 4, 8, 13, 5, 39, 42537, tzinfo=tzutc()), + msg='Initial commit of layout files', revision=44, author='hjalti', + changelist=None, project_id=self.pid1), + LogEntry(date=datetime.datetime(2016, 4, 8, 13, 6, 18, 947830, tzinfo=tzutc()), + msg=None, revision=45, author='andy', changelist=None, project_id=self.pid1), + LogEntry(date=datetime.datetime(2016, 4, 8, 14, 22, 24, 411916, tzinfo=tzutc()), + msg="Add the eye lattices to the main group\n\nOtherwise when you link the " + "agent group, those two lattices would be\nlinked as regular objects, and " + "you'd need to move both proxy+lattices\nindividually.\n\n\n", + revision=46, author='pablo', changelist=None, project_id=self.pid1), + ] + + self.SVN_LOG_BATCH_2 = [ + LogEntry(date=datetime.datetime(2016, 4, 13, 17, 54, 50, 244305, tzinfo=tzutc()), + msg='first initial agent model rework.', revision=47, author='andy', + changelist=None, project_id=self.pid1), + LogEntry(date=datetime.datetime(2016, 4, 14, 15, 57, 30, 951714, tzinfo=tzutc()), + msg='third day of puching verts around', revision=48, author='andy', + changelist=None, project_id=self.pid1), + LogEntry(date=datetime.datetime(2016, 4, 21, 8, 21, 19, 390478, tzinfo=tzutc()), + msg='last weeks edit. a couple of facial expression tests.\nstarting to ' + 'modify the agent head heavily... W A R N I N G', + revision=49, author='andy', changelist=None, project_id=self.pid1), + LogEntry(date=datetime.datetime(2016, 4, 25, 9, 18, 17, 23841, tzinfo=tzutc()), + msg='some expression tests.', revision=50, author='andy', changelist=None, + project_id=self.pid1), + LogEntry(date=datetime.datetime(2016, 4, 25, 10, 12, 23, 233796, tzinfo=tzutc()), + msg='older version of the layout', revision=51, author='hjalti', + changelist=None, project_id=self.pid1), + ] + + self.SVN_LOG_BATCH_WITH_TASK_MARKERS = [ + LogEntry(date=datetime.datetime(2016, 4, 5, 10, 8, 5, 19211, tzinfo=tzutc()), + msg='Initial commit', revision=1, author='fsiddi', changelist=None, + project_id=self.pid1), + LogEntry(date=datetime.datetime(2016, 4, 8, 13, 5, 39, 42537, tzinfo=tzutc()), + msg='[T1234] modeled Hendrik IJzerbroot', revision=2, author='andy', + changelist=None, project_id=self.pid1), + LogEntry(date=datetime.datetime(2016, 4, 8, 13, 6, 18, 947830, tzinfo=tzutc()), + msg='[T4415] scene layout, which also closes [T4433]', revision=3, + author='hjalti', + changelist=None, project_id=ObjectId(24 * 'b')), + ] + self.client = subversion.obtain(SVN_SERVER_URL) # Passing in a real client to Mock() will ensure that isinstance() checks return True. self.mock_client = mock.Mock(self.client, name='svn_client') @@ -96,9 +108,9 @@ class TestCommitLogObserver(unittest.TestCase): self.mock_client.log_default = mock.Mock(name='log_default') self.mock_client.log_default.side_effect = [ # First call, only four commits. - SVN_LOG_BATCH_1, + self.SVN_LOG_BATCH_1, # Second call, five commits. - SVN_LOG_BATCH_2 + self.SVN_LOG_BATCH_2 ] self.observer.last_seen_revision = 42 @@ -116,7 +128,7 @@ class TestCommitLogObserver(unittest.TestCase): from attract import subversion self.mock_client.log_default = mock.Mock(name='log_default', - return_value=SVN_LOG_BATCH_WITH_TASK_MARKERS) + return_value=self.SVN_LOG_BATCH_WITH_TASK_MARKERS) blinks = [] def record_blink(sender, **kwargs): @@ -128,12 +140,15 @@ class TestCommitLogObserver(unittest.TestCase): self.observer.fetch_and_observe() self.assertEqual(3, len(blinks)) - self.assertEqual({'log_entry': SVN_LOG_BATCH_WITH_TASK_MARKERS[1], 'shortcode': 'T1234'}, - blinks[0]) - self.assertEqual({'log_entry': SVN_LOG_BATCH_WITH_TASK_MARKERS[2], 'shortcode': 'T4415'}, - blinks[1]) - self.assertEqual({'log_entry': SVN_LOG_BATCH_WITH_TASK_MARKERS[2], 'shortcode': 'T4433'}, - blinks[2]) + self.assertEqual( + {'log_entry': self.SVN_LOG_BATCH_WITH_TASK_MARKERS[1], 'shortcode': 'T1234'}, + blinks[0]) + self.assertEqual( + {'log_entry': self.SVN_LOG_BATCH_WITH_TASK_MARKERS[2], 'shortcode': 'T4415'}, + blinks[1]) + self.assertEqual( + {'log_entry': self.SVN_LOG_BATCH_WITH_TASK_MARKERS[2], 'shortcode': 'T4433'}, + blinks[2]) def test_svn_error(self): """SVN errors should not crash the observer.""" @@ -158,13 +173,15 @@ class TestCommitLogObserver(unittest.TestCase): msg='Ünicøde is good', revision='123', author='børk', - changelist='nothing') + changelist='nothing', + project_id=self.pid1) self.assertEqual(tuple(entry), ( datetime.datetime(2016, 10, 21, 15, 40, 17, 0, tzinfo=tzutc()), 'Ünicøde is good', '123', 'børk', - 'nothing' + 'nothing', + self.pid1, )) self.assertRaises(ValueError, subversion.create_log_entry, @@ -172,31 +189,36 @@ class TestCommitLogObserver(unittest.TestCase): msg='Ünicøde is good', revision='123', author='børk', - changelist='nothing') + changelist='nothing', + project_id=self.pid1) entry = subversion.create_log_entry(date_text='2016-10-21 17:40:17 +0200', msg='Ünicøde is good', revision='123', - author='børk') + author='børk', + project_id=self.pid1) self.assertEqual(tuple(entry), ( datetime.datetime(2016, 10, 21, 15, 40, 17, 0, tzinfo=tzutc()), 'Ünicøde is good', '123', 'børk', - None + None, + self.pid1, )) entry = subversion.create_log_entry( date=datetime.datetime(2016, 10, 21, 15, 40, 17, 0, tzinfo=tzutc()), msg='Ünicøde is good', revision='123', - author='børk') + author='børk', + project_id=self.pid1) self.assertEqual(tuple(entry), ( datetime.datetime(2016, 10, 21, 15, 40, 17, 0, tzinfo=tzutc()), 'Ünicøde is good', '123', 'børk', - None + None, + self.pid1, )) @@ -238,6 +260,93 @@ class PushCommitTest(AbstractAttractTest): blinks[0]['log_entry'].msg) self.assertEqual(datetime.datetime(2016, 10, 21, 15, 40, 17, 0, tzinfo=tzutc()), blinks[0]['log_entry'].date) + self.assertEqual(self.proj_id, blinks[0]['log_entry'].project_id) + + def test_two_projects(self): + from attract import cli, subversion + + proj_id2, project2 = self.ensure_project_exists(project_overrides={ + '_id': ObjectId(24 * 'f'), + 'url': 'proj2', + }) + + with self.app.test_request_context(): + _, token1 = cli.create_svner_account('svner1@example.com', self.project['url']) + _, token2 = cli.create_svner_account('svner2@example.com', project2['url']) + + blinks = [] + + def record_blink(sender, **kwargs): + blinks.append(kwargs) + + subversion.task_logged.connect(record_blink) + + push_data = { + 'repo': 'strange-repo™', + 'revision': '4', + 'msg': 'မြန်မာဘာသာ is beautiful.\n\nThis solves task [T431134]', + 'author': 'Haha', + 'date': '2016-10-21 17:40:17 +0200', + } + self.post('/attract/api/%s/subversion/log' % self.project['url'], + json=push_data, + auth_token=token1['token']) + self.post('/attract/api/%s/subversion/log' % project2['url'], + json=push_data, + auth_token=token2['token']) + + self.assertEqual(2, len(blinks)) + + self.assertEqual('T431134', blinks[0]['shortcode']) + self.assertEqual('မြန်မာဘာသာ is beautiful.\n\nThis solves task [T431134]', + blinks[0]['log_entry'].msg) + self.assertEqual(datetime.datetime(2016, 10, 21, 15, 40, 17, 0, tzinfo=tzutc()), + blinks[0]['log_entry'].date) + self.assertEqual(self.proj_id, blinks[0]['log_entry'].project_id) + + self.assertEqual('T431134', blinks[1]['shortcode']) + self.assertEqual('မြန်မာဘာသာ is beautiful.\n\nThis solves task [T431134]', + blinks[1]['log_entry'].msg) + self.assertEqual(datetime.datetime(2016, 10, 21, 15, 40, 17, 0, tzinfo=tzutc()), + blinks[1]['log_entry'].date) + self.assertEqual(proj_id2, blinks[1]['log_entry'].project_id) + + def test_wrong_project(self): + from attract import cli, subversion + + proj_id2, project2 = self.ensure_project_exists(project_overrides={ + '_id': ObjectId(24 * 'f'), + 'url': 'proj2', + }) + + with self.app.test_request_context(): + _, token1 = cli.create_svner_account('svner1@example.com', self.project['url']) + _, token2 = cli.create_svner_account('svner2@example.com', project2['url']) + + blinks = [] + + def record_blink(sender, **kwargs): + blinks.append(kwargs) + + subversion.task_logged.connect(record_blink) + + push_data = { + 'repo': 'strange-repo™', + 'revision': '4', + 'msg': 'မြန်မာဘာသာ is beautiful.\n\nThis solves task [T431134]', + 'author': 'Haha', + 'date': '2016-10-21 17:40:17 +0200', + } + self.post('/attract/api/%s/subversion/log' % self.project['url'], + json=push_data, + auth_token=token2['token'], + expected_status=403) + self.post('/attract/api/%s/subversion/log' % project2['url'], + json=push_data, + auth_token=token1['token'], + expected_status=403) + + self.assertEqual(0, len(blinks)) class SvnTaskLoggedTest(AbstractAttractTest):