From 629c468939cb290f24100db7f07b0a6838209945 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Wed, 31 Jan 2024 16:18:19 -0800
Subject: [PATCH 01/11] Rewrite test_approve_testing.py to match current
 implementation

On close inspection, this test suite has become a little weird
over the years.

When it was introduced and most of it was written, approve_testing
did somewhat more than it does now. It handled autokarma push
(which is now handled by Update.check_karma_thresholds), and it
carried much more of the logic for deciding if updates met the
relevant requirements or not (most of which is now handled by
Update.meets_testing_requirements; all that's left in
approve_testing is the check of whether the update meets the
autopush-for-time-in-testing threshold).

A lot of the tests are really checking those things - all the
minute variations of "is the update critical path or not?",
"does the update have autokarma enabled?", various time spent
in testing and karma levels are really about testing that stuff.
It doesn't make sense to test that here any more. Those things
should be tested elsewhere, mainly in the tests of
meets_testing_requirements. At least one of these tests -
test_autotime_update_with_autokarma_met_karma_and_time_requirements_get_pushed -
actually still passes if you completely remove the call to
approve_testing_main()!

This completely rewrites the test suite to focus on testing what
approve_testing actually does now. To do this I evaluated every
existing test, and also evaluated the current code of approve_
testing to try and make sure I was testing every path through it.
I'll include those notes in the pull request as they're long.
Some of the existing tests that are still useful are kept, but
tweaked slightly to take advantage of the new setup_method and
assertion helpers, to reduce duplication.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 .../tests/tasks/test_approve_testing.py       | 1115 ++++-------------
 1 file changed, 236 insertions(+), 879 deletions(-)

diff --git a/bodhi-server/tests/tasks/test_approve_testing.py b/bodhi-server/tests/tasks/test_approve_testing.py
index b50cb26357..1399839cec 100644
--- a/bodhi-server/tests/tasks/test_approve_testing.py
+++ b/bodhi-server/tests/tasks/test_approve_testing.py
@@ -21,8 +21,9 @@
 from datetime import datetime, timedelta
 from unittest.mock import call, patch
 
-from fedora_messaging import api, testing as fml_testing
+from fedora_messaging import testing as fml_testing
 import pytest
+import sqlalchemy.exc
 
 from bodhi.messages.schemas import update as update_schemas
 from bodhi.server.config import config
@@ -55,111 +56,256 @@ class TestMain(BaseTaskTestCase):
     This class contains tests for the main() function.
     """
 
-    @patch('bodhi.server.models.mail')
-    def test_autokarma_update_meeting_time_requirements_gets_one_comment(self, mail):
+    def setup_method(self):
         """
-        Ensure that an update that meets the required time in testing gets only one comment from
-        Bodhi to that effect, even on subsequent runs of main().
+        Get an update to work with and set some common attributes that
+        make sense for all or most of the tests.
         """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = True
-        update.autotime = False
-        update.request = None
-        update.stable_karma = 10
-        update.status = models.UpdateStatus.testing
+        super(TestMain, self).setup_method(self)
         # Clear pending messages
         self.db.info['messages'] = []
-
-        update.date_testing = datetime.utcnow() - timedelta(days=7)
+        # Get an update to work with
+        self.update = self.db.query(models.Update).all()[0]
+        self.update.status = models.UpdateStatus.testing
+        self.update.request = None
+        self.update.date_approved = None
+        self.update.release.composed_by_bodhi = False
         with fml_testing.mock_sends():
             self.db.commit()
-        expected_message = update_schemas.UpdateRequirementsMetStableV1.from_dict(
-            {'update': update})
-
-        with fml_testing.mock_sends(expected_message):
-            approve_testing_main()
-            # The approve testing script changes the update, so let's put the changed
-            # update into our expected message body.
-            expected_message.body['update'] = models.Update.query.first().__json__()
-        # Now we will run main() again, but this time we expect Bodhi not to add any
-        # further comments.
-        with fml_testing.mock_sends():
-            approve_testing_main()
-
-        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
-        comment_q = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id)
-        assert comment_q.count() == 1
-        assert comment_q[0].text == config.get('testing_approval_msg')
-        assert mail.send.call_count == 1
-
-    # Set the release's mandatory days in testing to 0 to set up the condition for this test.
+        # approve_testing never sends mail directly, so we always just
+        # want to mock this out and not worry about it
+        self.mailpatcher = patch('bodhi.server.models.mail')
+        self.mailpatcher.start()
+
+    def teardown_method(self):
+        """Stop the mail patcher on teardown."""
+        super(TestMain, self).teardown_method(self)
+        self.mailpatcher.stop()
+
+    def _assert_not_pushed(self):
+        """Run all checks to ensure we did not push the update."""
+        assert self.update.status == models.UpdateStatus.testing
+        assert self.update.request is None
+        assert self.update.date_approved is None
+        assert self.update.date_pushed is None
+        assert not self.update.pushed
+
+    def _assert_commented(self, times):
+        """Assert the testing_approval_msg was posted exactly (times) times."""
+        try:
+            bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
+            bodhicomments = self.db.query(models.Comment).filter_by(
+                update_id=self.update.id, user_id=bodhi.id
+            )
+        except sqlalchemy.exc.NoResultFound:
+            bodhicomments = []
+        approvalcomments = [
+            comment for comment in bodhicomments
+            if comment.text == config.get('testing_approval_msg')
+        ]
+        assert len(approvalcomments) == times
+
+    # for robustness, mock this as True so we *would* do something without the early return
+    @patch('bodhi.server.models.Update.meets_testing_requirements', True)
     @patch.dict(config, [('fedora.mandatory_days_in_testing', 0)])
-    def test_autokarma_update_without_mandatory_days_in_testing(self):
-        """
-        If the Update's release doesn't have a mandatory days in testing, main() should ignore it
-        (and should not comment on the update at all, even if it does reach karma levels.)
+    def test_no_mandatory_days_or_autotime(self):
+        """Ensure that if the update has no mandatory days in testing
+        and autotime is False, we do nothing.
         """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = True
-        update.autotime = False
-        update.request = None
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        update.date_testing = datetime.utcnow() - timedelta(days=7)
-        # Let's delete all the comments to make our assertion at the end of this simpler.
-        for c in update.comments:
-            self.db.delete(c)
+        self.update.autotime = False
+        # this should publish no messages
         with fml_testing.mock_sends():
-            self.db.commit()
-
+            approve_testing_main()
+        # we should not have pushed
+        self._assert_not_pushed()
+        # we should not have posted approval comment
+        self._assert_commented(0)
+        # we should not have set date_approved
+        assert self.update.date_approved is None
+
+    @patch('bodhi.server.models.Update.meets_testing_requirements', False)
+    def test_update_not_approved(self):
+        """
+        Ensure that if the update does not meet testing requirements, we do
+        nothing.
+        """
+        # for robustness, make it look like the update meets autotime
+        # requirements, we should do nothing even so
+        self.update.autotime = True
+        self.update.stable_days = 0
+        # this should publish no messages
+        with fml_testing.mock_sends():
+            approve_testing_main()
+        # we should not have pushed
+        self._assert_not_pushed()
+        # we should not have posted approval comment
+        self._assert_commented(0)
+        # we should not have set date_approved
+        assert self.update.date_approved is None
+
+    @pytest.mark.parametrize("autotime_enabled", (True, False))
+    @patch('bodhi.server.models.Update.meets_testing_requirements', True)
+    def test_update_approved_only(self, autotime_enabled):
+        """
+        Ensure that if the update meets testing requirements but does not meet
+        time-based autopush requirements, we comment on it exactly once, even
+        on multiple runs.
+        """
+        # different ways of failing autotime requirements
+        if autotime_enabled:
+            # autotime is enabled, but we have not been in testing long enough
+            self.update.autotime = True
+            self.update.stable_days = 7
+        else:
+            # we have been in testing long enough, but autotime is not enabled
+            self.update.autotime = False
+            self.update.stable_days = 0
+        with fml_testing.mock_sends(update_schemas.UpdateRequirementsMetStableV1):
+            approve_testing_main()
+        # we should not have pushed
+        self._assert_not_pushed()
+        # we should have posted approval comment once
+        self._assert_commented(1)
+        # we should not have set date_approved (currently we only do that
+        # when pushing, not approving)
+        assert self.update.date_approved is None
+        # re-run, this time no additional message should be published
         with fml_testing.mock_sends():
             approve_testing_main()
+        # we should still only have posted approval comment once
+        self._assert_commented(1)
 
-        # The bodhi user shouldn't exist, since it shouldn't have made any comments
-        assert self.db.query(models.User).filter_by(name='bodhi').count() == 0
-        assert self.db.query(models.Comment).count() == 0
-        assert update.date_approved is None
+    @pytest.mark.parametrize("stable_days", (0, 7, 14))
+    @pytest.mark.parametrize("has_stable_comment", (True, False))
+    @pytest.mark.parametrize("composed_by_bodhi", (True, False))
+    @pytest.mark.parametrize('from_side_tag', (None, 'f17-build-side-1234'))
+    @patch('bodhi.server.models.Update.add_tag')
+    @patch('bodhi.server.models.Update.remove_tag')
+    @patch('bodhi.server.buildsys.DevBuildsys.deleteTag')
+    @patch('bodhi.server.models.Update.meets_testing_requirements', True)
+    def test_update_approved_and_autotime(self, delete_tag, remove_tag, add_tag,
+                                          from_side_tag, composed_by_bodhi, has_stable_comment,
+                                          stable_days):
+        """
+        Ensure that if the update meets testing requirements *and* the autotime
+        push threshold, we push it, publish RequirementsMetStable unless the
+        update has the ready-for-stable comment (indicating it was previously
+        approved), and set date_approved, but do not comment.
+        """
+        self.update.autotime = True
+        # a sprinkling of possible cases just to make sure our logic isn't broken
+        # and we're not somehow relying on defaults
+        self.update.stable_days = stable_days
+        if stable_days > 0:
+            # in this case we need to make sure we look like we've been in testing
+            # this one
+            self.update.date_testing = datetime.utcnow() - timedelta(days=stable_days + 1)
+        self.update.release.composed_by_bodhi = composed_by_bodhi
+        self.update.from_tag = from_side_tag
+        # we publish UpdateRequirementsMet unless the update already has the
+        # stable comment indicating it was previously approved
+        # we publish UpdateRequestStable on the composed_by_bodhi path, because
+        # we call update.set_request() which does that
+        with patch('bodhi.server.models.Update.has_stable_comment', has_stable_comment):
+            if composed_by_bodhi:
+                if has_stable_comment:
+                    with fml_testing.mock_sends(update_schemas.UpdateRequestStableV1):
+                        approve_testing_main()
+                else:
+                    with fml_testing.mock_sends(update_schemas.UpdateRequirementsMetStableV1,
+                                                update_schemas.UpdateRequestStableV1):
+                        approve_testing_main()
+            else:
+                if has_stable_comment:
+                    with fml_testing.mock_sends():
+                        approve_testing_main()
+                else:
+                    with fml_testing.mock_sends(update_schemas.UpdateRequirementsMetStableV1):
+                        approve_testing_main()
+        # we should not have posted approval comment (yes, even if it
+        # wasn't posted before; it's not useful information if we're
+        # autopushing)
+        self._assert_commented(0)
+        # we should have set date_approved
+        assert self.update.date_approved is not None
+        # we should have pushed. this logic is not split out because it's
+        # long and awkward to split out and only used here
+        if self.update.release.composed_by_bodhi:
+            # we should have set the request, but not done the push
+            assert self.update.request == models.UpdateRequest.stable
+            assert self.update.status == models.UpdateStatus.testing
+            assert self.update.date_stable is None
+        else:
+            # we should have actually done the push
+            assert self.update.request is None
+            assert self.update.date_stable is not None
+            assert self.update.status == models.UpdateStatus.stable
+            assert self.update.pushed
+            assert self.update.date_pushed is not None
+
+            if from_side_tag:
+                assert remove_tag.call_args_list == \
+                    [call(f'{from_side_tag}-signing-pending'),
+                     call(f'{from_side_tag}-testing-pending'),
+                     call(from_side_tag)]
+
+                assert add_tag.call_args_list == \
+                    [call('f17-updates')]
+                assert delete_tag.call_args_list == \
+                    [call(f'{from_side_tag}-signing-pending'),
+                     call(f'{from_side_tag}-testing-pending'),
+                     call(from_side_tag)]
+            else:
+                # First pass, it adds f17=updates-pending, then since we're pushing
+                # to stable directly, it adds f17-updates (the stable tag) then
+                # removes f17-updates-testing-pending and f17-updates-pending
+                assert remove_tag.call_args_list == \
+                    [call('f17-updates-testing-pending'), call('f17-updates-pending'),
+                     call('f17-updates-signing-pending'), call('f17-updates-testing'),
+                     call('f17-updates-candidate')]
+
+                assert add_tag.call_args_list == \
+                    [call('f17-updates')]
+                delete_tag.assert_not_called()
 
-    def test_autokarma_update_not_meeting_testing_requirements(self):
+    @pytest.mark.parametrize(('from_tag', 'update_status'),
+                             [('f17-build-side-1234', models.UpdateStatus.pending),
+                             (None, models.UpdateStatus.obsolete)])
+    @patch("bodhi.server.buildsys.DevBuildsys.getLatestBuilds", return_value=[{
+        'creation_time': '2007-08-25 19:38:29.422344'}])
+    @patch('bodhi.server.models.Update.meets_testing_requirements', True)
+    def test_update_conflicting_build_not_pushed(self, build_creation_time,
+                                                 from_tag, update_status):
         """
-        If an autokarma update has not met the testing requirements, bodhi should not comment on the
-        update.
+        Ensure that an update that has conflicting builds is not pushed.
         """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = True
-        update.request = None
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        # 6 days isn't enough time to meet the testing requirements.
-        update.date_testing = datetime.utcnow() - timedelta(days=6)
-        # Let's delete all the comments to make our assertion at the end of this simpler.
-        for c in update.comments:
-            self.db.delete(c)
-        with fml_testing.mock_sends():
-            self.db.commit()
+        self.update.release.composed_by_bodhi = False
+        self.update.from_tag = from_tag
 
-        with fml_testing.mock_sends():
+        # message publishing happens before the conflicting build check, so
+        # even when there's a conflicting build, we publish this message
+        # (and set date_approved)
+        with fml_testing.mock_sends(update_schemas.UpdateRequirementsMetStableV1):
             approve_testing_main()
 
-        # The bodhi user shouldn't exist, since it shouldn't have made any comments
-        assert self.db.query(models.User).filter_by(name='bodhi').count() == 0
-        assert self.db.query(models.Comment).count() == 0
-        assert update.date_approved is None
+        assert self.update.status == update_status
+        assert self.update.date_approved is None
+
+        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
+        cmnts = self.db.query(models.Comment).filter_by(update_id=self.update.id, user_id=bodhi.id)
+        assert cmnts.count() == 1
+        assert cmnts[0].text == "This update cannot be pushed to stable. "\
+            "These builds bodhi-2.0-1.fc17 have a more recent build in koji's "\
+            f"{self.update.release.stable_tag} tag."
 
+    @pytest.mark.parametrize('composed_by_bodhi', (True, False))
     @patch('bodhi.server.models.Update.comment', side_effect=IOError('The DB died lol'))
     @patch('bodhi.server.tasks.approve_testing.log')
-    @pytest.mark.parametrize('composed_by_bodhi', (True, False))
+    @patch('bodhi.server.models.Update.meets_testing_requirements', True)
     def test_exception_handler(self, log, comment, composed_by_bodhi):
         """The Exception handler prints the Exception, rolls back and closes the db, and exits."""
-        update = self.db.query(models.Update).all()[0]
-        update.autotime = False
-        update.date_testing = datetime.utcnow() - timedelta(days=15)
-        update.request = None
-        update.status = models.UpdateStatus.testing
-        update.release.composed_by_bodhi = composed_by_bodhi
+        self.update.autotime = False
+        self.update.release.composed_by_bodhi = composed_by_bodhi
         self.db.flush()
 
         with patch.object(self.db, 'commit'):
@@ -174,28 +320,23 @@ def test_exception_handler(self, log, comment, composed_by_bodhi):
             author='bodhi',
             email_notification=composed_by_bodhi,
         )
-        log.info.assert_called_with(f'{update.alias} now meets testing requirements')
+        log.info.assert_called_with(f'{self.update.alias} now meets testing requirements')
         log.exception.assert_called_with("There was an error approving testing updates.")
 
+    @pytest.mark.parametrize('composed_by_bodhi', (True, False))
     @patch('bodhi.server.models.Update.comment', side_effect=[None, IOError('The DB died lol')])
     @patch('bodhi.server.tasks.approve_testing.log')
-    @pytest.mark.parametrize('composed_by_bodhi', (True, False))
-    def test_exception_handler_on_the_second_update(
-            self, log, comment, composed_by_bodhi):
+    @patch('bodhi.server.models.Update.meets_testing_requirements', True)
+    def test_exception_handler_on_the_second_update(self, log, comment, composed_by_bodhi):
         """
         Ensure, that when the Exception is raised, all previous transactions are commited,
         the Exception handler prints the Exception, rolls back and closes the db, and exits.
         """
-        update = self.db.query(models.Update).all()[0]
-        update.autotime = False
-        update.date_testing = datetime.utcnow() - timedelta(days=15)
-        update.request = None
-        update.release.composed_by_bodhi = composed_by_bodhi
-        update.status = models.UpdateStatus.testing
+        self.update.autotime = False
+        self.update.release.composed_by_bodhi = composed_by_bodhi
 
         update2 = self.create_update(['bodhi2-2.0-1.fc17'])
         update2.autotime = False
-        update2.date_testing = datetime.utcnow() - timedelta(days=15)
         update2.request = None
         update2.status = models.UpdateStatus.testing
         self.db.flush()
@@ -213,790 +354,6 @@ def test_exception_handler_on_the_second_update(
             email_notification=composed_by_bodhi,
         )
         assert comment.call_args_list == [comment_expected_call, comment_expected_call]
-        log.info.assert_any_call(f'{update.alias} now meets testing requirements')
+        log.info.assert_any_call(f'{self.update.alias} now meets testing requirements')
         log.info.assert_any_call(f'{update2.alias} now meets testing requirements')
         log.exception.assert_called_with("There was an error approving testing updates.")
-
-    def test_non_autokarma_critpath_update_meeting_karma_requirements_gets_one_comment(self):
-        """
-        Ensure that a non-autokarma critical path update that meets the required karma threshold
-        and required time in testing gets only one comment from Bodhi to that effect, even on
-        subsequent runs of main(). There was an issue[0] where Bodhi wasn't correctly detecting when
-        it should add these comments, and with detecting that it has already commented on
-        critical path updates, and would repeatedly comment that these updates could be pushed.
-        This test ensures that issue stays fixed.
-
-        [0] https://github.com/fedora-infra/bodhi/issues/1009
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = False
-        # Make this update a critpath update to force meets_testing_requirements into a different
-        # code path.
-        update.critpath = True
-        # It's been in testing long enough to get the comment from bodhi that it can be pushed.
-        update.date_testing = datetime.utcnow() - timedelta(days=15)
-        update.request = None
-        update.stable_karma = 1
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        update.comment(self.db, 'testing', author='hunter2', karma=1)
-        with fml_testing.mock_sends(api.Message):
-            self.db.commit()
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-        # Now we will run main() again, but this time we expect Bodhi not to add any
-        # further comments.
-        with fml_testing.mock_sends():
-            approve_testing_main()
-
-        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
-        comment_q = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id)
-        assert comment_q.count() == 1
-        assert comment_q[0].text == config.get('testing_approval_msg')
-
-    def test_non_autokarma_critpath_update_not_meeting_time_requirements_gets_no_comment(self):
-        """
-        Ensure that a non-autokarma critical path update that does not meet the required time in
-        testing does not get any comment from bodhi saying it can be pushed to stable.
-        There was an issue[0] where Bodhi was incorrectly detecting that the update could be pushed
-        and was commenting to that effect. This test ensures that issue stays fixed.
-
-        [0] https://github.com/fedora-infra/bodhi/issues/1009
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        # Make this update a critpath update to force meets_testing_requirements into a different
-        # code path.
-        update.critpath = True
-        update.request = None
-        update.stable_karma = 1
-        update.status = models.UpdateStatus.testing
-        self.db.flush()
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        approve_testing_main()
-        # Now we will run main() again, but this time we expect Bodhi not to add any
-        # further comments.
-        with fml_testing.mock_sends():
-            approve_testing_main()
-
-        # The update should have one +1, which is as much as the stable karma but not as much as the
-        # required +2 to go stable.
-        assert update._composite_karma == (1, 0)
-        # The bodhi user shouldn't exist, since it shouldn't have made any comments
-        assert self.db.query(models.User).filter_by(name='bodhi').count() == 0
-        # There are two comments, but none from the non-existing bodhi user.
-        assert self.db.query(models.Comment).count() == 2
-        usernames = [
-            c.user.name
-            for c in self.db.query(models.Comment).order_by(models.Comment.timestamp).all()]
-        assert usernames == ['guest', 'anonymous']
-
-    def test_non_autokarma_update_meeting_karma_requirements_gets_one_comment(self):
-        """
-        Ensure that a non-autokarma update that meets the required karma threshold gets only one
-        comment from Bodhi to that effect, even on subsequent runs of main(). There was an issue[0]
-        where Bodhi wasn't correctly detecting that it has already commented on updates, and would
-        repeatedly comment that non-autokarma updates could be pushed. This test ensures that issue
-        stays fixed.
-
-        [0] https://github.com/fedora-infra/bodhi/issues/1009
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = False
-        update.request = None
-        update.stable_karma = 1
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        update.comment(self.db, 'testing', author='hunter2', karma=1)
-        with fml_testing.mock_sends(api.Message):
-            self.db.commit()
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-        # Now we will run main() again, but this time we expect Bodhi not to add any
-        # further comments.
-        with fml_testing.mock_sends():
-            approve_testing_main()
-
-        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
-        comment_q = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id)
-        assert comment_q.count() == 1
-        assert comment_q[0].text == config.get('testing_approval_msg')
-
-    def test_non_autokarma_critpath_update_meeting_time_requirements_gets_one_comment(self):
-        """
-        Ensure that a critpath update that meets the required time in testing (14 days) gets a
-        comment from Bodhi indicating that the update has met the required time in testing.
-        There was an issue[0] where Bodhi was indicating that the update had been in testing for
-        only 7 days, when the update had been in testing for 14 days.
-
-        [0] https://github.com/fedora-infra/bodhi/issues/1361
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = False
-        update.request = None
-        update.stable_karma = 10
-        update.critpath = True
-        update.status = models.UpdateStatus.testing
-        update.date_testing = datetime.utcnow() - timedelta(days=14)
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-        # Now we will run main() again, but this time we expect Bodhi not to add any
-        # further comments.
-        with fml_testing.mock_sends():
-            approve_testing_main()
-
-        update = self.db.query(models.Update).all()[0]
-        assert update.critpath is True
-        assert update.mandatory_days_in_testing == 14
-
-        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
-        comment_q = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id)
-        assert comment_q.count() == 1
-        assert comment_q[0].text == config.get('testing_approval_msg')
-        assert update.release.mandatory_days_in_testing == 7
-        assert update.mandatory_days_in_testing == 14
-
-    def test_non_autokarma_update_with_unmet_karma_requirement(self):
-        """
-        A non-autokarma update without enough karma should not get comments from Bodhi.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.request = None
-        update.stable_karma = 10
-        update.status = models.UpdateStatus.testing
-        self.db.flush()
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        with fml_testing.mock_sends():
-            approve_testing_main()
-
-        # The update should have one positive karma and no negative karmas
-        assert update._composite_karma == (1, 0)
-        # The bodhi user shouldn't exist, since it shouldn't have made any comments
-        assert self.db.query(models.User).filter_by(name='bodhi').count() == 0
-        # There are two comments, but none from the non-existing bodhi user.
-        assert self.db.query(models.Comment).count() == 2
-        usernames = [
-            c.user.name
-            for c in self.db.query(models.Comment).order_by(models.Comment.timestamp).all()]
-        assert usernames == ['guest', 'anonymous']
-
-    def test_non_autokarma_update_with_unmet_karma_requirement_after_time_met(self):
-        """
-        A non-autokarma update without enough karma that reaches mandatory days in testing should
-        get a comment from Bodhi that the update can be pushed to stable.
-
-        See https://github.com/fedora-infra/bodhi/issues/1094
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = False
-        update.request = None
-        update.stable_karma = 10
-        update.status = models.UpdateStatus.testing
-        update.date_testing = datetime.utcnow() - timedelta(days=7)
-        self.db.flush()
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-
-        # The update should have one positive karma and no negative karmas
-        assert update._composite_karma == (1, 0)
-        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
-        comment_q = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id)
-        assert comment_q.count() == 1
-        assert comment_q[0].text == config.get('testing_approval_msg')
-
-    # Set the release's mandatory days in testing to 0 to set up the condition for this test.
-    @patch.dict(config, [('fedora.mandatory_days_in_testing', 0)])
-    def test_non_autokarma_update_without_mandatory_days_in_testing(self):
-        """
-        If the Update's release doesn't have a mandatory days in testing, main() should ignore it
-        (and should not comment on the update at all, even if it does reach karma levels.)
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = False
-        update.request = None
-        update.stable_karma = 1
-        update.status = models.UpdateStatus.testing
-        self.db.flush()
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        approve_testing_main()
-
-        # The update should have one positive karma and no negative karmas
-        assert update._composite_karma == (1, 0)
-        # The bodhi user shouldn't exist, since it shouldn't have made any comments
-        assert self.db.query(models.User).filter_by(name='bodhi').count() == 0
-        # There are two comments, but none from the non-existing bodhi user.
-        assert self.db.query(models.Comment).count() == 2
-        usernames = [
-            c.user.name
-            for c in self.db.query(models.Comment).order_by(models.Comment.timestamp).all()]
-        assert usernames == ['guest', 'anonymous']
-
-    @patch.dict(config, [('fedora.mandatory_days_in_testing', 14)])
-    def test_subsequent_comments_after_initial_push_comment(self):
-        """
-        If a user edits an update after Bodhi comments a testing_approval_msg,
-        Bodhi should send an additional testing_approval_msg when the revised
-        update is eligible to be pushed to stable.
-
-        See https://github.com/fedora-infra/bodhi/issues/1310
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.request = None
-        update.status = models.UpdateStatus.testing
-        update.date_testing = datetime.utcnow() - timedelta(days=14)
-        update.autotime = False
-        self.db.flush()
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-        update.comment(self.db, "Removed build", 0, 'bodhi')
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-
-        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
-        cmnts = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id)
-        # There are 3 comments: testing_approval_msg, build change, testing_approval_msg
-        assert cmnts.count() == 3
-        assert cmnts[0].text == config.get('testing_approval_msg')
-        assert cmnts[1].text == 'Removed build'
-        assert cmnts[2].text == config.get('testing_approval_msg')
-
-    def test_autotime_update_meeting_test_requirements_gets_pushed(self):
-        """
-        Ensure that an autotime update that meets the test requirements gets pushed to stable.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 10
-        update.stable_days = 7
-        update.date_testing = datetime.utcnow() - timedelta(days=7)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        with fml_testing.mock_sends(update_schemas.UpdateRequirementsMetStableV1,
-                                    update_schemas.UpdateRequestStableV1):
-            approve_testing_main()
-
-        assert update.request == models.UpdateRequest.stable
-        assert update.date_approved is not None
-
-    def test_autotime_update_does_not_meet_stable_days_doesnt_get_pushed(self):
-        """
-        Ensure that an autotime update that meets the test requirements but has a longer
-        stable days define does not get push.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 10
-        update.stable_days = 10
-        update.date_testing = datetime.utcnow() - timedelta(days=7)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-
-        assert update.request is None
-        assert update.date_approved is None
-
-    def test_autotime_update_meeting_stable_days_get_pushed(self):
-        """
-        Ensure that an autotime update that meets the stable days gets pushed.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 10
-        update.stable_days = 10
-        update.date_testing = datetime.utcnow() - timedelta(days=10)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        with fml_testing.mock_sends(api.Message, api.Message):
-            approve_testing_main()
-
-        assert update.request == models.UpdateRequest.stable
-        assert update.date_approved is not None
-
-    def test_no_autotime_update_meeting_stable_days_and_test_requirement(self):
-        """
-        Ensure that a normal update that meets the stable days and test requirements
-        doe not get pushed.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = False
-        update.request = None
-        update.stable_karma = 10
-        update.stable_days = 10
-        update.date_testing = datetime.utcnow() - timedelta(days=10)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-
-        assert update.request is None
-        assert update.date_approved is None
-
-    @patch.dict(config, [('fedora.mandatory_days_in_testing', 2)])
-    def test_autotime_update_does_not_meet_test_requirements(self):
-        """
-        Ensure that an autotime update that does not meet the test requirements
-        does not pushed to stable.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_days = update.mandatory_days_in_testing
-        update.stable_karma = 10
-        update.date_testing = datetime.utcnow() - timedelta(days=1)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        approve_testing_main()
-
-        assert update.request is None
-        assert update.date_approved is None
-
-    @patch.dict(config, [('fedora.mandatory_days_in_testing', 0)])
-    def test_autotime_update_does_no_mandatory_days_in_testing(self):
-        """
-        Ensure that an autotime update that does not have mandatory days in testing
-        does get pushed to stable.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 10
-        update.date_testing = datetime.utcnow()
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        with fml_testing.mock_sends(api.Message, api.Message):
-            approve_testing_main()
-
-        assert update.request == models.UpdateRequest.stable
-        assert update.date_approved is not None
-
-    @patch.dict(config, [('fedora.mandatory_days_in_testing', 0)])
-    def test_autotime_update_zero_day_in_testing_meeting_test_requirements_gets_pushed(self):
-        """
-        Ensure that an autotime update with 0 mandatory_days_in_testing that meets
-        the test requirements gets pushed to stable.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 10
-        update.stable_days = 0
-        update.date_testing = datetime.utcnow() - timedelta(days=0)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        with fml_testing.mock_sends(api.Message, api.Message):
-            approve_testing_main()
-
-        assert update.request == models.UpdateRequest.stable
-        assert update.status == models.UpdateStatus.testing
-        assert update.date_stable is None
-        assert update.date_approved is not None
-
-    @pytest.mark.parametrize('from_side_tag', (None, 'f17-build-side-1234'))
-    @patch.dict(config, [('fedora.mandatory_days_in_testing', 0)])
-    @patch('bodhi.server.models.Update.add_tag')
-    @patch('bodhi.server.models.Update.remove_tag')
-    @patch('bodhi.server.buildsys.DevBuildsys.deleteTag')
-    @patch('bodhi.server.models.mail')
-    def test_autotime_update_zero_day_in_testing_no_gated_gets_pushed_to_rawhide(
-            self, mail, delete_tag, remove_tag, add_tag, from_side_tag):
-        """
-        Ensure that an autotime update with 0 mandatory_days_in_testing that meets
-        the test requirements gets pushed to stable in rawhide.
-
-        Test for normal updates and such from a side tags, where it and its
-        auxiliary tags need to be removed.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.release.composed_by_bodhi = False
-        update.stable_karma = 10
-        update.stable_days = 0
-        update.date_testing = datetime.utcnow() - timedelta(days=0)
-        update.status = models.UpdateStatus.testing
-        update.from_tag = from_side_tag
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-
-        assert update.request is None
-        assert update.date_stable is not None
-        assert update.status == models.UpdateStatus.stable
-        assert update.pushed
-        assert update.date_pushed is not None
-        assert update.date_approved is not None
-
-        if not from_side_tag:
-            # First pass, it adds f17=updates-pending, then since we're pushing
-            # to stable directly, it adds f17-updates (the stable tag) then
-            # removes f17-updates-testing-pending and f17-updates-pending
-            assert remove_tag.call_args_list == \
-                [call('f17-updates-testing-pending'), call('f17-updates-pending'),
-                 call('f17-updates-signing-pending'), call('f17-updates-testing'),
-                 call('f17-updates-candidate')]
-
-            assert add_tag.call_args_list == \
-                [call('f17-updates')]
-            delete_tag.assert_not_called()
-        else:
-            assert remove_tag.call_args_list == \
-                [call(f'{from_side_tag}-signing-pending'),
-                 call(f'{from_side_tag}-testing-pending'),
-                 call(from_side_tag)]
-
-            assert add_tag.call_args_list == \
-                [call('f17-updates')]
-            assert delete_tag.call_args_list == \
-                [call(f'{from_side_tag}-signing-pending'),
-                 call(f'{from_side_tag}-testing-pending'),
-                 call(from_side_tag)]
-
-        assert mail.send.call_count == 1
-
-    @patch.dict(config, [('fedora.mandatory_days_in_testing', 0)])
-    @patch.dict(config, [('test_gating.required', True)])
-    def test_autotime_update_zero_day_in_testing_fail_gating_is_not_pushed(self):
-        """
-        Ensure that an autotime update with 0 mandatory days in testing that failed gating
-        does not get pushed to stable.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 10
-        update.stable_days = 0
-        update.test_gating_status = models.TestGatingStatus.failed
-        update.date_testing = datetime.utcnow() - timedelta(days=0)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        approve_testing_main()
-
-        assert update.request is None
-        assert update.date_approved is None
-
-    def test_autotime_update_negative_karma_does_not_get_pushed(self):
-        """
-        Ensure that an autotime update with negative karma does not get pushed.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 10
-        update.stable_days = 0
-        update.date_testing = datetime.utcnow() - timedelta(days=0)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        update.comment(self.db, u'Failed to work', author=u'luke', karma=-1)
-        with fml_testing.mock_sends(api.Message):
-            self.db.commit()
-
-        approve_testing_main()
-
-        assert update.request is None
-        assert update.date_approved is None
-
-    def test_autotime_update_no_autokarma_met_karma_requirements_get_comments(self):
-        """
-        Ensure that an autotime update which met the karma requirements but has autokarma off
-        get a comment to let the packager know that he can push the update to stable.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 1
-        update.stable_days = 10
-        update.date_testing = datetime.utcnow() - timedelta(days=0)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        update.comment(self.db, u'Works great', author=u'luke', karma=1)
-        with fml_testing.mock_sends(api.Message):
-            self.db.commit()
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-
-        assert update.request is None
-        assert update.date_approved is not None
-
-        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
-        cmnts = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id)
-        assert cmnts.count() == 1
-        assert cmnts[0].text == config.get('testing_approval_msg')
-
-    def test_autotime_update_no_autokarma_met_karma_and_time_requirements_get_pushed(self):
-        """
-        Ensure that an autotime update which met the karma and time requirements but
-        has autokarma off gets pushed.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 1
-        update.stable_days = 0
-        update.date_testing = datetime.utcnow() - timedelta(days=0)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        update.comment(self.db, u'Works great', author=u'luke', karma=1)
-        with fml_testing.mock_sends(api.Message):
-            self.db.commit()
-
-        with fml_testing.mock_sends(api.Message, api.Message):
-            approve_testing_main()
-
-        assert update.request == models.UpdateRequest.stable
-        assert update.date_approved is not None
-
-        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
-        cmnts = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id)
-        assert cmnts.count() == 1
-        assert cmnts[0].text == 'This update has been submitted for stable by bodhi. '
-
-    def test_autotime_update_with_autokarma_met_karma_and_time_requirements_get_pushed(self):
-        """
-        Ensure that an autotime update which met the karma and time requirements and has autokarma
-        and autotime enable gets pushed.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = True
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 1
-        update.stable_days = 0
-        update.date_testing = datetime.utcnow() - timedelta(days=0)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-
-        with fml_testing.mock_sends(api.Message, api.Message, api.Message):
-            update.comment(self.db, u'Works great', author=u'luke', karma=1)
-            self.db.commit()
-
-        approve_testing_main()
-
-        assert update.request == models.UpdateRequest.stable
-        assert update.date_approved is not None
-
-        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
-        cmnts = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id)
-        assert cmnts.count() == 1
-        assert cmnts[0].text == 'This update has been submitted for stable by bodhi. '
-
-    def test_autotime_update_no_autokarma_negative_karma_not_pushed(self):
-        """
-        Ensure that an autotime update which negative karma does not get pushed.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 1
-        update.stable_days = 0
-        update.date_testing = datetime.utcnow() - timedelta(days=8)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        update.comment(self.db, u'Broken', author=u'luke', karma=-1)
-        with fml_testing.mock_sends(api.Message):
-            self.db.commit()
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-
-        assert update.request is None
-        assert update.date_approved is None
-        assert update.autotime is False
-
-    @patch("bodhi.server.buildsys.DevBuildsys.getLatestBuilds", return_value=[{
-        'creation_time': '2007-08-25 19:38:29.422344'}])
-    @pytest.mark.parametrize(('from_tag', 'update_status'),
-                             [('f17-build-side-1234', models.UpdateStatus.pending),
-                             (None, models.UpdateStatus.obsolete)])
-    def test_update_conflicting_build_not_pushed(self, build_creation_time,
-                                                 from_tag, update_status):
-        """
-        Ensure that an update that have conflicting builds will not get pushed.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 1
-        update.stable_days = 7
-        update.date_testing = datetime.utcnow() - timedelta(days=8)
-        update.status = models.UpdateStatus.testing
-        update.release.composed_by_bodhi = False
-        update.from_tag = from_tag
-
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        with fml_testing.mock_sends(api.Message):
-            approve_testing_main()
-
-        assert update.status == update_status
-        assert update.date_approved is None
-
-        bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
-        cmnts = self.db.query(models.Comment).filter_by(update_id=update.id, user_id=bodhi.id)
-        assert cmnts.count() == 1
-        assert cmnts[0].text == "This update cannot be pushed to stable. "\
-            "These builds bodhi-2.0-1.fc17 have a more recent build in koji's "\
-            f"{update.release.stable_tag} tag."
-
-    def test_autotime_update_gets_pushed_dont_send_duplicated_notification(self):
-        """
-        Ensure not emitting UpdateRequirementsMetStable notification when an autotime update
-        gets pushed to stable if that was emitted before.
-
-        When an update reaches its mandatory_days_in_testing threshold, the
-        UpdateRequirementsMetStableV1 notification is sent along with a comment that informs
-        the maintainer they can manually push the update to stable. The update IS NOT
-        automatically pushed now.
-        Then, when the update reaches its stable_days threshold, it is automatically pushed,
-        but we don't want to emit UpdateRequirementsMetStable a second time.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 10
-        update.stable_days = 10
-        update.date_testing = datetime.utcnow() - timedelta(days=7)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        assert not update.has_stable_comment
-
-        with fml_testing.mock_sends(update_schemas.UpdateRequirementsMetStableV1):
-            approve_testing_main()
-
-        assert update.has_stable_comment
-        assert update.request is None
-        assert update.status == models.UpdateStatus.testing
-
-        update.date_testing = datetime.utcnow() - timedelta(days=10)
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        with fml_testing.mock_sends(update_schemas.UpdateRequestStableV1):
-            approve_testing_main()
-
-        assert update.request == models.UpdateRequest.stable
-
-    def test_autotime_update_with_stable_comment_set_stable_on_branched(self):
-        """
-        Ensure update is pushed to stable on releases not composed by Bodhi if
-        the update already has a stable comment.
-        """
-        update = self.db.query(models.Update).all()[0]
-        update.autokarma = False
-        update.autotime = True
-        update.request = None
-        update.stable_karma = 10
-        update.stable_days = 10
-        update.release.composed_by_bodhi = False
-        update.date_testing = datetime.utcnow() - timedelta(days=7)
-        update.status = models.UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        assert not update.has_stable_comment
-
-        with fml_testing.mock_sends(update_schemas.UpdateRequirementsMetStableV1):
-            approve_testing_main()
-
-        assert update.has_stable_comment
-        assert update.request is None
-        assert update.status == models.UpdateStatus.testing
-
-        update.date_testing = datetime.utcnow() - timedelta(days=10)
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        # No further notifications emitted
-        approve_testing_main()
-
-        assert update.status == models.UpdateStatus.stable

