From dff6670c01543053ad63710fd8e958e6da77a2f9 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 20 2017 13:37:52 +0000 Subject: [PATCH 1/4] Add a default_priority field to the projects This field must be one of the priorities associated with the project and will be used as the default priority of the new tickets created. Fixes https://pagure.io/pagure/issue/2691 Signed-off-by: Pierre-Yves Chibon --- diff --git a/alembic/versions/e4dbfcd20f42_add_default_priority_to_projects.py b/alembic/versions/e4dbfcd20f42_add_default_priority_to_projects.py new file mode 100644 index 0000000..650a0c9 --- /dev/null +++ b/alembic/versions/e4dbfcd20f42_add_default_priority_to_projects.py @@ -0,0 +1,28 @@ +"""Add default_priority to projects + +Revision ID: e4dbfcd20f42 +Revises: 21292448a775 +Create Date: 2017-10-20 13:34:01.323657 + +""" + +# revision identifiers, used by Alembic. +revision = 'e4dbfcd20f42' +down_revision = '21292448a775' + + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ''' Add default_priority column to projects table''' + + op.add_column( + 'projects', + sa.Column('default_priority', sa.Text, nullable=True) + ) + +def downgrade(): + ''' Revert the default_priority column added''' + op.drop_column('projects', 'default_priority') diff --git a/pagure/forms.py b/pagure/forms.py index e784597..a07b8f3 100644 --- a/pagure/forms.py +++ b/pagure/forms.py @@ -664,6 +664,26 @@ class DefaultBranchForm(PagureForm): ] +class DefaultPriorityForm(PagureForm): + """Form to change the default priority for a repository""" + priority = wtforms.SelectField( + 'default_priority', + [wtforms.validators.optional()], + choices=[] + ) + + def __init__(self, *args, **kwargs): + """ Calls the default constructor with the normal argument but + uses the list of collection provided to fill the choices of the + drop-down list. + """ + super(DefaultPriorityForm, self).__init__(*args, **kwargs) + if 'priorities' in kwargs: + self.priority.choices = [ + (priority, priority) for priority in kwargs['priorities'] + ] + + class EditCommentForm(PagureForm): """ Form to verify that comment is not empty """ diff --git a/pagure/lib/model.py b/pagure/lib/model.py index 81f2390..5df8631 100644 --- a/pagure/lib/model.py +++ b/pagure/lib/model.py @@ -374,6 +374,7 @@ class Project(BASE): ), nullable=True) _priorities = sa.Column(sa.Text, nullable=True) + default_priority = sa.Column(sa.Text, nullable=True) _milestones = sa.Column(sa.Text, nullable=True) _quick_replies = sa.Column(sa.Text, nullable=True) _reports = sa.Column(sa.Text, nullable=True) diff --git a/pagure/templates/settings.html b/pagure/templates/settings.html index 0eafd21..aa2ea17 100644 --- a/pagure/templates/settings.html +++ b/pagure/templates/settings.html @@ -590,6 +590,7 @@ and repo.settings.get('issue_tracker', True) %}
+
Priorities
@@ -657,6 +658,47 @@
+ + {% if repo.priorities %} +
+ Default Priority +
+
+

+ The default priority will be set to all issues created after + it has been set. +

