From 68a45265f379bf05689ef9397bac75a9ee24056f Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Apr 11 2017 16:11:08 +0000
Subject: [PATCH 1/2] Fix list the of changes when updating the project's settings


We were announcing more changes than they actually were because we
were comparing what the HTML gives us ('y') to what we stored in the
database (True).

---

diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py
index f1b46c3..ce72e8a 100644
--- a/pagure/lib/__init__.py
+++ b/pagure/lib/__init__.py
@@ -1725,25 +1725,33 @@ def update_project_settings(session, repo, settings, user):
     new_settings = repo.settings
     for key in new_settings:
         if key in settings:
+            if key == 'Minimum_score_to_merge_pull-request':
+                try:
+                    settings[key] = int(settings[key]) \
+                        if settings[key] else -1
+                except (ValueError, TypeError):
+                    raise pagure.exceptions.PagureException(
+                        "Please enter a numeric value for the 'minimum "
+                        "score to merge pull request' field.")
+            elif key == 'Web-hooks':
+                settings[key] = settings[key] or None
+            else:
+                # All the remaining keys are boolean, so True is provided
+                # as 'y' by the html, let's convert it back
+                settings[key] = settings[key] in ['y', True]
+
             if new_settings[key] != settings[key]:
                 update.append(key)
-                if key == 'Minimum_score_to_merge_pull-request':
-                    try:
-                        settings[key] = int(settings[key]) \
-                            if settings[key] else -1
-                    except (ValueError, TypeError):
-                        raise pagure.exceptions.PagureException(
-                            "Please enter a numeric value for the 'minimum "
-                            "score to merge pull request' field.")
-                elif key == 'Web-hooks':
-                    settings[key] = settings[key] or None
                 new_settings[key] = settings[key]
         else:
-            update.append(key)
             val = False
             if key == 'Web-hooks':
                 val = None
-            new_settings[key] = val
+
+            # Ensure the default value is different from what is stored.
+            if new_settings[key] != val:
+                update.append(key)
+                new_settings[key] = val
 
     if not update:
         return 'No settings to change'
diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py
index 8f0f52d..a38dc5d 100644
--- a/tests/test_pagure_lib.py
+++ b/tests/test_pagure_lib.py
@@ -1405,7 +1405,7 @@ class PagureLibtests(tests.Modeltests):
                 'pull_requests': False,
                 'Only_assignee_can_merge_pull-request': None,
                 'Minimum_score_to_merge_pull-request': None,
-                'Web-hooks': '',
+                'Web-hooks': 'https://pagure.io/foobar',
                 'Enforce_signed-off_commits_in_pull-request': False,
                 'issues_default_to_private': False,
                 'fedmsg_notifications': True,

From 2c7de69f05521b033fa168dbb9e0ea0ad4755d20 Mon Sep 17 00:00:00 2001
From: Pierre-Yves Chibon <pingou@pingoured.fr>
Date: Apr 11 2017 16:11:08 +0000
Subject: [PATCH 2/2] When testing update_project_settings check what is send to fedmsg


This ensure we are correctly detecting which fields was updated and which
wasn't.

---

diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py
index a38dc5d..0fcb6fc 100644
--- a/tests/test_pagure_lib.py
+++ b/tests/test_pagure_lib.py
@@ -1366,7 +1366,8 @@ class PagureLibtests(tests.Modeltests):
             self.assertTrue(os.path.exists(path))
             shutil.rmtree(path)
 
-    def test_update_project_settings(self):
+    @patch('pagure.lib.notify.log')
+    def test_update_project_settings(self, mock_log):
         """ Test the update_project_settings of pagure.lib. """
 
         tests.create_projects(self.session)
@@ -1395,6 +1396,7 @@ class PagureLibtests(tests.Modeltests):
             user='pingou',
         )
         self.assertEqual(msg, 'No settings to change')
+        mock_log.assert_not_called()
 
         msg = pagure.lib.update_project_settings(
             session=self.session,
@@ -1414,6 +1416,18 @@ class PagureLibtests(tests.Modeltests):
             user='pingou',
         )
         self.assertEqual(msg, 'Edited successfully settings of repo: test2')
+        mock_log.assert_called_once()
+        args = mock_log.call_args
+        self.assertEqual(len(args), 2)
+        self.assertEqual(args[0][0].fullname, 'test2')
+        self.assertEqual(
+            args[1]['msg']['fields'],
+            [
+                'Web-hooks', 'project_documentation',
+                'issue_tracker', 'pull_requests'
+            ]
+        )
+        self.assertEqual(args[1]['topic'], 'project.edit')
 
         # After
         repo = pagure.lib.get_project(self.session, 'test2')