From 0231d1e46a6e3e5900214b9f6b3318281410a1fe Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Thu, 1 Feb 2024 14:32:23 -0800
Subject: [PATCH 02/11] Refactor meets_testing_requirements tests

This makes these tests simpler, clearer and more comprehensive by
using parametrize(), adding assertions and comments about
significant assumptions made by the tests, and reorganizing and
renaming the tests into sensible groups.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 bodhi-server/tests/test_models.py | 310 +++++++++++++-----------------
 1 file changed, 134 insertions(+), 176 deletions(-)

diff --git a/bodhi-server/tests/test_models.py b/bodhi-server/tests/test_models.py
index 7ac735f91a..b0187bc76a 100644
--- a/bodhi-server/tests/test_models.py
+++ b/bodhi-server/tests/test_models.py
@@ -2653,203 +2653,180 @@ def test_backref_different_build_types(self):
 @mock.patch('bodhi.server.models.work_on_bugs_task', mock.Mock())
 @mock.patch('bodhi.server.models.fetch_test_cases_task', mock.Mock())
 class TestUpdateMeetsTestingRequirements(BasePyTestCase):
-    """Test the Update.meets_testing_requirements() method."""
-
-    def test_autokarma_update_reaching_stable_karma(self):
+    """
+    Test the Update.meets_testing_requirements() method.
+
+    It is significant and not obvious that the update used by these tests
+    starts out with karma of +1/-0. These tests also rely on manual and automatic
+    karma and time thresholds that are either Bodhi's defaults, or specified in
+    the tests/testing.ini configuration file.
+    """
+
+    @pytest.mark.parametrize('critpath', (True, False))
+    @pytest.mark.parametrize("autokarma", (True, False))
+    def test_update_reaching_stable_karma_not_critpath_min_karma(self, autokarma, critpath):
         """
-        Assert that meets_testing_requirements() correctly returns True for autokarma updates
-        that haven't reached the days in testing but have reached the stable_karma threshold.
+        Assert that meets_testing_requirements() returns the correct result for updates
+        that haven't reached the days in testing or critpath_min_karma, but have reached
+        stable_karma.
         """
         update = model.Update.query.first()
-        update.autokarma = True
+        update.autokarma = autokarma
+        update.critpath = critpath
         update.status = UpdateStatus.testing
         update.stable_karma = 1
-        # Now let's add some karma to get it to the required threshold
-        with mock_sends(Message):
-            update.comment(self.db, 'testing', author='hunter2', karma=1)
-
-        # meets_testing_requirement() should return True since the karma threshold has been reached
-        assert update.meets_testing_requirements
 
-    def test_critpath_14_days_negative_karma(self):
-        """critpath packages in testing for 14 days shouldn't go stable with negative karma."""
+        # check, and make it clear, what we're assuming the karma value is now
+        # and the critpath_min_karma value we're testing against
+        assert update.karma == 1
+        assert update.release.critpath_min_karma == 2
+        # this means 'False for critpath, True for non-critpath'. critpath
+        # should be required to meet critpath_min_karma, but non-critpath
+        # should not
+        assert update.meets_testing_requirements is not critpath
+
+    @pytest.mark.parametrize('critpath', (True, False))
+    @pytest.mark.parametrize('autokarma', (True, False))
+    def test_update_below_stable_karma(self, autokarma, critpath):
+        """It should return False for all updates below stable karma, critpath_min_karma
+        and time."""
         update = model.Update.query.first()
-        update.critpath = True
-        update.status = model.UpdateStatus.testing
-        update.request = None
-        update.date_testing = datetime.utcnow() - timedelta(days=15)
+        update.autokarma = autokarma
+        update.critpath = critpath
+        # clear existing karma
+        update.comments = []
+        update.status = UpdateStatus.testing
         update.stable_karma = 1
-        update.comment(self.db, 'testing', author='enemy', karma=-1)
-        # This gets the update to positive karma, but not to the required 2 karma needed for
-        # critpath.
-        update.comment(self.db, 'testing', author='bro', karma=1)
 
+        # meets_testing_requirement() should return False since no karma threshold has been
+        # reached (note that this Update does not have any karma).
+        assert update.karma == 0
         assert not update.meets_testing_requirements
 
-    def test_critpath_14_days_no_negative_karma(self):
-        """critpath packages in testing for 14 days can go stable without negative karma."""
+    @pytest.mark.parametrize('critpath', (True, False))
+    def test_update_reaching_time_in_testing(self, critpath):
+        """It should return True for updates that meet time in testing - including
+        critpath updates that meet the critpath time-in-testing, if there is no
+        negative karma."""
         update = model.Update.query.first()
-        update.critpath = True
         update.status = model.UpdateStatus.testing
+        update.critpath = critpath
         update.request = None
-        update.date_testing = datetime.utcnow() - timedelta(days=15)
-        update.stable_karma = 1
-
-        assert update.meets_testing_requirements
-
-    def test_critpath_karma_2_met(self):
-        """critpath packages should be allowed to go stable when meeting required karma."""
-        update = model.Update.query.first()
-        update.critpath = True
-        update.stable_karma = 1
-        with mock_sends(Message, Message, Message, Message, Message):
-            update.comment(self.db, 'testing', author='enemy', karma=-1)
-            update.comment(self.db, 'testing', author='bro', karma=1)
-            # Despite meeting the stable_karma, the function should still not
-            # mark this as meeting testing requirements because critpath packages
-            # have a higher requirement for minimum karma. So let's get it a second one.
-            update.comment(self.db, 'testing', author='ham', karma=1)
+        if critpath:
+            # the default critpath.stable_after_days_without_negative_karma
+            # value is 14
+            update.date_testing = datetime.utcnow() - timedelta(days=15)
+        else:
+            # testing.ini specifies fedora.mandatory_days_in_testing = 7
+            update.date_testing = datetime.utcnow() - timedelta(days=8)
+        # to ensure we don't meet karma requirements
+        update.stable_karma = 10
 
         assert update.meets_testing_requirements
 
-    def test_critpath_karma_2_required(self):
-        """critpath packages should require a minimum karma."""
+    @pytest.mark.parametrize('critpath', (True, False))
+    def test_update_below_time_in_testing(self, critpath):
+        """It should return False for updates that don't yet meet time in testing."""
         update = model.Update.query.first()
-        update.critpath = True
-        update.stable_karma = 1
+        update.status = model.UpdateStatus.testing
+        update.critpath = critpath
+        update.request = None
+        if critpath:
+            # the default critpath.stable_after_days_without_negative_karma
+            # value is 14
+            update.date_testing = datetime.utcnow() - timedelta(days=13)
+        else:
+            # testing.ini specifies fedora.mandatory_days_in_testing = 7
+            update.date_testing = datetime.utcnow() - timedelta(days=6)
+        # to ensure we don't meet karma requirements
+        update.stable_karma = 10
 
-        # Despite meeting the stable_karma, the function should still not mark this as meeting
-        # testing requirements because critpath packages have a higher requirement for minimum
-        # karma.
         assert not update.meets_testing_requirements
 
-    def test_critpath_negative_karma(self):
-        """
-        Assert that meets_testing_requirements() correctly returns False for critpath updates
-        with negative karma.
-        """
+    @pytest.mark.parametrize('critpath', (True, False))
+    def test_update_reaching_time_in_testing_negative_karma(self, critpath):
+        """If an update meets the mandatory_days_in_testing and stable_karma thresholds
+        but not the critpath_min_karma threshold, it should return False for critical path
+        updates but True for non-critical-path updates."""
         update = model.Update.query.first()
-        update.critpath = True
+        update.critpath = critpath
+        update.status = model.UpdateStatus.testing
+        update.request = None
+        if critpath:
+            # the default critpath.stable_after_days_without_negative_karma
+            # value is 14
+            update.date_testing = datetime.utcnow() - timedelta(days=15)
+        else:
+            # testing.ini specifies fedora.mandatory_days_in_testing = 7
+            update.date_testing = datetime.utcnow() - timedelta(days=8)
+        update.stable_karma = 1
         update.comment(self.db, 'testing', author='enemy', karma=-1)
-        assert not update.meets_testing_requirements
+        # This gets the update back to positive karma, but not to the required
+        # +2 karma needed to clear the critpath_min_karma requirement
+        update.comment(self.db, 'testing', author='bro', karma=1)
 
-    def test_karma_2_met(self):
-        """Regular packages should be allowed to go stable when meeting required karma."""
+        assert update.karma == 1
+        assert update.release.critpath_min_karma == 2
+        # this means 'False for critpath, True for non-critpath'
+        assert update.meets_testing_requirements is not critpath
+
+    @pytest.mark.parametrize('critpath', (True, False))
+    def test_update_reaching_critpath_min_karma(self, critpath):
+        """Both critpath and non-critpath packages should be allowed to go stable when
+        meeting the policy minimum karma requirement for critpath packages, even if there
+        is negative karma."""
         update = model.Update.query.first()
+        update.critpath = critpath
         update.stable_karma = 3
         update.comment(self.db, 'testing', author='enemy', karma=-1)
         update.comment(self.db, 'testing', author='bro', karma=1)
-        # Despite meeting the stable_karma, the function should still not mark this as meeting
-        # testing requirements because critpath packages have a higher requirement for minimum
-        # karma. So let's get it a second one.
+        # Despite meeting the stable_karma, the function should still not
+        # mark this as meeting testing requirements because critpath packages
+        # have a higher requirement for minimum karma - at this point we
+        # are in the same state as test_critpath_14_days_negative_karma.
+        # So add a third +1 so total is +2, which meets critpath_min_karma
         update.comment(self.db, 'testing', author='ham', karma=1)
 
+        assert update.karma == 2
+        assert update.release.critpath_min_karma == 2
         assert update.meets_testing_requirements
 
-    def test_non_autokarma_update_below_stable_karma(self):
-        """It should return False for non-autokarma updates below stable karma and time."""
-        update = model.Update.query.first()
-        update.autokarma = False
-        update.comments = []
-        update.status = UpdateStatus.testing
-        update.stable_karma = 1
-
-        # meets_testing_requirement() should return False since the karma threshold has not been
-        # reached (note that this Update does not have any karma).
-        assert not update.meets_testing_requirements
-
-    def test_non_autokarma_update_reaching_stable_karma(self):
-        """
-        Assert that meets_testing_requirements() correctly returns True for non-autokarma updates
-        that haven't reached the days in testing but have reached the stable_karma threshold.
-        """
-        update = model.Update.query.first()
-        update.autokarma = False
-        update.status = UpdateStatus.testing
-        update.stable_karma = 1
-
-        # meets_testing_requirement() should return True since the karma threshold has been reached
-        assert update.meets_testing_requirements
-
-    def test_test_gating_faild_no_testing_requirements(self):
-        """
-        The Update.meets_testing_requirements() should return False, if the test gating
-        status of an update is failed.
-        """
-        config["test_gating.required"] = True
-        update = model.Update.query.first()
-        update.autokarma = False
-        update.stable_karma = 1
-        update.test_gating_status = TestGatingStatus.failed
-        update.comment(self.db, 'I found $100 after applying this update.', karma=1,
-                       author='bowlofeggs')
-        # Assert that our preconditions from the docblock are correct.
-        assert not update.meets_testing_requirements
-
-    def test_test_gating_queued_no_testing_requirements(self):
-        """
-        The Update.meets_testing_requirements() should return False, if the test gating
-        status of an update is queued.
-        """
-        config["test_gating.required"] = True
-        update = model.Update.query.first()
-        update.autokarma = False
-        update.stable_karma = 1
-        update.test_gating_status = TestGatingStatus.queued
-        update.comment(self.db, 'I found $100 after applying this update.', karma=1,
-                       author='bowlofeggs')
-        # Assert that our preconditions from the docblock are correct.
-        assert not update.meets_testing_requirements
-
-    def test_test_gating_running_no_testing_requirements(self):
-        """
-        The Update.meets_testing_requirements() should return False, if the test gating
-        status of an update is running.
-        """
-        config["test_gating.required"] = True
-        update = model.Update.query.first()
-        update.autokarma = False
-        update.stable_karma = 1
-        update.test_gating_status = TestGatingStatus.running
-        update.comment(self.db, 'I found $100 after applying this update.', karma=1,
-                       author='bowlofeggs')
-        # Assert that our preconditions from the docblock are correct.
-        assert not update.meets_testing_requirements
-
-    def test_test_gating_missing_testing_requirements(self):
-        """
-        The Update.meets_testing_requirements() should return True, if the test gating
-        status of an update is missing.
-        """
-        config["test_gating.required"] = True
-        update = model.Update.query.first()
-        update.autokarma = False
-        update.stable_karma = 1
-        update.test_gating_status = None
-        update.comment(self.db, 'I found $100 after applying this update.', karma=1,
-                       author='bowlofeggs')
-        # Assert that our preconditions from the docblock are correct.
-        assert update.meets_testing_requirements
-
-    def test_test_gating_waiting_testing_requirements(self):
+    @pytest.mark.parametrize('critpath', (True, False))
+    @pytest.mark.parametrize(
+        "status,expected", (
+            (TestGatingStatus.waiting, False),
+            (TestGatingStatus.queued, False),
+            (TestGatingStatus.ignored, True),
+            (TestGatingStatus.running, False),
+            (TestGatingStatus.passed, True),
+            (TestGatingStatus.failed, False),
+            (None, True),
+            (TestGatingStatus.greenwave_failed, False)
+        )
+    )
+    def test_test_gating_status(self, status, expected, critpath):
         """
-        The Update.meets_testing_requirements() should return False, if the test gating
-        status of an update is waiting.
+        Test meets_testing_requirements() with various test gating statuses,
+        with gating required and the update otherwise expected to pass checks.
+        The 'expected' indicates whether we expect the check to pass or fail for
+        this status. Results should be same for critpath and non-critpath.
         """
         config["test_gating.required"] = True
         update = model.Update.query.first()
         update.autokarma = False
-        update.stable_karma = 1
-        update.test_gating_status = TestGatingStatus.waiting
-        update.comment(self.db, 'I found $100 after applying this update.', karma=1,
-                       author='bowlofeggs')
+        update.critpath = critpath
+        update.test_gating_status = status
+        update.comment(self.db, 'testing', author='bro', karma=1)
         # Assert that our preconditions from the docblock are correct.
-        assert not update.meets_testing_requirements
+        assert update.karma == 2
+        assert update.release.critpath_min_karma == 2
+        assert update.meets_testing_requirements is expected
 
-    def test_test_gating_off(self):
+    def test_test_gating_not_required(self):
         """
         The Update.meets_testing_requirements() should return True if the
-        testing gating is not required, regardless of its test gating status.
+        testing gating is not required, even with a gating status which would
+        usually cause it to return False.
         """
         config["test_gating.required"] = False
         update = model.Update.query.first()
@@ -2859,28 +2836,9 @@ def test_test_gating_off(self):
         update.comment(self.db, 'I found $100 after applying this update.', karma=1,
                        author='bowlofeggs')
         # Assert that our preconditions from the docblock are correct.
+        assert update.karma == 2
         assert update.meets_testing_requirements
 
-    def test_time_in_testing_met(self):
-        """It should return True for non-critpath updates that meet time in testing."""
-        update = model.Update.query.first()
-        update.status = model.UpdateStatus.testing
-        update.request = None
-        update.date_testing = datetime.utcnow() - timedelta(days=8)
-        update.stable_karma = 10
-
-        assert update.meets_testing_requirements
-
-    def test_time_in_testing_unmet(self):
-        """It should return False for non-critpath updates that don't yet meet time in testing."""
-        update = model.Update.query.first()
-        update.status = model.UpdateStatus.testing
-        update.request = None
-        update.date_testing = datetime.utcnow() - timedelta(days=6)
-        update.stable_karma = 10
-
-        assert not update.meets_testing_requirements
-
 
 @mock.patch('bodhi.server.models.work_on_bugs_task', mock.Mock())
 @mock.patch('bodhi.server.models.fetch_test_cases_task', mock.Mock())

From 21ec2e2c62fbeac7effd65cf548396caf1f19f69 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Thu, 1 Feb 2024 16:56:06 -0800
Subject: [PATCH 03/11] Refactor all tests for check_karma_thresholds

The existing tests that ultimately test the stuff handled
by check_karma_thresholds are spread around, both within
individual test files and across them. There's kind of a grab
bag of tests of different paths that have been added a bit
haphazardly over the years.

This tries to reconcile them all into a set of comprehensive
and consistent tests that all live in one place. I did quite
a lot of checking to try and make sure everything that was
currently tested still is, and that some things which weren't
currently tested are now. This is a precursor to changing
some of the behaviour of check_karma_thresholds in later
commits.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 bodhi-server/tests/services/test_updates.py | 786 +++++++-------------
 bodhi-server/tests/test_models.py           | 162 +---
 2 files changed, 305 insertions(+), 643 deletions(-)

diff --git a/bodhi-server/tests/services/test_updates.py b/bodhi-server/tests/services/test_updates.py
index 36f53aaf0e..13cc21dc6b 100644
--- a/bodhi-server/tests/services/test_updates.py
+++ b/bodhi-server/tests/services/test_updates.py
@@ -1595,6 +1595,9 @@ def test_provenpackager_request_privs(self, *args):
         self.db.info['messages'] = []
         self.db.commit()
 
+        # FIXME: this test is crazy nonsense. this bit especially. why are we
+        # triggering an autopush here?
+        # https://github.com/fedora-infra/bodhi/issues/5597
         assert update.karma == 0
         update.comment(self.db, "foo", 1, 'foo')
         update = Build.query.filter_by(nvr=nvr).one().update
@@ -3690,123 +3693,6 @@ def test_edit_locked_update(self, *args):
         build = self.db.query(RpmBuild).filter_by(nvr=nvr).one()
         assert up.builds == [build]
 