+
+ +
+ {{ tag_form.csrf_token }} +
+
+
+ Default priority +
+
+
+ {{ priority_form.priority(class_="c-select") }} +
+ +
+
+ +
+
+
+
+ {% endif %} + diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index d79c370..e7751f5 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -1035,6 +1035,9 @@ def view_settings(repo, username=None, namespace=None): branches = repo_obj.listall_branches() branches_form = pagure.forms.DefaultBranchForm(branches=branches) + priority_form = pagure.forms.DefaultPriorityForm( + priorities=repo.priorities.values()) + if form.validate_on_submit(): settings = {} for key in flask.request.form: @@ -1068,6 +1071,7 @@ def view_settings(repo, username=None, namespace=None): if flask.request.method == 'GET' and branchname: branches_form.branches.data = branchname + priority_form.priorities.data = repo.default_priority return flask.render_template( 'settings.html', @@ -1079,6 +1083,7 @@ def view_settings(repo, username=None, namespace=None): form=form, tag_form=tag_form, branches_form=branches_form, + priority_form=priority_form, tags=tags, plugins=plugins, branchname=branchname, @@ -1281,6 +1286,57 @@ def update_priorities(repo, username=None, namespace=None): namespace=repo.namespace)) +@APP.route('//update/default_priority', methods=['POST']) +@APP.route('///update/default_priority', methods=['POST']) +@APP.route('/fork///update/default_priority', methods=['POST']) +@APP.route( + '/fork////update/default_priority', + methods=['POST']) +@login_required +def default_priority(repo, username=None, namespace=None): + """ Update the default priority of a project. + """ + if admin_session_timedout(): + flask.flash('Action canceled, try it again', 'error') + url = flask.url_for( + 'view_settings', username=username, repo=repo, + namespace=namespace) + return flask.redirect( + flask.url_for('auth_login', next=url)) + + repo = flask.g.repo + + if not repo.settings.get('issue_tracker', True): + flask.abort(404, 'No issue tracker found for this project') + + if not flask.g.repo_admin: + flask.abort( + 403, + 'You are not allowed to change the settings for this project') + + form = pagure.forms.DefaultPriorityForm( + priorities=repo.priorities.values()) + + if form.validate_on_submit(): + priority = form.priority.data or None + if priority in repo.priorities.values() or priority is None: + repo.default_priority = priority + try: + SESSION.add(repo) + SESSION.commit() + if priority: + flask.flash('Default priority set to %s' % priority) + else: + flask.flash('Default priority reset') + except SQLAlchemyError as err: # pragma: no cover + SESSION.rollback() + flask.flash(str(err), 'error') + + return flask.redirect(flask.url_for( + 'view_settings', username=username, repo=repo.name, + namespace=repo.namespace)) + + @APP.route('//update/milestones', methods=['POST']) @APP.route('///update/milestones', methods=['POST']) @APP.route('/fork///update/milestones', methods=['POST']) diff --git a/tests/test_pagure_flask_ui_priorities.py b/tests/test_pagure_flask_ui_priorities.py index 762e9a8..493c8f0 100644 --- a/tests/test_pagure_flask_ui_priorities.py +++ b/tests/test_pagure_flask_ui_priorities.py @@ -20,7 +20,7 @@ import tempfile import os import pygit2 -from mock import patch +from mock import patch, MagicMock sys.path.insert(0, os.path.join(os.path.dirname( os.path.abspath(__file__)), '..')) @@ -444,6 +444,140 @@ class PagureFlaskPrioritiestests(tests.Modeltests): repo = pagure.lib._get_project(self.session, 'test') self.assertEqual(repo.priorities, {}) + @patch('pagure.lib.git.update_git', MagicMock(return_value=True)) + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def test_default_priority(self): + """ Test updating the default priority of a repo. """ + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True) + + # Check the default priorities + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(repo.priorities, {}) + self.assertEqual(repo.default_priority, None) + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + + csrf_token = self.get_csrf() + + # Set some priorities + data = { + 'priority_weigth': [1, 2, 3], + 'priority_title': ['High', 'Normal', 'Low'], + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/update/priorities', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + # Check the redirect + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn('

Settings for test

', output.data) + # Check the ordering + self.assertTrue( + output.data.find('High') < output.data.find('Normal')) + self.assertTrue( + output.data.find('Normal') < output.data.find('Low')) + # Check the result of the action -- Priority recorded + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual( + repo.priorities, + {u'': u'', u'1': u'High', u'2': u'Normal', u'3': u'Low'} + ) + + # Try setting the default priority -- no csrf + data = {'priority': 'High'} + output = self.app.post( + '/test/update/default_priority', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + # Check the redirect + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn('

Settings for test

