PR#1627: Allow dropping of assignments for an issue (Fixes: #1255) -

pagure

#1627 Allow dropping of assignments for an issue (Fixes: #1255)

Merged Dec 15, 2016 by pingou. Opened Dec 1, 2016 by pfrields.

pfrields/pagure issue1255 into master

Download 1627.patch

pfrields commented Dec 1, 2016

Signed-off-by: Paul W. Frields stickster@gmail.com

Unit test coming shortly.

pingou commented Dec 1, 2016

Indentation looks odd here, mind pushing it a little to the right?

pingou commented Dec 1, 2016

I know it wasn't there before, but I'd propose: return 'Nothing to change'

pingou commented Dec 1, 2016

Ok, so I see only one test failing (jenkins is drunk), I propose the following change:

diff --git a/ pagure/api/issue.py b/ pagure/api/issue.py
index 1ef6edf..41ed130 100644
--- a/ pagure/api/issue.py      
+++ b/ pagure/api/issue.py      
@@ -825,7 +825,7 @@ def api_assign_issue(repo, issueid, username=None, namespace=None):
     form = pagure.forms.AssignIssueForm(csrf_enabled=False)
     if form.validate_on_submit():
-        assignee = form.assignee.data
+        assignee = form.assignee.data or None
         try:
             # New comment
             message = pagure.lib.add_issue_assignee(
diff --git a/ pagure/lib/__init__.py b/ pagure/lib/__init__.py
index 1a1ea44..a4fb42c 100644
--- a/ pagure/lib/__init__.py   
+++ b/ pagure/lib/__init__.py   
@@ -414,7 +414,7 @@ def add_issue_assignee(session, issue, assignee, user, ticketfolder,
         return 'Assignee reset'
     elif not assignee and issue.assignee is None:
-        return
+        return 'Nothing to change'
     # Validate the assignee
     assignee_obj = get_user(session, assignee)
diff --git a/ tests/test_pagure_flask_api_issue.py b/ tests/test_pagure_flask_api_issue.py
index e044330..71c850c 100644
--- a/ tests/test_pagure_flask_api_issue.py     
+++ b/ tests/test_pagure_flask_api_issue.py     
@@ -1443,34 +1443,76 @@ class PagureFlaskApiIssuetests(tests.Modeltests):
         issue = pagure.lib.search_issues(self.session, repo, issueid=1)
         self.assertEqual(len(issue.comments), 0)
+        # No change
+        repo = pagure.lib.get_project(self.session, 'test')
+        issue = pagure.lib.search_issues(self.session, repo, issueid=1)
+        self.assertEqual(issue.status, 'Open')
+
         data = {
-            'title': 'test issue',
+            'assignee': 'pingou',
         }
-        # Incomplete request
+        # Valid request
         output = self.app.post(
             '/api/0/test/issue/1/assign', data=data, headers=headers)
-        self.assertEqual(output.status_code, 400)
+        self.assertEqual(output.status_code, 200)
         data = json.loads(output.data)
         self.assertDictEqual(
             data,
-            {
-              "error": "Invalid or incomplete input submited",
-              "error_code": "EINVALIDREQ",
-              "errors": {"assignee": ["This field is required."]}
-            }
+            {'message': 'Issue assigned'}
         )
-        # No change
+        # Un-assign
+        output = self.app.post(
+            '/api/0/test/issue/1/assign', data=data, headers=headers)
+        self.assertEqual(output.status_code, 200)
+        data = json.loads(output.data)
+        self.assertDictEqual(
+            data,
+            {'message': 'Assignee reset'}
+        )
         repo = pagure.lib.get_project(self.session, 'test')
         issue = pagure.lib.search_issues(self.session, repo, issueid=1)
-        self.assertEqual(issue.status, 'Open')
+        self.assertEqual(issue.assignee, None)
-        data = {
-            'assignee': 'pingou',
-        }
+        # Un-assign
+        data = {'assignee': None}
+        output = self.app.post(
+            '/api/0/test/issue/1/assign', data=data, headers=headers)
+        self.assertEqual(output.status_code, 200)
+        data = json.loads(output.data)
+        self.assertDictEqual(
+            data,
+            {'message': 'Nothing to change'}
+        )
+        repo = pagure.lib.get_project(self.session, 'test')
+        issue = pagure.lib.search_issues(self.session, repo, issueid=1)
+        self.assertEqual(issue.assignee, None)
-        # Valid request
+        # Re-assign for the rest of the tests
+        data = {'assignee': 'pingou'}
+        output = self.app.post(
+            '/api/0/test/issue/1/assign', data=data, headers=headers)
+        self.assertEqual(output.status_code, 200)
+        data = json.loads(output.data)
+        self.assertDictEqual(
+            data,
+            {'message': 'Issue assigned'}
+        )
+
+        # Un-assign
+        data = {'assignee': ''}
+        output = self.app.post(
+            '/api/0/test/issue/1/assign', data=data, headers=headers)
+        self.assertEqual(output.status_code, 200)
+        data = json.loads(output.data)
+        self.assertDictEqual(
+            data,
+            {'message': 'Assignee reset'}
+        )
+
+        # Re-assign for the rest of the tests
+        data = {'assignee': 'pingou'}
         output = self.app.post(
             '/api/0/test/issue/1/assign', data=data, headers=headers)
         self.assertEqual(output.status_code, 200)

Basically, we :
- Set the assignee to None if the content of the data is ''
- Be explicit when nothing changed
- remove the old check that a POST request without assignee would not work
- check that a POST request without assignee does reset the assignee
- check that a follow-up POST request with assignee as None does not change anything
- re-assign the issue
- un-assign again, but in a different way
- re-assign again so the rest of the tests keep working :)