-    def test_pending_update_on_stable_karma_reached_autopush_enabled(self, *args):
-        """Ensure that a pending update stays in testing if it hits stable karma while pending."""
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = True
-        args['stable_karma'] = 2
-        args['unstable_karma'] = -2
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            self.app.post_json('/updates/', args)
-
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-        up.status = UpdateStatus.pending
-        self.db.commit()
-
-        up.comment(self.db, 'WFM', author='dustymabe', karma=1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        with fml_testing.mock_sends(api.Message, api.Message):
-            up.comment(self.db, 'LGTM', author='bowlofeggs', karma=1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        assert up.karma == 2
-        assert up.request == UpdateRequest.stable
-        assert up.status == UpdateStatus.pending
-
-    def test_pending_urgent_update_on_stable_karma_reached_autopush_enabled(self, *args):
-        """
-        Ensure that a pending urgent update directly requests for stable if
-        it hits stable karma before reaching testing state.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = True
-        args['stable_karma'] = 2
-        args['unstable_karma'] = -2
-        args['severity'] = 'urgent'
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            self.app.post_json('/updates/', args)
-
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-        up.status = UpdateStatus.pending
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        up.comment(self.db, 'WFM', author='dustymabe', karma=1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        with fml_testing.mock_sends(api.Message, api.Message):
-            up.comment(self.db, 'LGTM', author='bowlofeggs', karma=1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        assert up.karma == 2
-        assert up.request == UpdateRequest.stable
-        assert up.status == UpdateStatus.pending
-
-    def test_pending_update_on_stable_karma_not_reached(self, *args):
-        """ Ensure that pending update does not directly request for stable
-        if it does not hit stable karma before reaching testing state """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = True
-        args['stable_karma'] = 2
-        args['unstable_karma'] = -2
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            self.app.post_json('/updates/', args)
-
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-        up.status = UpdateStatus.pending
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        up.comment(self.db, 'WFM', author='dustymabe', karma=1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        assert up.karma == 1
-        assert up.request == UpdateRequest.testing
-        assert up.status == UpdateStatus.pending
-
-    def test_pending_update_on_stable_karma_reached_autopush_disabled(self, *args):
-        """ Ensure that pending update has option to request for stable directly
-        if it hits stable karma before reaching testing state """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = False
-        args['stable_karma'] = 2
-        args['unstable_karma'] = -2
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            self.app.post_json('/updates/', args)
-
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-        up.status = UpdateStatus.pending
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        up.comment(self.db, 'WFM', author='dustymabe', karma=1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        up.comment(self.db, 'LGTM', author='bowlofeggs', karma=1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        assert up.karma == 2
-        assert up.status == UpdateStatus.pending
-        assert up.request == UpdateRequest.testing
-
-        text = str(config.get('testing_approval_msg'))
-        up.comment(self.db, text, author='bodhi')
-        assert up.comments[-1]['text'] == (
-            'This update can be pushed to stable now if the maintainer wishes'
-        )
-
     def test_obsoletion_locked_with_open_request(self, *args):
         nvr = 'bodhi-2.0.0-2.fc17'
         args = self.get_update(nvr)
@@ -4059,128 +3945,6 @@ def test_revoke_action_for_testing_request(self, *args):
         assert resp.json['update']['request'] is None
         assert resp.json['update']['status'] == 'unpushed'
 
-    def test_obsolete_if_unstable_with_autopush_enabled_when_pending(self, *args):
-        """
-        Send update to obsolete state if it reaches unstable karma on
-        pending state where request is testing when Autopush is enabled. Make sure that it
-        does not go to update-testing state.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = True
-        args['stable_karma'] = 1
-        args['unstable_karma'] = -1
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            response = self.app.post_json('/updates/', args)
-
-        up = Update.get(response.json['alias'])
-        up.status = UpdateStatus.pending
-        up.request = UpdateRequest.testing
-        up.comment(self.db, 'Failed to work', author='ralph', karma=-1)
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-        assert up.karma == -1
-        assert up.status == UpdateStatus.obsolete
-        assert up.request is None
-
-    def test_obsolete_if_unstable_with_autopush_disabled_when_pending(self, *args):
-        """
-        Don't automatically send update to obsolete state if it reaches unstable karma on
-        pending state when Autopush is disabled.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = False
-        args['stable_karma'] = 1
-        args['unstable_karma'] = -1
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            response = self.app.post_json('/updates/', args)
-
-        up = Update.get(response.json['alias'])
-        up.status = UpdateStatus.pending
-        up.request = UpdateRequest.testing
-        up.comment(self.db, 'Failed to work', author='ralph', karma=-1)
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-        assert up.karma == -1
-        assert up.status == UpdateStatus.pending
-        assert up.request == UpdateRequest.testing
-
-    def test_obsolete_if_unstable_karma_not_reached_with_autopush_enabled_when_pending(
-            self, *args):
-        """
-        Don't send update to obsolete state if it does not reach unstable karma threshold
-        on pending state when Autopush is enabled.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = True
-        args['stable_karma'] = 2
-        args['unstable_karma'] = -2
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            response = self.app.post_json('/updates/', args)
-
-        up = Update.get(response.json['alias'])
-        up.status = UpdateStatus.pending
-        up.request = UpdateRequest.testing
-        up.comment(self.db, 'Failed to work', author='ralph', karma=-1)
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-        assert up.karma == -1
-        assert up.status == UpdateStatus.pending
-        assert up.request == UpdateRequest.testing
-
-    def test_obsolete_if_unstable_with_autopush_enabled_when_testing(self, *args):
-        """
-        Send update to obsolete state if it reaches unstable karma threshold on
-        testing state where request is stable when Autopush is enabled. Make sure that the
-        autopush remains enabled and the update does not go to stable state.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = True
-        args['stable_karma'] = 2
-        args['unstable_karma'] = -2
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            response = self.app.post_json('/updates/', args)
-
-        up = Update.get(response.json['alias'])
-        up.status = UpdateStatus.testing
-        up.request = UpdateRequest.stable
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        up.comment(self.db, 'Failed to work', author='ralph', karma=-1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        up.comment(self.db, 'WFM', author='puiterwijk', karma=1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        up.comment(self.db, 'It has bug', author='bowlofeggs', karma=-1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        up.comment(self.db, 'Still not working', author='bob', karma=-1)
-        up = self.db.query(Build).filter_by(nvr=nvr).one().update
-
-        assert up.karma == -2
-        assert up.autokarma is True
-        assert up.status == UpdateStatus.obsolete
-        assert up.request is None
-
     def test_request_after_unpush(self, *args):
         """Test request of this update after unpushing"""
         args = self.get_update('bodhi-2.0.0-3.fc17')
@@ -4984,288 +4748,303 @@ def test_karma_threshold_with_disabled_autopush(self, *args):
         assert up['stable_karma'] == 4
         assert up['unstable_karma'] == -4
 
-    def test_disable_autopush_for_critical_updates(self, *args):
-        """Make sure that autopush is disabled if a critical update receives any negative karma"""
-        user = User(name='bob')
-        self.db.add(user)
-        self.db.commit()
-
-        nvr = 'kernel-3.11.5-300.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = True
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        assert resp.json['critpath']
-        assert resp.json['request'] == 'testing'
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-        up.status = UpdateStatus.testing
-        up.request = None
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        # A user gives negative karma first
-        up.comment(self.db, 'Failed to work', author='ralph', karma=-1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        # Another user gives positive karma
-        up.comment(self.db, 'wfm', author='bowlofeggs', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        assert up.karma == 0
-        assert up.status == UpdateStatus.testing
-        assert up.request is None
-
-        # Autopush gets disabled since there is a negative karma from ralph
-        assert up.autokarma is False
-
-    def test_autopush_critical_update_with_no_negative_karma(self, *args):
-        """Autopush critical update when it has no negative karma"""
-        user = User(name='bob')
-        self.db.add(user)
-        self.db.commit()
-
-        nvr = 'kernel-3.11.5-300.fc17'
+    @pytest.mark.parametrize("status", (UpdateStatus.testing, UpdateStatus.pending))
+    @pytest.mark.parametrize("req", (UpdateRequest.stable, None))
+    @pytest.mark.parametrize("composed_by_bodhi", (True, False))
+    @pytest.mark.parametrize("autokarma", (True, False))
+    def test_obsolete_if_unstable(self, autokarma, composed_by_bodhi, req, status):
+        """Pending or testing updates should be obsoleted on reaching the auto-unpush threshold,
+        except if the status is pending and autokarma is disabled. Whether the update is
+        composed_by_bodhi should not matter. "Obsoleted" should mean the update status is
+        set to obsolete and its request to None. Obsoletion should not happen with just one
+        negative karma (if the threshold is -2)."""
+        nvr = 'bodhi-2.0.0-2.fc17'
         args = self.get_update(nvr)
-        args['autokarma'] = True
-        args['stable_karma'] = 2
+        args['autokarma'] = autokarma
+        args['autotime'] = False
+        args['stable_karma'] = 1
         args['unstable_karma'] = -2
 
         with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
                                     update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        assert resp.json['critpath']
-        assert resp.json['request'] == 'testing'
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-        up.status = UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        up.comment(self.db, 'LGTM', author='ralph', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        with fml_testing.mock_sends(api.Message, api.Message):
-            up.comment(self.db, 'LGTM', author='bowlofeggs', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-        assert up.karma == 2
-
-        # No negative karma: Update gets automatically marked as stable
-        assert up.autokarma is True
-
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-        assert up.request == UpdateRequest.stable
-
-    def test_manually_push_critical_update_with_negative_karma(self, *args):
-        """
-        Manually push critical update when it has negative karma
-        Autopush gets disabled after it receives negative karma
-        A user gives negative karma, but another 3 users give positive karma
-        The critical update should be manually pushed because of the negative karma
-        """
-        user = User(name='bob')
-        self.db.add(user)
-        self.db.commit()
-
-        nvr = 'kernel-3.11.5-300.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = True
-        args['stable_karma'] = 3
-        args['unstable_karma'] = -3
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        assert resp.json['critpath']
-        assert resp.json['request'] == 'testing'
-
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-        up.status = UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        up.comment(self.db, 'Failed to work', author='ralph', karma=-1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        up.comment(self.db, 'LGTM', author='bowlofeggs', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        up.comment(self.db, 'wfm', author='luke', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        up.comment(self.db, 'LGTM', author='puiterwijk', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        up.comment(self.db, 'LGTM', author='trishnag', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        assert up.karma == 3
-        assert up.autokarma is False
-        # The request should still be at testing. This assertion tests for
-        # https://github.com/fedora-infra/bodhi/issues/989 where karma comments were resetting the
-        # request to None.
-        assert up.request == UpdateRequest.testing
-        assert up.status == UpdateStatus.testing
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        resp = self.app.get(f'/updates/{up.alias}',
-                            headers={'Accept': 'text/html'})
-        assert 'text/html' in resp.headers['Content-Type']
-        assert 'kernel-3.11.5-300.fc17' in resp
-
-    def test_manually_push_critical_update_with_autopush_turned_off(self, *args):
-        """
-        Manually push critical update when it has Autopush turned off
-        and make sure the update doesn't get Autopushed
-        """
-        user = User(name='bob')
-        self.db.add(user)
-        self.db.commit()
-
-        nvr = 'kernel-3.11.5-300.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = False
-        args['stable_karma'] = 3
-        args['unstable_karma'] = -3
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        assert resp.json['critpath']
-        assert resp.json['request'] == 'testing'
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-        up.status = UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        up.comment(self.db, 'LGTM Now', author='ralph', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        up.comment(self.db, 'wfm', author='luke', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        up.comment(self.db, 'LGTM', author='puiterwijk', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
+            response = self.app.post_json('/updates/', args)
 
-        assert up.karma == 3
-        assert up.autokarma is False
-        # The request should still be at testing. This assertion tests for
-        # https://github.com/fedora-infra/bodhi/issues/989 where karma comments were resetting the
-        # request to None.
-        assert up.request == UpdateRequest.testing
-        assert up.status == UpdateStatus.testing
-        # Let's clear any messages that might get sent
+        up = Update.get(response.json['alias'])
+        up.status = status
+        up.request = req
+        up.release.composed_by_bodhi = composed_by_bodhi
+        up.comment(self.db, "foo", -1, 'biz')
+        # check we did not obsolete with just one negative karma
+        assert up.karma == -1
+        assert up.status is status
+        assert up.request is req
+        # throw away messages queued for publishing so far
         self.db.info['messages'] = []
+        # now hit the threshold. we call comment() outside of the mock_sends
+        # context manager, because we need the changes to the update state
+        # caused by the obsoletion to have taken effect when we construct the
+        # expected message
+        up.comment(self.db, "foo", -1, 'bowlofeggs')
+        if status is UpdateStatus.pending and not autokarma:
+            with fml_testing.mock_sends(update_schemas.UpdateCommentV1):
+                # doing a db commit causes the message to be published
+                self.db.commit()
+            assert up.status is status
+            assert up.request is req
+        else:
+            with fml_testing.mock_sends(
+                update_schemas.UpdateKarmaThresholdV1.from_dict(
+                    {'update': up, 'status': 'unstable'}
+                ),
+                update_schemas.UpdateCommentV1
+            ):
+                # doing a db commit causes the message to be published
+                self.db.commit()
+            assert up.status is UpdateStatus.obsolete
+            assert up.request is None
+        # the obsolete path should not disable autokarma, but we can't assert
+        # this unconditionally because we might have hit the earlier disable-
+        # autokarma path
+        if (
+            status is not UpdateStatus.testing
+            or req is UpdateRequest.stable
+            or not composed_by_bodhi
+        ):
+            assert up.autokarma is autokarma
+        assert up.karma == -2
 
-        resp = self.app.get(f'/updates/{up.alias}',
-                            headers={'Accept': 'text/html'})
-        assert 'text/html' in resp.headers['Content-Type']
-        assert 'kernel-3.11.5-300.fc17' in resp
-
-    @pytest.mark.parametrize('rawhide_workflow', (True, False))
-    def test_disable_autopush_non_critical_update_with_negative_karma(self, rawhide_workflow):
-        """Disable autokarma on non-critical updates upon negative comment."""
-        user = User(name='bob')
-        self.db.add(user)
-        self.db.commit()
-
+    def _prepare_autopush_update(self):
+        """Shared preparation step for several subsequent tests which need an update that
+        would qualify for autopush. Combining all these tests into one giant parametrized
+        test gives too many combinations and takes too long to run, and the assertion logic
+        gets awkward, so instead we split the tests out but reuse some prep code."""
         nvr = 'bodhi-2.0.0-2.fc17'
         args = self.get_update(nvr)
         args['autokarma'] = True
-        args['stable_karma'] = 3
-        args['unstable_karma'] = -3
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        assert resp.json['request'] == 'testing'
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-        up.status = UpdateStatus.testing
-        if rawhide_workflow:
-            up.release.composed_by_bodhi = False
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        up.comment(self.db, 'Failed to work', author='ralph', karma=-1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        if not rawhide_workflow:
-            expected_comment = config.get('disable_automatic_push_to_stable')
-            assert len(up.comments) == 4
-            assert up.comments[-1].text == expected_comment
+        args['autotime'] = False
+        args['stable_karma'] = 2
+        with fml_testing.mock_sends(
+            update_schemas.UpdateReadyForTestingV3,
+            update_schemas.UpdateRequestTestingV1,
+        ):
+            response = self.app.post_json('/updates/', args)
+        up = Update.get(response.json['alias'])
+        # just to be clear where we stand, these are not tests but expected
+        # attributes for autopush to kick in
+        assert up.status in (UpdateStatus.pending, UpdateStatus.testing)
+        assert up.release.composed_by_bodhi
+        assert up.release.state is ReleaseState.current
+        up.request = None
+        up.date_approved = None
+        return up
+
+    @mock.patch('bodhi.server.models.notifications.publish', autospec=True)
+    def _reach_autopush_threshold(self, up, publish):
+        """Another shared preparation step, like _prepare_autopush_update. Returns the
+        messages published when the feedback that meets the autopush threshold is posted.
+        """
+        status = up.status
+        request = up.request
+        # just for clarity that stable_karma is not lower and both are reached
+        assert up.release.critpath_min_karma == 2
+        up.comment(self.db, "foo", 1, 'biz')
+        # nothing should have changed yet, on any path. we don't need to test the
+        # messages published so far here, either
+        publish.reset_mock()
+        assert up.karma == 1
+        assert up.status is status
+        assert up.request is request
+        assert up.date_approved is None
+        # now we reach the threshold
+        up.comment(self.db, "foo", 1, 'bowlofeggs')
+        assert up.karma == 2
+        # we never actually change the status on any tested path, only set the
+        # request if we're pushing
+        assert up.status is status
+        return [call[0][0] for call in publish.call_args_list]
+
+    @pytest.mark.parametrize("status", (UpdateStatus.testing, UpdateStatus.pending))
+    @pytest.mark.parametrize('critpath', (True, False))
+    @pytest.mark.parametrize('cbb', (True, False))
+    @pytest.mark.parametrize('autokarma', (True, False))
+    def test_autopush_reached_main(self, autokarma, cbb, critpath, status):
+        """The update should be autopushed (request set to stable) if update reaches
+        stable_karma, autokarma is on and release is composed by Bodhi, whether or not the update
+        is critical path (so long as stable_karma is not lower than critpath_min_karma), and
+        whether its status is testing or pending. This test covers the main cases for autopush.
+        Subsequent tests cover some other cases where updates that otherwise would be autopushed
+        are not; these aren't covered in this test for reasons explained in the docstring of
+        _prepare_autopush_update."""
+        up = self._prepare_autopush_update()
+        up.autokarma = autokarma
+        up.release.composed_by_bodhi = cbb
+        up.critpath = critpath
+        up.status = status
+        msgs = self._reach_autopush_threshold(up)
+
+        # we always set date_approved if release is composed by bodhi
+        assert bool(up.date_approved) is cbb
+
+        if (cbb and autokarma):
+            # we should have autopushed
+            assert up.request is UpdateRequest.stable
+            assert msgs == [
+                update_schemas.UpdateRequestStableV1.from_dict(
+                    {'update': up, 'agent': 'bodhi'}
+                ),
+                update_schemas.UpdateKarmaThresholdV1.from_dict(
+                    {'update': up, 'status': 'stable'}
+                ),
+                # the most recent comment is the "has been pushed to stable"
+                # comment, which is from bodhi, who is a system user, so no
+                # message is published. the second-to-last comment is the
+                # one from _reach_autopush_threshold() and we should see a
+                # message for that.
+                update_schemas.UpdateCommentV1.from_dict(
+                    {'comment': up['comments'][-2], 'agent': 'bowlofeggs'}
+                )
+            ]
         else:
-            assert len(up.comments) == 3
-
-        up.comment(self.db, 'LGTM Now', author='ralph', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        up.comment(self.db, 'wfm', author='luke', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        up.comment(self.db, 'LGTM', author='puiterwijk', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        assert up.karma == 3
-        if not rawhide_workflow:
-            assert up.autokarma is False
+            # we should not
+            assert up.request is None
+            assert msgs == [
+                # in this case, the comment from _reach_autopush_threshold() is the
+                # most recent one
+                update_schemas.UpdateCommentV1.from_dict(
+                    {'comment': up['comments'][-1], 'agent': 'bowlofeggs'}
+                )
+            ]
+
+    def test_autopush_reached_obsolete(self):
+        """Autopush should not happen if it otherwise would, but the update is obsolete."""
+        up = self._prepare_autopush_update()
+        up.status = UpdateStatus.obsolete
+        self._reach_autopush_threshold(up)
+        assert up.request is None
+        assert up.date_approved is None
+
+    @pytest.mark.parametrize("status", (UpdateStatus.testing, UpdateStatus.pending))
+    def test_autopush_reached_frozen(self, status):
+        """Autopush should not happen if it otherwise would, but the update's release is frozen
+        and the release status is not testing (pending)."""
+        up = self._prepare_autopush_update()
+        up.release.state = ReleaseState.frozen
+        up.status = status
+        self._reach_autopush_threshold(up)
+        if status is UpdateStatus.pending:
+            assert up.request is None
+            assert up.date_approved is None
         else:
-            assert up.autokarma is True
-
-        # Request and Status remains testing since the autopush is disabled
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-        assert up.request == UpdateRequest.testing
-        assert up.status == UpdateStatus.testing
-
-    def test_autopush_non_critical_update_with_no_negative_karma(self, *args):
-        """
-        Make sure autopush doesn't get disabled for Non Critical update if it
-        does not receive any negative karma. Test update gets automatically
-        marked as stable.
-        """
-        user = User(name='bob')
-        self.db.add(user)
-        self.db.commit()
+            assert up.request is UpdateRequest.stable
+            assert up.date_approved is not None
 
+    def test_autopush_reached_gating_failed(self):
+        """Autopush should not happen if it otherwise would, but the update failed gating."""
+        up = self._prepare_autopush_update()
+        up.test_gating_status = TestGatingStatus.failed
+        with mock.patch('bodhi.server.models.Update.update_test_gating_status'):
+            with mock.patch.dict('bodhi.server.models.config', {'test_gating.required': True}):
+                self._reach_autopush_threshold(up)
+        assert up.request is None
+        assert up.date_approved is None
+
+    @pytest.mark.parametrize('status', (UpdateStatus.testing, UpdateStatus.pending))
+    @pytest.mark.parametrize('critpath', (True, False))
+    def test_autopush_reached_critpath_not(self, critpath, status):
+        """If the stable_karma threshold is reached but it is lower than the release's
+        critpath_min_karma threshold and that is not reached, autopush should happen for
+        a non-critpath update but not for a critpath update. For a critpath update, if
+        status is testing, the request should not be changed; if it's pending, the
+        request should be changed to testing.
+        """
+        up = self._prepare_autopush_update()
+        up.critpath = critpath
+        up.status = status
+        up.stable_karma = 1
+        assert up.release.critpath_min_karma == 2
+        # we don't use _reach_autopush_threshold here because this is a bit different
+        with mock.patch('bodhi.server.models.notifications.publish', autospec=True) as publish:
+            _, caveats = up.comment(self.db, "foo", 1, 'biz')
+            msgs = [type(call[0][0]) for call in publish.call_args_list]
+        assert up.karma == 1
+        if critpath and status is UpdateStatus.testing:
+            assert up.request is None
+            # we should not set this, really, but currently we do
+            assert up.date_approved is not None
+            assert msgs == [update_schemas.UpdateCommentV1]
+            assert caveats == (
+                [{'name': 'karma',
+                  'description': ('This critical path update has not yet been approved for pushing '
+                                  'to the stable repository.  It must first reach a karma of 2, '
+                                  'consisting of 0 positive karma from proventesters, along with 2 '
+                                  'additional karma from the community. Or, it must spend 14 days '
+                                  'in testing without any negative feedback')}])
+        else:
+            if critpath:
+                assert up.request is UpdateRequest.testing
+                reqmsg = update_schemas.UpdateRequestTestingV1
+            else:
+                assert up.request is UpdateRequest.stable
+                reqmsg = update_schemas.UpdateRequestStableV1
+            assert up.date_approved is not None
+            assert msgs == [
+                reqmsg,
+                update_schemas.UpdateKarmaThresholdV1,
+                update_schemas.UpdateCommentV1
+            ]
+
+    def test_autopush_reached_disabled_keep_request(self):
+        """Test that, when we reach stable_karma on an update with autokarma disabled, we
+        do not reset the update's request. This is a regression test for
+        https://github.com/fedora-infra/bodhi/issues/989
+        """
+        up = self._prepare_autopush_update()
+        up.autokarma = False
+        up.request = UpdateRequest.testing
+        self._reach_autopush_threshold(up)
+        assert up.request is UpdateRequest.testing
+
+    @pytest.mark.parametrize('status', (UpdateStatus.testing, UpdateStatus.pending))
+    @pytest.mark.parametrize("cbb", (True, False))
+    @pytest.mark.parametrize("critpath", (True, False))
+    @pytest.mark.parametrize("autotime", (True, False))
+    @pytest.mark.parametrize("autokarma", (True, False))
+    @pytest.mark.parametrize("negk", (True, False))
+    def test_autopush_disabled_on_negative_karma(
+            self, negk, autokarma, autotime, critpath, cbb, status):
+        """check_karma_thresholds() should disable all autopush settings if negative
+        karma is received, on updates in testing status for releases composed by Bodhi,
+        whether or not the update is critical path."""
         nvr = 'bodhi-2.0.0-2.fc17'
         args = self.get_update(nvr)
-        args['autokarma'] = True
+        args['autokarma'] = autokarma
+        args['autotime'] = autotime
         args['stable_karma'] = 2
-        args['unstable_karma'] = -2
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        assert resp.json['request'] == 'testing'
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-        up.status = UpdateStatus.testing
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        up.comment(self.db, 'LGTM Now', author='ralph', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        with fml_testing.mock_sends(api.Message, api.Message):
-            up.comment(self.db, 'WFM', author='puiterwijk', karma=1)
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-
-        # No negative karma: Update gets automatically marked as stable
-        assert up.autokarma is True
+        with fml_testing.mock_sends(
+            update_schemas.UpdateReadyForTestingV3,
+            update_schemas.UpdateRequestTestingV1,
+        ):
+            response = self.app.post_json('/updates/', args)
+        up = Update.get(response.json['alias'])
+        up.status = status
+        up.request = None
+        up.release.composed_by_bodhi = cbb
+        up.critpath = critpath
+        if negk:
+            up.comment(self.db, "foo", -1, 'biz')
+        else:
+            up.comment(self.db, "foo", 0, 'biz')
+        if negk and cbb and status is UpdateStatus.testing:
+            assert up.autokarma is False
+            assert up.autotime is False
+            if autokarma or autotime:
+                assert up.comments[-1].text == config.get('disable_automatic_push_to_stable')
+        else:
+            assert up.autokarma is autokarma
+            assert up.autotime is autotime
 
-        up = self.db.query(Update).filter_by(alias=resp.json['alias']).one()
-        assert up.request == UpdateRequest.stable
+        assert up.request is None
+        assert up.status == status
 
     def test_edit_button_not_present_when_stable(self, *args):
         """
@@ -5797,6 +5576,7 @@ def test_submit_older_build_to_stable(self, *args):
                         stable_karma=3, unstable_karma=-3)
         update.comment(self.db, "foo1", 1, 'foo1')
         update.comment(self.db, "foo2", 1, 'foo2')
+        # this is triggering a karma-autopush to get the newer update pushed stable
         with fml_testing.mock_sends(api.Message, api.Message, api.Message, api.Message):
             update.comment(self.db, "foo3", 1, 'foo3')
         self.db.add(update)
diff --git a/bodhi-server/tests/test_models.py b/bodhi-server/tests/test_models.py
index b0187bc76a..c6794aec32 100644
--- a/bodhi-server/tests/test_models.py
+++ b/bodhi-server/tests/test_models.py
@@ -3598,51 +3598,6 @@ def test__composite_karma_one_negative_two_positive(self):
 
         assert self.obj._composite_karma == (2, -1)
 
-    def test_check_karma_thresholds_obsolete(self):
-        """check_karma_thresholds() should no-op on an obsolete update."""
-        self.obj.status = UpdateStatus.obsolete
-        self.obj.request = None
-        self.obj.comment(self.db, "foo", 1, 'biz')
-        self.obj.stable_karma = 1
-
-        self.obj.check_karma_thresholds(self.db, 'bowlofeggs')
-
-        assert self.obj.request is None
-        assert self.obj.status == UpdateStatus.obsolete
-
-    def test_check_karma_thresholds_gating_fail(self):
-        """check_karma_thresholds should no-op on an update that meets
-        the threshold but does not meet gating requirements.
-        """
-        config["test_gating.required"] = True
-        self.obj.status = UpdateStatus.testing
-        self.obj.request = None
-        self.obj.autokarma = True
-        self.obj.comment(self.db, "foo", 1, 'biz')
-        self.obj.stable_karma = 1
-        self.obj.test_gating_status = TestGatingStatus.failed
-
-        self.obj.check_karma_thresholds(self.db, 'bowlofeggs')
-
-        assert self.obj.request is None
-        assert self.obj.status == UpdateStatus.testing
-
-    def test_check_karma_thresholds_frozen_release(self):
-        """check_karma_thresholds should no-op on an update those
-        release is in frozen state.
-        """
-        self.obj.status = UpdateStatus.pending
-        self.obj.request = UpdateRequest.testing
-        self.obj.autokarma = True
-        self.obj.comment(self.db, "foo", 1, 'biz')
-        self.obj.stable_karma = 1
-        self.obj.release.state = ReleaseState.frozen
-
-        self.obj.check_karma_thresholds(self.db, 'bowlofeggs')
-
-        assert self.obj.request is UpdateRequest.testing
-        assert self.obj.status == UpdateStatus.pending
-
     def test_critpath_approved_no_release_requirements(self):
         """critpath_approved() should use the broad requirements if the release doesn't have any."""
         self.obj.critpath = True
@@ -3678,43 +3633,6 @@ def test_last_modified_no_dates(self):
             self.obj.last_modified
         assert 'Update has no timestamps set:' in str(exc.value)
 
-    def test_stable_karma(self):
-        update = self.obj
-        update.request = None
-        update.status = UpdateStatus.testing
-        assert update.karma == 0
-        assert update.request is None
-        update.comment(self.db, "foo", 1, 'foo')
-        assert update.karma == 1
-        assert update.request is None
-        update.comment(self.db, "foo", 1, 'bar')
-        assert update.karma == 2
-        assert update.request is None
-        # Let's flush out any messages that have been sent.
-        self.db.info['messages'] = []
-        expected_message_0 = update_schemas.UpdateCommentV1.from_dict(
-            {'comment': self.obj['comments'][0], 'agent': 'biz'})
-        expected_message_1 = update_schemas.UpdateKarmaThresholdV1.from_dict(
-            {'update': self.obj, 'status': 'stable'})
-        expected_message_2 = update_schemas.UpdateRequestStableV1.from_dict(
-            {'update': self.obj, 'agent': 'bodhi'})
-
-        with mock_sends(expected_message_2, expected_message_1, expected_message_0):
-            update.comment(self.db, "foo", 1, 'biz')
-            # comment alters the update a bit, so we need to adjust the expected messages to
-            # reflect those changes so the mock_sends() check will pass.
-            expected_message_0.body['comment'] = self.obj['comments'][-2].__json__()
-            # Since we cheated and copied comment 0, we need to change the headers to show biz
-            # as the user instead of foo.
-            expected_message_0._headers['fedora_messaging_user_biz'] = True
-            del expected_message_0._headers['fedora_messaging_user_foo']
-            expected_message_1.body['update'] = self.obj.__json__()
-            expected_message_2.body['update'] = self.obj.__json__()
-            self.db.commit()
-
-        assert update.karma == 3
-        assert update.request == UpdateRequest.stable
-
     def test_obsolete_if_unstable_unstable(self):
         """Test obsolete_if_unstable() when all conditions are met for instability."""
         self.obj.autokarma = True
@@ -3741,39 +3659,6 @@ def test_revoke_no_request(self):
             self.obj.revoke()
         assert str(exc.value) == 'Can only revoke an update with an existing request'
 
-    def test_unstable_karma(self):
-        update = self.obj
-        update.status = UpdateStatus.testing
-        assert update.karma == 0
-        assert update.status == UpdateStatus.testing
-        update.comment(self.db, "foo", -1, 'foo')
-        assert update.status == UpdateStatus.testing
-        assert update.karma == -1
-        update.comment(self.db, "bar", -1, 'bar')
-        assert update.status == UpdateStatus.testing
-        assert update.karma == -2
-        # Let's flush out any messages that have been sent.
-        self.db.info['messages'] = []
-        expected_message_0 = update_schemas.UpdateCommentV1.from_dict(
-            {'comment': self.obj['comments'][0], 'agent': 'biz'})
-        expected_message_1 = update_schemas.UpdateKarmaThresholdV1.from_dict(
-            {'update': self.obj, 'status': 'unstable'})
-
-        with mock_sends(expected_message_1, expected_message_0):
-            update.comment(self.db, "biz", -1, 'biz')
-            # comment alters the update a bit, so we need to adjust the expected messages to
-            # reflect those changes so the mock_sends() check will pass.
-            expected_message_0.body['comment'] = self.obj['comments'][-2].__json__()
-            # Since we cheated and copied comment 0, we need to change the headers to show biz
-            # as the user instead of foo.
-            expected_message_0._headers['fedora_messaging_user_biz'] = True
-            del expected_message_0._headers['fedora_messaging_user_foo']
-            expected_message_1.body['update'] = self.obj.__json__()
-            self.db.commit()
-
-        assert update.karma == -3
-        assert update.status == UpdateStatus.obsolete
-
     def test_update_bugs(self):
         update = self.obj
         assert len(update.bugs) == 2
@@ -3897,10 +3782,18 @@ def test_set_request_pending_stable(self):
         req.koji = buildsys.get_session()
         assert self.obj.status == UpdateStatus.pending
         self.obj.stable_karma = 1
-        with mock_sends(Message):
-            self.obj.comment(self.db, 'works', karma=1, author='bowlofeggs')
+        # disable autokarma, so sending the comment doesn't do the request
+        self.obj.autokarma = False
+        self.obj.comment(self.db, 'works', karma=1, author='bowlofeggs')
+        # make sure we are actually doing something here
+        assert self.obj.request is not UpdateRequest.stable
 
-        self.obj.set_request(self.db, UpdateRequest.stable, req.user.name)
+        expected_messages = (
+            update_schemas.UpdateCommentV1,
+            update_schemas.UpdateRequestStableV1,
+        )
+        with mock_sends(*expected_messages):
+            self.obj.set_request(self.db, UpdateRequest.stable, req.user.name)
 
         assert self.obj.request == UpdateRequest.stable
         assert self.obj.status == UpdateStatus.pending
@@ -4173,11 +4066,19 @@ def test_set_request_string_action(self):
         req.errors = cornice.Errors()
         req.koji = buildsys.get_session()
         assert self.obj.status == UpdateStatus.pending
+        # disable autokarma, so sending the comment doesn't do the request
+        self.obj.autokarma = False
         self.obj.stable_karma = 1
-        with mock_sends(Message):
-            self.obj.comment(self.db, 'works', karma=1, author='bowlofeggs')
+        self.obj.comment(self.db, 'works', karma=1, author='bowlofeggs')
+        # make sure we are actually doing something here
+        assert self.obj.request is not UpdateRequest.stable
 
-        self.obj.set_request(self.db, 'stable', req.user.name)
+        expected_messages = (
+            update_schemas.UpdateCommentV1,
+            update_schemas.UpdateRequestStableV1,
+        )
+        with mock_sends(*expected_messages):
+            self.obj.set_request(self.db, 'stable', req.user.name)
 
         assert self.obj.request == UpdateRequest.stable
         assert self.obj.status == UpdateStatus.pending
@@ -4300,25 +4201,6 @@ def test_status_comment_obsolete(self):
 
         assert [c.text for c in self.obj.comments] == ['This update has been obsoleted.']
 
-    @mock.patch.dict(config, {'critpath.num_admin_approvals': 2})
-    def test_comment_critpath_unapproved(self):
-        """Test a comment reaching karma threshold when update is not critpath approved."""
-        self.obj.autokarma = True
-        self.obj.critpath = True
-        self.obj.stable_karma = 1
-        self.obj.status = UpdateStatus.testing
-
-        # This should cause a caveat.
-        comments, caveats = self.obj.comment(self.db, 'testing 3', author='me3', karma=1)
-
-        assert caveats == (
-            [{'name': 'karma',
-              'description': ('This critical path update has not yet been approved for pushing to '
-                              'the stable repository.  It must first reach a karma of 2, '
-                              'consisting of 2 positive karma from proventesters, along with 0 '
-                              'additional karma from the community. Or, it must spend 14 days in '
-                              'testing without any negative feedback')}])
-
     def test_comment_emails_other_commenters(self):
         """comment() should send e-mails to the other maintainers."""
         bowlofeggs = model.User(name='bowlofeggs', email='bowlofeggs@fp.o')

From 006cf7e2c184739efa9307083eadef66eafdf403 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Fri, 8 Dec 2023 14:14:21 -0800
Subject: [PATCH 04/11] Remove redundant `obsolete_if_unstable`

`obsolete_if_unstable` was introduced by #901 to cover one path:
obsoleting a 'pending' update if it reached its negative karma
threshold. At the time, `check_karma_thresholds` only reached
its negative karma check for updates in 'testing' state, it did
not run on updates in 'pending' state.

However, #1556 extended `check_karma_thresholds` to also run on
updates in 'pending' state, which actually rendered
`obsolete_if_unstable` entirely redundant, except in one case -
calling `comment()` with `check_karma` set to `False` - which
was introduced solely to make it possible to test
`obsolete_if_unstable`.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 bodhi-server/bodhi/server/models.py | 25 +++----------------------
 bodhi-server/tests/test_models.py   | 10 ----------
 2 files changed, 3 insertions(+), 32 deletions(-)

diff --git a/bodhi-server/bodhi/server/models.py b/bodhi-server/bodhi/server/models.py
index 5ff8427c77..fd04eba6f6 100644
--- a/bodhi-server/bodhi/server/models.py
+++ b/bodhi-server/bodhi/server/models.py
@@ -3530,28 +3530,12 @@ def update_bugs(self, bug_ids, session):
         session.flush()
         return new
 
-    def obsolete_if_unstable(self, db):
-        """
-        Obsolete the update if it reached the negative karma threshold while pending.
-
-        Args:
-            db (sqlalchemy.orm.session.Session): A database session.
-        """
-        if self.autokarma and self.status is UpdateStatus.pending \
-                and self.request is UpdateRequest.testing\
-                and self.karma <= self.unstable_karma:
-            log.info("%s has reached unstable karma thresholds", self.alias)
-            self.obsolete(db)
-            log.debug("%s has been obsoleted.", self.alias)
-        return
-
     def comment(self, session, text, karma=0, author=None, karma_critpath=0,
-                bug_feedback=None, testcase_feedback=None, check_karma=True,
-                email_notification=True):
+                bug_feedback=None, testcase_feedback=None, email_notification=True):
         """Add a comment to this update.
 
         If the karma reaches the 'stable_karma' value, then request that this update be marked
-        as stable. If it reaches the 'unstable_karma', it is unpushed.
+        as stable. If it reaches the 'unstable_karma' value, then unpush it (obsolete it).
         """
         if not author:
             raise ValueError('You must provide a comment author')
