From 94c2432c49b16253c8316154fde4e3bb743c5244 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Feb 25 2019 15:28:59 +0000 Subject: Fix adding multiple tags to an issue In commit 4d961f80509c9279d563674132dd51dbb040f3e3 we forbid the use of comas in tag names. This is fine, except when seeing and updating an issue, tags are sent as a coma-separated list. That commit thus broke adding multiple tags to a single issue, kinda annoying. To fix this, this commit introduce a new regex which is basically the same regex as before but which allows comas in there. Thus allowing to have multiple tags in an issue without allowing comas in the tag names. To ensure we don't break this by accident in the future again, we're also adding a couple of unit-tests checking that the behavior is consistent with the expectations. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/forms.py b/pagure/forms.py index 52d8395..032559e 100644 --- a/pagure/forms.py +++ b/pagure/forms.py @@ -34,7 +34,12 @@ from pagure.config import config as pagure_config from pagure.utils import urlpattern, is_admin STRICT_REGEX = "^[a-zA-Z0-9-_]+$" +# This regex is used when creating tags, there we do not want to allow ',' +# as otherwise it breaks the UI. TAGS_REGEX = "^[a-zA-Z0-9-_ .:]+$" +# In the issue page tags are sent as a comma-separated list, so in order to +# allow having multiple tags in an issue, we need to allow ',' in them. +TAGS_REGEX_MULTI = "^[a-zA-Z0-9-_, .:]+$" FALSE_VALUES = ("false", "", False, "False", 0, "0") WTF_VERSION = tuple() @@ -437,7 +442,7 @@ class UpdateIssueForm(PagureForm): "tag", [ wtforms.validators.Optional(), - wtforms.validators.Regexp(TAGS_REGEX, flags=re.IGNORECASE), + wtforms.validators.Regexp(TAGS_REGEX_MULTI, flags=re.IGNORECASE), wtforms.validators.Length(max=255), ], ) diff --git a/tests/test_pagure_flask_ui_issues.py b/tests/test_pagure_flask_ui_issues.py index 6a73e24..ff1ea45 100644 --- a/tests/test_pagure_flask_ui_issues.py +++ b/tests/test_pagure_flask_ui_issues.py @@ -1816,6 +1816,84 @@ class PagureFlaskIssuestests(tests.Modeltests): @patch('pagure.lib.git.update_git') @patch('pagure.lib.notify.send_email') + def test_update_issue_add_tags(self, p_send_email, p_ugt): + """ Test the update_issue endpoint. """ + p_send_email.return_value = True + p_ugt.return_value = True + + tests.create_projects(self.session) + tests.create_projects_git( + os.path.join(self.path, 'repos'), bare=True) + + # Create issues to play with + repo = pagure.lib.query.get_authorized_project(self.session, 'test') + msg = pagure.lib.query.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + # Before update, list tags + tags = pagure.lib.query.get_tags_of_project(self.session, repo) + self.assertEqual([tag.tag for tag in tags], []) + self.assertEqual(repo.issues[0].tags_text, []) + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(self.app.application, user): + csrf_token = self.get_csrf() + + # Add two tags to the project + data = { + 'tag': 'green', + 'tag_description': 'lorem ipsum', + 'tag_color': '#fff', + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/update/tags', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + + data = { + 'tag': 'red', + 'tag_description': 'lorem ipsum', + 'tag_color': '#rrr', + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/update/tags', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + + # Add two tags to the issue + data = { + 'csrf_token': csrf_token, + 'tag': 'red,green', + } + output = self.app.post( + '/test/issue/1/update', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn( + 'Issue #1: Test issue - test - Pagure', + output_text) + self.assertIn( + '', + output_text) + self.assertIn( + ' Issue tagged with: green, red', output_text) + + # After update, list tags + tags = pagure.lib.query.get_tags_of_project(self.session, repo) + self.assertEqual([tag.tag for tag in tags], ['green', 'red']) + self.assertEqual(repo.issues[0].tags_text, []) + + @patch('pagure.lib.git.update_git') + @patch('pagure.lib.notify.send_email') def test_update_issue(self, p_send_email, p_ugt): """ Test the update_issue endpoint. """ p_send_email.return_value = True @@ -4149,6 +4227,62 @@ class PagureFlaskIssuestests(tests.Modeltests): @patch('pagure.lib.git.update_git') @patch('pagure.lib.notify.send_email') + def test_update_tags_with_coma(self, p_send_email, p_ugt): + """ Test the update_tags endpoint with a tag containing a coma. """ + p_send_email.return_value = True + p_ugt.return_value = True + + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos')) + + # Create issues to play with + repo = pagure.lib.query.get_authorized_project(self.session, 'test') + msg = pagure.lib.query.new_issue( + session=self.session, + repo=repo, + title='Test issue', + content='We should work on this', + user='pingou', + ) + self.session.commit() + self.assertEqual(msg.title, 'Test issue') + + # Before update, list tags + tags = pagure.lib.query.get_tags_of_project(self.session, repo) + self.assertEqual([tag.tag for tag in tags], []) + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(self.app.application, user): + + csrf_token = self.get_csrf() + + # Tag with a colon ':' + data = { + 'tag': ['green,red2'], + 'tag_color': ['#000'], + 'tag_description': [''], + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/update/tags', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + output_text = output.get_data(as_text=True) + self.assertIn( + 'Settings - test - Pagure', output_text) + self.assertIn( + '
Project ' + 'Settings
', output_text) + self.assertIn( + ' Tag: green,red2 contains one or more invalid ' + 'characters\n', output_text) + + # After update, list tags + tags = pagure.lib.query.get_tags_of_project(self.session, repo) + self.assertEqual(sorted([tag.tag for tag in tags]), []) + + @patch('pagure.lib.git.update_git') + @patch('pagure.lib.notify.send_email') def test_view_issue_namespace_comment(self, p_send_email, p_ugt): """ Test comment on the view_issue endpoint on namespaced project. """