', output.data) + # Check the result of the action -- default_priority no change + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(repo.default_priority, None) + + # Try setting the default priority + data = {'priority': 'High', 'csrf_token': csrf_token} + output = self.app.post( + '/test/update/default_priority', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + # Check the redirect + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn('

Settings for test

', output.data) + self.assertIn( + '\n Default priority set ' + 'to High', output.data) + # Check the result of the action -- default_priority no change + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(repo.default_priority, 'High') + + # Try setting a wrong default priority + data = {'priority': 'Smooth', 'csrf_token': csrf_token} + output = self.app.post( + '/test/update/default_priority', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + # Check the redirect + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn('

Settings for test

', output.data) + # Check the result of the action -- default_priority no change + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(repo.default_priority, 'High') + + # reset the default priority + data = {'csrf_token': csrf_token, 'priority': ''} + output = self.app.post( + '/test/update/default_priority', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + # Check the redirect + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn('

Settings for test

', output.data) + self.assertIn( + '\n Default priority reset', + output.data) + # Check the result of the action -- default_priority no change + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(repo.default_priority, None) + + # Check the behavior if the project disabled the issue tracker + settings = repo.settings + settings['issue_tracker'] = False + repo.settings = settings + self.session.add(repo) + self.session.commit() + + output = self.app.post( + '/test/update/default_priority', data=data) + self.assertEqual(output.status_code, 404) + + # Check for an invalid project + output = self.app.post( + '/foo/update/default_priority', data=data) + self.assertEqual(output.status_code, 404) + + # Check for a non-admin user + settings = repo.settings + settings['issue_tracker'] = True + repo.settings = settings + self.session.add(repo) + self.session.commit() + + user.username = 'ralph' + with tests.user_set(pagure.APP, user): + output = self.app.post( + '/test/update/default_priority', data=data) + self.assertEqual(output.status_code, 403) + if __name__ == '__main__': unittest.main(verbosity=2) From 606ecbeb1dd8674605bca7ab7e93608a60b9db3e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 20 2017 13:37:52 +0000 Subject: [PATCH 2/4] If the default priority is removed from the list of priorities, reset it So that the default priority doesn't stay "Foo" if "Foo" is no longer part of the list of priorities. Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/repo.py b/pagure/ui/repo.py index e7751f5..a127c3c 100644 --- a/pagure/ui/repo.py +++ b/pagure/ui/repo.py @@ -1274,6 +1274,11 @@ def update_priorities(repo, username=None, namespace=None): priorities[''] = '' try: repo.priorities = priorities + if repo.default_priority not in priorities.values(): + flask.flash( + 'Default priority reset as it is no longer one of ' + 'set priorities.') + repo.default_priority = None SESSION.add(repo) SESSION.commit() flask.flash('Priorities updated') diff --git a/tests/test_pagure_flask_ui_priorities.py b/tests/test_pagure_flask_ui_priorities.py index 493c8f0..e47e8f7 100644 --- a/tests/test_pagure_flask_ui_priorities.py +++ b/tests/test_pagure_flask_ui_priorities.py @@ -578,6 +578,100 @@ class PagureFlaskPrioritiestests(tests.Modeltests): '/test/update/default_priority', data=data) self.assertEqual(output.status_code, 403) + @patch('pagure.lib.git.update_git', MagicMock(return_value=True)) + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def test_default_priority_reset_when_updating_priorities(self): + """ Test updating the default priority of a repo when updating the + priorities. + """ + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True) + + # Check the default priorities + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(repo.priorities, {}) + self.assertEqual(repo.default_priority, None) + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + + csrf_token = self.get_csrf() + + # Set some priorities + data = { + 'priority_weigth': [1, 2, 3], + 'priority_title': ['High', 'Normal', 'Low'], + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/update/priorities', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + # Check the redirect + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn('

Settings for test