@@ -3606,7 +3590,7 @@ def comment(self, session, text, karma=0, author=None, karma_critpath=0,
 
             log.info("Updated %s karma to %d", self.alias, self.karma)
 
-            if check_karma and author not in config.get('system_users'):
+            if author not in config.get('system_users'):
                 try:
                     self.check_karma_thresholds(session, 'bodhi')
                 except LockedUpdateException:
@@ -3620,9 +3604,6 @@ def comment(self, session, text, karma=0, author=None, karma_critpath=0,
                         'name': 'karma', 'description': str(e),
                     })
 
-            # Obsolete pending update if it reaches unstable karma threshold
-            self.obsolete_if_unstable(session)
-
         session.flush()
 
         for feedback_dict in bug_feedback:
diff --git a/bodhi-server/tests/test_models.py b/bodhi-server/tests/test_models.py
index c6794aec32..77b2c7a941 100644
--- a/bodhi-server/tests/test_models.py
+++ b/bodhi-server/tests/test_models.py
@@ -3633,16 +3633,6 @@ def test_last_modified_no_dates(self):
             self.obj.last_modified
         assert 'Update has no timestamps set:' in str(exc.value)
 
-    def test_obsolete_if_unstable_unstable(self):
-        """Test obsolete_if_unstable() when all conditions are met for instability."""
-        self.obj.autokarma = True
-        self.obj.status = UpdateStatus.pending
-        self.obj.request = UpdateRequest.testing
-        self.obj.unstable_karma = -1
-        self.obj.comment(self.db, 'foo', -1, 'foo', check_karma=False)
-
-        assert self.obj.status == UpdateStatus.obsolete
-
     @mock.patch('bodhi.server.models.log.warning')
     def test_remove_tag_emptystring(self, warning):
         """Test remove_tag() with a tag of ''."""

From 8d4e19da80dde2770ff8d6fc0564921e456f9b9c Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Fri, 2 Feb 2024 19:28:40 -0800
Subject: [PATCH 05/11] Always obsolete updates that reach unstable_karma
 threshold

Before #901, we obsoleted updates that reached their
unstable_karma threshold regardless of autokarma setting, but
only for updates in 'testing' status, not 'pending' status. #901
intended to fix that.

In the review, @bowlofeggs said "we should only do this [obsolete
updates in pending state that reach their unstable_karma
threshold] if autokarma is turned on", but he did not apparently
realize that was inconsistent with the existing behavior in the
testing state. So after #901, we would obsolete
updates from pending only if autokarma was enabled, but we would
always obsolete updates from testing regardless of the autokarma
setting. This behaviour remained the same afer #1556.

I don't think it makes any sense to preserve this inconsistent
behavior. This PR comes down on the side of always obsoleting
regardless of the setting of autokarma, as I think in practice
this happens most often to updates in 'testing' state not
updates in 'pending' state, so it's the most similar behaviour.
It's also the older behaviour. It would be easy to flip it to
only ever unpush if autokarma is enabled, if we decide that's
better.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 bodhi-server/bodhi/server/models.py           | 11 +++---
 .../bodhi/server/templates/new_update.html    |  4 +--
 bodhi-server/tests/services/test_updates.py   | 36 ++++++++-----------
 3 files changed, 20 insertions(+), 31 deletions(-)

diff --git a/bodhi-server/bodhi/server/models.py b/bodhi-server/bodhi/server/models.py
index fd04eba6f6..365372e258 100644
--- a/bodhi-server/bodhi/server/models.py
+++ b/bodhi-server/bodhi/server/models.py
@@ -3847,13 +3847,10 @@ def check_karma_thresholds(self, db, agent):
                     "%s update has reached the stable karma threshold and can be pushed to "
                     "stable now if the maintainer wishes"), self.alias)
         elif self.unstable_karma and self.karma <= self.unstable_karma:
-            if self.status is UpdateStatus.pending and not self.autokarma:
-                pass
-            else:
-                log.info("Automatically unpushing %s", self.alias)
-                self.obsolete(db)
-                notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
-                    dict(update=self, status='unstable')))
+            log.info("Automatically obsoleting %s (reached unstable karma threshold)", self.alias)
+            self.obsolete(db)
+            notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
+                dict(update=self, status='unstable')))
 
     @property
     def builds_json(self):
diff --git a/bodhi-server/bodhi/server/templates/new_update.html b/bodhi-server/bodhi/server/templates/new_update.html
index 8923164957..5642fddae9 100644
--- a/bodhi-server/bodhi/server/templates/new_update.html
+++ b/bodhi-server/bodhi/server/templates/new_update.html
@@ -318,7 +318,7 @@ <h6 class="fw-bold">Name</h6>
           <div class="card-body pt-0">
               <div class="row ms-3">
                   <div class="col-6">
-                      <h6 class="fw-bold" title="This is the threshold of positive karma required to automatically push an update from testing to stable.">Stable Karma</h6>
+                      <h6 class="fw-bold" title="This is the threshold of positive karma required to automatically push an update from testing to stable. This happens only if 'Auto-request stable based on karma?' is checked.">Stable Karma</h6>
                       <input type="number" class="form-control" name="stable_karma" min="1" required
                       % if update:
                       value="${update.stable_karma}"
@@ -328,7 +328,7 @@ <h6 class="fw-bold" title="This is the threshold of positive karma required to a
                     />
                   </div>
                   <div class="col-6">
-                      <h6 class="fw-bold" title="If checked, this option allows bodhi to automatically move your update from testing to stable once enough time was spent in testing.">Unstable Karma</h6>
+                      <h6 class="fw-bold" title="This is the threshold of negative karma required to automatically unpush an update. This will happen whether or not 'Auto-request stable based on karma?' is checked.">Unstable Karma</h6>
                       <input type="number" class="form-control" name="unstable_karma" max="-1" required
                       % if update:
                       value="${update.unstable_karma}"
diff --git a/bodhi-server/tests/services/test_updates.py b/bodhi-server/tests/services/test_updates.py
index 13cc21dc6b..19524d2935 100644
--- a/bodhi-server/tests/services/test_updates.py
+++ b/bodhi-server/tests/services/test_updates.py
@@ -4753,11 +4753,10 @@ def test_karma_threshold_with_disabled_autopush(self, *args):
     @pytest.mark.parametrize("composed_by_bodhi", (True, False))
     @pytest.mark.parametrize("autokarma", (True, False))
     def test_obsolete_if_unstable(self, autokarma, composed_by_bodhi, req, status):
-        """Pending or testing updates should be obsoleted on reaching the auto-unpush threshold,
-        except if the status is pending and autokarma is disabled. Whether the update is
-        composed_by_bodhi should not matter. "Obsoleted" should mean the update status is
-        set to obsolete and its request to None. Obsoletion should not happen with just one
-        negative karma (if the threshold is -2)."""
+        """Pending or testing updates should be obsoleted on reaching the auto-unpush threshold.
+        Whether the update is composed_by_bodhi should not matter. "Obsoleted" should mean the
+        update status is set to obsolete and its request to None. Obsoletion should not happen
+        with just one negative karma (if the threshold is -2)."""
         nvr = 'bodhi-2.0.0-2.fc17'
         args = self.get_update(nvr)
         args['autokarma'] = autokarma
@@ -4785,23 +4784,16 @@ def test_obsolete_if_unstable(self, autokarma, composed_by_bodhi, req, status):
         # caused by the obsoletion to have taken effect when we construct the
         # expected message
         up.comment(self.db, "foo", -1, 'bowlofeggs')
-        if status is UpdateStatus.pending and not autokarma:
-            with fml_testing.mock_sends(update_schemas.UpdateCommentV1):
-                # doing a db commit causes the message to be published
-                self.db.commit()
-            assert up.status is status
-            assert up.request is req
-        else:
-            with fml_testing.mock_sends(
-                update_schemas.UpdateKarmaThresholdV1.from_dict(
-                    {'update': up, 'status': 'unstable'}
-                ),
-                update_schemas.UpdateCommentV1
-            ):
-                # doing a db commit causes the message to be published
-                self.db.commit()
-            assert up.status is UpdateStatus.obsolete
-            assert up.request is None
+        with fml_testing.mock_sends(
+            update_schemas.UpdateKarmaThresholdV1.from_dict(
+                {'update': up, 'status': 'unstable'}
+            ),
+            update_schemas.UpdateCommentV1
+        ):
+            # doing a db commit causes the message to be published
+            self.db.commit()
+        assert up.status is UpdateStatus.obsolete
+        assert up.request is None
         # the obsolete path should not disable autokarma, but we can't assert
         # this unconditionally because we might have hit the earlier disable-
         # autokarma path

From 0683865975b25465b31e67f4679fb48754e9f89d Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Fri, 8 Dec 2023 15:56:13 -0800
Subject: [PATCH 06/11] Always disable autopush on negative karma, regardless
 of status

