From 138ab4df826322f19d225eea8c1c7c0d732b71d5 Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Nov 22 2017 14:40:26 +0000
Subject: Allow priorities and milestones to be 0 or -1


Up until now priorities could not be 0 or -1, not 0 because we weren't
explicitly when doing ``if issue.priority`` and not ``-1`` because it was
a reserved value that meant to say: neither a value nor None.

With this change, the default value is a random object which allows us
to set the value to anything we like including ``None`` (which means
resetting the priority).

Fixes https://pagure.io/pagure/issue/2482
Fixes https://pagure.io/pagure/issue/1888

Signed-off-by: Pierre-Yves Chibon <pingou@pingoured.fr>

---

diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py
index 537eafc..89a5b27 100644
--- a/pagure/lib/__init__.py
+++ b/pagure/lib/__init__.py
@@ -63,6 +63,13 @@ PAGURE_CI = None
 _log = logging.getLogger(__name__)
 
 
+class NoneObject(object):
+    """ Custom None object not to be used anywhere but allowing to set
+    values to None.
+    """
+    pass
+
+
 def set_redis(host, port, dbname):
     """ Set the redis connection with the specified information. """
     global REDIS
@@ -1622,9 +1629,26 @@ def new_tag(session, tag_name, tag_description, tag_color, project_id):
 
 
 def edit_issue(session, issue, ticketfolder, user, repo=None,
-               title=None, content=None, status=None, close_status=-1,
-               priority=-1, milestone=-1, private=None):
+               title=None, content=None, status=None,
+               close_status=NoneObject, priority=NoneObject,
+               milestone=NoneObject, private=None):
     ''' Edit the specified issue.
+
+    :arg session: the session to use to connect to the database.
+    :arg issue: the pagure.lib.model.Issue object to edit.
+    :arg ticketfolder: the path to the git repo storing the meta-data of
+        the issues of this repo
+    :arg user: the username of the user editing the issue,
+    :kwarg repo: somehow this isn't used anywhere here...
+    :kwarg title: the new title of the issue if it's being changed
+    :kwarg content: the new content of the issue if it's being changed
+    :kwarg status: the new status of the issue if it's being changed
+    :kwarg close_status: the new close_status of the issue if it's being
+        changed
+    :kwarg priority: the new priority of the issue if it's being changed
+    :kwarg milestone: the new milestone of the issue if it's being changed
+    :kwarg private: the new private of the issue if it's being changed
+
     '''
     user_obj = get_user(session, user)
     if status and status != 'Open' and issue.parents:
@@ -1649,12 +1673,12 @@ def edit_issue(session, issue, ticketfolder, user, repo=None,
             issue.closed_at = datetime.datetime.utcnow()
         elif issue.close_status:
             issue.close_status = None
-            close_status = -1
+            close_status = NoneObject
             edit.append('close_status')
         edit.append('status')
         messages.append(
             'Issue status updated to: %s (was: %s)' % (status, old_status))
-    if close_status != -1 and close_status != issue.close_status:
+    if close_status != NoneObject and close_status != issue.close_status:
         old_status = issue.close_status
         issue.close_status = close_status
         edit.append('close_status')
@@ -1666,7 +1690,7 @@ def edit_issue(session, issue, ticketfolder, user, repo=None,
             issue.closed_at = datetime.datetime.utcnow()
             edit.append('status')
         messages.append(msg)
-    if priority != -1:
+    if priority != NoneObject:
         priorities = issue.project.priorities
         try:
             priority = int(priority)
@@ -1694,7 +1718,7 @@ def edit_issue(session, issue, ticketfolder, user, repo=None,
         if old_private:
             msg += ' (was: %s)' % old_private
         messages.append(msg)
-    if milestone != -1 and milestone != issue.milestone:
+    if milestone != NoneObject and milestone != issue.milestone:
         old_milestone = issue.milestone
         issue.milestone = milestone
         edit.append('milestone')
diff --git a/pagure/templates/issue.html b/pagure/templates/issue.html
index 34c21e0..2fb1d90 100644
--- a/pagure/templates/issue.html
+++ b/pagure/templates/issue.html
@@ -274,7 +274,7 @@
           <fieldset class="form-group issue-metadata-display">
             <label><strong>Priority</strong></label>
             <div id="priority_plain">
-              {% if issue.priority %}
+              {% if issue.priority is not none  %}
               <span >{{ repo.priorities[issue.priority | string] }}</span>
               {% endif %}
             </div>
diff --git a/pagure/templates/issues.html b/pagure/templates/issues.html
index c7924b1..2c2ed3a 100644
--- a/pagure/templates/issues.html
+++ b/pagure/templates/issues.html
@@ -263,9 +263,11 @@
                         issue.last_updated | humanize}}</span>
             </td>
             {% endif %}
-            <td class="td-priority nowrap">
-              {% if issue.priority %}
-
+            <td class="td-priority nowrap" data-sort-value="{{
+              issue.priority
+              if issue.priority
+              else repo.priorities | increment_largest_priority | string }}">
+              {% if issue.priority is not none %}
                 <a href="{{ url_for('view_issues',
                     repo=repo.name,
                     username=username,
