From 813a96d358943f0af52789389741ce8ea0730623 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Bompard?= Date: Thu, 25 Jul 2024 08:25:24 +0200 Subject: [PATCH] Read and store current values in the DB as well MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Aurélien Bompard --- devel/ansible/roles/dev/files/config.toml | 3 +++ fedbadges.toml.example | 3 +++ fedbadges/cached.py | 7 ------ fedbadges/consumer.py | 6 ++++- fedbadges/rules.py | 21 ++++++++++++++--- poetry.lock | 7 +++--- tests/conftest.py | 3 ++- tests/test_rule_matching.py | 28 +++++++++++------------ 8 files changed, 48 insertions(+), 30 deletions(-) diff --git a/devel/ansible/roles/dev/files/config.toml b/devel/ansible/roles/dev/files/config.toml index d9b2ac3..8b7f268 100644 --- a/devel/ansible/roles/dev/files/config.toml +++ b/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" diff --git a/fedbadges.toml.example b/fedbadges.toml.example index 7007195..ced3d3c 100644 --- a/fedbadges.toml.example +++ b/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" diff --git a/fedbadges/cached.py b/fedbadges/cached.py index b6aded2..89a0dcf 100644 --- a/fedbadges/cached.py +++ b/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( diff --git a/fedbadges/consumer.py b/fedbadges/consumer.py index 9bec9db..f7006c9 100644 --- a/fedbadges/consumer.py +++ b/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() diff --git a/fedbadges/rules.py b/fedbadges/rules.py index 979404e..28840c5 100644 --- a/fedbadges/rules.py +++ b/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 ) diff --git a/poetry.lock b/poetry.lock index 0826257..e0f5778 100644 --- a/poetry.lock +++ b/poetry.lock @@ -2193,7 +2193,6 @@ files = [ {file = "PyYAML-6.0.1-cp311-cp311-win_amd64.whl", hash = "sha256:bf07ee2fef7014951eeb99f56f39c9bb4af143d8aa3c21b1677805985307da34"}, {file = "PyYAML-6.0.1-cp312-cp312-macosx_10_9_x86_64.whl", hash = "sha256:855fb52b0dc35af121542a76b9a84f8d1cd886ea97c84703eaa6d88e37a2ad28"}, {file = "PyYAML-6.0.1-cp312-cp312-macosx_11_0_arm64.whl", hash = "sha256:40df9b996c2b73138957fe23a16a4f0ba614f4c0efce1e9406a184b6d07fa3a9"}, - {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_aarch64.manylinux2014_aarch64.whl", hash = "sha256:a08c6f0fe150303c1c6b71ebcd7213c2858041a7e01975da3a99aed1e7a378ef"}, {file = "PyYAML-6.0.1-cp312-cp312-manylinux_2_17_x86_64.manylinux2014_x86_64.whl", hash = "sha256:6c22bec3fbe2524cde73d7ada88f6566758a8f7227bfbf93a408a9d86bcc12a0"}, {file = "PyYAML-6.0.1-cp312-cp312-musllinux_1_1_x86_64.whl", hash = "sha256:8d4e9c88387b0f5c7d5f281e55304de64cf7f9c0021a3525bd3b1c542da3b0e4"}, {file = "PyYAML-6.0.1-cp312-cp312-win32.whl", hash = "sha256:d483d2cdf104e7c9fa60c544d92981f12ad66a457afae824d146093b8c294c54"}, @@ -2945,13 +2944,13 @@ typing-extensions = "*" [[package]] name = "tahrir-api" -version = "1.2.0" +version = "1.3.0" description = "An API for interacting with the Tahrir database" optional = false python-versions = "<4.0.0,>=3.9.0" files = [ - {file = "tahrir_api-1.2.0-py3-none-any.whl", hash = "sha256:db258cfaa4ddaf40d2d29d89ab02b88124b744590b372963fab0f21071a69fa4"}, - {file = "tahrir_api-1.2.0.tar.gz", hash = "sha256:b4c340939b7895ecd1d3748d55542330dc2110e51e72af84907df4f5893633eb"}, + {file = "tahrir_api-1.3.0-py3-none-any.whl", hash = "sha256:6f2d9cd80873e20accbe0f36db1924d88ee7e3a8fb57f8ed10b251a3b7e9a260"}, + {file = "tahrir_api-1.3.0.tar.gz", hash = "sha256:884abb862884244a42e2c12ec80d6dcfb1a90ee3e2688d0f64114d78c1727c74"}, ] [package.dependencies] diff --git a/tests/conftest.py b/tests/conftest.py index e11a395..80f986c 100644 --- a/tests/conftest.py +++ b/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() diff --git a/tests/test_rule_matching.py b/tests/test_rule_matching.py index 7ab8f8d..21150bc 100644 --- a/tests/test_rule_matching.py +++ b/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)