Only disabling autopush for updates in 'testing' status makes no
sense. @trishnaguha explained in
https://github.com/fedora-infra/bodhi/pull/1556#discussion_r123996802
that their rationale was that updates in 'pending' status should
get obsoleted (instead?), but this...just isn't right. We only
obsolete updates when they reach the negative karma threshold,
which is usually much stricter than just a single -1 (by default
it's a karma *total* of -3). We want to disable autopush for any
update as soon as it gets a single -1, regardless of whether it's
in pending or testing status, and regardless of whether we might
later obsolete it.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 bodhi-server/bodhi/server/models.py          | 4 ++--
 bodhi-server/tests/services/test_comments.py | 4 ++++
 bodhi-server/tests/services/test_updates.py  | 9 ++++-----
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/bodhi-server/bodhi/server/models.py b/bodhi-server/bodhi/server/models.py
index 365372e258..34bc8846af 100644
--- a/bodhi-server/bodhi/server/models.py
+++ b/bodhi-server/bodhi/server/models.py
@@ -3821,8 +3821,8 @@ def check_karma_thresholds(self, db, agent):
             return
         # If an update receives negative karma disable autopush
         # exclude rawhide updates see #4566
-        if (self.autokarma or self.autotime) and self._composite_karma[1] != 0 and self.status is \
-                UpdateStatus.testing and self.request is not UpdateRequest.stable and \
+        if (self.autokarma or self.autotime) and self._composite_karma[1] != 0 and \
+                self.request is not UpdateRequest.stable and \
                 self.release.composed_by_bodhi:
             log.info("Disabling Auto Push since the update has received negative karma")
             self.autokarma = False
diff --git a/bodhi-server/tests/services/test_comments.py b/bodhi-server/tests/services/test_comments.py
index 2b81575d6c..23082e76ed 100644
--- a/bodhi-server/tests/services/test_comments.py
+++ b/bodhi-server/tests/services/test_comments.py
@@ -186,6 +186,8 @@ def test_commenting_twice_with_double_positive_karma(self):
         assert res.json_body['comment']['update']['karma'] == 1
 
     def test_commenting_twice_with_positive_then_negative_karma(self):
+        up = self.db.query(Build).filter_by(nvr=up2).one().update
+        up.autokarma = up.autotime = 0
         with fml_testing.mock_sends(update_schemas.UpdateCommentV1):
             res = self.app.post_json('/comments/', self.make_comment(up2, karma=1))
 
@@ -208,6 +210,8 @@ def test_commenting_twice_with_positive_then_negative_karma(self):
         assert res.json_body['caveats'][0]['description'] == 'Your karma standing was reversed.'
 
     def test_commenting_with_negative_karma(self):
+        up = self.db.query(Build).filter_by(nvr=up2).one().update
+        up.autokarma = up.autotime = 0
         with fml_testing.mock_sends(update_schemas.UpdateCommentV1):
             res = self.app.post_json('/comments/', self.make_comment(up2, karma=-1))
 
diff --git a/bodhi-server/tests/services/test_updates.py b/bodhi-server/tests/services/test_updates.py
index 19524d2935..c15eda4c83 100644
--- a/bodhi-server/tests/services/test_updates.py
+++ b/bodhi-server/tests/services/test_updates.py
@@ -4798,8 +4798,7 @@ def test_obsolete_if_unstable(self, autokarma, composed_by_bodhi, req, status):
         # this unconditionally because we might have hit the earlier disable-
         # autokarma path
         if (
-            status is not UpdateStatus.testing
-            or req is UpdateRequest.stable
+            req is UpdateRequest.stable
             or not composed_by_bodhi
         ):
             assert up.autokarma is autokarma
@@ -5005,8 +5004,8 @@ def test_autopush_reached_disabled_keep_request(self):
     def test_autopush_disabled_on_negative_karma(
             self, negk, autokarma, autotime, critpath, cbb, status):
         """check_karma_thresholds() should disable all autopush settings if negative
-        karma is received, on updates in testing status for releases composed by Bodhi,
-        whether or not the update is critical path."""
+        karma is received, for updates on releases composed by Bodhi, whether or not
+        the update is critical path."""
         nvr = 'bodhi-2.0.0-2.fc17'
         args = self.get_update(nvr)
         args['autokarma'] = autokarma
@@ -5026,7 +5025,7 @@ def test_autopush_disabled_on_negative_karma(
             up.comment(self.db, "foo", -1, 'biz')
         else:
             up.comment(self.db, "foo", 0, 'biz')
-        if negk and cbb and status is UpdateStatus.testing:
+        if negk and cbb:
             assert up.autokarma is False
             assert up.autotime is False
             if autokarma or autotime:

From 976b95e568e8b1b62b589b6d0a64608aac399792 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Fri, 8 Dec 2023 18:51:39 -0800
Subject: [PATCH 07/11] Drop a bogus branch from check_karma_thresholds

This has been hanging around for a while, but it's fairly
useless. At some point this is where we were planning to post
the testing_approval_msg message if the update reached the
critpath.min_karma threshold, but that is actually done by
the approve_testing.py script nowadays. All we do here is log a
message, and it's actually a bogus message anyway, because
stable_karma is *not* the manual push threshold. stable_karma
is the autopush threshold; it's really meaningless if autokarma
is not set. So it doesn't make any sense to log this message
when we hit stable_karma but autopush is not set. We should just
do nothing on that path.

Note this slightly changes when we set date_approved, but the
current behaviour around date_approved makes no sense anyway,
so the next commit revamps it entirely to make much more sense.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 bodhi-server/bodhi/server/models.py         | 16 +++++-----------
 bodhi-server/tests/services/test_updates.py |  5 ++---
 2 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/bodhi-server/bodhi/server/models.py b/bodhi-server/bodhi/server/models.py
index 34bc8846af..31eeeb27bb 100644
--- a/bodhi-server/bodhi/server/models.py
+++ b/bodhi-server/bodhi/server/models.py
@@ -3830,22 +3830,16 @@ def check_karma_thresholds(self, db, agent):
             text = config.get('disable_automatic_push_to_stable')
             self.comment(db, text, author='bodhi')
         elif self.stable_karma and self.karma >= self.stable_karma \
-                and self.release.composed_by_bodhi:
+                and self.release.composed_by_bodhi and self.autokarma:
             if config.get('test_gating.required') and not self.test_gating_passed:
                 log.info("%s reached stable karma threshold, but does not meet gating "
                          "requirements", self.alias)
                 return
             self.date_approved = datetime.utcnow()
-            if self.autokarma:
-                log.info("Automatically marking %s as stable", self.alias)
-                self.set_request(db, UpdateRequest.stable, agent)
-                notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
-                    dict(update=self, status='stable')))
-            else:
-                # Add the stable approval message now
-                log.info((
-                    "%s update has reached the stable karma threshold and can be pushed to "
-                    "stable now if the maintainer wishes"), self.alias)
+            log.info("Automatically marking %s as stable", self.alias)
+            self.set_request(db, UpdateRequest.stable, agent)
+            notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
+                dict(update=self, status='stable')))
         elif self.unstable_karma and self.karma <= self.unstable_karma:
             log.info("Automatically obsoleting %s (reached unstable karma threshold)", self.alias)
             self.obsolete(db)
diff --git a/bodhi-server/tests/services/test_updates.py b/bodhi-server/tests/services/test_updates.py
index c15eda4c83..dca2c2b98a 100644
--- a/bodhi-server/tests/services/test_updates.py
+++ b/bodhi-server/tests/services/test_updates.py
@@ -4873,9 +4873,6 @@ def test_autopush_reached_main(self, autokarma, cbb, critpath, status):
         up.status = status
         msgs = self._reach_autopush_threshold(up)
 
-        # we always set date_approved if release is composed by bodhi
-        assert bool(up.date_approved) is cbb
-
         if (cbb and autokarma):
             # we should have autopushed
             assert up.request is UpdateRequest.stable
@@ -4895,6 +4892,7 @@ def test_autopush_reached_main(self, autokarma, cbb, critpath, status):
                     {'comment': up['comments'][-2], 'agent': 'bowlofeggs'}
                 )
             ]
+            assert up.date_approved is not None
         else:
             # we should not
             assert up.request is None
@@ -4905,6 +4903,7 @@ def test_autopush_reached_main(self, autokarma, cbb, critpath, status):
                     {'comment': up['comments'][-1], 'agent': 'bowlofeggs'}
                 )
             ]
+            assert up.date_approved is None
 
     def test_autopush_reached_obsolete(self):
         """Autopush should not happen if it otherwise would, but the update is obsolete."""

From b3cedf0ee27ee162ac14db10adf3430112307134 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Wed, 31 Jan 2024 11:39:19 -0800
Subject: [PATCH 08/11] Refactor approve_testing.py and set date_approved
 consistently

This was a logical consequence of the previous change to drop
a branch in check_karma_thresholds. That change affected when
date_approved got set, and investigating that, I realized it is
not set at all consistently. With this, we consistently set
date_approved to be the first time - now always as a
`datetime.datetime` instance - an update becomes eligible for
manual push. Previously it was sometimes set then, sometimes
set when an update was autopushed, and sometimes never set at
all.

We also drop an early bailout in approve_testing.py that is no
longer ever hit in the real world. It initially bailed any time
the release had no "mandatory days in testing", which would have
been true for Rawhide (and early Branched). But when autotime
was added, it was changed to bail only if the release had no
mandatory days in testing *and* the update did not have autotime
set. Updates for Rawhide and other releases like it (ones not
"composed by Bodhi") are always forced to have autotime enabled,
and all releases that *are* "composed by Bodhi" have mandatory
days in testing, so we just never really satisfy the conditions
any more. I verified this by asking nirik to check Bodhi's logs
for the message that should be logged when this bailout is hit;
there were no occurrences of it. Anyway, I don't think an early
bailout is ever really justifiable any more, because we have
gating checks for all branches of Fedora these days.

This refactors approve_testing a bit just to make the different
paths through it clearer, and separate different work into
different functions. We also add comments to make it clearer how
the different autopush paths actually *work*, which was not at
all clear to me before researching this commit.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 bodhi-server/bodhi/server/models.py           |  14 +-
 .../bodhi/server/tasks/approve_testing.py     | 270 ++++++++++--------
 .../tests/tasks/test_approve_testing.py       |  31 +-
 3 files changed, 177 insertions(+), 138 deletions(-)

diff --git a/bodhi-server/bodhi/server/models.py b/bodhi-server/bodhi/server/models.py
index 31eeeb27bb..95e4f80b71 100644
--- a/bodhi-server/bodhi/server/models.py
+++ b/bodhi-server/bodhi/server/models.py
@@ -3798,7 +3798,14 @@ def check_requirements(self, session, settings):
 
     def check_karma_thresholds(self, db, agent):
         """
-        Check if we have reached either karma threshold, and adjust state as necessary.
+        Check if we have reached some karma thresholds, and adjust state as necessary.
+
+        If we receive negative karma, disable autopush (both time and karma). If we
+        reach the karma autopush threshold (and the release is composed by Bodhi), set
+        the request to stable. If we reach the auto-unpush threshold, obsolete the
+        update. This method does **NOT** handle commenting and setting date_approved
+        when the update reaches the manual push karma threshold. That is done by the
+        approve_testing.py script.
 
         This method will call :meth:`set_request` if necessary. If the update is locked, it will
         ignore karma thresholds and raise an Exception.
@@ -3831,11 +3838,14 @@ def check_karma_thresholds(self, db, agent):
             self.comment(db, text, author='bodhi')
         elif self.stable_karma and self.karma >= self.stable_karma \
                 and self.release.composed_by_bodhi and self.autokarma:
+            # Updates for releases not "composed by Bodhi" (Rawhide,
+            # ELN...) are pushed stable only by approve_testing.py
             if config.get('test_gating.required') and not self.test_gating_passed:
                 log.info("%s reached stable karma threshold, but does not meet gating "
                          "requirements", self.alias)
                 return
-            self.date_approved = datetime.utcnow()
+            if not self.date_approved:
+                self.date_approved = datetime.utcnow()
             log.info("Automatically marking %s as stable", self.alias)
             self.set_request(db, UpdateRequest.stable, agent)
             notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
diff --git a/bodhi-server/bodhi/server/tasks/approve_testing.py b/bodhi-server/bodhi/server/tasks/approve_testing.py
index edfc8bc432..22f4456d6d 100644
--- a/bodhi-server/bodhi/server/tasks/approve_testing.py
+++ b/bodhi-server/bodhi/server/tasks/approve_testing.py
@@ -15,11 +15,30 @@
 # You should have received a copy of the GNU General Public License
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
-"""Comment on updates after they reach the mandatory amount of time in the testing repository."""
 
-import logging
+"""
+Make appropriate changes to updates that reach certain karma or time-in-stable thresholds.
+
+This script is intended to run as a regular job (cronjob or the like). It finds all updates in
+UpdateStatus.testing with no request, then does one of three things. If the update does not meet
+testing requirements (i.e. it doesn't have enough karma or time in testing to even be manually
+pushed, or it fails gating checks), we do nothing. Otherwise, we set the date_approved
+for the update if it's not already set, then choose one of the two other options.
+
+If the update meets testing requirements, has autopush after a certain time in testing (autotime)
+enabled, and has reached that threshold, we push it. Note this is the **ONLY** way updates
+for releases "not composed by Bodhi" (Rawhide, ELN, Branched for the first few weeks) are ever
+pushed stable. The other way updates can be autopushed stable - Update.check_karma_thresholds(),
+called by Update.comment() - opts out of handling "not composed by Bodhi" release updates.
 
-from sqlalchemy import func
+If the update meets testing requirements but does not have autotime enabled or has
+not reached that threshold, we check to see if the update already has a comment saying it is
+now "approved" for push to stable. If not, we post that comment, and publish the
+UpdateRequirementsMetStableV1 message.
+"""
+
+import datetime
+import logging
 
 from bodhi.messages.schemas import update as update_schemas
 from bodhi.server import Session, notifications, buildsys
@@ -32,18 +51,13 @@
 
 
 def main():
-    """
-    Comment on updates that are eligible to be pushed to stable.
-
-    Queries for updates in the testing state that have a NULL request, and run approve_update on
-    them.
-    """
+    """Query for updates in the testing state that have a NULL request, and run process_update."""
     db_factory = transactional_session_maker()
     try:
         with db_factory() as db:
             testing = db.query(Update).filter_by(status=UpdateStatus.testing, request=None)
             for update in testing:
-                approve_update(update, db)
+                process_update(update, db)
                 db.commit()
     except Exception:
         log.exception("There was an error approving testing updates.")
@@ -51,117 +65,149 @@ def main():
         db_factory._end_session()
 
 
-def approve_update(update: Update, db: Session):
-    """Add a comment to an update if it is ready for stable.
+def autopush_update(update: Update, db: Session):
+    """
+    Push an update that has autopush for time enabled and has reached the required threshold.
+
+    For releases composed by Bodhi, we set the request and leave the compose process to do the
+    status change. For releases not composed by Bodhi, we do the status change here.
+    """
+    if not update.has_stable_comment:
+        notifications.publish(update_schemas.UpdateRequirementsMetStableV1.from_dict(
+            dict(update=update)))
+    log.info(f"Automatically marking {update.alias} as stable")
+    # For releases composed by Bodhi, just set the request, and leave
+    # the rest to the composer
+    if update.release.composed_by_bodhi:
+        update.set_request(db=db, action=UpdateRequest.stable, username="bodhi")
+        return
+    # For releases not composed by Bodhi, do all the work here
+    # Both side-tag and non-side-tag updates
+    conflicting_builds = update.find_conflicting_builds()
+    if conflicting_builds:
+        builds_str = str.join(", ", conflicting_builds)
+        update.comment(
+            db,
+            "This update cannot be pushed to stable. "
+            f"These builds {builds_str} have a more recent "
+            f"build in koji's {update.release.stable_tag} tag.",
+            author="bodhi")
+        update.request = None
+        if update.from_tag is not None:
+            update.status = UpdateStatus.pending
+            update.remove_tag(
+                update.release.get_pending_testing_side_tag(update.from_tag))
+        else:
+            update.status = UpdateStatus.obsolete
+            update.remove_tag(update.release.pending_testing_tag)
+            update.remove_tag(update.release.candidate_tag)
+        db.commit()
+        log.info(f"{update.alias} has conflicting builds - bailing")
+        return
+    update.add_tag(update.release.stable_tag)
+    update.status = UpdateStatus.stable
+    update.request = None
+    update.pushed = True
+    update.date_stable = datetime.datetime.utcnow()
+    update.comment(db, "This update has been submitted for stable by bodhi",
+                   author=u'bodhi')
+    update.modify_bugs()
+    db.commit()
+    if update.from_tag:
+        # Merging the side tag should happen here
+        pending_signing_tag = update.release.get_pending_signing_side_tag(
+            update.from_tag)
+        testing_tag = update.release.get_pending_testing_side_tag(update.from_tag)
+        update.remove_tag(pending_signing_tag)
+        update.remove_tag(testing_tag)
+        update.remove_tag(update.from_tag)
+        # Delete side-tag and its children after Update has enter stable
+        # We can't fully rely on Koji's auto-purge-when-empty because
+        # there may be older nvrs tagged in the side-tag
+        koji = buildsys.get_session()
+        koji.multicall = True
+        koji.deleteTag(pending_signing_tag)
+        koji.deleteTag(testing_tag)
+        koji.deleteTag(update.from_tag)
+        koji.multiCall()
+    else:
+        # Non side-tag updates
+        update.remove_tag(update.release.pending_testing_tag)
+        update.remove_tag(update.release.pending_stable_tag)
+        update.remove_tag(update.release.pending_signing_tag)
+        update.remove_tag(update.release.testing_tag)
+        update.remove_tag(update.release.candidate_tag)
+
+
+def approved_comment_message(update: Update, db: Session):
+    """Post "approved" comment and publish UpdatesRequirementsMetStable message."""
+    # If this update was already commented, skip it
+    if update.has_stable_comment:
+        log.info(f"{update.alias} has already the comment that it can be pushed to stable - "
+                 "bailing")
+        return
+    # post the comment
+    update.comment(
+        db,
+        str(config.get('testing_approval_msg')),
+        author='bodhi',
+        # Only send email notification about the update reaching
+        # testing approval on releases composed by bodhi
+        email_notification=update.release.composed_by_bodhi
+    )
+    # publish the message
+    notifications.publish(update_schemas.UpdateRequirementsMetStableV1.from_dict(
+        dict(update=update)))
+
+
+def process_update(update: Update, db: Session):
+    """
+    Check requirements, update date_approved, then call appropriate handler function.
+
+    In all cases, this will set date_approved if the update "meets testing requirements" -
+    which means it has reached either the minimum karma or time threshold to be pushed
+    stable, and its gating status is passed - and that date has not been set before.
+
+    After that, if the update has automatic push for time enabled and has reached the required
+    time threshold, this will call autopush_update to handle pushing it. It is intentional
+    that we do not called approved_comment_message() in this case - there is no point alerting
+    the maintainer that the update can be pushed manually if we are already going to push it
+    automatically. See issue #3846.
+
+    Otherwise - if the update is approved but is not being autopushed for time - this will
+    call approved_comment_message() to post the approval comment and publish the
+    UpdateRequirementsMetStable message, if it has not been done before.
 
-    Check that the update is eligible to be pushed to stable but hasn't had comments from Bodhi to
-    this effect. Add a comment stating that the update may now be pushed to stable.
+    It has not yet proven necessary to check the karma autopush threshold here. For releases that
+    are "composed by Bodhi", Update.check_karma_thresholds() pushes updates as soon as they
+    reach the karma autopush threshold. For releases that are not "composed by Bodhi", on update
+    creation, autotime is forced to True and the time threshold is forced to 0, thus updates for
+    these releases are *always* eligible for autotime push here, as soon as they pass gating,
+    there is no case in which autokarma would be relevant.
 
     Args:
         update: an update in testing that may be ready for stable.
+        db: a database session.
     """
-    if not update.release.mandatory_days_in_testing and not update.autotime:
-        # If this release does not have any testing requirements and is not autotime,
-        # skip it
-        log.info(f"{update.alias} doesn't have mandatory days in testing - bailing")
-        return
-    # If updates have reached the testing threshold, say something! Keep in mind
-    # that we don't care about karma here, because autokarma updates get their request set
-    # to stable by the Update.comment() workflow when they hit the required threshold. Thus,
-    # this function only needs to consider the time requirements because these updates have
-    # not reached the karma threshold.
+    # meets_testing_requirements will be True if all non-karma / non-time
+    # requirements are met, and the update has reached the minimum karma
+    # threshold or wait period for a manual push to be allowed, so this
+    # means "update is eligible to be manually pushed stable"
     if not update.meets_testing_requirements:
         log.info(f"{update.alias} has not met testing requirements - bailing")
         return
     log.info(f'{update.alias} now meets testing requirements')
-    # If the update is going to be pushed automatically to stable, do not
-    # double comment that the maintainer can push it manually (#3846)
-    if not update.autotime or update.days_in_testing < update.stable_days:
-        # If this update was already commented, skip it
-        if update.has_stable_comment:
-            log.info(f"{update.alias} has already the comment that it can be pushed to stable - "
-                     "bailing")
-            return
-        # Only send email notification about the update reaching
-        # testing approval on releases composed by bodhi
-        update.comment(
-            db,
-            str(config.get('testing_approval_msg')),
-            author='bodhi',
-            email_notification=update.release.composed_by_bodhi
-        )
-        notifications.publish(update_schemas.UpdateRequirementsMetStableV1.from_dict(
-            dict(update=update)))
+    # always set date_approved, if it has never been set before: this
+    # date indicates "first date update became eligible for manual push"
+    if not update.date_approved:
+        update.date_approved = datetime.datetime.utcnow()
     if update.autotime and update.days_in_testing >= update.stable_days:
-        if not update.has_stable_comment:
-            notifications.publish(update_schemas.UpdateRequirementsMetStableV1.from_dict(
-                dict(update=update)))
-        log.info(f"Automatically marking {update.alias} as stable")
-        # For now only rawhide update can be created using side tag
-        # Do not add the release.pending_stable_tag if the update
-        # was created from a side tag.
-        if update.release.composed_by_bodhi:
-            if not update.date_approved:
-                update.date_approved = func.current_timestamp()
-            update.set_request(db=db, action=UpdateRequest.stable, username="bodhi")
-        # For updates that are not included in composes run by bodhi itself,
-        # mark them as stable
-        else:
-            # Single and Multi build update
-            conflicting_builds = update.find_conflicting_builds()
-            if conflicting_builds:
-                builds_str = str.join(", ", conflicting_builds)
-                update.comment(
-                    db,
-                    "This update cannot be pushed to stable. "
-                    f"These builds {builds_str} have a more recent "
-                    f"build in koji's {update.release.stable_tag} tag.",
-                    author="bodhi")
-                update.request = None
-                if update.from_tag is not None:
-                    update.status = UpdateStatus.pending
-                    update.remove_tag(
-                        update.release.get_pending_testing_side_tag(update.from_tag))
-                else:
-                    update.status = UpdateStatus.obsolete
-                    update.remove_tag(update.release.pending_testing_tag)
-                    update.remove_tag(update.release.candidate_tag)
-                db.commit()
-                log.info(f"{update.alias} has conflicting builds - bailing")
-                return
-            update.add_tag(update.release.stable_tag)
-            update.status = UpdateStatus.stable
-            update.request = None
-            update.pushed = True
-            update.date_stable = update.date_approved = func.current_timestamp()
-            update.comment(db, "This update has been submitted for stable by bodhi",
-                           author=u'bodhi')
-            update.modify_bugs()
-            db.commit()
-            # Multi build update
-            if update.from_tag:
-                # Merging the side tag should happen here
-                pending_signing_tag = update.release.get_pending_signing_side_tag(
-                    update.from_tag)
-                testing_tag = update.release.get_pending_testing_side_tag(update.from_tag)
-                update.remove_tag(pending_signing_tag)
-                update.remove_tag(testing_tag)
-                update.remove_tag(update.from_tag)
-                # Delete side-tag and its children after Update has enter stable
-                # We can't fully rely on Koji's auto-purge-when-empty because
-                # there may be older nvrs tagged in the side-tag
-                koji = buildsys.get_session()
-                koji.multicall = True
-                koji.deleteTag(pending_signing_tag)
-                koji.deleteTag(testing_tag)
-                koji.deleteTag(update.from_tag)
-                koji.multiCall()
-            else:
-                # Single build update
-                update.remove_tag(update.release.pending_testing_tag)
-                update.remove_tag(update.release.pending_stable_tag)
-                update.remove_tag(update.release.pending_signing_tag)
-                update.remove_tag(update.release.testing_tag)
-                update.remove_tag(update.release.candidate_tag)
+        # if update *additionally* meets the time-based autopush threshold,
+        # push it
+        autopush_update(update, db)
+    else:
+        # otherwise, post the comment and publish the message announcing
+        # it is eligible for manual push, if this has not been done
+        approved_comment_message(update, db)
 
     log.info(f'{update.alias} processed by approve_testing')
diff --git a/bodhi-server/tests/tasks/test_approve_testing.py b/bodhi-server/tests/tasks/test_approve_testing.py
index 1399839cec..dd4fb2be6a 100644
--- a/bodhi-server/tests/tasks/test_approve_testing.py
+++ b/bodhi-server/tests/tasks/test_approve_testing.py
@@ -86,7 +86,6 @@ def _assert_not_pushed(self):
         """Run all checks to ensure we did not push the update."""
         assert self.update.status == models.UpdateStatus.testing
         assert self.update.request is None
-        assert self.update.date_approved is None
         assert self.update.date_pushed is None
         assert not self.update.pushed
 
@@ -105,24 +104,6 @@ def _assert_commented(self, times):
         ]
         assert len(approvalcomments) == times
 
-    # for robustness, mock this as True so we *would* do something without the early return
-    @patch('bodhi.server.models.Update.meets_testing_requirements', True)
-    @patch.dict(config, [('fedora.mandatory_days_in_testing', 0)])
-    def test_no_mandatory_days_or_autotime(self):
-        """Ensure that if the update has no mandatory days in testing
-        and autotime is False, we do nothing.
-        """
-        self.update.autotime = False
-        # this should publish no messages
-        with fml_testing.mock_sends():
-            approve_testing_main()
-        # we should not have pushed
-        self._assert_not_pushed()
-        # we should not have posted approval comment
-        self._assert_commented(0)
-        # we should not have set date_approved
-        assert self.update.date_approved is None
-
     @patch('bodhi.server.models.Update.meets_testing_requirements', False)
     def test_update_not_approved(self):
         """
@@ -166,9 +147,8 @@ def test_update_approved_only(self, autotime_enabled):
         self._assert_not_pushed()
         # we should have posted approval comment once
         self._assert_commented(1)
-        # we should not have set date_approved (currently we only do that
-        # when pushing, not approving)
-        assert self.update.date_approved is None
+        # we should have set date_approved
+        assert self.update.date_approved is not None
         # re-run, this time no additional message should be published
         with fml_testing.mock_sends():
             approve_testing_main()
@@ -284,12 +264,15 @@ def test_update_conflicting_build_not_pushed(self, build_creation_time,
 
         # message publishing happens before the conflicting build check, so
         # even when there's a conflicting build, we publish this message
-        # (and set date_approved)
         with fml_testing.mock_sends(update_schemas.UpdateRequirementsMetStableV1):
             approve_testing_main()
 
         assert self.update.status == update_status
-        assert self.update.date_approved is None
+        # date_approved is also set before the conflicting build check, so
+        # we do set it in this case. this isn't really wrong, because that
+        # date really indicates the first date an update met all the requirements
+        # to be manually submitted stable, which is still the case here
+        assert self.update.date_approved is not None
 
         bodhi = self.db.query(models.User).filter_by(name='bodhi').one()
         cmnts = self.db.query(models.Comment).filter_by(update_id=self.update.id, user_id=bodhi.id)

From 2b8d29f175d3cf9b7e75ece6984b2a2180447af9 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Fri, 8 Dec 2023 12:15:32 -0800
Subject: [PATCH 09/11] Overhaul update "met requirements" checks

There are several problems in the general area of "checking if
an update 'meets requirements'".

Most obviously, as discussed in the comments on
https://github.com/fedora-infra/bodhi/issues/3938 , there are
two similar-but-different functions for this,
`Update.meets_testing_requirements()` and
`Update.check_requirements()`. `Update.set_request()` also sort
of re-implements the same thing in-line, and there is also
`Update.critpath_approved()` which does something similar.
These paths all implement somewhat different requirements,
which has various weird and confusing results (like the way
you can sometimes push an update stable from the CLI but not
from the web UI, or the way sometimes updates can be submitted
stable successfully but then get ejected from the stable push
for not meeting requirements).

This commit introduces `meets_requirements_why`, which returns
a boolean and a string explanation, and turns the existing
methods into thin wrappers around it.

The block in the API `set_request()` function which checked
requirements is dropped because it's now redundant; the DB
model `set_request()` which it calls does the same requirement
check. There's no need to do it twice.

The code block commented "Disable pushing critical path updates
for pending releases directly to stable" does not really do
that. It just applied the requirements for critical path
updates on push to stable (the block immediately under it
did the same for non-critpath updates). The boutique requirements
to do with a "critpath.num_admin_approvals" setting were for a
very old policy which gave special status to feedback from
"proven testers"; that setting has been 0 in both Bodhi
deployments for years and is not likely to come back, so we can
just get rid of the setting and simplify the code path.

This commit clarifies and simplifies the meanings of the two
types of stable push threshold settings, and renames some of
them. The `stable_karma` and `stable_days` properties of each
update are now *only* relevant to autopush. They are never used
in "can this update be pushed manually?" calculations. The
`mandatory_days_in_testing` and `min_karma` settings act *only*
as policy minimums for manual push eligibility. They can still
be set with prefix modifiers for the release and the 'status',
as before (to allow Fedora to apply different policies to
different releases at different points in the cycle). If an
update reaches either currently-applicable policy minimum, it
is eligible for stable push. If it does not, it is not. This
results in clearer logic and more consistent requirements that
are more in line with the distribution policies.

critpath.stable_after_days_without_negative_karma is renamed
to critpath.mandatory_days_in_testing and now simply sets
a days-in-testing threshold that applies to critical path
updates only. The "without_negative_karma" logic is removed,
because it has no basis in the Fedora or EPEL update policies;
they never specified such a rule.

critpath.min_karma is renamed to min_karma and applied to all
updates, because this is what the current Fedora and EPEL update
policies say. They both apply the same minimum karma thresholds
to *all* updates. We *could* instead add min_karma and keep
critpath.min_karma, but this is somewhat more work and I don't
want to do more work than is actually necessary here. If the
distros ever do decide to split the thresholds again, we can
re-introduce critpath.min_karma.

The functions and properties in the models are renamed in line
with the new names of the settings, again for sanity's sake.

This is of course an API change and a backwards-incompatible
configuration change. As there are only two real deployments
of Bodhi (both run out of the same ansible repo) and I don't
think anything uses the code interfaces, this seems like it
shouldn't be too much of a problem.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 bodhi-server/bodhi/server/config.py           |  11 +-
 bodhi-server/bodhi/server/models.py           | 249 +++---
 bodhi-server/bodhi/server/services/updates.py |  13 +-
 bodhi-server/bodhi/server/tasks/composer.py   |   2 +-
 bodhi-server/production.ini                   |  12 +-
 bodhi-server/tests/services/test_updates.py   | 760 +++++-------------
 .../tests/tasks/test_approve_testing.py       |   3 +
 bodhi-server/tests/tasks/test_composer.py     | 228 +++---
 bodhi-server/tests/test_models.py             | 245 +++---
 bodhi-server/tests/testing.ini                |   6 +-
 devel/ci/integration/bodhi/production.ini     |  10 +-
 devel/development.ini.example                 |   4 +-
 docs/user/testing.rst                         |  36 +-
 13 files changed, 584 insertions(+), 995 deletions(-)

diff --git a/bodhi-server/bodhi/server/config.py b/bodhi-server/bodhi/server/config.py
index 08a5e98ee8..0589808863 100644
--- a/bodhi-server/bodhi/server/config.py
+++ b/bodhi-server/bodhi/server/config.py
@@ -360,13 +360,7 @@ class BodhiConfig(dict):
         'critpath.jsonpath': {
             'value': '/etc/bodhi/critpath',
             'validator': str},
-        'critpath.min_karma': {
-            'value': 2,
-            'validator': int},
-        'critpath.num_admin_approvals': {
-            'value': 2,
-            'validator': int},
-        'critpath.stable_after_days_without_negative_karma': {
+        'critpath.mandatory_days_in_testing': {
             'value': 14,
             'validator': int},
         'critpath.type': {
@@ -452,6 +446,9 @@ class BodhiConfig(dict):
         'mako.directories': {
             'value': 'bodhi.server:templates',
             'validator': str},
+        'min_karma': {
+            'value': 2,
+            'validator': int},
         'mandatory_packager_groups': {
             'value': ['packager'],
             'validator': _generate_list_validator()},
diff --git a/bodhi-server/bodhi/server/models.py b/bodhi-server/bodhi/server/models.py
index 95e4f80b71..a4acb8d52b 100644
--- a/bodhi-server/bodhi/server/models.py
+++ b/bodhi-server/bodhi/server/models.py
@@ -27,6 +27,7 @@
 import time
 import typing
 import uuid
+import warnings
 
 from mediawiki import MediaWiki
 from packaging.version import parse as parse_version
@@ -887,22 +888,22 @@ class Release(Base):
     composes = relationship('Compose', back_populates='release')
 
     @property
-    def critpath_min_karma(self) -> int:
+    def min_karma(self) -> int:
         """
-        Return the min_karma for critpath updates for this release.
+        Return the min_karma for updates for this release.
 
-        If the release doesn't specify a min_karma, the default critpath.min_karma setting is used
+        If the release doesn't specify a min_karma, the default min_karma setting is used
         instead.
 
         Returns:
-            The minimum karma required for critical path updates for this release.
+            The minimum karma required for updates for this release.
         """
         if self.setting_status:
             min_karma = config.get(
-                f'{self.setting_prefix}.{self.setting_status}.critpath.min_karma', None)
+                f'{self.setting_prefix}.{self.setting_status}.min_karma', None)
             if min_karma:
                 return int(min_karma)
-        return config.get('critpath.min_karma')
+        return config.get('min_karma')
 
     @property
     def version_int(self):
@@ -948,11 +949,11 @@ def critpath_mandatory_days_in_testing(self):
         """
         if self.setting_status:
             days = config.get(f'{self.setting_prefix}.{self.setting_status}'
-                              f'.critpath.stable_after_days_without_negative_karma', None)
+                              f'.critpath.mandatory_days_in_testing', None)
             if days is not None:
                 return int(days)
 
-        return config.get('critpath.stable_after_days_without_negative_karma', 0)
+        return config.get('critpath.mandatory_days_in_testing', 0)
 
     @property
     def collection_name(self):
@@ -2530,6 +2531,16 @@ def new(cls, request, data):
                                f"release value of {up.mandatory_days_in_testing} days"
             })
 
+        # We also need to make sure stable_karma is not set below
+        # the policy minimum for this release
+        if release.min_karma > up.stable_karma:
+            up.stable_karma = release.min_karma
+            caveats.append({
+                'name': 'stable karma',
+                'description': "The stable karma required was set to the mandatory "
+                               f"release value of {release.min_karma}"
+            })
+
         log.debug(f"Adding new update to the db {up.alias}.")
         db.add(up)
         log.debug(f"Triggering db commit for new update {up.alias}.")
@@ -2599,6 +2610,16 @@ def edit(cls, request, data):
                                f"release value of {up.mandatory_days_in_testing} days"
             })
 
+        # We also need to make sure stable_karma is not set below
+        # the policy minimum for this release
+        if up.release.min_karma > data.get('stable_karma', up.stable_karma):
+            data['stable_karma'] = up.release.min_karma
+            caveats.append({
+                'name': 'stable karma',
+                'description': "The stable karma required was raised to the mandatory "
+                               f"release value of {up.release.min_karma}"
+            })
+
         # Determine which builds have been added
         new_builds = []
         for build in data['builds']:
@@ -3128,57 +3149,25 @@ def set_request(self, db, action, username):
                 'The release of this update is frozen and the update has not yet been '
                 'pushed to testing. It is currently not possible to push it to stable.')
 
-        # Disable pushing critical path updates for pending releases directly to stable
-        if action == UpdateRequest.stable and self.critpath:
-            if config.get('critpath.num_admin_approvals') is not None:
-                if not self.critpath_approved:
-                    stern_note = (
-                        'This critical path update has not yet been approved for pushing to the '
-                        'stable repository.  It must first reach a karma of %s, consisting of %s '
-                        'positive karma from proventesters, along with %d additional karma from '
-                        'the community. Or, it must spend %s days in testing without any negative '
-                        'feedback')
-                    additional_karma = config.get('critpath.min_karma') \
-                        - config.get('critpath.num_admin_approvals')
-                    stern_note = stern_note % (
-                        config.get('critpath.min_karma'),
-                        config.get('critpath.num_admin_approvals'),
-                        additional_karma,
-                        config.get('critpath.stable_after_days_without_negative_karma'))
-                    if config.get('test_gating.required'):
-                        stern_note += ' Additionally, it must pass automated tests.'
-                    notes.append(stern_note)
-
-                    if self.status is UpdateStatus.testing:
-                        self.request = None
-                        raise BodhiException('. '.join(notes))
-                    else:
-                        log.info('Forcing critical path update into testing')
-                        action = UpdateRequest.testing
-
-        # Ensure this update meets the minimum testing requirements
-        flash_notes = ''
-        if action == UpdateRequest.stable and not self.critpath:
-            # Check if we've met the karma requirements
-            if self.karma >= self.stable_karma or self.critpath_approved:
-                log.debug('%s meets stable karma requirements' % self.alias)
-            else:
-                # If we haven't met the stable karma requirements, check if it
-                # has met the mandatory time-in-testing requirements
-                if self.mandatory_days_in_testing:
-                    if not self.has_stable_comment and \
-                       not self.meets_testing_requirements:
-                        if self.release.id_prefix == "FEDORA-EPEL":
-                            flash_notes = config.get('not_yet_tested_epel_msg')
-                        else:
-                            flash_notes = config.get('not_yet_tested_msg')
-                        if self.status is UpdateStatus.testing:
-                            self.request = None
-                            raise BodhiException(flash_notes)
-                        elif self.request is UpdateRequest.testing:
-                            raise BodhiException(flash_notes)
-                        else:
-                            action = UpdateRequest.testing
+        # Don't allow push to stable unless testing requirements are met
+        if action == UpdateRequest.stable:
+            met, reason = self.meets_requirements_why
+            if not met:
+                if self.release.id_prefix == "FEDORA-EPEL":
+                    msg = config.get('not_yet_tested_epel_msg')
+                else:
+                    msg = config.get('not_yet_tested_msg')
+                msg = f'{msg}: {reason}'
+                notes.append(msg)
+
+                if self.status is UpdateStatus.testing:
+                    self.request = None
+                    raise BodhiException('. '.join(notes))
+                elif self.request is UpdateRequest.testing:
+                    raise BodhiException('. '.join(notes))
+                else:
+                    log.info('Forcing update into testing')
+                    action = UpdateRequest.testing
 
         # Add the appropriate 'pending' koji tag to this update, so tools
         # could compose repositories of them for testing.
@@ -3200,10 +3189,9 @@ def set_request(self, db, action, username):
         self.request = action
 
         notes = notes and '. '.join(notes) + '.' or ''
-        flash_notes = flash_notes and '. %s' % flash_notes
         log.debug(
-            "%s has been submitted for %s. %s%s" % (
-                self.alias, action.description, notes, flash_notes))
+            "%s has been submitted for %s. %s" % (
+                self.alias, action.description, notes))
 
         comment_text = 'This update has been submitted for %s by %s. %s' % (
             action.description, username, notes)
@@ -3597,8 +3585,8 @@ def comment(self, session, text, karma=0, author=None, karma_critpath=0,
                     pass
                 except BodhiException as e:
                     # This gets thrown if the karma is pushed over the
-                    # threshold, but it is a critpath update that is not
-                    # critpath_approved. ... among other cases.
+                    # threshold, but it does not meet testing requirements
+                    # for some reason (failed gating test...)
                     log.exception('Problem checking the karma threshold.')
                     caveats.append({
                         'name': 'karma', 'description': str(e),
@@ -3774,27 +3762,23 @@ def product_version(self):
 
     def check_requirements(self, session, settings):
         """
-        Check that an update meets its self-prescribed policy to be pushed.
+        Check that an update meets all requirements to be pushed stable.
+
+        See https://docs.fedoraproject.org/en-US/fesco/Updates_Policy .
+        Deprecated in favor of meets_requirements_why property.
 
         Args:
             session (sqlalchemy.orm.session.Session): A database session. Unused.
-            settings (bodhi.server.config.BodhiConfig): Bodhi's settings.
+            settings (bodhi.server.config.BodhiConfig): Bodhi's settings. Unused.
         Returns:
             tuple: A tuple containing (result, reason) where result is a bool
                 and reason is a str.
         """
-        # TODO - check require_bugs and require_testcases also?
-        if config.get('test_gating.required'):
-            if self.test_gating_status == TestGatingStatus.passed:
-                return (True, "All required tests passed or were waived.")
-            elif self.test_gating_status in (TestGatingStatus.ignored, None):
-                return (True, "No checks required.")
-            elif self.test_gating_status == TestGatingStatus.waiting:
-                return (False, "Required tests for this update are still running.")
-            else:
-                return (False, "Required tests did not pass on this update.")
-
-        return (True, "No checks required.")
+        warnings.warn(
+            "check_requirements is deprecated, use meets_requirements_why instead",
+            DeprecationWarning,
+        )
+        return self.meets_requirements_why
 
     def check_karma_thresholds(self, db, agent):
         """
@@ -3896,55 +3880,73 @@ def critpath_approved(self):
         Returns:
             bool: True if this update meets critpath testing requirements, False otherwise.
         """
-        # https://fedorahosted.org/bodhi/ticket/642
-        if self.meets_testing_requirements:
-            return True
-        min_karma = self.release.critpath_min_karma
-        if self.release.setting_status:
-            num_admin_approvals = config.get(
-                f'{self.release.setting_prefix}.{self.release.setting_status}'
-                '.critpath.num_admin_approvals')
-            if num_admin_approvals is not None and min_karma:
-                return self.num_admin_approvals >= int(num_admin_approvals) and \
-                    self.karma >= min_karma
-        return self.num_admin_approvals >= config.get('critpath.num_admin_approvals') and \
-            self.karma >= min_karma
+        warnings.warn(
+            "critpath_approved is deprecated, use meets_testing_requirements instead",
+            DeprecationWarning,
+        )
+        return self.meets_testing_requirements
 
     @property
-    def meets_testing_requirements(self):
+    def meets_requirements_why(self):
         """
-        Return whether or not this update meets its release's testing requirements.
+        Return whether or not this update meets its release's testing requirements and why.
+
+        Checks gating requirements and then minimum karma and wait requirements, see
+        https://docs.fedoraproject.org/en-US/fesco/Updates_Policy . Note this is
+        checking the minimum policy requirements for a manual push (as specified in
+        the release configuration), not the per-update thresholds for an automatic push.
+        Before updates-testing enablement the minimum wait will be 0, so unless the gating
+        check fails, this will always return True.
 
-        If this update's release does not have a mandatory testing requirement, then
-        simply return True.
+        FIXME: we should probably wire up the test case and bug feedback requirements
+        that are shown in the UI but not currently enforced.
 
         Returns:
-            bool: True if the update meets testing requirements, False otherwise.
+            tuple: A tuple containing (result, reason) where result is a bool
+                and reason is a str.
         """
-        num_days = self.mandatory_days_in_testing
+        req_days = self.mandatory_days_in_testing
+        req_karma = self.release.min_karma
 
-        if config.get('test_gating.required') and not self.test_gating_passed:
-            return False
+        if config.get('test_gating.required'):
+            tgs = self.test_gating_status
+            if tgs in (TestGatingStatus.waiting, TestGatingStatus.queued, TestGatingStatus.running):
+                return (False, "Required tests for this update are queued or running.")
+            elif tgs == TestGatingStatus.greenwave_failed:
+                return (False, "Greenwave failed to respond.")
+            elif tgs == TestGatingStatus.passed:
+                basemsg = "All required tests passed or were waived"
+            elif tgs == TestGatingStatus.ignored:
+                basemsg = "No checks required"
+            else:
+                return (False, "Required tests did not pass on this update.")
+        else:
+            basemsg = "Test gating is disabled,"
 
-        if self.karma >= self.release.critpath_min_karma:
-            return True
+        if self.karma >= req_karma:
+            return (True, basemsg + f" and the update has at least {req_karma} karma.")
 
-        if self.critpath:
-            # Ensure there is no negative karma. We're looking at the sum of
-            # each users karma for this update, which takes into account
-            # changed votes.
-            if self._composite_karma[1] < 0:
-                return False
-            return self.days_in_testing >= num_days
+        if not req_days:
+            return (True, basemsg + " and there is no minimum wait set.")
 
-        if not num_days:
-            return True
+        # Any update that reaches req_days has met the testing requirements.
+        if self.days_in_testing >= req_days:
+            return (True, basemsg + " and update meets the wait time requirement.")
+        return (False,
+                basemsg + f" but update has less than {req_karma} karma and has been in testing "
+                f"less than {req_days} days.")
 
-        if self.karma >= self.stable_karma:
-            return True
+    @property
+    def meets_testing_requirements(self):
+        """
+        Return whether or not this update meets its release's testing requirements.
 
-        # Any update that reaches num_days has met the testing requirements.
-        return self.days_in_testing >= num_days
+        Same as meets_requirements_why, but without the reason.
+
+        Returns:
+            bool: True if the update meets testing requirements, False otherwise.
+        """
+        return self.meets_requirements_why[0]
 
     @property
     def has_stable_comment(self):
@@ -3997,25 +3999,6 @@ def days_in_testing(self):
         else:
             return 0
 
-    @property
-    def num_admin_approvals(self):
-        """
-        Return the number of Releng/QA approvals of this update.
-
-        Returns:
-            int: The number of admin approvals found in the comments of this update.
-        """
-        approvals = 0
-        for comment in self.comments_since_karma_reset:
-            if comment.karma != 1:
-                continue
-            admin_groups = config.get('admin_groups')
-            for group in comment.user.groups:
-                if group.name in admin_groups:
-                    approvals += 1
-                    break
-        return approvals
-
     @property
     def test_cases(self):
         """
diff --git a/bodhi-server/bodhi/server/services/updates.py b/bodhi-server/bodhi/server/services/updates.py
index a9b29c9d42..e7d40ec6c7 100644
--- a/bodhi-server/bodhi/server/services/updates.py
+++ b/bodhi-server/bodhi/server/services/updates.py
@@ -211,21 +211,10 @@ def set_request(request):
                            "Pushing back to testing a stable update is not allowed")
         return
 
-    if action == UpdateRequest.stable:
-        settings = request.registry.settings
-        result, reason = update.check_requirements(request.db, settings)
-        if not result:
-            log.info(
-                f'Unable to set request for {update.alias} to stable due to failed requirements: '
-                f'{reason}')
-            request.errors.add('body', 'request',
-                               'Requirement not met %s' % reason)
-            return
-
     try:
         update.set_request(request.db, action, request.identity.name)
     except BodhiException as e:
-        log.info("Failed to set the request: %s", e)
+        log.info(f"Failed to set the request: {e}")
         request.errors.add('body', 'request', str(e))
     except Exception as e:
         log.exception("Unhandled exception in set_request")
diff --git a/bodhi-server/bodhi/server/tasks/composer.py b/bodhi-server/bodhi/server/tasks/composer.py
index 2b874ba11d..ae5d8ae5db 100644
--- a/bodhi-server/bodhi/server/tasks/composer.py
+++ b/bodhi-server/bodhi/server/tasks/composer.py
@@ -469,7 +469,7 @@ def perform_gating(self):
         """Eject Updates that don't meet testing requirements from the compose."""
         log.debug('Performing gating.')
         for update in self.compose.updates:
-            result, reason = update.check_requirements(self.db, config)
+            result, reason = update.meets_requirements_why
             if not result:
                 log.warning("%s failed gating: %s" % (update.alias, reason))
                 self.eject_from_compose(update, reason)
diff --git a/bodhi-server/production.ini b/bodhi-server/production.ini
index d1ba5e5244..47acfb32a9 100644
--- a/bodhi-server/production.ini
+++ b/bodhi-server/production.ini
@@ -408,10 +408,10 @@ use = egg:bodhi-server
 # critpath.num_admin_approvals = 2
 
 # The net karma required to submit a critical path update to a pending release.
-# critpath.min_karma = 2
+# min_karma = 2
 
 # Allow critpath to submit for stable after 2 weeks with no negative karma
-# critpath.stable_after_days_without_negative_karma = 14
+# critpath.mandatory_days_in_testing = 14
 
 # The minimum amount of time an update must spend in testing before
 # it can reach the stable repository
@@ -431,12 +431,12 @@ fedora_epel.mandatory_days_in_testing = 14
 #f28.status = pre_beta
 #f28.pre_beta.mandatory_days_in_testing = 3
 #f28.pre_beta.critpath.num_admin_approvals = 0
-#f28.pre_beta.critpath.min_karma = 1
-#f28.pre_beta.critpath.stable_after_days_without_negative_karma = 3
+#f28.pre_beta.min_karma = 1
+#f28.pre_beta.critpath.mandatory_days_in_testing = 3
 #f28.post_beta.mandatory_days_in_testing = 7
 #f28.post_beta.critpath.num_admin_approvals = 0
-#f28.post_beta.critpath.min_karma = 2
-#f28.post_beta.critpath.stable_after_days_without_negative_karma = 5
+#f28.post_beta.min_karma = 2
+#f28.post_beta.critpath.mandatory_days_in_testing = 5
 
 
 ##
diff --git a/bodhi-server/tests/services/test_updates.py b/bodhi-server/tests/services/test_updates.py
index dca2c2b98a..7380a20d78 100644
--- a/bodhi-server/tests/services/test_updates.py
+++ b/bodhi-server/tests/services/test_updates.py
@@ -688,22 +688,34 @@ def test_new_update_with_invalid_stable_days(self, publish, *args):
         assert up['errors'][0]['description'] == "-1 is less than minimum value 0"
 
     @mock.patch('bodhi.server.notifications.publish')
-    def test_edit_update_stable_days(self, publish, *args):
+    def test_new_update_stable_days(self, publish, *args):
         args = self.get_update(u'bodhi-2.0.0-2.fc17')
         args['stable_days'] = '50'
         r = self.app.post_json('/updates/', args)
         assert r.json['stable_days'] == 50
 
     @mock.patch('bodhi.server.notifications.publish')
-    def test_edit_update_too_low_stable_days(self, publish, *args):
+    def test_new_update_too_low_stable_days(self, publish, *args):
         args = self.get_update(u'bodhi-2.0.0-2.fc17')
         args['stable_days'] = '1'
+        args['stable_karma'] = '50'
         r = self.app.post_json('/updates/', args)
         assert r.json['stable_days'] == 7
         assert r.json['caveats'][0]['description'] == (
             'The number of stable days required was set to the mandatory release value of 7 days'
         )
 
+    @mock.patch('bodhi.server.notifications.publish')
+    def test_new_update_too_low_stable_karma(self, publish, *args):
+        args = self.get_update(u'bodhi-2.0.0-2.fc17')
+        args['stable_days'] = '50'
+        args['stable_karma'] = '1'
+        r = self.app.post_json('/updates/', args)
+        assert r.json['stable_karma'] == 2
+        assert r.json['caveats'][0]['description'] == (
+            'The stable karma required was set to the mandatory release value of 2'
+        )
+
     @mock.patch.dict('bodhi.server.validators.config', {'acl_system': u'dummy'})
     def test_new_update_with_multiple_bugs_as_str(self, *args):
         update = self.get_update('bodhi-2.0.0-2.fc17')
@@ -1117,6 +1129,7 @@ def test_test_gating_status_failed(self, info):
         up.locked = False
         up.test_gating_status = TestGatingStatus.failed
         up.date_testing = datetime.utcnow() - timedelta(days=8)
+        up.status = UpdateStatus.testing
         up.request = None
         post_data = dict(update=nvr, request='stable', csrf_token=self.get_csrf_token())
 
@@ -1126,11 +1139,11 @@ def test_test_gating_status_failed(self, info):
         assert up.request is None
         assert res.json_body['status'] == 'error'
         assert res.json_body['errors'][0]['description'] == (
-            "Requirement not met Required tests did not pass on this update."
+            f"{config.get('not_yet_tested_msg')}: Required tests did not pass on this update."
         )
         info_logs = '\n'.join([c[1][0] for c in info.mock_calls])
-        assert (f'Unable to set request for {up.alias} to stable due to failed requirements: '
-                'Required tests did not pass on this update.') in info_logs
+        assert (f"Failed to set the request: {config.get('not_yet_tested_msg')}: "
+                "Required tests did not pass on this update.") in info_logs
 
     @mock.patch.dict(config, {'test_gating.required': True})
     def test_test_gating_status_passed(self):
@@ -1150,11 +1163,11 @@ def test_test_gating_status_passed(self):
         assert res.json['update']['request'] == 'stable'
 
     @mock.patch('bodhi.server.services.updates.Update.set_request',
-                side_effect=BodhiException('BodhiException. oops!'))
-    @mock.patch('bodhi.server.services.updates.Update.check_requirements',
-                return_value=(True, "a fake reason"))
+                side_effect=BodhiException('oops!'))
+    @mock.patch('bodhi.server.services.updates.Update.meets_requirements_why',
+                (True, "a fake reason"))
     @mock.patch('bodhi.server.services.updates.log.info')
-    def test_BodhiException_exception(self, log_info, check_requirements, send_request, *args):
+    def test_BodhiException_exception(self, log_info, send_request, *args):
         """Ensure that an BodhiException Exception is handled by set_request()."""
         nvr = 'bodhi-2.0-1.fc17'
 
@@ -1169,16 +1182,16 @@ def test_BodhiException_exception(self, log_info, check_requirements, send_reque
                                  status=400)
 
         assert res.json_body['status'] == 'error'
-        assert res.json_body['errors'][0]['description'] == 'BodhiException. oops!'
+        assert res.json_body['errors'][0]['description'] == 'oops!'
         assert log_info.call_count == 1
-        assert log_info.call_args_list[0][0][0] == "Failed to set the request: %s"
+        assert log_info.call_args_list[0][0][0] == "Failed to set the request: oops!"
 
     @mock.patch('bodhi.server.services.updates.Update.set_request',
                 side_effect=IOError('IOError. oops!'))
-    @mock.patch('bodhi.server.services.updates.Update.check_requirements',
-                return_value=(True, "a fake reason"))
+    @mock.patch('bodhi.server.services.updates.Update.meets_requirements_why',
+                (True, "a fake reason"))
     @mock.patch('bodhi.server.services.updates.log.exception')
-    def test_unexpected_exception(self, log_exception, check_requirements, send_request, *args):
+    def test_unexpected_exception(self, log_exception, send_request, *args):
         """Ensure that an unexpected Exception is handled by set_request()."""
         nvr = 'bodhi-2.0-1.fc17'
 
@@ -1580,7 +1593,11 @@ def test_provenpackager_request_privs(self, *args):
 
         # Ensure we can't push it until it meets the requirements
         assert res.json_body['status'] == 'error'
-        assert res.json_body['errors'][0]['description'] == config.get('not_yet_tested_msg')
+        exp = (
+            f"{config.get('not_yet_tested_msg')}: Test gating is disabled, but update has "
+            "less than 2 karma and has been in testing less than 7 days."
+        )
+        assert res.json['errors'][0]['description'] == exp
 
         update = Build.query.filter_by(nvr=nvr).one().update
         assert update.stable_karma == 3
@@ -1682,178 +1699,21 @@ def test_provenpackager_request_privs(self, *args):
         res = app.get(f'/updates/{update.alias}', status=200)
         assert res.json_body['can_edit'] is False
 
-    def test_provenpackager_request_update_queued_in_test_gating(self, *args):
-        """Ensure provenpackagers cannot request changes for any update which
-        test gating status is `queued`"""
-        nvr = 'bodhi-2.1-1.fc17'
-        user = User(name='bob')
-        self.db.add(user)
-        group = self.db.query(Group).filter_by(name='provenpackager').one()
-        user.groups.append(group)
-        self.db.commit()
-
-        up_data = self.get_update(nvr)
-        up_data['csrf_token'] = self.app.get('/csrf').json_body['csrf_token']
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            res = self.app.post_json('/updates/', up_data)
-
-        build = self.db.query(RpmBuild).filter_by(nvr=nvr).one()
-        build.update.test_gating_status = TestGatingStatus.queued
-        assert build.update.request == UpdateRequest.testing
-
-        # Try and submit the update to stable as a provenpackager
-        post_data = dict(update=nvr, request='stable',
-                         csrf_token=self.app.get('/csrf').json_body['csrf_token'])
-        with mock.patch.dict(config, {'test_gating.required': True}):
-            res = self.app.post_json(f'/updates/{build.update.alias}/request',
-                                     post_data, status=400)
-
-        # Ensure we can't push it until it passed test gating
-        assert res.json_body['status'] == 'error'
-        assert res.json_body['errors'][0]['description'] == (
-            'Requirement not met Required tests did not pass on this update.'
-        )
-
-    def test_provenpackager_request_update_running_in_test_gating(self, *args):
-        """Ensure provenpackagers cannot request changes for any update which
-        test gating status is `running`"""
-        nvr = 'bodhi-2.1-1.fc17'
-        user = User(name='bob')
-        self.db.add(user)
-        group = self.db.query(Group).filter_by(name='provenpackager').one()
-        user.groups.append(group)
-        self.db.commit()
-
-        up_data = self.get_update(nvr)
-        up_data['csrf_token'] = self.app.get('/csrf').json_body['csrf_token']
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            res = self.app.post_json('/updates/', up_data)
-
-        build = self.db.query(RpmBuild).filter_by(nvr=nvr).one()
-        build.update.test_gating_status = TestGatingStatus.running
-        assert build.update.request == UpdateRequest.testing
-
-        # Try and submit the update to stable as a provenpackager
-        post_data = dict(update=nvr, request='stable',
-                         csrf_token=self.app.get('/csrf').json_body['csrf_token'])
-        with mock.patch.dict(config, {'test_gating.required': True}):
-            res = self.app.post_json(f'/updates/{build.update.alias}/request',
-                                     post_data, status=400)
-
-        # Ensure we can't push it until it passed test gating
-        assert res.json_body['status'] == 'error'
-        assert res.json_body['errors'][0]['description'] == (
-            'Requirement not met Required tests did not pass on this update.'
-        )
-
-    def test_provenpackager_request_update_failed_test_gating(self, *args):
-        """Ensure provenpackagers cannot request changes for any update which
-        test gating status is `failed`"""
-        nvr = 'bodhi-2.1-1.fc17'
-        user = User(name='bob')
-        self.db.add(user)
-        group = self.db.query(Group).filter_by(name='provenpackager').one()
-        user.groups.append(group)
-        self.db.commit()
-
-        up_data = self.get_update(nvr)
-        up_data['csrf_token'] = self.app.get('/csrf').json_body['csrf_token']
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            res = self.app.post_json('/updates/', up_data)
-
-        build = self.db.query(RpmBuild).filter_by(nvr=nvr).one()
-        build.update.test_gating_status = TestGatingStatus.failed
-        assert build.update.request == UpdateRequest.testing
-
-        # Try and submit the update to stable as a provenpackager
-        post_data = dict(update=nvr, request='stable',
-                         csrf_token=self.app.get('/csrf').json_body['csrf_token'])
-        with mock.patch.dict(config, {'test_gating.required': True}):
-            res = self.app.post_json(f'/updates/{build.update.alias}/request',
-                                     post_data, status=400)
-
-        # Ensure we can't push it until it passed test gating
-        assert res.json_body['status'] == 'error'
-        assert res.json_body['errors'][0]['description'] == (
-            'Requirement not met Required tests did not pass on this update.'
-        )
-
-    def test_provenpackager_request_update_ignored_by_test_gating(self, *args):
-        """Ensure provenpackagers can request changes for any update which
-        test gating status is `ignored`"""
-        nvr = 'bodhi-2.1-1.fc17'
-        user = User(name='bob')
-        self.db.add(user)
-        group = self.db.query(Group).filter_by(name='provenpackager').one()
-        user.groups.append(group)
-        self.db.commit()
-
-        up_data = self.get_update(nvr)
-        up_data['csrf_token'] = self.app.get('/csrf').json_body['csrf_token']
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            res = self.app.post_json('/updates/', up_data)
-
-        assert 'does not have commit access to bodhi' not in res
-        build = self.db.query(RpmBuild).filter_by(nvr=nvr).one()
-        build.update.test_gating_status = TestGatingStatus.ignored
-        assert build.update.request == UpdateRequest.testing
-
-        # Try and submit the update to stable as a provenpackager
-        post_data = dict(update=nvr, request='stable',
-                         csrf_token=self.app.get('/csrf').json_body['csrf_token'])
-        with mock.patch.dict(config, {'test_gating.required': True}):
-            res = self.app.post_json(f'/updates/{build.update.alias}/request',
-                                     post_data, status=400)
-
-        # Ensure the reason we cannot push isn't test gating this time
-        assert res.json_body['status'] == 'error'
-        assert res.json_body['errors'][0]['description'] == config.get('not_yet_tested_msg')
-
-    def test_provenpackager_request_update_waiting_on_test_gating(self, *args):
-        """Ensure provenpackagers cannot request changes for any update which
-        test gating status is `waiting`"""
-        nvr = 'bodhi-2.1-1.fc17'
-        user = User(name='bob')
-        self.db.add(user)
-        group = self.db.query(Group).filter_by(name='provenpackager').one()
-        user.groups.append(group)
-        self.db.commit()
-
-        up_data = self.get_update(nvr)
-        up_data['csrf_token'] = self.app.get('/csrf').json_body['csrf_token']
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            res = self.app.post_json('/updates/', up_data)
-
-        build = self.db.query(RpmBuild).filter_by(nvr=nvr).one()
-        build.update.test_gating_status = TestGatingStatus.waiting
-        assert build.update.request == UpdateRequest.testing
-
-        # Try and submit the update to stable as a provenpackager
-        post_data = dict(update=nvr, request='stable',
-                         csrf_token=self.app.get('/csrf').json_body['csrf_token'])
-        with mock.patch.dict(config, {'test_gating.required': True}):
-            res = self.app.post_json(f'/updates/{build.update.alias}/request',
-                                     post_data, status=400)
-
-        # Ensure we can't push it until it passed test gating
-        assert res.json_body['status'] == 'error'
-        assert res.json_body['errors'][0]['description'] == (
-            'Requirement not met Required tests for this update are still running.'
+    @pytest.mark.parametrize(
+        "status,canchange",
+        (
+            (TestGatingStatus.queued, False),
+            (TestGatingStatus.running, False),
+            (TestGatingStatus.failed, False),
+            (TestGatingStatus.failed, False),
+            (TestGatingStatus.waiting, False),
+            (None, False),
+            (TestGatingStatus.passed, True),
         )
-
-    def test_provenpackager_request_update_with_none_test_gating(self, *args):
-        """Ensure provenpackagers cannot request changes for any update which
-        test gating status is `None`"""
+    )
+    def test_provenpackager_request_update_test_gating_status(self, status, canchange):
+        """Ensure provenpackagers can request changes when test gating
+        is passed or ignored, but otherwise cannot."""
         nvr = 'bodhi-2.1-1.fc17'
         user = User(name='bob')
         self.db.add(user)
@@ -1868,20 +1728,33 @@ def test_provenpackager_request_update_with_none_test_gating(self, *args):
                                     update_schemas.UpdateRequestTestingV1):
             res = self.app.post_json('/updates/', up_data)
 
-        build = self.db.query(RpmBuild).filter_by(nvr=nvr).one()
-        build.update.test_gating_status = None
-        assert build.update.request == UpdateRequest.testing
+        update = Build.query.filter_by(nvr=nvr).one().update
+        update.test_gating_status = status
+        # have the update meet karma requirements
+        update.comment(self.db, 'LGTM', author='ralph', karma=1)
+        update.comment(self.db, 'LGTM', author='test', karma=1)
+        # Clear pending messages
+        self.db.info['messages'] = []
+        assert update.request == UpdateRequest.testing
 
         # Try and submit the update to stable as a provenpackager
         post_data = dict(update=nvr, request='stable',
                          csrf_token=self.app.get('/csrf').json_body['csrf_token'])
         with mock.patch.dict(config, {'test_gating.required': True}):
-            res = self.app.post_json(f'/updates/{build.update.alias}/request',
-                                     post_data, status=400)
-
-        # Ensure the reason we can't push is not test gating
-        assert res.json_body['status'] == 'error'
-        assert res.json_body['errors'][0]['description'] == config.get('not_yet_tested_msg')
+            if canchange is False:
+                res = self.app.post_json(f'/updates/{update.alias}/request', post_data, status=400)
+            else:
+                with fml_testing.mock_sends(update_schemas.UpdateRequestStableV1):
+                    res = self.app.post_json(f'/updates/{update.alias}/request', post_data)
+
+        if canchange is False:
+            # Ensure we can't push it until it passed test gating
+            assert res.json_body['status'] == 'error'
+            assert res.json_body['errors'][0]['description'] in (
+                f"{config.get('not_yet_tested_msg')}: Required tests did not pass on this update.",
+                f"{config.get('not_yet_tested_msg')}: "
+                "Required tests for this update are queued or running.",
+            )
 
     def test_404(self):
         self.app.get('/a', status=404)
@@ -3693,6 +3566,64 @@ def test_edit_locked_update(self, *args):
         build = self.db.query(RpmBuild).filter_by(nvr=nvr).one()
         assert up.builds == [build]
 
+    def test_edit_update_too_low_stable_days(self, *args):
+        """Check stable days below the minimum is increased on edit"""
+        nvr = 'bodhi-2.0.0-2.fc17'
+        args = self.get_update(nvr)
+        args['stable_days'] = 50
+        args['stable_karma'] = 50
+
+        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
+                                    update_schemas.UpdateRequestTestingV1):
+            self.app.post_json('/updates/', args, status=200)
+
+        up = self.db.query(Build).filter_by(nvr=nvr).one().update
+        up.locked = True
+        up.status = UpdateStatus.testing
+        up.request = None
+        # Clear pending messages
+        self.db.info['messages'] = []
+
+        args['edited'] = up.alias
+        args['stable_days'] = 1
+
+        with fml_testing.mock_sends(update_schemas.UpdateEditV2):
+            up = self.app.post_json('/updates/', args, status=200).json_body
+
+        assert up['stable_days'] == 7
+        assert up['caveats'][0]['description'] == (
+            'The number of stable days required was raised to the mandatory release value of 7 days'
+        )
+
+    def test_edit_update_too_low_stable_karma(self, *args):
+        """Check stable karma below the minimum is increased on edit"""
+        nvr = 'bodhi-2.0.0-2.fc17'
+        args = self.get_update(nvr)
+        args['stable_days'] = 50
+        args['stable_karma'] = 50
+
+        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
+                                    update_schemas.UpdateRequestTestingV1):
+            self.app.post_json('/updates/', args, status=200)
+
+        up = self.db.query(Build).filter_by(nvr=nvr).one().update
+        up.locked = True
+        up.status = UpdateStatus.testing
+        up.request = None
+        # Clear pending messages
+        self.db.info['messages'] = []
+
+        args['edited'] = up.alias
+        args['stable_karma'] = 1
+
+        with fml_testing.mock_sends(update_schemas.UpdateEditV2):
+            up = self.app.post_json('/updates/', args, status=200).json_body
+
+        assert up['stable_karma'] == 2
+        assert up['caveats'][0]['description'] == (
+            'The stable karma required was raised to the mandatory release value of 2'
+        )
+
     def test_obsoletion_locked_with_open_request(self, *args):
         nvr = 'bodhi-2.0.0-2.fc17'
         args = self.get_update(nvr)