', output.data) + # Check the ordering + self.assertTrue( + output.data.find('High') < output.data.find('Normal')) + self.assertTrue( + output.data.find('Normal') < output.data.find('Low')) + # Check the result of the action -- Priority recorded + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual( + repo.priorities, + {u'': u'', u'1': u'High', u'2': u'Normal', u'3': u'Low'} + ) + + # Try setting the default priority + data = {'priority': 'High', 'csrf_token': csrf_token} + output = self.app.post( + '/test/update/default_priority', data=data, + follow_redirects=True) + self.assertEqual(output.status_code, 200) + # Check the redirect + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn('

Settings for test

', output.data) + self.assertIn( + '\n Default priority set ' + 'to High', output.data) + # Check the result of the action -- default_priority no change + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(repo.default_priority, 'High') + + # Remove the Hight priority + data = { + 'priority_weigth': [1, 2], + 'priority_title': ['Normal', 'Low'], + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/update/priorities', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + # Check the redirect + self.assertIn( + 'Settings - test - Pagure', output.data) + self.assertIn('

Settings for test

', output.data) + self.assertIn( + '\n Priorities updated', + output.data) + self.assertIn( + '\n Default priority reset ' + 'as it is no longer one of set priorities.', + output.data) + # Check the ordering + self.assertTrue( + output.data.find('Normal') < output.data.find('Low')) + # Check the result of the action -- Priority recorded + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual( + repo.priorities, + {u'': u'', u'1': u'Normal', u'2': u'Low'} + ) + # Default priority is now None + self.assertIsNone(repo.default_priority) + if __name__ == '__main__': unittest.main(verbosity=2) From 521996273ca83c5bc550a1818acee3b077100800 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 20 2017 13:37:52 +0000 Subject: [PATCH 3/4] If the project has a default priority, set it when creating a new ticket Fixes https://pagure.io/pagure/issue/2691 Signed-off-by: Pierre-Yves Chibon --- diff --git a/pagure/ui/issues.py b/pagure/ui/issues.py index ef1f0e8..9465906 100644 --- a/pagure/ui/issues.py +++ b/pagure/ui/issues.py @@ -894,6 +894,12 @@ def new_issue(repo, username=None, namespace=None): flask.g.fas_user.username)) try: + priority = None + if repo.default_priority: + for key, val in repo.priorities.items(): + if repo.default_priority == val: + priority = key + issue = pagure.lib.new_issue( SESSION, repo=repo, @@ -901,6 +907,7 @@ def new_issue(repo, username=None, namespace=None): content=content, private=private or False, user=flask.g.fas_user.username, + priority=priority, ticketfolder=APP.config['TICKETS_FOLDER'], ) SESSION.commit() diff --git a/tests/test_pagure_flask_ui_priorities.py b/tests/test_pagure_flask_ui_priorities.py index e47e8f7..089b369 100644 --- a/tests/test_pagure_flask_ui_priorities.py +++ b/tests/test_pagure_flask_ui_priorities.py @@ -1,7 +1,7 @@ # -*- coding: utf-8 -*- """ - (c) 2016 - Copyright Red Hat Inc + (c) 2016-2017 - Copyright Red Hat Inc Authors: Pierre-Yves Chibon @@ -46,7 +46,6 @@ class PagureFlaskPrioritiestests(tests.Modeltests): pagure.ui.repo.SESSION = self.session pagure.ui.issues.SESSION = self.session - @patch('pagure.lib.git.update_git') @patch('pagure.lib.notify.send_email') def test_ticket_with_no_priority(self, p_send_email, p_ugt): @@ -672,6 +671,52 @@ class PagureFlaskPrioritiestests(tests.Modeltests): # Default priority is now None self.assertIsNone(repo.default_priority) + @patch('pagure.lib.git.update_git', MagicMock(return_value=True)) + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + def test_default_priority_on_new_ticket(self): + """ Test updating the default priority of a repo. """ + tests.create_projects(self.session) + tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True) + + # Set some priority and the default one + repo = pagure.get_authorized_project(self.session, 'test') + repo.priorities = {'1': 'High', '2': 'Normal'} + repo.default_priority = 'Normal' + self.session.add(repo) + self.session.commit() + + # Check the default priorities + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(repo.priorities, {u'1': u'High', u'2': u'Normal'}) + self.assertEqual(repo.default_priority, 'Normal') + + user = tests.FakeUser() + user.username = 'pingou' + with tests.user_set(pagure.APP, user): + + csrf_token = self.get_csrf() + + data = { + 'title': 'Test issue', + 'issue_content': 'We really should improve on this issue', + 'status': 'Open', + 'csrf_token': csrf_token, + } + output = self.app.post( + '/test/new_issue', data=data, follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + 'Issue #1: Test issue - test - Pagure', + output.data) + self.assertIn( + '', + output.data) + + repo = pagure.get_authorized_project(self.session, 'test') + self.assertEqual(len(repo.issues), 1) + self.assertEqual(repo.issues[0].priority, 2) + if __name__ == '__main__': unittest.main(verbosity=2) From 282d02871559053c3f15181956f7c43dd445ebb5 Mon Sep 17 00:00:00 2001 From: Pierre-Yves Chibon Date: Oct 20 2017 13:37:52 +0000 Subject: [PATCH 4/4] Fix unit-tests for the change made to disallow deleting a project sometime Basically, we do not allow a project to be deleted when its ACLs are being refreshed in the backend. It seems we sometime have a small race condition in the tests, so here we're making sure we're testing when it is locked and when it is not. Signed-off-by: Pierre-Yves Chibon --- diff --git a/tests/test_pagure_flask_ui_repo_delete_project.py b/tests/test_pagure_flask_ui_repo_delete_project.py index 95a61de..076ec67 100644 --- a/tests/test_pagure_flask_ui_repo_delete_project.py +++ b/tests/test_pagure_flask_ui_repo_delete_project.py @@ -134,10 +134,48 @@ class PagureFlaskDeleteRepotests(tests.Modeltests): @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) @patch('pagure.ui.repo.admin_session_timedout', MagicMock(return_value=False)) + def test_delete_fork_when_project_off_refreshing(self): + """ Test the delete_repo endpoint for a fork when only deleting main + project is forbidden but the fork is being refreshed in the backend + """ + project = pagure.get_authorized_project( + self.session, project_name='test', user='pingou') + self.assertIsNotNone(project) + # Ensure the project isn't read-only + project.read_only = True + self.session.add(project) + self.session.commit() + + user = tests.FakeUser(username='pingou') + with tests.user_set(pagure.APP, user): + output = self.app.post( + '/fork/pingou/test/delete', follow_redirects=True) + self.assertEqual(output.status_code, 200) + self.assertIn( + '\n The ACLs of this project ' + 'are being refreshed in the backend this prevents the ' + 'project from being deleted. Please wait for this task to ' + 'finish before trying again. Thanks!', output.data) + + projects = pagure.lib.search_projects(self.session) + self.assertEqual(len(projects), 4) + + @patch.dict('pagure.APP.config', {'ENABLE_DEL_PROJECTS': False}) + @patch.dict('pagure.APP.config', {'ENABLE_DEL_FORKS': True}) + @patch('pagure.lib.notify.send_email', MagicMock(return_value=True)) + @patch('pagure.ui.repo.admin_session_timedout', + MagicMock(return_value=False)) def test_delete_fork_when_project_off(self): """ Test the delete_repo endpoint for a fork when only deleting main project is forbidden. """ + project = pagure.get_authorized_project( + self.session, project_name='test', user='pingou') + self.assertIsNotNone(project) + # Ensure the project isn't read-only + project.read_only = False + self.session.add(project) + self.session.commit() user = tests.FakeUser(username='pingou') with tests.user_set(pagure.APP, user): @@ -181,8 +219,10 @@ class PagureFlaskDeleteRepotests(tests.Modeltests): with tests.user_set(pagure.APP, user): output = self.app.get('/fork/pingou/test/settings') self.assertEqual(output.status_code, 200) - self.assertNotIn('
', output.data) + self.assertIn( + '  Delete the forks/pingou/test project', output.data)