From 8b4a8fdf101a23921deec31b35254fe1fefc62ba Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Mar 06 2020 12:56:09 +0000
Subject: Do not commit to the DB empty rows


If there are existing overrides and we're resetting both of them, then
delete the entire record.
If there are no existing overrides, check that we are creating one before
creating a new record.

Adjust tests to make sure this is the behavior that we have.

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

---

diff --git a/pagure_distgit/plugin.py b/pagure_distgit/plugin.py
index 79f1d22..7df6bb6 100644
--- a/pagure_distgit/plugin.py
+++ b/pagure_distgit/plugin.py
@@ -423,20 +423,23 @@ def bzoverride_patch_endpoint(repo, namespace):
 
     form = pagure_distgit.forms.BZOverrideForm(csrf_enabled=False)
     if form.validate_on_submit():
+        fedora_assignee = None
         if form.fedora_assignee.data:
-            fedora_assignee = form.fedora_assignee.data.strip()
-        else:
-            fedora_assignee = None
+            fedora_assignee = form.fedora_assignee.data.strip() or None
+
+        epel_assignee = None
         if form.epel_assignee.data:
-            epel_assignee = form.epel_assignee.data.strip()
-        else:
-            epel_assignee = None
+            epel_assignee = form.epel_assignee.data.strip() or None
+
         try:
             if repo.bzoverride:
-                repo.bzoverride.fedora_assignee = fedora_assignee
-                repo.bzoverride.epel_assignee = epel_assignee
-                flask.g.session.add(repo)
-            else:
+                if fedora_assignee is None and epel_assignee is None:
+                    flask.g.session.delete(repo.bzoverride)
+                else:
+                    repo.bzoverride.fedora_assignee = fedora_assignee
+                    repo.bzoverride.epel_assignee = epel_assignee
+                    flask.g.session.add(repo)
+            elif fedora_assignee is not None or epel_assignee is not None:
                 mapping = model.PagureBZOverride(
                     project_id=repo.id,
                     epel_assignee=epel_assignee,
diff --git a/pagure_distgit_tests/bugzilla_overrides_tests.py b/pagure_distgit_tests/bugzilla_overrides_tests.py
index 6b86a9c..d1520d5 100644
--- a/pagure_distgit_tests/bugzilla_overrides_tests.py
+++ b/pagure_distgit_tests/bugzilla_overrides_tests.py
@@ -3,8 +3,10 @@ from __future__ import print_function
 import os
 import json
 
-import tests
 
+import pagure.lib.query
+
+import tests
 from pagure_distgit import plugin
 
 
@@ -116,6 +118,10 @@ class PagureFlaskApiProjectBZOverrideTests(tests.Modeltests):
         self.assertDictEqual(
             data, {"epel_assignee": "pingou", "fedora_assignee": "foo"}
         )
+        repo = pagure.lib.query.get_authorized_project(
+            self.session, "test3", namespace="somenamespace",
+        )
+        self.assertIsNotNone(repo.bzoverride)
 
         datainput = {"fedora_assignee": ""}
         output = self.app.post(
@@ -128,6 +134,10 @@ class PagureFlaskApiProjectBZOverrideTests(tests.Modeltests):
         self.assertDictEqual(
             data, {"epel_assignee": "pingou", "fedora_assignee": "pingou"}
         )
+        repo = pagure.lib.query.get_authorized_project(
+            self.session, "test3", namespace="somenamespace",
+        )
+        self.assertIsNotNone(repo.bzoverride)
 
     def test_reset_epel_assignees(self):
         """Test the bz endpoint when resetting the EPEL assignee.
@@ -155,11 +165,16 @@ class PagureFlaskApiProjectBZOverrideTests(tests.Modeltests):
             data, {"epel_assignee": "pingou", "fedora_assignee": "pingou"}
         )
 
+        repo = pagure.lib.query.get_authorized_project(
+            self.session, "test3", namespace="somenamespace",
+        )
+        self.assertIsNotNone(repo.bzoverride)
+
     def test_reset_both_assignees(self):
         """Test the bz endpoint when resetting the both assignees.
         """
         headers = {"Authorization": "token aaabbbcccddd"}
-        datainput = {"epel_assignee": "", "fedora_assignee": "pingou"}
+        datainput = {}
         output = self.app.post(
             "/_dg/bzoverrides/somenamespace/test3",
             data=datainput,
@@ -171,6 +186,12 @@ class PagureFlaskApiProjectBZOverrideTests(tests.Modeltests):
             data, {"epel_assignee": "pingou", "fedora_assignee": "pingou"}
         )
 
+        repo = pagure.lib.query.get_authorized_project(
+            self.session, "test3", namespace="somenamespace",
+        )
+        print(repo.bzoverride)
+        self.assertIsNone(repo.bzoverride)
+
     def test_changing_assignees_logged_in_invalid_user(self):
         """Test the bz endpoint when changing the both assignees when logged in
         the UI but with an user that is not allowed.
@@ -241,6 +262,7 @@ class PagureFlaskApiProjectBZOverrideTests(tests.Modeltests):
         """
         user = tests.FakeUser(username="pingou")
         with tests.user_set(self.app.application, user):
+            # Changing one of them before the reset
             datainput = {"epel_assignee": "foo"}
             output = self.app.post(
                 "/_dg/bzoverrides/somenamespace/test3", data=datainput
@@ -251,6 +273,7 @@ class PagureFlaskApiProjectBZOverrideTests(tests.Modeltests):
                 data, {"epel_assignee": "foo", "fedora_assignee": "pingou"}
             )
 
+            # Resetting
             output = self.app.post("/_dg/bzoverrides/somenamespace/test3")
             self.assertEqual(output.status_code, 200)
             data = json.loads(output.get_data(as_text=True))