@@ -3980,12 +3911,16 @@ def test_invalid_stable_request(self, *args):
             {'request': 'stable', 'csrf_token': self.get_csrf_token()},
             status=400)
         assert resp.json['status'] == 'error'
-        assert resp.json['errors'][0]['description'] == config.get('not_yet_tested_msg')
+        exp = (
+            f"{config.get('not_yet_tested_msg')}: Test gating is disabled, but update has "
+            "less than 2 karma and has been in testing less than 7 days."
+        )
+        assert resp.json['errors'][0]['description'] == exp
 
-    def test_request_to_stable_based_on_stable_karma(self, *args):
+    def test_request_to_stable_based_on_minimum_karma(self, *args):
         """
-        Test request to stable before an update reaches stable karma
-        and after it reaches stable karma when autokarma is disabled
+        Test request to stable before and after an update reaches minimum
+        karma threshold when autokarma is disabled
         """
         user = User(name='bob')
         self.db.add(user)
@@ -3994,7 +3929,6 @@ def test_request_to_stable_based_on_stable_karma(self, *args):
         nvr = 'bodhi-2.0.0-2.fc17'
         args = self.get_update(nvr)
         args['autokarma'] = False
-        args['stable_karma'] = 1
         with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
                                     update_schemas.UpdateRequestTestingV1):
             response = self.app.post_json('/updates/', args)
@@ -4003,13 +3937,15 @@ def test_request_to_stable_based_on_stable_karma(self, *args):
         up.status = UpdateStatus.testing
         up.request = None
         assert len(up.builds) == 1
+        # just to set our base expectation
+        assert up.release.min_karma == 2
         up.test_gating_status = TestGatingStatus.passed
         # Clear pending messages
         self.db.info['messages'] = []
         self.db.commit()
 
-        # Checks failure for requesting to stable push before the update reaches stable karma
-        up.comment(self.db, 'Not working', author='ralph', karma=0)
+        # Checks failure for requesting to stable push before the update reaches min karma
+        up.comment(self.db, 'LGTM', author='ralph', karma=1)
 
         with fml_testing.mock_sends(api.Message):
             self.app.post_json(
@@ -4021,8 +3957,8 @@ def test_request_to_stable_based_on_stable_karma(self, *args):
         assert up.request is None
         assert up.status == UpdateStatus.testing
 
-        # Checks Success for requesting to stable push after the update reaches stable karma
-        up.comment(self.db, 'LGTM', author='ralph', karma=1)
+        # Checks Success for requesting to stable push after the update reaches min karma
+        up.comment(self.db, 'LGTM', author='foo', karma=2)
 
         with fml_testing.mock_sends(api.Message, api.Message):
             self.app.post_json(
@@ -4837,7 +4773,7 @@ def _reach_autopush_threshold(self, up, publish):
         status = up.status
         request = up.request
         # just for clarity that stable_karma is not lower and both are reached
-        assert up.release.critpath_min_karma == 2
+        assert up.release.min_karma == 2
         up.comment(self.db, "foo", 1, 'biz')
         # nothing should have changed yet, on any path. we don't need to test the
         # messages published so far here, either
@@ -4861,7 +4797,7 @@ def _reach_autopush_threshold(self, up, publish):
     def test_autopush_reached_main(self, autokarma, cbb, critpath, status):
         """The update should be autopushed (request set to stable) if update reaches
         stable_karma, autokarma is on and release is composed by Bodhi, whether or not the update
-        is critical path (so long as stable_karma is not lower than critpath_min_karma), and
+        is critical path (so long as stable_karma is not lower than min_karma), and
         whether its status is testing or pending. This test covers the main cases for autopush.
         Subsequent tests cover some other cases where updates that otherwise would be autopushed
         are not; these aren't covered in this test for reasons explained in the docstring of
@@ -4942,40 +4878,41 @@ def test_autopush_reached_gating_failed(self):
     @pytest.mark.parametrize('critpath', (True, False))
     def test_autopush_reached_critpath_not(self, critpath, status):
         """If the stable_karma threshold is reached but it is lower than the release's
-        critpath_min_karma threshold and that is not reached, autopush should happen for
-        a non-critpath update but not for a critpath update. For a critpath update, if
-        status is testing, the request should not be changed; if it's pending, the
-        request should be changed to testing.
+        min_karma threshold and that is not reached, autopush should not happen
+        (whether or not the update is critical path). It should be very uncommon to
+        encounter this situation these days because we disallow setting the stable_karma
+        threshold lower than min_karma when creating or editing an update, but it
+        could theoretically happen if min_karma for a release was increased while an
+        update which had stable_karma set to the previous minimum value was in-flight.
         """
         up = self._prepare_autopush_update()
         up.critpath = critpath
         up.status = status
         up.stable_karma = 1
-        assert up.release.critpath_min_karma == 2
+        assert up.release.min_karma == 2
         # we don't use _reach_autopush_threshold here because this is a bit different
         with mock.patch('bodhi.server.models.notifications.publish', autospec=True) as publish:
             _, caveats = up.comment(self.db, "foo", 1, 'biz')
             msgs = [type(call[0][0]) for call in publish.call_args_list]
         assert up.karma == 1
-        if critpath and status is UpdateStatus.testing:
+        if status is UpdateStatus.testing:
             assert up.request is None
             # we should not set this, really, but currently we do
             assert up.date_approved is not None
             assert msgs == [update_schemas.UpdateCommentV1]
+            days = 7
+            if critpath:
+                days = 14
             assert caveats == (
                 [{'name': 'karma',
-                  'description': ('This critical path update has not yet been approved for pushing '
-                                  'to the stable repository.  It must first reach a karma of 2, '
-                                  'consisting of 0 positive karma from proventesters, along with 2 '
-                                  'additional karma from the community. Or, it must spend 14 days '
-                                  'in testing without any negative feedback')}])
+                  'description': ('This update has not yet met the minimum testing requirements '
+                                  'defined in the <a href="https://fedoraproject.org/wiki/'
+                                  'Package_update_acceptance_criteria">Package Update Acceptance '
+                                  'Criteria</a>: Test gating is disabled, but update has less than '
+                                  f'2 karma and has been in testing less than {days} days.')}])
         else:
-            if critpath:
-                assert up.request is UpdateRequest.testing
-                reqmsg = update_schemas.UpdateRequestTestingV1
-            else:
-                assert up.request is UpdateRequest.stable
-                reqmsg = update_schemas.UpdateRequestStableV1
+            assert up.request is UpdateRequest.testing
+            reqmsg = update_schemas.UpdateRequestTestingV1
             assert up.date_approved is not None
             assert msgs == [
                 reqmsg,
@@ -5059,43 +4996,29 @@ def test_edit_button_not_present_when_stable(self, *args):
         assert 'Push to Stable' not in resp
         assert '<span class="fa fa-fw fa-pencil-square-o"></span> Edit' not in resp
 
-    @mock.patch.dict('bodhi.server.models.config', {'test_gating.required': True})
-    @mock.patch('bodhi.server.models.Update.update_test_gating_status')
-    def test_push_to_stable_button_not_present_when_test_gating_status_failed(self, update_tg):
-        """The push to stable button should not appear if the test_gating_status is failed."""
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        update_tg.return_value = TestGatingStatus.failed
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args, headers={'Accept': 'application/json'})
-
-        update = Update.get(resp.json['alias'])
-        update.status = UpdateStatus.testing
-        update.request = None
-        update.pushed = True
-        update.autokarma = False
-        update.stable_karma = 1
-        update.test_gating_status = TestGatingStatus.failed
-        update.comment(self.db, 'works', 1, 'bowlofeggs')
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-        self.registry.settings['test_gating.required'] = True
-
-        resp = self.app.get(f'/updates/{update.alias}', headers={'Accept': 'text/html'})
-
-        assert 'Push to Stable' not in resp
-        assert 'Edit' in resp
-
-    @mock.patch.dict('bodhi.server.models.config', {'test_gating.required': True})
-    @mock.patch('bodhi.server.models.Update.update_test_gating_status')
-    def test_push_to_stable_button_present_when_test_gating_status_passed(self, update_tg):
-        """The push to stable button should appear if the test_gating_status is passed."""
+    @pytest.mark.parametrize(
+        'tgon,tgmet,karmamet,timemet,urgent,critpath,frozen,present',
+        (
+            (True, True, True, False, False, False, False, True),
+            (True, True, True, False, True, False, False, True),
+            (True, True, True, False, False, True, False, True),
+            (True, True, True, False, True, False, True, False),
+            (True, False, True, True, False, False, False, False),
+            (False, False, True, False, False, False, False, True),
+            (False, False, False, True, False, False, False, True),
+            (False, False, False, False, False, False, False, False),
+            (False, False, False, False, True, False, False, False),
+            (False, False, False, False, False, True, False, False),
+        )
+    )
+    def test_push_to_stable_button_presence(
+        self, tgon, tgmet, karmamet, timemet, urgent, critpath, frozen, present
+    ):
+        """The push to stable button should be present when the update meets
+        requirements, but not otherwise. Whether the update is urgent or
+        critical path should not matter."""
         nvr = 'bodhi-2.0.0-2.fc17'
         args = self.get_update(nvr)
-        update_tg.return_value = TestGatingStatus.passed
-
         with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
                                     update_schemas.UpdateRequestTestingV1):
             resp = self.app.post_json('/updates/', args, headers={'Accept': 'application/json'})
@@ -5105,197 +5028,38 @@ def test_push_to_stable_button_present_when_test_gating_status_passed(self, upda
         update.request = None
         update.pushed = True
         update.autokarma = False
-        update.stable_karma = 1
-        update.test_gating_status = TestGatingStatus.passed
-        update.comment(self.db, 'works', 1, 'bowlofeggs')
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-        self.registry.settings['test_gating.required'] = True
-
-        resp = self.app.get(f'/updates/{update.alias}', headers={'Accept': 'text/html'})
-
-        assert 'Push to Stable' in resp
-        assert 'Edit' in resp
-
-    def test_push_to_stable_button_present_when_karma_reached(self, *args):
-        """
-        Assert that the "Push to Stable" button appears when the required karma is
-        reached.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        update = Update.get(resp.json['alias'])
-        update.status = UpdateStatus.testing
-        update.request = None
-        update.pushed = True
-        update.autokarma = False
-        update.stable_karma = 1
-        update.comment(self.db, 'works', 1, 'bowlofeggs')
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        resp = self.app.get(f'/updates/{update.alias}', headers={'Accept': 'text/html'})
-
-        # Checks Push to Stable text in the html page for this update
-        assert 'text/html' in resp.headers['Content-Type']
-        assert nvr in resp
-        assert 'Push to Stable' in resp
-        assert 'Edit' in resp
-
-    def test_push_to_stable_button_present_when_karma_reached_urgent(self, *args):
-        """
-        Assert that the "Push to Stable" button appears when the required karma is
-        reached for an urgent update.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        update = Update.get(resp.json['alias'])
-        update.severity = UpdateSeverity.urgent
-        update.status = UpdateStatus.testing
-        update.request = None
-        update.pushed = True
-        update.autokarma = False
-        update.stable_karma = 1
-        update.comment(self.db, 'works', 1, 'bowlofeggs')
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        resp = self.app.get(f'/updates/{update.alias}', headers={'Accept': 'text/html'})
-
-        # Checks Push to Stable text in the html page for this update
-        assert 'text/html' in resp.headers['Content-Type']
-        assert nvr in resp
-        assert 'Push to Stable' in resp
-        assert 'Edit' in resp
-
-    def test_push_to_stable_button_present_when_time_reached(self, *args):
-        """
-        Assert that the "Push to Stable" button appears when the required time in testing is
-        reached.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        update = Update.get(resp.json['alias'])
-        update.status = UpdateStatus.testing
-        update.request = None
-        update.pushed = True
-        # This update has been in testing a while, so a "Push to Stable" button should appear.
-        update.date_testing = datetime.now() - timedelta(days=30)
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        resp = self.app.get(f'/updates/{update.alias}', headers={'Accept': 'text/html'})
-
-        # Checks Push to Stable text in the html page for this update
-        assert 'text/html' in resp.headers['Content-Type']
-        assert nvr in resp
-        assert 'Push to Stable' in resp
-        assert 'Edit' in resp
-
-    def test_push_to_stable_button_present_when_time_reached_and_urgent(self, *args):
-        """
-        Assert that the "Push to Stable" button appears when the required time in testing is
-        reached.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        update = Update.get(resp.json['alias'])
-        update.severity = UpdateSeverity.urgent
-        update.status = UpdateStatus.testing
-        update.request = None
-        update.pushed = True
-        # This urgent update has been in testing a while, so a "Push to Stable" button should
-        # appear.
-        update.date_testing = datetime.now() - timedelta(days=30)
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        resp = self.app.get(f'/updates/{update.alias}', headers={'Accept': 'text/html'})
-
-        # Checks Push to Stable text in the html page for this update
-        assert 'text/html' in resp.headers['Content-Type']
-        assert nvr in resp
-        assert 'Push to Stable' in resp
-        assert 'Edit' in resp
-
-    def test_push_to_stable_button_present_when_time_reached_critpath(self, *args):
-        """
-        Assert that the "Push to Stable" button appears when it should for a critpath update.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        update = Update.get(resp.json['alias'])
-        update.status = UpdateStatus.testing
-        update.request = None
-        update.pushed = True
-        update.critpath = True
-        # This update has been in testing a while, so a "Push to Stable" button should appear.
-        update.date_testing = datetime.now() - timedelta(days=30)
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        resp = self.app.get(f'/updates/{update.alias}', headers={'Accept': 'text/html'})
-
-        # Checks Push to Stable text in the html page for this update
-        assert 'text/html' in resp.headers['Content-Type']
-        assert nvr in resp
-        assert 'Push to Stable' in resp
-        assert 'Edit' in resp
-
-    def test_push_to_stable_button_not_present_when_karma_reached_and_frozen_release(self, *args):
-        """
-        Assert that the "Push to Stable" button is not displayed when the required karma is
-        reached, but the release is frozen and the update is still pending.
-        """
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        update = Update.get(resp.json['alias'])
-        update.status = UpdateStatus.pending
-        update.request = UpdateRequest.testing
-        update.pushed = False
-        update.autokarma = False
-        update.stable_karma = 1
-        update.release.state = ReleaseState.frozen
-        update.comment(self.db, 'works', 1, 'bowlofeggs')
+        if urgent:
+            update.severity = UpdateSeverity.urgent
+        if critpath:
+            update.critpath = True
+        if frozen:
+            # specific set of conditions we want to make sure the
+            # button does not appear in
+            update.release.state = ReleaseState.frozen
+            update.status = UpdateStatus.pending
+            update.pushed = False
+        if tgmet:
+            update.test_gating_status = TestGatingStatus.passed
+        else:
+            update.test_gating_status = TestGatingStatus.failed
+        if karmamet:
+            update.comment(self.db, 'works', 1, 'bowlofeggs')
+            update.comment(self.db, 'works', 1, 'ralph')
+        if timemet:
+            # This update has been in testing a while, so a "Push to Stable" button should appear.
+            update.date_testing = datetime.now() - timedelta(days=30)
         # Let's clear any messages that might get sent
         self.db.info['messages'] = []
 
-        resp = self.app.get(f'/updates/{update.alias}', headers={'Accept': 'text/html'})
+        with mock.patch.dict(config, [('test_gating.required', tgon)]):
+            resp = self.app.get(f'/updates/{update.alias}', headers={'Accept': 'text/html'})
 
-        # Checks Push to Stable text in the html page for this update
         assert 'text/html' in resp.headers['Content-Type']
         assert nvr in resp
-        assert 'Push to Stable' not in resp
+        if present:
+            assert 'Push to Stable' in resp
+        else:
+            assert 'Push to Stable' not in resp
         assert 'Edit' in resp
 
     def assert_severity_html(self, severity, text=()):
@@ -5389,115 +5153,6 @@ def test_update_severity_label_absent_when_severity_is_None(self, *args):
         assert nvr in resp
         assert '<strong>Update Severity</strong>' not in resp
 
-    def test_manually_push_to_stable_based_on_karma_request_none(self, *args):
-        """
-        Test manually push to stable when autokarma is disabled
-        and karma threshold is reached
-
-        This test starts the update with request None.
-        """
-        user = User(name='bob')
-        self.db.add(user)
-        self.db.commit()
-
-        # Makes autokarma disabled
-        # Sets stable karma to 1
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = False
-        args['stable_karma'] = 1
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        # Marks it as no request
-        upd = Update.get(resp.json['alias'])
-        upd.status = UpdateStatus.testing
-        upd.request = None
-        upd.pushed = True
-        upd.date_testing = datetime.now() - timedelta(days=1)
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        # Checks karma threshold is reached
-        # Makes sure stable karma is not None
-        # Ensures Request doesn't get set to stable automatically since autokarma is disabled
-        upd.comment(self.db, 'LGTM', author='ralph', karma=1)
-        upd = Update.get(upd.alias)
-        assert upd.karma == 1
-        assert upd.stable_karma == 1
-        assert upd.status == UpdateStatus.testing
-        assert upd.request is None
-        assert upd.autokarma is False
-        assert upd.pushed is True
-
-        text = str(config.get('testing_approval_msg'))
-        upd.comment(self.db, text, author='bodhi')
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        # Checks Push to Stable text in the html page for this update
-        resp = self.app.get(f'/updates/{upd.alias}',
-                            headers={'Accept': 'text/html'})
-        assert 'text/html' in resp.headers['Content-Type']
-        assert upd.builds[0].nvr in resp
-        assert 'Push to Stable' in resp
-
-    def test_manually_push_to_stable_based_on_karma_request_testing(self, *args):
-        """
-        Test manually push to stable when autokarma is disabled
-        and karma threshold is reached. Ensure that the option/button to push to
-        stable is not present prior to entering the stable request state.
-
-        This test starts the update with request testing.
-        """
-
-        # Disabled
-        # Sets stable karma to 1
-        nvr = 'bodhi-2.0.0-2.fc17'
-        args = self.get_update(nvr)
-        args['autokarma'] = False
-        args['stable_karma'] = 1
-
-        with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
-                                    update_schemas.UpdateRequestTestingV1):
-            resp = self.app.post_json('/updates/', args)
-
-        # Marks it as testing
-        upd = Update.get(resp.json['alias'])
-        upd.status = UpdateStatus.testing
-        upd.pushed = True
-        upd.request = None
-        upd.date_testing = datetime.now() - timedelta(days=1)
-        # Clear pending messages
-        self.db.info['messages'] = []
-        self.db.commit()
-
-        # Checks karma threshold is reached
-        # Makes sure stable karma is not None
-        # Ensures Request doesn't get set to stable automatically since autokarma is disabled
-        upd.comment(self.db, 'LGTM', author='ralph', karma=1)
-        upd = Update.get(upd.alias)
-        assert upd.karma == 1
-        assert upd.stable_karma == 1
-        assert upd.status == UpdateStatus.testing
-        assert upd.request is None
-        assert upd.autokarma is False
-
-        text = str(config.get('testing_approval_msg'))
-        upd.comment(self.db, text, author='bodhi')
-        # Let's clear any messages that might get sent
-        self.db.info['messages'] = []
-
-        # Checks Push to Stable text in the html page for this update
-        resp = self.app.get(f'/updates/{upd.alias}',
-                            headers={'Accept': 'text/html'})
-        assert 'text/html' in resp.headers['Content-Type']
-        assert upd.builds[0].nvr in resp
-        assert 'Push to Stable' in resp
-
     @mock.patch('bodhi.server.services.updates.handle_side_and_related_tags_task', mock.Mock())
     @mock.patch('bodhi.server.models.tag_update_builds_task', mock.Mock())
     def test_edit_update_with_expired_override(self, *args):
@@ -5741,7 +5396,6 @@ def test_meets_testing_requirements_since_karma_reset_critpath(self, *args):
         update.request = None
         update.critpath = True
         update.autokarma = True
-        update.date_testing = datetime.utcnow() + timedelta(days=-20)
         update.comment(self.db, 'lgtm', author='friend', karma=1)
         update.comment(self.db, 'lgtm', author='friend2', karma=1)
         update.comment(self.db, 'bad', author='enemy', karma=-1)
diff --git a/bodhi-server/tests/tasks/test_approve_testing.py b/bodhi-server/tests/tasks/test_approve_testing.py
index dd4fb2be6a..494cad2706 100644
--- a/bodhi-server/tests/tasks/test_approve_testing.py
+++ b/bodhi-server/tests/tasks/test_approve_testing.py
@@ -175,6 +175,9 @@ def test_update_approved_and_autotime(self, delete_tag, remove_tag, add_tag,
         self.update.autotime = True
         # a sprinkling of possible cases just to make sure our logic isn't broken
         # and we're not somehow relying on defaults
+        if stable_days < 7:
+            # our test config sets it to 7, so need to override it
+            config["fedora.mandatory_days_in_testing"] = stable_days
         self.update.stable_days = stable_days
         if stable_days > 0:
             # in this case we need to make sure we look like we've been in testing
diff --git a/bodhi-server/tests/tasks/test_composer.py b/bodhi-server/tests/tasks/test_composer.py
index 2b470cf313..96b4dec889 100644
--- a/bodhi-server/tests/tasks/test_composer.py
+++ b/bodhi-server/tests/tasks/test_composer.py
@@ -483,8 +483,14 @@ def test_tags(self, *args):
             comp = session.query(Compose).first()
             if comp is not None:
                 session.delete(comp)
-            # Set the update request to stable and the release to pending
             up = session.query(Update).one()
+            # Have the update reach the stable karma threshold
+            assert up.karma == 1
+            up.comment(session, "foo", 1, 'foo')
+            # Clear pending messages
+            self.db.info['messages'] = []
+            assert up.karma == 2
+            # Set the update request to stable and the release to pending
             up.release.state = ReleaseState.pending
             up.request = UpdateRequest.stable
 
@@ -948,6 +954,10 @@ def test_security_update_priority(self, *args):
 
         with self.db_factory() as db:
             up = db.query(Update).one()
+            # Have the update reach the stable karma threshold
+            assert up.karma == 1
+            up.comment(db, "foo", 1, 'foo')
+            assert up.karma == 2
             user = db.query(User).first()
 
             # Create a security update for a different release
@@ -980,6 +990,10 @@ def test_security_update_priority(self, *args):
                 type=UpdateType.security)
 
             db.add(update)
+            # Have the update reach the stable karma threshold
+            update.comment(db, "foo", 1, 'foo')
+            update.comment(db, "test", 1, 'test')
+            assert update.karma == 2
 
             update.test_gating_status = TestGatingStatus.passed
 
@@ -1030,6 +1044,10 @@ def test_security_update_priority_testing(self, *args):
             up = db.query(Update).one()
             up.type = UpdateType.security
             up.request = UpdateRequest.testing
+            # Have the update reach the stable karma threshold
+            assert up.karma == 1
+            up.comment(db, "foo", 1, 'foo')
+            assert up.karma == 2
             user = db.query(User).first()
 
             # Create a security update for a different release
@@ -1062,6 +1080,10 @@ def test_security_update_priority_testing(self, *args):
                 type=UpdateType.enhancement)
 
             db.add(update)