diff --git a/tests/test_pagure_flask_ui_priorities.py b/tests/test_pagure_flask_ui_priorities.py
index 089b369..2815fd7 100644
--- a/tests/test_pagure_flask_ui_priorities.py
+++ b/tests/test_pagure_flask_ui_priorities.py
@@ -443,6 +443,432 @@ class PagureFlaskPrioritiestests(tests.Modeltests):
             repo = pagure.lib._get_project(self.session, 'test')
             self.assertEqual(repo.priorities, {})
 
+    @patch('pagure.lib.git.update_git')
+    @patch('pagure.lib.notify.send_email')
+    def test_reset_priorities_None(self, p_send_email, p_ugt):
+        """ Test resetting the priorities of a repo. """
+        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)
+
+        # Start from scrach on priorities
+        repo = pagure.lib._get_project(self.session, 'test')
+        self.assertEqual(repo.priorities, {})
+
+        user = tests.FakeUser()
+        user.username = 'pingou'
+        with tests.user_set(pagure.APP, user):
+
+            # Get the CSRF token
+            output = self.app.get('/test/settings')
+            self.assertEqual(output.status_code, 200)
+            self.assertIn(
+                '<title>Settings - test - Pagure</title>', output.data)
+            self.assertIn('<h3>Settings for test</h3>', output.data)
+
+            csrf_token = output.data.split(
+                'name="csrf_token" type="hidden" value="')[1].split('">')[0]
+
+            # 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)
+            self.assertIn(
+                '<title>Settings - test - Pagure</title>', output.data)
+            self.assertIn('<h3>Settings for test</h3>', output.data)
+
+            # Check the result of the action -- Priority recorded
+            repo = pagure.lib._get_project(self.session, 'test')
+            self.assertEqual(
+                repo.priorities,
+                {u'': u'', u'1': u'High', u'2': u'Normal', u'3': u'Low'}
+            )
+
+            # Create an issue
+            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(
+                '<title>Issue #1: Test issue - test - Pagure</title>',
+                output.data)
+            self.assertIn(
+                '<a class="btn btn-primary btn-sm" '
+                'href="/test/issue/1/edit" title="Edit this issue">',
+                output.data)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
+            # Check that the ticket *does* have priorities
+            output = self.app.get('/test/issue/1')
+            self.assertEqual(output.status_code, 200)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
+            # Reset the priorities
+            data = {
+                'priority': None,
+                'csrf_token': csrf_token,
+            }
+            output = self.app.post(
+                '/test/update/priorities', data=data, follow_redirects=True)
+            self.assertEqual(output.status_code, 200)
+            self.assertIn(
+                '<title>Settings - test - Pagure</title>', output.data)
+            self.assertIn('<h3>Settings for test</h3>', output.data)
+
+            # Check that the issue list renders fine
+            output = self.app.get('/test/issues')
+            self.assertEqual(output.status_code, 200)
+
+            # Check that the ticket *does not* have priorities
+            output = self.app.get('/test/issue/1')
+            self.assertEqual(output.status_code, 200)
+            self.assertNotIn('<div id="priority_plain">', output.data)
+            self.assertNotIn('<option value="1">High</option>', output.data)
+
+            # Check the result of the action -- Priority recorded
+            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_set_priority_1_and_back(self):
+        """ Test setting the priority of a ticket to 1. """
+
+        tests.create_projects(self.session)
+        tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True)
+
+        # Start from scrach on priorities
+        repo = pagure.lib._get_project(self.session, 'test')
+        self.assertEqual(repo.priorities, {})
+
+        user = tests.FakeUser()
+        user.username = 'pingou'
+        with tests.user_set(pagure.APP, user):
+
+            # Get the CSRF token
+            output = self.app.get('/test/settings')
+            self.assertEqual(output.status_code, 200)
+            self.assertIn(
+                '<title>Settings - test - Pagure</title>', output.data)
+            self.assertIn('<h3>Settings for test</h3>', output.data)
+
+            csrf_token = output.data.split(
+                'name="csrf_token" type="hidden" value="')[1].split('">')[0]
+
+            # Set some priorities
+            data = {
+                'priority_weigth': [-1, 0, 1, 2, 3],
+                'priority_title': [
+                    'Sky Falling', 'Urgent', '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)
+            self.assertIn(
+                '<title>Settings - test - Pagure</title>', output.data)
+            self.assertIn('<h3>Settings for test</h3>', output.data)
+
+            # Check the result of the action -- Priority recorded
+            repo = pagure.lib._get_project(self.session, 'test')
+            self.assertEqual(
+                repo.priorities,
+                {u'': u'', u'-1': u'Sky Falling', u'0': u'Urgent',
+                 u'1': u'High', u'2': u'Normal', u'3': u'Low'}
+            )
+
+            # Create an issue
+            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(
+                '<title>Issue #1: Test issue - test - Pagure</title>',
+                output.data)
+            self.assertIn(
+                '<a class="btn btn-primary btn-sm" '
+                'href="/test/issue/1/edit" title="Edit this issue">',
+                output.data)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
+            # Check that the ticket *does* have priorities
+            output = self.app.get('/test/issue/1')
+            self.assertEqual(output.status_code, 200)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn(
+                '<option value="-1">Sky Falling</option>', output.data)
+            self.assertIn('<option value="0">Urgent</option>', output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
+            # Set the priority to High
+
+            data = {
+                'priority': '1',
+                'csrf_token': csrf_token,
+            }
+
+            output = self.app.post(
+                '/test/issue/1/update', data=data, follow_redirects=True)
+            self.assertEqual(output.status_code, 200)
+            self.assertIn(
+                '<title>Issue #1: Test issue - test - Pagure</title>',
+                output.data)
+            self.assertIn(
+                '<a class="btn btn-primary btn-sm" '
+                'href="/test/issue/1/edit" title="Edit this issue">',
+                output.data)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn(
+                '<option value="-1">Sky Falling</option>', output.data)
+            self.assertIn('<option value="0">Urgent</option>', output.data)
+            self.assertIn(
+                '<option selected value="1">High</option>', output.data)
+
+            # Reset the priority
+            data = {
+                'priority': '',
+                'csrf_token': csrf_token,
+            }
+
+            output = self.app.post(
+                '/test/issue/1/update', data=data, follow_redirects=True)
+            self.assertEqual(output.status_code, 200)
+            self.assertIn(
+                '<title>Issue #1: Test issue - test - Pagure</title>',
+                output.data)
+            self.assertIn(
+                '<a class="btn btn-primary btn-sm" '
+                'href="/test/issue/1/edit" title="Edit this issue">',
+                output.data)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn(
+                '<option value="-1">Sky Falling</option>', output.data)
+            self.assertIn('<option value="0">Urgent</option>', output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
+    @patch('pagure.lib.git.update_git', MagicMock(return_value=True))
+    @patch('pagure.lib.notify.send_email', MagicMock(return_value=True))
+    def test_set_priority_0(self):
+        """ Test setting the priority of a ticket to 0. """
+
+        tests.create_projects(self.session)
+        tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True)
+
+        # Start from scrach on priorities
+        repo = pagure.lib._get_project(self.session, 'test')
+        self.assertEqual(repo.priorities, {})
+
+        user = tests.FakeUser()
+        user.username = 'pingou'
+        with tests.user_set(pagure.APP, user):
+
+            # Get the CSRF token
+            output = self.app.get('/test/settings')
+            self.assertEqual(output.status_code, 200)
+            self.assertIn(
+                '<title>Settings - test - Pagure</title>', output.data)
+            self.assertIn('<h3>Settings for test</h3>', output.data)
+
+            csrf_token = output.data.split(
+                'name="csrf_token" type="hidden" value="')[1].split('">')[0]
+
+            # Set some priorities
+            data = {
+                'priority_weigth': [-1, 0, 1, 2, 3],
+                'priority_title': [
+                    'Sky Falling', 'Urgent', '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)
+            self.assertIn(
+                '<title>Settings - test - Pagure</title>', output.data)
+            self.assertIn('<h3>Settings for test</h3>', output.data)
+
+            # Check the result of the action -- Priority recorded
+            repo = pagure.lib._get_project(self.session, 'test')
+            self.assertEqual(
+                repo.priorities,
+                {u'': u'', u'-1': u'Sky Falling', u'0': u'Urgent',
+                 u'1': u'High', u'2': u'Normal', u'3': u'Low'}
+            )
+
+            # Create an issue
+            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(
+                '<title>Issue #1: Test issue - test - Pagure</title>',
+                output.data)
+            self.assertIn(
+                '<a class="btn btn-primary btn-sm" '
+                'href="/test/issue/1/edit" title="Edit this issue">',
+                output.data)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
+            # Check that the ticket *does* have priorities
+            output = self.app.get('/test/issue/1')
+            self.assertEqual(output.status_code, 200)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn(
+                '<option value="-1">Sky Falling</option>', output.data)
+            self.assertIn('<option value="0">Urgent</option>', output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
+            # Set the priority to Urgent
+
+            data = {
+                'priority': '0',
+                'csrf_token': csrf_token,
+            }
+
+            output = self.app.post(
+                '/test/issue/1/update', data=data, follow_redirects=True)
+            self.assertEqual(output.status_code, 200)
+            self.assertIn(
+                '<title>Issue #1: Test issue - test - Pagure</title>',
+                output.data)
+            self.assertIn(
+                '<a class="btn btn-primary btn-sm" '
+                'href="/test/issue/1/edit" title="Edit this issue">',
+                output.data)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn(
+                '<option value="-1">Sky Falling</option>', output.data)
+            self.assertIn(
+                '<option selected value="0">Urgent</option>',
+                output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
+    @patch('pagure.lib.git.update_git', MagicMock(return_value=True))
+    @patch('pagure.lib.notify.send_email', MagicMock(return_value=True))
+    def test_set_priority_minus1(self):
+        """ Test setting the priority of a ticket to -1. """
+
+        tests.create_projects(self.session)
+        tests.create_projects_git(os.path.join(self.path, 'repos'), bare=True)
+
+        # Start from scrach on priorities
+        repo = pagure.lib._get_project(self.session, 'test')
+        self.assertEqual(repo.priorities, {})
+
+        user = tests.FakeUser()
+        user.username = 'pingou'
+        with tests.user_set(pagure.APP, user):
+
+            # Get the CSRF token
+            output = self.app.get('/test/settings')
+            self.assertEqual(output.status_code, 200)
+            self.assertIn(
+                '<title>Settings - test - Pagure</title>', output.data)
+            self.assertIn('<h3>Settings for test</h3>', output.data)
+
+            csrf_token = output.data.split(
+                'name="csrf_token" type="hidden" value="')[1].split('">')[0]
+
+            # Set some priorities
+            data = {
+                'priority_weigth': [-1, 0, 1, 2, 3],
+                'priority_title': [
+                    'Sky Falling', 'Urgent', '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)
+            self.assertIn(
+                '<title>Settings - test - Pagure</title>', output.data)
+            self.assertIn('<h3>Settings for test</h3>', output.data)
+
+            # Check the result of the action -- Priority recorded
+            repo = pagure.lib._get_project(self.session, 'test')
+            self.assertEqual(
+                repo.priorities,
+                {u'': u'', u'-1': u'Sky Falling', u'0': u'Urgent',
+                 u'1': u'High', u'2': u'Normal', u'3': u'Low'}
+            )
+
+            # Create an issue
+            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(
+                '<title>Issue #1: Test issue - test - Pagure</title>',
+                output.data)
+            self.assertIn(
+                '<a class="btn btn-primary btn-sm" '
+                'href="/test/issue/1/edit" title="Edit this issue">',
+                output.data)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
+            # Check that the ticket *does* have priorities
+            output = self.app.get('/test/issue/1')
+            self.assertEqual(output.status_code, 200)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn(
+                '<option value="-1">Sky Falling</option>', output.data)
+            self.assertIn('<option value="0">Urgent</option>', output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
+            # Set the priority to Sky Falling
+
+            data = {
+                'priority': '-1',
+                'csrf_token': csrf_token,
+            }
+
+            output = self.app.post(
+                '/test/issue/1/update', data=data, follow_redirects=True)
+            self.assertEqual(output.status_code, 200)
+            self.assertIn(
+                '<title>Issue #1: Test issue - test - Pagure</title>',
+                output.data)
+            self.assertIn(
+                '<a class="btn btn-primary btn-sm" '
+                'href="/test/issue/1/edit" title="Edit this issue">',
+                output.data)
+            self.assertIn('<div id="priority_plain">', output.data)
+            self.assertIn(
+                '<option selected value="-1">Sky Falling</option>',
+                output.data)
+            self.assertIn('<option value="0">Urgent</option>', output.data)
+            self.assertIn('<option value="1">High</option>', output.data)
+
     @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):