Read and store current values in the DB as well

Signed-off-by: Aurélien Bompard <[email protected]>

devel/ansible/roles/dev/files/config.toml

@@ -40,6 +40,9 @@ badges_repo = "tests/test_badges"
 # production, this will be a postgres URI.
 database_uri = "sqlite:////home/vagrant/tahrir.db"
 
+# Email domain
+email_domain = "tinystage.test"
+
 # Datanommer database URI
 datanommer_db_uri = "postgresql://datanommer:datanommer@localhost/messages"
 datagrepper_url = "https://apps.fedoraproject.org/datagrepper"

fedbadges.toml.example

@@ -40,6 +40,9 @@ badges_repo = "tests/test_badges"
 # production, this will be a postgres URI.
 database_uri = "sqlite:////tmp/badges.db"
 
+# Email domain
+email_domain = "fedoraproject.org"
+
 # Datanommer database URI
 datanommer_db_uri = "postgresql://datanommer:datanommer@localhost/messages"
 datagrepper_url = "https://apps.fedoraproject.org/datagrepper"

fedbadges/cached.py

@@ -30,13 +30,6 @@ def set(self, key, value):
 
 
 def get_cached_messages_count(badge_id: str, candidate: str, get_previous_fn):
-    # This could also be stored in the database, but:
-    # - rules that have a "previous" query can regenerate the value
-    # - rules that don't have a "previous" query currently don't need to count as they award
-    #   the badge on the first occurence
-    # If at some point in the future we have rules that need counting but can't have a "previous"
-    # query, then this data will not be rebuildable anymore and we should store it in a database
-    # table linking badges and users.
     key = f"messages_count|{badge_id}|{candidate}"
     try:
         current_value = cache.get_or_create(

fedbadges/consumer.py

@@ -77,9 +77,13 @@ def _initialize_cache(self):
         configure_cache(**cache_args)
 
     def _initialize_tahrir_connection(self):
+        if "email_domain" not in self.config:
+            raise ValueError("Missing configuration key: 'email_domain'")
+
         database_uri = self.config.get("database_uri")
         if not database_uri:
             raise ValueError("Badges consumer requires a database uri")
+
         issuer = self.config["badge_issuer"]
         self.tahrir = tahrir_api.dbapi.TahrirDatabase(
             dburi=database_uri,
@@ -101,7 +105,7 @@ def _initialize_datanommer_connection(self):
         datanommer.models.init(self.config["datanommer_db_uri"])
 
     def award_badge(self, username, badge_rule, link=None):
-        email = f"{username}@fedoraproject.org"
+        email = f"{username}@{self.config['email_domain']}"
         client = self._get_tahrir_client(self.tahrir.session)
         client.add_person(email)
         self.tahrir.session.commit()

fedbadges/rules.py

@@ -246,6 +246,23 @@ def _get_candidates(self, msg: Message, tahrir: TahrirDatabase):
 
         return candidates
 
+    def _get_current_value(self, candidate: str, previous_count_fn, tahrir: TahrirDatabase):
+        candidate_email = f"{candidate}@{self.config['email_domain']}"
+        # First: the database
+        messages_count = tahrir.get_current_value(self.badge_id, candidate_email)
+        # If not found, use the cache
+        if messages_count is None:
+            # When we drop this cache sometime in the future, remember to keep the
+            # distributed lock (dogpile.lock) when running previous_count_fn()
+            messages_count = get_cached_messages_count(self.badge_id, candidate, previous_count_fn)
+        else:
+            # Found in DB! Add one (the current message)
+            messages_count += 1
+        # Store the value in the DB for next time
+        tahrir.set_current_value(self.badge_id, candidate_email, messages_count)
+        tahrir.session.commit()
+        return messages_count
+
     def matches(self, msg: Message, tahrir: TahrirDatabase):
         # First, do a lightweight check to see if the msg matches a pattern.
         if not self.trigger.matches(msg):
@@ -273,9 +290,7 @@ def matches(self, msg: Message, tahrir: TahrirDatabase):
         try:
             awardees = set()
             for candidate in candidates:
-                messages_count = get_cached_messages_count(
-                    self.badge_id, candidate, previous_count_fn
-                )
+                messages_count = self._get_current_value(candidate, previous_count_fn, tahrir)
                 log.debug(
                     "Rule %s: message count for %s is %s", self.badge_id, candidate, messages_count
                 )

tests/conftest.py

@@ -26,6 +26,7 @@ def fm_config(tmp_path):
     test_config = dict(
         badges_repo="tests/test_badges",
         database_uri=f"sqlite:///{tmp_path.as_posix()}/badges.db",
+        email_domain="example.com",
         datanommer_db_uri=f"sqlite:///{tmp_path.as_posix()}/datanommer.db",
         datagrepper_url="https://example.com/datagrepper",
         distgit_hostname="src.example.com",
@@ -41,7 +42,7 @@ def fm_config(tmp_path):
         ),
     )
     with patch.dict(conf["consumer_config"], test_config):
-        yield
+        yield test_config
 
 
 @pytest.fixture()

tests/test_rule_matching.py

@@ -44,7 +44,7 @@ def test_full_specification():
         )
 
 