+            # Have the update reach the stable karma threshold
+            update.comment(db, "foo", 1, 'foo')
+            update.comment(db, "test", 1, 'test')
+            assert update.karma == 2
 
             update.test_gating_status = TestGatingStatus.passed
 
@@ -1100,6 +1122,10 @@ def test_security_updates_parallel(self, *args):
 
         with self.db_factory() as db:
             up = db.query(Update).one()
+            # Have the update reach the stable karma threshold
+            assert up.karma == 1
+            up.comment(db, "foo", 1, 'foo')
+            assert up.karma == 2
             up.type = UpdateType.security
             up.status = UpdateStatus.testing
             up.request = UpdateRequest.stable
@@ -1135,6 +1161,10 @@ def test_security_updates_parallel(self, *args):
                 type=UpdateType.security)
 
             db.add(update)
+            # Have the update reach the stable karma threshold
+            update.comment(db, "foo", 1, 'foo')
+            update.comment(db, "test", 1, 'test')
+            assert update.karma == 2
 
             update.test_gating_status = TestGatingStatus.passed
 
@@ -1237,10 +1267,11 @@ def test_compose_late_exit(self, *args):
     @mock.patch('bodhi.server.tasks.composer.PungiComposerThread._wait_for_repo_signature')
     @mock.patch('bodhi.server.tasks.composer.PungiComposerThread._wait_for_sync')
     @mock.patch('bodhi.server.tasks.composer.time.sleep')
-    def test_clean_old_composes_false(self, *args):
-        """Test work() with clean_old_composes set to False."""
+    @pytest.mark.parametrize('clean_old', (False, True))
+    def test_clean_old_composes_param(self, sleep, wfs, wfrs, stage, scr, clean_old):
+        """Test work() with clean_old_composes set to False or True."""
         self.expected_sems = 1
-        config["clean_old_composes"] = False
+        config["clean_old_composes"] = clean_old
 
         # Set the request to stable right out the gate so we can test gating
         self.set_stable_request('bodhi-2.0-1.fc17')
@@ -1275,6 +1306,7 @@ def test_clean_old_composes_false(self, *args):
                               'ralph', self.db_factory, compose_dir)
         t.keep_old_composes = 2
         expected_messages = (
+            update_schemas.UpdateCommentV1,
             compose_schemas.ComposeComposingV1,
             override_schemas.BuildrootOverrideUntagV1,
             update_schemas.UpdateCompleteStableV1,
@@ -1283,119 +1315,39 @@ def test_clean_old_composes_false(self, *args):
                 success=True, repo='f17-updates', ctype='rpm', agent='ralph')))
 
         with self.db_factory() as session:
-            with mock.patch('bodhi.server.tasks.composer.subprocess.Popen') as Popen:
-                release = session.query(Release).filter_by(name='F17').one()
-                Popen.side_effect = self._generate_fake_pungi(t, 'stable_tag', release)
-                with mock_sends(*expected_messages):
-                    t.run()
-
-        actual_dirs = set([
-            d for d in os.listdir(compose_dir)
-            if os.path.isdir(os.path.join(compose_dir, d))
-            and not d.startswith("Fedora-17-updates")])
-
-        # No dirs should have been removed since we had clean_old_composes set False.
-        assert actual_dirs == dirs
-        # The cool file should still be here
-        actual_files = [f for f in os.listdir(compose_dir)
-                        if os.path.isfile(os.path.join(compose_dir, f))]
-        assert actual_files == ['COOL_FILE.txt']
-
-        assert Popen.mock_calls == \
-            [mock.call(
-                [config['pungi.cmd'], '--config', '{}/pungi.conf'.format(t._pungi_conf_dir),
-                 '--quiet', '--print-output-dir', '--target-dir', t.compose_dir, '--old-composes',
-                 t.compose_dir, '--no-latest-link', '--label', t._label],
-                cwd=t.compose_dir, shell=False, stderr=-1,
-                stdin=mock.ANY,
-                stdout=mock.ANY)]
-        d = datetime.datetime.utcnow()
-        assert t._checkpoints == \
-            {'completed_repo': os.path.join(
-                compose_dir, 'Fedora-17-updates-{}{:02}{:02}.0'.format(d.year, d.month, d.day)),
-             'compose_done': True,
-             'determine_and_perform_tag_actions': True,
-             'modify_bugs': True,
-             'send_stable_announcements': True,
-             'send_testing_digest': True,
-             'status_comments': True}
-        assert os.path.exists(compose_dir)
-
-    @mock.patch('bodhi.server.tasks.composer.PungiComposerThread._sanity_check_repo')
-    @mock.patch('bodhi.server.tasks.composer.PungiComposerThread._stage_repo')
-    @mock.patch('bodhi.server.tasks.composer.PungiComposerThread._wait_for_repo_signature')
-    @mock.patch('bodhi.server.tasks.composer.PungiComposerThread._wait_for_sync')
-    @mock.patch('bodhi.server.tasks.composer.time.sleep')
-    def test_clean_old_composes_true(self, *args):
-        """Test work() with clean_old_composes set to True."""
-        config["clean_old_composes"] = True
-        self.expected_sems = 1
-
-        # Set the request to stable right out the gate so we can test gating
-        self.set_stable_request('bodhi-2.0-1.fc17')
-        task = self._make_task()
-        compose_dir = os.path.join(self.tempdir, 'cool_dir')
-        config["compose_dir"] = compose_dir
-
-        # Set up some directories that look similar to what might be found in production, with
-        # some directories that don't match the pattern of ending in -<timestamp>.
-        dirs = [
-            'dist-5E-epel-161003.0724', 'dist-5E-epel-161011.0458', 'dist-5E-epel-161012.1854',
-            'dist-5E-epel-161013.1711', 'dist-5E-epel-testing-161001.0424',
-            'dist-5E-epel-testing-161003.0856', 'dist-5E-epel-testing-161006.0053',
-            'dist-6E-epel-161002.2331', 'dist-6E-epel-161003.2046',
-            'dist-6E-epel-testing-161001.0528', 'epel7-161003.0724', 'epel7-161003.2046',
-            'epel7-161004.1423', 'epel7-161005.1122', 'epel7-testing-161001.0424',
-            'epel7-testing-161003.0621', 'epel7-testing-161003.2217', 'f23-updates-161002.2331',
-            'f23-updates-161003.1302', 'f23-updates-161004.1423', 'f23-updates-161005.0259',
-            'f23-updates-testing-161001.0424', 'f23-updates-testing-161003.0621',
-            'f23-updates-testing-161003.2217', 'f24-updates-161002.2331',
-            'f24-updates-161003.1302', 'f24-updates-testing-161001.0424',
-            'this_should_get_left_alone', 'f23-updates-should_be_untouched',
-            'f23-updates.repocache', 'f23-updates-testing-blank']
-        [os.makedirs(os.path.join(compose_dir, d)) for d in dirs]
-        # Now let's make a few files here and there.
-        with open(os.path.join(compose_dir, 'dist-5E-epel-161003.0724', 'oops.txt'), 'w') as oops:
-            oops.write('This compose failed to get cleaned and left this file around, oops!')
-        with open(os.path.join(compose_dir, 'COOL_FILE.txt'), 'w') as cool_file:
-            cool_file.write('This file should be allowed to hang out here because it\'s cool.')
-
-        t = RPMComposerThread(self.semmock, task['composes'][0],
-                              'ralph', self.db_factory, compose_dir)
-        t.keep_old_composes = 2
-        expected_messages = (
-            compose_schemas.ComposeComposingV1,
-            override_schemas.BuildrootOverrideUntagV1,
-            update_schemas.UpdateCompleteStableV1,
-            errata_schemas.ErrataPublishV1,
-            compose_schemas.ComposeCompleteV1.from_dict(dict(
-                success=True, repo='f17-updates', ctype='rpm', agent='ralph')))
+            # have the update reach karma threshold
+            up = session.query(Update).one()
+            assert up.karma == 1
+            up.comment(session, "foo", 1, 'foo')
+            assert up.karma == 2
 
-        with self.db_factory() as session:
             with mock.patch('bodhi.server.tasks.composer.subprocess.Popen') as Popen:
                 release = session.query(Release).filter_by(name='F17').one()
                 Popen.side_effect = self._generate_fake_pungi(t, 'stable_tag', release)
                 with mock_sends(*expected_messages):
                     t.run()
 
-        # We expect these and only these directories to remain.
-        expected_dirs = {
-            'dist-5E-epel-161012.1854', 'dist-5E-epel-161013.1711',
-            'dist-5E-epel-testing-161003.0856', 'dist-5E-epel-testing-161006.0053',
-            'dist-6E-epel-161002.2331', 'dist-6E-epel-161003.2046',
-            'dist-6E-epel-testing-161001.0528', 'epel7-161004.1423', 'epel7-161005.1122',
-            'epel7-testing-161003.0621', 'epel7-testing-161003.2217', 'f23-updates-161004.1423',
-            'f23-updates-161005.0259', 'f23-updates-testing-161003.0621',
-            'f23-updates-testing-161003.2217', 'f24-updates-161002.2331',
-            'f24-updates-161003.1302', 'f24-updates-testing-161001.0424',
-            'this_should_get_left_alone', 'f23-updates-should_be_untouched',
-            'f23-updates.repocache', 'f23-updates-testing-blank'}
         actual_dirs = set([
             d for d in os.listdir(compose_dir)
             if os.path.isdir(os.path.join(compose_dir, d))
             and not d.startswith("Fedora-17-updates")])
 
-        # Assert that remove_old_composes removes the correct items and leaves the rest in place.
+        if clean_old:
+            # We expect these and only these directories to remain.
+            expected_dirs = {
+                'dist-5E-epel-161012.1854', 'dist-5E-epel-161013.1711',
+                'dist-5E-epel-testing-161003.0856', 'dist-5E-epel-testing-161006.0053',
+                'dist-6E-epel-161002.2331', 'dist-6E-epel-161003.2046',
+                'dist-6E-epel-testing-161001.0528', 'epel7-161004.1423', 'epel7-161005.1122',
+                'epel7-testing-161003.0621', 'epel7-testing-161003.2217', 'f23-updates-161004.1423',
+                'f23-updates-161005.0259', 'f23-updates-testing-161003.0621',
+                'f23-updates-testing-161003.2217', 'f24-updates-161002.2331',
+                'f24-updates-161003.1302', 'f24-updates-testing-161001.0424',
+                'this_should_get_left_alone', 'f23-updates-should_be_untouched',
+                'f23-updates.repocache', 'f23-updates-testing-blank'}
+        else:
+            # No dirs should have been removed since we had clean_old_composes set False.
+            expected_dirs = dirs
         assert actual_dirs == expected_dirs
         # The cool file should still be here
         actual_files = [f for f in os.listdir(compose_dir)
@@ -1440,6 +1392,7 @@ def test_compose(self, *args):
                               'ralph', self.db_factory, compose_dir)
         t.keep_old_composes = 2
         expected_messages = (
+            update_schemas.UpdateCommentV1,
             compose_schemas.ComposeComposingV1,
             override_schemas.BuildrootOverrideUntagV1,
             update_schemas.UpdateCompleteStableV1,
@@ -1448,6 +1401,12 @@ def test_compose(self, *args):
                 success=True, repo='f17-updates', ctype='rpm', agent='ralph')))
 
         with self.db_factory() as session:
+            # have the update reach karma threshold
+            up = session.query(Update).one()
+            assert up.karma == 1
+            up.comment(session, "foo", 1, 'foo')
+            assert up.karma == 2
+
             with mock.patch('bodhi.server.tasks.composer.subprocess.Popen') as Popen:
                 release = session.query(Release).filter_by(name='F17').one()
                 Popen.side_effect = self._generate_fake_pungi(t, 'stable_tag', release)
@@ -1509,6 +1468,10 @@ def test_compose_module(self, *args):
                 type=UpdateType.security)
 
             db.add(update)
+            # Have the update reach the stable karma threshold
+            update.comment(db, "foo", 1, 'foo')
+            update.comment(db, "test", 1, 'test')
+            assert update.karma == 2
 
             update.test_gating_status = TestGatingStatus.passed
 
@@ -1622,6 +1585,7 @@ def test_test_gating_status_failed(self, *args):
             self.semmock, task['composes'][0], 'ralph', self.db_factory,
             self.tempdir)
         expected_messages = (
+            update_schemas.UpdateCommentV1,
             compose_schemas.ComposeComposingV1.from_dict({
                 'repo': u'f17-updates',
                 'ctype': 'rpm',
@@ -1632,6 +1596,12 @@ def test_test_gating_status_failed(self, *args):
                 success=True, repo='f17-updates', ctype='rpm', agent='ralph')))
 
         with self.db_factory() as session:
+            # have the update reach karma threshold
+            up = session.query(Update).one()
+            assert up.karma == 1
+            up.comment(session, "foo", 1, 'foo')
+            assert up.karma == 2
+
             with mock.patch('bodhi.server.tasks.composer.subprocess.Popen') as Popen:
                 release = session.query(Release).filter_by(name='F17').one()
                 Popen.side_effect = self._generate_fake_pungi(t, 'stable_tag', release)
@@ -1665,6 +1635,7 @@ def test_test_gating_status_passed(self, *args):
             self.semmock, task['composes'][0], 'ralph', self.db_factory,
             self.tempdir)
         expected_messages = (
+            update_schemas.UpdateCommentV1,
             compose_schemas.ComposeComposingV1.from_dict(
                 {'repo': 'f17-updates', 'updates': [u.builds[0].nvr], 'agent': 'ralph',
                  'ctype': 'rpm'}),
@@ -1676,6 +1647,12 @@ def test_test_gating_status_passed(self, *args):
                 success=True, ctype='rpm', repo='f17-updates', agent='ralph')))
 
         with self.db_factory() as session:
+            # have the update reach karma threshold
+            up = session.query(Update).one()
+            assert up.karma == 1
+            up.comment(session, "foo", 1, 'foo')
+            assert up.karma == 2
+
             with mock.patch('bodhi.server.tasks.composer.subprocess.Popen') as Popen:
                 release = session.query(Release).filter_by(name='F17').one()
                 Popen.side_effect = self._generate_fake_pungi(t, 'stable_tag', release)
@@ -1684,7 +1661,7 @@ def test_test_gating_status_passed(self, *args):
                     # t.run() modified some of the objects we used to construct the expected
                     # messages above, so we need to inject the altered data into them so the
                     # assertions are correct.
-                    expected_messages[1].body['override'] = u.builds[0].override.__json__()
+                    expected_messages[2].body['override'] = u.builds[0].override.__json__()
 
         u = Build.query.filter_by(nvr='bodhi-2.0-1.fc17').one().update
         assert u.comments[-1].text == 'This update has been pushed to stable.'
@@ -1735,6 +1712,15 @@ def test_modify_stable_bugs(self, close, comment, *args):
         self.set_stable_request('bodhi-2.0-1.fc17')
         task = self._make_task()
 
+        with self.db_factory() as session:
+            # have the update reach karma threshold
+            up = session.query(Update).one()
+            assert up.karma == 1
+            up.comment(session, "foo", 1, 'foo')
+            # Clear pending messages
+            self.db.info['messages'] = []
+            assert up.karma == 2
+
         t = RPMComposerThread(self.semmock, task['composes'][0],
                               'ralph', self.db_factory, self.tempdir)
 
@@ -1785,7 +1771,13 @@ def test_status_comment_stable(self, *args):
         with self.db_factory() as session:
             up = session.query(Update).one()
             up.request = UpdateRequest.stable
-            assert len(up.comments) == 2
+            # Have the update reach the stable karma threshold
+            assert up.karma == 1
+            up.comment(session, "foo", 1, 'foo')
+            # Clear pending messages
+            self.db.info['messages'] = []
+            assert up.karma == 2
+            assert len(up.comments) == 3
 
         with mock_sends(*[base_schemas.BodhiMessage] * 6):
             task = self._make_task()
@@ -1794,7 +1786,7 @@ def test_status_comment_stable(self, *args):
 
         with self.db_factory() as session:
             up = session.query(Update).one()
-            assert len(up.comments) == 3
+            assert len(up.comments) == 4
             assert up.comments[-1]['text'] == 'This update has been pushed to stable.'
 
     @mock.patch('bodhi.server.tasks.composer.PungiComposerThread._sanity_check_repo')
@@ -1835,7 +1827,13 @@ def test_unlock_updates(self, *args):
         with self.db_factory() as session:
             up = session.query(Update).one()
             up.request = UpdateRequest.stable
-            assert len(up.comments) == 2
+            # Have the update reach the stable karma threshold
+            assert up.karma == 1
+            up.comment(session, "foo", 1, 'foo')
+            # Clear pending messages
+            self.db.info['messages'] = []
+            assert up.karma == 2
+            assert len(up.comments) == 3
 
         with mock_sends(*[base_schemas.BodhiMessage] * 6):
             task = self._make_task()
@@ -2047,6 +2045,12 @@ def test_push_timestamps(self, *args):
             assert up.date_testing is not None
             assert up.date_stable is None
             up.request = UpdateRequest.stable