-def test_full_simple_success(fasproxy, tahrir_client, user_exists):
+def test_full_simple_success(fasproxy, tahrir_client, fm_config, user_exists):
     """A simple integration test for messages with zero users"""
     rule = fedbadges.rules.BadgeRule(
         dict(
@@ -62,7 +62,7 @@ def test_full_simple_success(fasproxy, tahrir_client, user_exists):
             ),
         ),
         1,
-        None,
+        fm_config,
         fasproxy,
     )
     rule.setup(tahrir_client)
@@ -74,7 +74,7 @@ def test_full_simple_success(fasproxy, tahrir_client, user_exists):
         assert rule.matches(msg, tahrir_client) == {"lmacken"}
 
 
-def test_full_simple_match_almost_succeed(fasproxy, tahrir_client):
+def test_full_simple_match_almost_succeed(fasproxy, tahrir_client, fm_config):
     """A simple integration test for messages with zero users"""
     rule = fedbadges.rules.BadgeRule(
         dict(
@@ -92,7 +92,7 @@ def test_full_simple_match_almost_succeed(fasproxy, tahrir_client):
             ),
         ),
         1,
-        None,
+        fm_config,
         fasproxy,
     )
     rule.setup(tahrir_client)
@@ -107,7 +107,7 @@ def test_full_simple_match_almost_succeed(fasproxy, tahrir_client):
         assert rule.matches(msg, tahrir_client) == set()
 
 
-def test_yaml_specified_awardee_success(fasproxy, tahrir_client, user_exists):
+def test_yaml_specified_awardee_success(fasproxy, tahrir_client, fm_config, user_exists):
     """Test that we can override msg.usernames."""
     # For instance, fas.group.member.remove contains two users,
     # the one being removed from a group, and the one doing the removing.
@@ -134,7 +134,7 @@ def test_yaml_specified_awardee_success(fasproxy, tahrir_client, user_exists):
             recipient="[message.body['agent']['username'], message.body['user']['username']]",
         ),
         1,
-        None,
+        fm_config,
         fasproxy,
     )
     rule.setup(tahrir_client)
@@ -153,7 +153,7 @@ def test_yaml_specified_awardee_success(fasproxy, tahrir_client, user_exists):
         assert rule.matches(msg, tahrir_client) == {"toshio", "ralph"}
 
 
-def test_yaml_specified_awardee_failure(fasproxy, tahrir_client, user_exists):
+def test_yaml_specified_awardee_failure(fasproxy, tahrir_client, fm_config, user_exists):
     """Test that when we don't override msg.usernames, we get 2 awardees."""
     rule = fedbadges.rules.BadgeRule(
         dict(
@@ -171,7 +171,7 @@ def test_yaml_specified_awardee_failure(fasproxy, tahrir_client, user_exists):
             ),
         ),
         1,
-        None,
+        fm_config,
         fasproxy,
     )
     rule.setup(tahrir_client)
@@ -192,7 +192,7 @@ def test_yaml_specified_awardee_failure(fasproxy, tahrir_client, user_exists):
         assert rule.matches(msg, tahrir_client) == {"toshio"}
 
 
-def test_against_duplicates(fasproxy, tahrir_client, user_exists):
+def test_against_duplicates(fasproxy, tahrir_client, fm_config, user_exists):
     """Test that matching fails if user already has the badge."""
 
     rule = fedbadges.rules.BadgeRule(
@@ -212,7 +212,7 @@ def test_against_duplicates(fasproxy, tahrir_client, user_exists):
             recipient="message.usernames",
         ),
         1,
-        None,
+        fm_config,
         fasproxy,
     )
     rule.setup(tahrir_client)
@@ -239,7 +239,7 @@ def test_against_duplicates(fasproxy, tahrir_client, user_exists):
         assert rule.matches(msg, tahrir_client) == set(["ralph"])
 
 
-def test_github_awardee(fasproxy, tahrir_client, fasjson_client, user_exists):
+def test_github_awardee(fasproxy, tahrir_client, fasjson_client, fm_config, user_exists):
     """Conversion from GitHub URI to FAS users"""
     rule = fedbadges.rules.BadgeRule(
         dict(
@@ -259,7 +259,7 @@ def test_github_awardee(fasproxy, tahrir_client, fasjson_client, user_exists):
             recipient_github2fas="Yes",
         ),
         1,
-        None,
+        fm_config,
         fasproxy,
     )
     rule.setup(tahrir_client)
@@ -281,7 +281,7 @@ def test_github_awardee(fasproxy, tahrir_client, fasjson_client, user_exists):
         )
 
 
-def test_krb_awardee(fasproxy, tahrir_client, user_exists):
+def test_krb_awardee(fasproxy, tahrir_client, fm_config, user_exists):
     """Conversion from Kerberos user to FAS users"""
     rule = fedbadges.rules.BadgeRule(
         dict(
@@ -301,7 +301,7 @@ def test_krb_awardee(fasproxy, tahrir_client, user_exists):
             recipient_krb2fas="Yes",
         ),
         1,
-        None,
+        fm_config,
         fasproxy,
     )
     rule.setup(tahrir_client)