+            # Have the update reach the stable karma threshold
+            assert up.karma == 1
+            up.comment(session, "foo", 1, 'foo')
+            # Clear pending messages
+            self.db.info['messages'] = []
+            assert up.karma == 2
 
         self.koji.clear()
         expected_messages = (
@@ -2134,6 +2138,12 @@ def test_expire_buildroot_overrides_exception(self, expire, exception_log, *args
             up = db.query(Update).one()
             up.release.state = ReleaseState.pending
             up.request = UpdateRequest.stable
+            # Have the update reach the stable karma threshold
+            assert up.karma == 1
+            up.comment(db, "foo", 1, 'foo')
+            # Clear pending messages
+            self.db.info['messages'] = []
+            assert up.karma == 2
         task = self._make_task()
         api_version = task.pop("api_version")
 
diff --git a/bodhi-server/tests/test_models.py b/bodhi-server/tests/test_models.py
index 77b2c7a941..7992b0a8de 100644
--- a/bodhi-server/tests/test_models.py
+++ b/bodhi-server/tests/test_models.py
@@ -313,7 +313,7 @@ def test_epel_without_testing_bug_epel_msg(self, warning):
         config.update({
             'testing_bug_msg': 'cool fedora stuff {update_url}',
             'base_address': 'b',
-            'critpath.min_karma': 1,
+            'min_karma': 1,
             'fedora_epel.mandatory_days_in_testing': 0
         })
         del config["testing_bug_epel_msg"]
@@ -769,8 +769,8 @@ def test_critpath_mandatory_days_in_testing_no_status(self):
         release has no status.
         """
         config.update({
-            'critpath.stable_after_days_without_negative_karma': 11,
-            'f11.current.critpath.stable_after_days_without_negative_karma': 42
+            'critpath.mandatory_days_in_testing': 11,
+            'f11.current.critpath.mandatory_days_in_testing': 42
         })
         assert self.obj.critpath_mandatory_days_in_testing == 11
 
@@ -780,7 +780,7 @@ def test_critpath_mandatory_days_in_testing_status_default(self):
         release has status, but no override set.
         """
         config.update({
-            'critpath.stable_after_days_without_negative_karma': 11,
+            'critpath.mandatory_days_in_testing': 11,
             'f11.status': 'current'
         })
         assert self.obj.critpath_mandatory_days_in_testing == 11
@@ -791,9 +791,9 @@ def test_critpath_mandatory_days_in_testing_status_override(self):
         release has status and override set.
         """
         config.update({
-            'critpath.stable_after_days_without_negative_karma': 11,
+            'critpath.mandatory_days_in_testing': 11,
             'f11.status': 'current',
-            'f11.current.critpath.stable_after_days_without_negative_karma': 42
+            'f11.current.critpath.mandatory_days_in_testing': 42
         })
         assert self.obj.critpath_mandatory_days_in_testing == 42
 
@@ -879,31 +879,31 @@ def test_inherit_override_tags_wrong_release_name(self):
 
 
 class TestReleaseCritpathMinKarma(BasePyTestCase):
-    """Tests for the Release.critpath_min_karma property."""
+    """Tests for the Release.min_karma property."""
 
     @mock.patch.dict(
-        config, {'critpath.min_karma': 2, 'f17.beta.critpath.min_karma': 42, 'f17.status': "beta"})
+        config, {'min_karma': 2, 'f17.beta.min_karma': 42, 'f17.status': "beta"})
     def test_setting_status_min(self):
         """If a min is defined for the release, it should be returned."""
         release = model.Release.query.first()
 
-        assert release.critpath_min_karma == 42
+        assert release.min_karma == 42
 
     @mock.patch.dict(
-        config, {'critpath.min_karma': 25, 'f17.status': "beta"})
+        config, {'min_karma': 25, 'f17.status': "beta"})
     def test_setting_status_no_min(self):
         """If no min is defined for the release, the general min karma config should be returned."""
         release = model.Release.query.first()
 
-        assert release.critpath_min_karma == 25
+        assert release.min_karma == 25
 
     @mock.patch.dict(
-        config, {'critpath.min_karma': 72})
+        config, {'min_karma': 72})
     def test_setting_status_no_setting_status(self):
         """If no status is defined for the release, the general min karma should be returned."""
         release = model.Release.query.first()
 
-        assert release.critpath_min_karma == 72
+        assert release.min_karma == 72
 
 
 class TestReleaseModular(ModelTest):
@@ -2654,7 +2654,7 @@ def test_backref_different_build_types(self):
 @mock.patch('bodhi.server.models.fetch_test_cases_task', mock.Mock())
 class TestUpdateMeetsTestingRequirements(BasePyTestCase):
     """
-    Test the Update.meets_testing_requirements() method.
+    Test the Update.meets_testing_requirements() method and friends.
 
     It is significant and not obvious that the update used by these tests
     starts out with karma of +1/-0. These tests also rely on manual and automatic
@@ -2664,11 +2664,14 @@ class TestUpdateMeetsTestingRequirements(BasePyTestCase):
 
     @pytest.mark.parametrize('critpath', (True, False))
     @pytest.mark.parametrize("autokarma", (True, False))
-    def test_update_reaching_stable_karma_not_critpath_min_karma(self, autokarma, critpath):
+    def test_update_reaching_stable_karma_not_min_karma(self, autokarma, critpath):
         """
-        Assert that meets_testing_requirements() returns the correct result for updates
-        that haven't reached the days in testing or critpath_min_karma, but have reached
-        stable_karma.
+        Assert that meets_testing_requirements() returns False for updates that
+        haven't reached the days in testing or min_karma, but have reached stable_karma.
+        It should be very uncommon to encounter this situation these days because we disallow
+        setting the stable_karma threshold lower than min_karma when creating or editing an
+        update, but it could theoretically happen if min_karma for a release was increased
+        while an update which had stable_karma set to the previous minimum value was in-flight.
         """
         update = model.Update.query.first()
         update.autokarma = autokarma
@@ -2677,18 +2680,17 @@ def test_update_reaching_stable_karma_not_critpath_min_karma(self, autokarma, cr
         update.stable_karma = 1
 
         # check, and make it clear, what we're assuming the karma value is now
-        # and the critpath_min_karma value we're testing against
+        # and the min_karma value we're testing against
         assert update.karma == 1
-        assert update.release.critpath_min_karma == 2
-        # this means 'False for critpath, True for non-critpath'. critpath
-        # should be required to meet critpath_min_karma, but non-critpath
-        # should not
-        assert update.meets_testing_requirements is not critpath
+        assert update.release.min_karma == 2
+        # this should always be False. in the past we allowed non-critpath
+        # updates to be pushed stable in this case but we no longer do
+        assert not update.meets_testing_requirements
 
     @pytest.mark.parametrize('critpath', (True, False))
     @pytest.mark.parametrize('autokarma', (True, False))
     def test_update_below_stable_karma(self, autokarma, critpath):
-        """It should return False for all updates below stable karma, critpath_min_karma
+        """It should return False for all updates below stable karma, min_karma
         and time."""
         update = model.Update.query.first()
         update.autokarma = autokarma
@@ -2713,7 +2715,7 @@ def test_update_reaching_time_in_testing(self, critpath):
         update.critpath = critpath
         update.request = None
         if critpath:
-            # the default critpath.stable_after_days_without_negative_karma
+            # the default critpath.mandatory_days_in_testing
             # value is 14
             update.date_testing = datetime.utcnow() - timedelta(days=15)
         else:
@@ -2732,7 +2734,7 @@ def test_update_below_time_in_testing(self, critpath):
         update.critpath = critpath
         update.request = None
         if critpath:
-            # the default critpath.stable_after_days_without_negative_karma
+            # the default critpath.mandatory_days_in_testing
             # value is 14
             update.date_testing = datetime.utcnow() - timedelta(days=13)
         else:
@@ -2745,15 +2747,16 @@ def test_update_below_time_in_testing(self, critpath):
 
     @pytest.mark.parametrize('critpath', (True, False))
     def test_update_reaching_time_in_testing_negative_karma(self, critpath):
-        """If an update meets the mandatory_days_in_testing and stable_karma thresholds
-        but not the critpath_min_karma threshold, it should return False for critical path
-        updates but True for non-critical-path updates."""
+        """If an update meets mandatory_days_in_testing but not min_karma, it
+        should still return True. In the past we did not allow critical path updates
+        through for days_in_testing if they had any negative karma, but this is not
+        part of the Fedora or EPEL update policies."""
         update = model.Update.query.first()
         update.critpath = critpath
         update.status = model.UpdateStatus.testing
         update.request = None
         if critpath:
-            # the default critpath.stable_after_days_without_negative_karma
+            # the default critpath.mandatory_days_in_testing
             # value is 14
             update.date_testing = datetime.utcnow() - timedelta(days=15)
         else:
@@ -2762,33 +2765,30 @@ def test_update_reaching_time_in_testing_negative_karma(self, critpath):
         update.stable_karma = 1
         update.comment(self.db, 'testing', author='enemy', karma=-1)
         # This gets the update back to positive karma, but not to the required
-        # +2 karma needed to clear the critpath_min_karma requirement
+        # +2 karma needed to clear the min_karma requirement
         update.comment(self.db, 'testing', author='bro', karma=1)
 
         assert update.karma == 1
-        assert update.release.critpath_min_karma == 2
-        # this means 'False for critpath, True for non-critpath'
-        assert update.meets_testing_requirements is not critpath
+        assert update.release.min_karma == 2
+        assert update.meets_testing_requirements is True
 
     @pytest.mark.parametrize('critpath', (True, False))
-    def test_update_reaching_critpath_min_karma(self, critpath):
+    def test_update_reaching_min_karma(self, critpath):
         """Both critpath and non-critpath packages should be allowed to go stable when
         meeting the policy minimum karma requirement for critpath packages, even if there
         is negative karma."""
         update = model.Update.query.first()
         update.critpath = critpath
+        # make sure no autopush gets in the way
         update.stable_karma = 3
         update.comment(self.db, 'testing', author='enemy', karma=-1)
         update.comment(self.db, 'testing', author='bro', karma=1)
-        # Despite meeting the stable_karma, the function should still not
-        # mark this as meeting testing requirements because critpath packages
-        # have a higher requirement for minimum karma - at this point we
-        # are in the same state as test_critpath_14_days_negative_karma.
-        # So add a third +1 so total is +2, which meets critpath_min_karma
+        # at this point we're at +1 as the update starts at +1. Add
+        # another +1 so total is +2, which meets min_karma
         update.comment(self.db, 'testing', author='ham', karma=1)
 
         assert update.karma == 2
-        assert update.release.critpath_min_karma == 2
+        assert update.release.min_karma == 2
         assert update.meets_testing_requirements
 
     @pytest.mark.parametrize('critpath', (True, False))
@@ -2800,7 +2800,7 @@ def test_update_reaching_critpath_min_karma(self, critpath):
             (TestGatingStatus.running, False),
             (TestGatingStatus.passed, True),
             (TestGatingStatus.failed, False),
-            (None, True),
+            (None, False),
             (TestGatingStatus.greenwave_failed, False)
         )
     )
@@ -2819,7 +2819,7 @@ def test_test_gating_status(self, status, expected, critpath):
         update.comment(self.db, 'testing', author='bro', karma=1)
         # Assert that our preconditions from the docblock are correct.
         assert update.karma == 2
-        assert update.release.critpath_min_karma == 2
+        assert update.release.min_karma == 2
         assert update.meets_testing_requirements is expected
 
     def test_test_gating_not_required(self):
@@ -2839,6 +2839,33 @@ def test_test_gating_not_required(self):
         assert update.karma == 2
         assert update.meets_testing_requirements
 
+    def test_other_interfaces(self):
+        """
+        Check the other related interfaces: meets_requirements_why
+        (around which meets_testing_requirements is now a wrapper),
+        critpath_approved (which is now a deprecated synonym for
+        meets_testing_requirements), and check_requirements (which is
+        now a deprecated synonym for meets_requirements_why).
+        """
+        config["test_gating.required"] = False
+        update = model.Update.query.first()
+        update.autokarma = False
+        update.stable_karma = 1
+        update.test_gating_status = TestGatingStatus.running
+        update.comment(self.db, 'I found $100 after applying this update.', karma=1,
+                       author='bowlofeggs')
+        # Assert that our preconditions from the docblock are correct.
+        assert update.karma == 2
+        expwhy = (
+            True,
+            "Test gating is disabled, and the update has at least 2 karma."
+        )
+        assert update.meets_requirements_why == expwhy
+        with pytest.deprecated_call():
+            assert update.check_requirements(self.db, config) == expwhy
+        with pytest.deprecated_call():
+            assert update.critpath_approved
+
 
 @mock.patch('bodhi.server.models.work_on_bugs_task', mock.Mock())
 @mock.patch('bodhi.server.models.fetch_test_cases_task', mock.Mock())
@@ -3087,7 +3114,7 @@ def test_mandatory_days_in_testing_critpath(self):
         update.critpath = True
 
         # Configured value.
-        expected = int(config.get('critpath.stable_after_days_without_negative_karma'))
+        expected = int(config.get('critpath.mandatory_days_in_testing'))
 
         assert update.mandatory_days_in_testing == expected
 
@@ -3121,7 +3148,7 @@ def test_days_to_stable_critpath(self):
         update.date_testing = datetime.utcnow() + timedelta(days=-4)
 
         critpath_days_to_stable = int(
-            config.get('critpath.stable_after_days_without_negative_karma'))
+            config.get('critpath.mandatory_days_in_testing'))
 
         assert update.days_to_stable == critpath_days_to_stable - 4
 
@@ -3132,9 +3159,10 @@ def test_days_to_stable_meets_testing_requirements(self):
         """
         update = self.obj
         update.autokarma = False
-        update.stable_karma = 1
         update.comment(self.db, 'I found $100 after applying this update.', karma=1,
                        author='bowlofeggs')
+        update.comment(self.db, 'This update saved my life!', karma=1,
+                       author='ralph')
         # Assert that our preconditions from the docblock are correct.
         assert update.meets_testing_requirements
 
@@ -3598,32 +3626,6 @@ def test__composite_karma_one_negative_two_positive(self):
 
         assert self.obj._composite_karma == (2, -1)
 
-    def test_critpath_approved_no_release_requirements(self):
-        """critpath_approved() should use the broad requirements if the release doesn't have any."""
-        self.obj.critpath = True
-        self.obj.comment(self.db, "foo", 1, 'biz')
-        release_name = self.obj.release.name.lower().replace('-', '')
-
-        with mock.patch.dict(
-                config,
-                {'{}.status'.format(release_name): 'stable', 'critpath.num_admin_approvals': 0,
-                 'critpath.min_karma': 1}):
-            assert self.obj.critpath_approved
-
-    def test_critpath_approved_release_requirements(self):
-        """critpath_approved() should use the release requirements if they are defined."""
-        self.obj.critpath = True
-        self.obj.comment(self.db, "foo", 1, 'biz')
-        release_name = self.obj.release.name.lower().replace('-', '')
-
-        with mock.patch.dict(
-                config,
-                {'{}.status'.format(release_name): 'stable', 'critpath.num_admin_approvals': 0,
-                 'critpath.min_karma': 1,
-                 '{}.{}.critpath.num_admin_approvals'.format(release_name, 'stable'): 0,
-                 '{}.{}.critpath.min_karma'.format(release_name, 'stable'): 2}):
-            assert not self.obj.critpath_approved
-
     def test_last_modified_no_dates(self):
         """last_modified() should raise ValueError if there are no available dates."""
         self.obj.date_submitted = None
@@ -3771,14 +3773,15 @@ def test_set_request_pending_stable(self):
         req.errors = cornice.Errors()
         req.koji = buildsys.get_session()
         assert self.obj.status == UpdateStatus.pending
-        self.obj.stable_karma = 1
         # disable autokarma, so sending the comment doesn't do the request
         self.obj.autokarma = False
         self.obj.comment(self.db, 'works', karma=1, author='bowlofeggs')
+        self.obj.comment(self.db, 'sure does', karma=1, author='ralph')
         # make sure we are actually doing something here
         assert self.obj.request is not UpdateRequest.stable
 
         expected_messages = (
+            update_schemas.UpdateCommentV1,
             update_schemas.UpdateCommentV1,
             update_schemas.UpdateRequestStableV1,
         )
@@ -3801,8 +3804,13 @@ def test_set_request_pending_stable_frozen_release(self):
         req.errors = cornice.Errors()
         req.koji = buildsys.get_session()
         assert self.obj.status == UpdateStatus.pending
-        self.obj.stable_karma = 1
         self.obj.release.state = ReleaseState.frozen
+        # disable autokarma, so sending the comment doesn't do the request
+        self.obj.autokarma = False
+        # make sure we meet the karma requirements, so we *could* push
+        # stable if we weren't frozen
+        self.obj.comment(self.db, 'works', karma=1, author='bowlofeggs')
+        self.obj.comment(self.db, 'sure does', karma=1, author='ralph')
 
         with pytest.raises(BodhiException) as exc:
             self.obj.set_request(self.db, UpdateRequest.stable, req.user.name)
@@ -3903,7 +3911,10 @@ def test_set_request_untested_stable(self):
         assert self.obj.status == UpdateStatus.pending
         with pytest.raises(BodhiException) as exc:
             self.obj.set_request(self.db, UpdateRequest.stable, req.user.name)
-        assert str(exc.value) == config.get('not_yet_tested_msg')
+        assert str(exc.value) == (
+            f"{config.get('not_yet_tested_msg')}: Test gating is disabled, but "
+            "update has less than 2 karma and has been in testing less than 7 days."
+        )
         assert self.obj.request == UpdateRequest.testing
         assert self.obj.status == UpdateStatus.pending
 
@@ -3991,7 +4002,10 @@ def test_set_request_stable_epel_requirements_not_met(self):
         with pytest.raises(BodhiException) as exc:
             with mock_sends():
                 self.obj.set_request(self.db, UpdateRequest.stable, req.user.name)
-        assert str(exc.value) == config['not_yet_tested_epel_msg']
+        assert str(exc.value) == (
+            f"{config['not_yet_tested_epel_msg']}: Test gating is disabled, "
+            "but update has less than 2 karma and has been in testing less than 14 days."
+        )
 
         assert self.obj.request is None
 
@@ -4037,17 +4051,10 @@ def test_set_request_stable_for_critpath_update_when_test_gating_enabled(self):
             self.obj.set_request(self.db, UpdateRequest.stable, req.user.name)
 
         expected_msg = (
-            'This critical path update has not yet been approved for pushing to the '
-            'stable repository.  It must first reach a karma of %s, consisting of %s '
-            'positive karma from proventesters, along with %d additional karma from '
-            'the community. Or, it must spend %s days in testing without any negative '
-            'feedback')
-        expected_msg = expected_msg % (
-            config.get('critpath.min_karma'),
-            config.get('critpath.num_admin_approvals'),
-            (config.get('critpath.min_karma') - config.get('critpath.num_admin_approvals')),
-            config.get('critpath.stable_after_days_without_negative_karma'))
-        expected_msg += ' Additionally, it must pass automated tests.'
+            'This update has not yet met the minimum testing requirements defined in the '
+            '<a href="https://fedoraproject.org/wiki/Package_update_acceptance_criteria">'
+            'Package Update Acceptance Criteria</a>: Required tests did not pass on this update.'
+        )
         assert str(exc.value) == expected_msg
 
     def test_set_request_string_action(self):
@@ -4058,12 +4065,13 @@ def test_set_request_string_action(self):
         assert self.obj.status == UpdateStatus.pending
         # disable autokarma, so sending the comment doesn't do the request
         self.obj.autokarma = False
-        self.obj.stable_karma = 1
         self.obj.comment(self.db, 'works', karma=1, author='bowlofeggs')
+        self.obj.comment(self.db, 'sure does', karma=1, author='ralph')
         # make sure we are actually doing something here
         assert self.obj.request is not UpdateRequest.stable
 
         expected_messages = (
+            update_schemas.UpdateCommentV1,
             update_schemas.UpdateCommentV1,
             update_schemas.UpdateRequestStableV1,
         )
@@ -4341,65 +4349,6 @@ def test_send_update_notice_status_testing(self, SMTP):
             [config['{}_test_announce_list'.format(release_name)]],
             msg.encode('utf-8'))
 
-    def test_check_requirements_test_gating_disabled(self):
-        '''Should pass regardless if gating is disabled.'''
-        self.obj.test_gating_status = model.TestGatingStatus.failed
-        with mock.patch.dict(config, {'test_gating.required': False}):
-            assert self.obj.check_requirements(self.db, config) == (True, 'No checks required.')
-
-    def test_check_requirements_test_gating_status_failed(self):
-        """check_requirements() should return False when test_gating_status is failed."""
-        self.obj.test_gating_status = model.TestGatingStatus.failed
-
-        with mock.patch.dict(config, {'test_gating.required': True}):
-            assert self.obj.check_requirements(self.db, config) == (
-                False, 'Required tests did not pass on this update.')
-
-    def test_check_requirements_test_gating_status_passed(self):
-        """check_requirements() should return True when test_gating_status is passed."""
-        self.obj.test_gating_status = model.TestGatingStatus.passed
-
-        with mock.patch.dict(config, {'test_gating.required': True}):
-            assert self.obj.check_requirements(self.db, config) == (
-                True, 'All required tests passed or were waived.')
-
-    def test_check_requirements_test_gating_status_ignored(self):
-        """check_requirements() should return True when test_gating_status is ignored."""
-        self.obj.test_gating_status = model.TestGatingStatus.ignored
-
-        with mock.patch.dict(config, {'test_gating.required': True}):
-            assert self.obj.check_requirements(self.db, config) == (
-                True, 'No checks required.')
-
-    def test_check_requirements_test_gating_status_waiting(self):
-        """check_requirements() should return False when test_gating_status is waiting."""
-        self.obj.test_gating_status = model.TestGatingStatus.waiting
-
-        with mock.patch.dict(config, {'test_gating.required': True}):
-            assert self.obj.check_requirements(self.db, config) == (
-                False, 'Required tests for this update are still running.')
-
-    def test_num_admin_approvals_after_karma_reset(self):
-        """Make sure number of admin approvals is counted correctly for the build."""
-        update = model.Update.query.first()
-
-        # Approval from admin 'bodhiadmin' {config.admin_groups}
-        user_group = [model.Group(name='bodhiadmin')]
-        user = model.User(name='bodhiadmin', groups=user_group)
-        comment = model.Comment(text='Test comment', karma=1, user=user)
-        self.db.add(comment)
-        update.comments.append(comment)
-
-        assert update.num_admin_approvals == 1
-
-        # This is a "karma reset event", so the above comments should not be counted in the karma.
-        user = model.User(name='bodhi')
-        comment = model.Comment(text="New build", karma=0, user=user)
-        self.db.add(comment)
-        update.comments.append(comment)
-
-        assert update.num_admin_approvals == 0
-
     def test_validate_release_failure(self):
         """Test the validate_release() method for the failure case."""
         user = self.db.query(model.User).first()
diff --git a/bodhi-server/tests/testing.ini b/bodhi-server/tests/testing.ini
index 6c3411dc13..4ed23bc647 100644
--- a/bodhi-server/tests/testing.ini
+++ b/bodhi-server/tests/testing.ini
@@ -14,9 +14,8 @@ fedora.mandatory_days_in_testing = 7
 fedora_epel.mandatory_days_in_testing = 14
 f7.status = post_beta
 f7.post_beta.mandatory_days_in_testing = 7
-f7.post_beta.critpath.num_admin_approvals = 0
-f7.post_beta.critpath.min_karma = 2
-f7.post_beta.critpath.stable_after_days_without_negative_karma = 3
+f7.post_beta.min_karma = 2
+f7.post_beta.critpath.mandatory_days_in_testing = 3
 cors_origins_ro = *
 cors_origins_rw = http://0.0.0.0:6543 http://bodhi-dev.example.com/ https://bodhi-dev.example.com/
 cors_connect_src = http://0.0.0.0:6543 http://bodhi-dev.example.com/ https://bodhi-dev.example.com/ http://localhost:6543 https://*.fedoraproject.org/ wss://hub.fedoraproject.org:9939/
@@ -65,7 +64,6 @@ pungi.cmd = /usr/bin/true
 pungi.conf.rpm = pungi.rpm.conf.j2
 pungi.conf.module = pungi.module.conf.j2
 critpath_pkgs = kernel
-critpath.num_admin_approvals = 0
 bugtracker = dummy
 stats_blacklist = bodhi autoqa
 test_case_base_url = https://fedoraproject.org/wiki/
diff --git a/devel/ci/integration/bodhi/production.ini b/devel/ci/integration/bodhi/production.ini
index 774a4e4111..ad1bc767ed 100644
--- a/devel/ci/integration/bodhi/production.ini
+++ b/devel/ci/integration/bodhi/production.ini
@@ -399,10 +399,12 @@ critpath_pkgs = kernel
 critpath.num_admin_approvals = 0
 
 # The net karma required to submit a critical path update to a pending release.
-# critpath.min_karma = 2
+# we set this to 0 otherwise we can't be sure test_updates_request will
+# pass, it might pick an update with karma lower than this value
+min_karma = 0
 
 # Allow critpath to submit for stable after 2 weeks with no negative karma
-# critpath.stable_after_days_without_negative_karma = 14
+# critpath.mandatory_days_in_testing = 14
 
 # The minimum amount of time an update must spend in testing before
 # it can reach the stable repository
@@ -423,10 +425,10 @@ fedora_epel.mandatory_days_in_testing = 14
 #f28.status = pre_beta
 #f28.pre_beta.mandatory_days_in_testing = 3
 #f28.pre_beta.critpath.num_admin_approvals = 0
-#f28.pre_beta.critpath.min_karma = 1
+#f28.pre_beta.min_karma = 1
 #f28.post_beta.mandatory_days_in_testing = 7
 #f28.post_beta.critpath.num_admin_approvals = 0
-#f28.post_beta.critpath.min_karma = 2
+#f28.post_beta.min_karma = 2
 
 
 ##
diff --git a/devel/development.ini.example b/devel/development.ini.example
index 4d97cdede6..28a0d95cca 100644
--- a/devel/development.ini.example
+++ b/devel/development.ini.example
@@ -13,8 +13,8 @@ fedora_epel.mandatory_days_in_testing = 14
 f7.status = post_beta
 f7.post_beta.mandatory_days_in_testing = 7
 f7.post_beta.critpath.num_admin_approvals = 0
-f7.post_beta.critpath.min_karma = 2
-f7.post_beta.critpath.stable_after_days_without_negative_karma = 3
+f7.post_beta.min_karma = 2
+f7.post_beta.critpath.mandatory_days_in_testing = 3
 cors_origins_ro = *
 cors_origins_rw = http://0.0.0.0:6543 http://bodhi-dev.example.com/ https://bodhi-dev.example.com/
 cors_connect_src = http://0.0.0.0:6543 http://bodhi-dev.example.com/ https://bodhi-dev.example.com/ http://localhost:6543 https://*.fedoraproject.org/ wss://hub.fedoraproject.org:9939/
diff --git a/docs/user/testing.rst b/docs/user/testing.rst
index 74751750ba..ad264682a4 100644
--- a/docs/user/testing.rst
+++ b/docs/user/testing.rst
@@ -38,24 +38,28 @@ Stable push
 An update can be pushed to stable repository either manually or automatically, through Bodhi's
 `autotime` or `autokarma` features.
 
-For becoming pushable to stable an update must fullfill some criteria set by the associated
-release. Tipically, each release has a `mandatory_days_in_testing` and a `critpath_min_karma`
-values that define the threshold after that the associated updates can be pushed to stable.
-The `mandatory_days_in_testing` define the minimum amount of days each update must spend in
-testing repository. The `critpath_min_karma` is the minimum karma that each update must reach.
-An update can be manually pushed to stable by its submitter whatever threshold is reached first.
-For example, if a release has a `critpath_min_karma` of +2, an update which reaches a +2 karma
-**can be pushed to stable even if it hasn't reached the `mandatory_days_in_testing`**.
+For becoming pushable to stable an update must fulfill some criteria set by the associated
+release. Typically, each release has `critpath.mandatory_days_in_testing`,
+`mandatory_days_in_testing` and `min_karma` values that define thresholds for associated updates
+to be pushed to stable. The `mandatory_days_in_testing` settings define an amount of days of
+waiting in the updates repository for critical path and non-critical-path updates respectively.
+The `min_karma` setting specifies a karma total threshold. An update can be manually pushed to
+stable by its submitter or anyone else with edit privileges on the update when **either**
+threshold is reached. For example, if a release has a `min_karma` of 2, an update which reaches
++2 karma **can be pushed to stable even if it hasn't reached the `mandatory_days_in_testing`**.
 
 When submitting an update, the user can enable the `autotime` and the `autokarma` features and
 set the related values `stable_days` and `stable_karma` for that specific update.
-The `stable_days` value define the minimum amount of days each update must spend in
-testing repository before the autopush is performed. It must be equal or greater than the release
-`mandatory_days_in_testing`. The `stable_karma` is the minimum karma that each update must reach
-before the autopush is performed. It must be equal or greater than the release `critpath_min_karma`.
-For example, if a release has a `mandatory_days_in_testing` of 7, an update which has `autotime`
-enabled and `stable_days` set to 10 will be pushable after 7 days **by manual action**, but the
-autopush will be performed **after 3 more days**.
-
+The `stable_days` value defines an amount of days the update must spend in testing repository
+before the autopush is done. It must be equal to or greater than the `mandatory_days_in_testing`
+value that applies to the update. The `stable_karma` value defines a karma total the update must
+reach before the autopush is done. It must be equal to or greater than the release `min_karma`.
+Autopush will happen as soon as **either** threshold is reached. For example, if a release has a
+`mandatory_days_in_testing` of 7, an update which has `autotime` enabled and `stable_days` set to
+10 will be pushable after 7 days **by manual action**, but the autopush will be performed
+**after 3 more days**. If an update has both `autotime` and `autokarma` enabled, with thresholds
+of 7 days and 3 karma respectively, it will be pushed stable as soon as it reaches +3 karma, even
+if 7 days have not yet passed; or it will be pushed stable after 7 days have passed, even if it
+has not yet reached +3 karma.
 
 .. _Greenwave: https://pagure.io/greenwave

From 495b793e04887cfd91def1186f821e694bc4523e Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Tue, 16 Apr 2024 15:47:13 -0700
Subject: [PATCH 10/11] Always apply obsoletion check in check_karma_thresholds

The "obsolete on unstable_karma" branch being `elif`ed with the
"disable autokarma on any negative karma" branch means that, if
we reach the unstable_karma threshold when autokarma is enabled,
we don't obsolete the update. That's clearly wrong. The most
likely way to hit this is for unstable_karma to be -1, so the
first negative karma causes both conditions to be met at once.
But it's also possible if e.g. unstable_karma is -2, the update
receives one negative karma, the maintainer re-enables autokarma,
then the update receives another negative karma.

We should *always* obsolete an update when it reaches
unstable_karma, regardless of anything else we do. If this means
we both obsolete it and disable autokarma at the same time, I
don't see any problem with that, it seems correct.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 bodhi-server/bodhi/server/models.py         | 14 +++++----
 bodhi-server/tests/services/test_updates.py | 33 +++++++++++++--------
 2 files changed, 29 insertions(+), 18 deletions(-)

diff --git a/bodhi-server/bodhi/server/models.py b/bodhi-server/bodhi/server/models.py
index a4acb8d52b..66efeec9b4 100644
--- a/bodhi-server/bodhi/server/models.py
+++ b/bodhi-server/bodhi/server/models.py
@@ -3810,6 +3810,12 @@ def check_karma_thresholds(self, db, agent):
         # Also return if the status of the update is not in testing and the release is frozen
         if self.status != UpdateStatus.testing and self.release.state == ReleaseState.frozen:
             return
+        # Obsolete if unstable karma threshold reached
+        if self.unstable_karma and self.karma <= self.unstable_karma:
+            log.info("Automatically obsoleting %s (reached unstable karma threshold)", self.alias)
+            self.obsolete(db)
+            notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
+                dict(update=self, status='unstable')))
         # If an update receives negative karma disable autopush
         # exclude rawhide updates see #4566
         if (self.autokarma or self.autotime) and self._composite_karma[1] != 0 and \
@@ -3820,7 +3826,8 @@ def check_karma_thresholds(self, db, agent):
             self.autotime = False
             text = config.get('disable_automatic_push_to_stable')
             self.comment(db, text, author='bodhi')
-        elif self.stable_karma and self.karma >= self.stable_karma \
+        # If update with autopush reaches threshold, set request stable
+        if self.stable_karma and self.karma >= self.stable_karma \
                 and self.release.composed_by_bodhi and self.autokarma:
             # Updates for releases not "composed by Bodhi" (Rawhide,
             # ELN...) are pushed stable only by approve_testing.py
@@ -3834,11 +3841,6 @@ def check_karma_thresholds(self, db, agent):
             self.set_request(db, UpdateRequest.stable, agent)
             notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
                 dict(update=self, status='stable')))
-        elif self.unstable_karma and self.karma <= self.unstable_karma:
-            log.info("Automatically obsoleting %s (reached unstable karma threshold)", self.alias)
-            self.obsolete(db)
-            notifications.publish(update_schemas.UpdateKarmaThresholdV1.from_dict(
-                dict(update=self, status='unstable')))
 
     @property
     def builds_json(self):
diff --git a/bodhi-server/tests/services/test_updates.py b/bodhi-server/tests/services/test_updates.py
index 7380a20d78..ab8b56efb9 100644
--- a/bodhi-server/tests/services/test_updates.py
+++ b/bodhi-server/tests/services/test_updates.py
@@ -4720,23 +4720,32 @@ def test_obsolete_if_unstable(self, autokarma, composed_by_bodhi, req, status):
         # caused by the obsoletion to have taken effect when we construct the
         # expected message
         up.comment(self.db, "foo", -1, 'bowlofeggs')
-        with fml_testing.mock_sends(
-            update_schemas.UpdateKarmaThresholdV1.from_dict(
+        if req:
+            # it's difficult to construct the expected messsage in this
+            # case because posting this comment obsoletes the update,
+            # publishes the UKT message, *then also* disables autopush,
+            # so we never quite have access to the state of the update
+            # right at the time the UKT message is published
+            expukt = update_schemas.UpdateKarmaThresholdV1
+        else:
+            # in this case we *only* obsolete the update, we don't also
+            # disable autopush, so we can construct the expected message -
+            # the current update state is the same as the state when the
+            # message was published
+            expukt = update_schemas.UpdateKarmaThresholdV1.from_dict(
                 {'update': up, 'status': 'unstable'}
-            ),
-            update_schemas.UpdateCommentV1
-        ):
+            )
+        with fml_testing.mock_sends(expukt, update_schemas.UpdateCommentV1):
             # doing a db commit causes the message to be published
             self.db.commit()
         assert up.status is UpdateStatus.obsolete
         assert up.request is None
-        # the obsolete path should not disable autokarma, but we can't assert
-        # this unconditionally because we might have hit the earlier disable-
-        # autokarma path
-        if (
-            req is UpdateRequest.stable
-            or not composed_by_bodhi
-        ):
+        # the obsolete path itself should not disable autokarma, but if
+        # the release is composed by bodhi, we *should* also have hit
+        # the 'disable autokarma for any negative karma' path
+        if composed_by_bodhi:
+            assert not up.autokarma
+        else:
             assert up.autokarma is autokarma
         assert up.karma == -2
 

From e42e89f978d271c2ed916f64d8ce2b6aff6c4882 Mon Sep 17 00:00:00 2001
From: Adam Williamson <awilliam@redhat.com>
Date: Wed, 17 Apr 2024 15:39:39 -0700
Subject: [PATCH 11/11] Add news fragment for PR #5630

This covers all the changes from the commits together. It's
rather long, but as short as I could make it...

Signed-off-by: Adam Williamson <awilliam@redhat.com>
---
 news/PR5630.bic | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
 create mode 100644 news/PR5630.bic

diff --git a/news/PR5630.bic b/news/PR5630.bic
new file mode 100644
index 0000000000..b02932efe9
--- /dev/null
+++ b/news/PR5630.bic
@@ -0,0 +1,14 @@
+Bodhi's update status checking has been overhauled, and some configuration options have changed.
+
+* critpath.num_admin_approvals is removed. This backed the old Fedora "proven testers" concept, which has not been used for some years.
+* critpath.min_karma is removed and is replaced by a new setting just called min_karma. This applies to all updates, not just critical path.
+* critpath.stable_after_days_without_negative_karma is renamed to critpath.mandatory_days_in_testing and its behavior has changed: there is no longer any check for 'no negative karma'. Critical path updates, like non-critical path updates, can now be manually pushed stable after reaching this time threshold even if they have negative karma.
+
+As before, these settings can be specified with prefixes to apply only to particular releases and milestones. min_karma and (critpath.)mandatory_days_in_testing now act strictly and consistently as minimum requirements for stable push. Any update may be pushed stable once it reaches either of those thresholds (and passes gating requirements, if gating is enabled). The update's stable_karma value is no longer ever considered in determining whether it may be pushed stable. stable_karma and stable_days are only used as triggers for automatic stable pushes (but for an update to be automatically pushed it must *also* reach either min_karma or (critpath.)mandatory_days_in_testing).
+
+The most obvious practical result of this change for Fedora is that, during phases where the policy minimum karma requirement is +2, you will no longer be able to make non-critical path updates pushable with +1 karma by setting this as their stable_karma value. Additionally:
+
+* It is no longer possible to set an update's request to 'stable' if it has previously met requirements but currently does not
+* Two cases where updates that reached their unstable_karma thresholds were not obsoleted are resolved
+* Updates in 'pending' as well as 'testing' status have autopush disabled upon receiving any negative karma
+* The 'date_approved' property of updates is more consistently set as the date the update first became eligible for stable push