Add Weblate Fedora Messaging schema #364

harriebird · 2024-10-08T10:06:19Z

Proposed changes

This adds a new schema for Weblate Fedora Messaging.
This solves issue fedora_messaging#268.

Checklist

Lint and unit tests pass locally with my changes.
I have added tests that prove my fix is effective or that my feature works.
I have added documentation to describe my feature.
I have squashed my commits into logic units.
I have described the changes in the commit messages.

Other information

codecov · 2024-10-08T10:07:04Z

Codecov Report

Attention: Patch coverage is 97.72727% with 3 lines in your changes missing coverage. Please review.

Project coverage is 98.25%. Comparing base (45163d1) to head (1c76d96).
Report is 108 commits behind head on main.

Files with missing lines	Patch %	Lines
weblate_schemas/messages.py	95.31%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##              main     #364      +/-   ##
===========================================
- Coverage   100.00%   98.25%   -1.75%     
===========================================
  Files            3        5       +2     
  Lines           43      172     +129     
===========================================
+ Hits            43      169     +126     
- Misses           0        3       +3

Flag	Coverage Δ
unittests	`98.25% <97.72%> (-1.75%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

nijel · 2024-10-08T14:06:53Z

Looks good for the body schema, now we need to wrap it in Python classes (see https://fedora-messaging.readthedocs.io/en/stable/user-guide/messages.html). Maybe header schema will be needed as well (I didn't investigate that).

nijel

Can we add some kind of testing that the message classes work on the expected payload?

nijel · 2024-10-09T14:07:43Z

weblate_schemas/message.py

+        return "https://weblate.org/static/weblate-128.png"
+
+
+class WeblateMessage(BaseMessage):


Probably should include versioning to allow future changes:

Suggested change

class WeblateMessage(BaseMessage):

class WeblateV1Message(BaseMessage):

You don't need to have it for the initial message schema, but will need that for any updated message schema. The first one could be WeblateMessage, but the changed one would need to be WeblateV2Message. Still a good idea to have the versioning in from start.

Keep in mind that you only need to do that if you are removing something from the message schema, adding things doesn't break anything.

nijel · 2024-10-09T14:08:24Z

weblate_schemas/message.py

+        """Initialize the WeblateMessage class with loading of the body_schema."""
+        super().__init__(**kwargs)
+        self.body_schema = load_schema("weblate-messaging.schema.json")
+


At least summary should also be present, based on https://pagure.io/fedora-infrastructure/issue/8291#comment-857387.

I would also recommend to add properties that are used by FMN as usernames and packages. See https://fedora-messaging.readthedocs.io/en/stable/user-guide/messages.html#example-schema

Also each Fedora application should have app_name property.

Zlopez

Few comments for the schema

Zlopez · 2024-10-10T09:19:19Z

weblate_schemas/message.py

+        return "https://weblate.org/static/weblate-128.png"
+
+
+class WeblateMessage(BaseMessage):


You don't need to have it for the initial message schema, but will need that for any updated message schema. The first one could be WeblateMessage, but the changed one would need to be WeblateV2Message. Still a good idea to have the versioning in from start.

Keep in mind that you only need to do that if you are removing something from the message schema, adding things doesn't break anything.

Zlopez · 2024-10-10T09:25:18Z

weblate_schemas/message.py

+        """Initialize the WeblateMessage class with loading of the body_schema."""
+        super().__init__(**kwargs)
+        self.body_schema = load_schema("weblate-messaging.schema.json")
+


I would also recommend to add properties that are used by FMN as usernames and packages. See https://fedora-messaging.readthedocs.io/en/stable/user-guide/messages.html#example-schema

Also each Fedora application should have app_name property.

harriebird · 2024-10-13T10:22:33Z

Hi everyone, thank you for your comments. The latest commit contains the following changes:

Renamed the message class to WeblateV1Message
Added properties summary and usernames into the WeblateV1Message class
Added entry point
Replaced jsonschema with jsonschema[format] to extend format validation support
Added new tests

nijel · 2024-10-14T09:29:58Z

requirements.txt

@@ -1,4 +1,5 @@
+fedora-messaging


This should be an optional dependency, there is no need to require it for somebody wanting other schemas.

nijel · 2024-10-14T09:30:26Z

requirements.txt

 fqdn
-jsonschema
+jsonschema[format]


Perhaps this belongs to requirements-test.txt?

nijel · 2024-10-14T09:32:03Z

setup.py

@@ -11,4 +11,13 @@
 with open("requirements.txt") as handle:
    REQUIRES = list(handle.read().splitlines())

-setup(install_requires=REQUIRES)
+setup(
+    name="weblate_schemas",


Already present in setup.cfg.

nijel · 2024-10-14T09:32:05Z

setup.py

+            "base.message=weblate_schemas.messages:BaseMessage",
+            "weblate.message=weblate_schemas.messages:WeblateV1Message",
+        ]
+    },


This should go to setup.cfg.

abompard · 2024-10-14T11:42:11Z

setup.py

+    name="weblate_schemas",
+    entry_points={
+        "fedora.messages": [
+            "base.message=weblate_schemas.messages:BaseMessage",


Please prefix the key here with weblate.. The base.message key is already used by Fedora Messaging

abompard · 2024-10-14T11:42:52Z

setup.py

+    entry_points={
+        "fedora.messages": [
+            "base.message=weblate_schemas.messages:BaseMessage",
+            "weblate.message=weblate_schemas.messages:WeblateV1Message",


I recommend suffixing this key with .v1, as it is the version 1 of the schema.

abompard · 2024-10-14T11:43:40Z

weblate_schemas/messages.py

+
+    def __init__(self, **kwargs) -> None:
+        """Initialize BaseMessage class."""
+        super().__init__(**kwargs)


This block is not necessary, no need to redefine __init__ if you're just calling the superclass.

abompard · 2024-10-14T11:45:01Z

weblate_schemas/messages.py

+        return self.body["user"]
+
+    @property
+    def id(self) -> int:


The id property is reserved by Fedora Messaging. You can use something like change_id if you want.

abompard · 2024-10-14T11:46:48Z

weblate_schemas/messages.py

+    def usernames(self):
+        """Return the usernames involved."""
+        users = {self.body["author"], self.body["user"]}
+        return list(users)


It would make testing easier if the list is sorted alphabetically, but this is just a non-blocking suggestion.

abompard · 2024-10-14T11:48:17Z

weblate_schemas/messages.py

+
+    def __str__(self) -> str:
+        """Return a human-readable representation of the message."""
+        return f"{self.action} {self.timestamp}"


FYI, the string representation of the message is what is going to be in the email body when people ask to be notified of these messages in FMN. You may want to put a little more text there, but it's just a non-blocking suggestion.

@harriebird Can we have this covered by the tests? It would be annoying if it would break.

@abompard action is a human-readable description of the action, so that looks okay. But I'm not sure whether to include the timestamp here.

Yeah you can put the timestamp there, it's really up to you. I would set something more human readable though, or just make it return self.summary if you can't or don't want to have a longer text.

weblate_schemas/messages.py

…for message classes

harriebird · 2024-10-14T16:52:48Z

Hi everyone, I just updated the code based on your comments. Below are the changes made:

Reverted changes in setup.py and moved the entry_points declaration to setup.cfg
Transferred fedora-messaging and jsonschema[format] to requirements-test.txt
Updated the summary property implementation
Updated the usernames property implementation
Changed the id property into change_id
Used get method in accessing the body dictionary
Added pip install -e . command in the dependency install of test.yml workflow

abompard · 2024-10-14T21:43:54Z

weblate_schemas/messages.py

+    @property
+    def agent_name(self) -> str:
+        """The username who cause the action."""
+        return self.body.get("user", "")


If there is no agent name, it should be None, not an empty string

abompard · 2024-10-14T21:46:24Z

weblate_schemas/messages.py

+    @property
+    def change_id(self) -> int:
+        """Return the change ID."""
+        return self.body.get("change_id")


According to the JSON schema, change_id is required, you can be sure it'll always be there, so you can use self.body["change_id"]. (just a suggestion, get() will work too)

abompard · 2024-10-14T21:47:12Z

weblate_schemas/messages.py

+    @property
+    def action(self) -> str:
+        """Return the change verbose name."""
+        return self.body.get("action")


According to the JSON schema, action is required, you can be sure it'll always be there, so you can use self.body["action"].

abompard · 2024-10-14T21:47:34Z

weblate_schemas/messages.py

+    @property
+    def timestamp(self) -> datetime:
+        """Return the timestamp of the change."""
+        return self.body.get("timestamp")


According to the JSON schema, timestamp is required, you can be sure it'll always be there, so you can use self.body["timestamp"].

abompard · 2024-10-14T21:48:50Z

weblate_schemas/messages.py

+    def summary(self) -> str:
+        """Return the message summary."""
+        return "{} event{} occurred in {}.".format(
+            self.body.get("action"),


Same remark here. By the way, you can use self.action: if you end up moving the data in the dictionary, you'll only have to update it in one place.

abompard · 2024-10-14T21:49:02Z

weblate_schemas/messages.py

+            " done by {}".format(self.body.get("user"))
+            if self.body.get("user", None)
+            else "",
+            self.body.get("timestamp"),


Same remark here

abompard · 2024-10-14T21:51:48Z

weblate_schemas/messages.py

+        """Return the usernames involved."""
+        users = []
+        users += [self.author] if self.author else []
+        users += [self.user] if self.user else []


Or something like:

users = list(sorted({name for name in (self.author, self.user) if name}))

(just a suggestion, not a blocker)

It is useful to convert to a list in the end, because sorted() returns an iterator, not an actual list.

abompard

LGTM! 👍

weblate_schemas/messages.py

nijel · 2024-11-01T14:01:42Z

weblate_schemas/messages.py

+
+    def __str__(self) -> str:
+        """Return a human-readable representation of the message."""
+        return f"{self.action} {self.timestamp}"


@harriebird Can we have this covered by the tests? It would be annoying if it would break.

@abompard action is a human-readable description of the action, so that looks okay. But I'm not sure whether to include the timestamp here.

for more information, see https://pre-commit.ci

weblate_schemas/messages.py

nijel · 2024-11-06T09:12:00Z

I've scheduled this for merge because I want to release this package soon anyway. In case we hit some issues later, those can be fixed in the next release.

harriebird mentioned this pull request Oct 8, 2024

Message schema WeblateOrg/fedora_messaging#268

Open

harriebird requested a review from nijel October 8, 2024 10:19

nijel reviewed Oct 9, 2024

View reviewed changes

Zlopez reviewed Oct 10, 2024

View reviewed changes

harriebird force-pushed the feature/add-weblate-fedora-messaging-schema branch from f045c55 to 5b03512 Compare October 13, 2024 10:12

nijel reviewed Oct 14, 2024

View reviewed changes

abompard suggested changes Oct 14, 2024

View reviewed changes

nijel reviewed Oct 14, 2024

View reviewed changes

weblate_schemas/messages.py Show resolved Hide resolved

harriebird added 4 commits October 15, 2024 00:14

Added Weblate Fedora Messaging schema file

5ba7767

Added test and update types of some fields

d88f091

Added a Message class for Weblate Fedora Messaging schema

6cd004f

Added summary and usernames field in WeblateV1Message and added test …

8126012

…for message classes

harriebird force-pushed the feature/add-weblate-fedora-messaging-schema branch 4 times, most recently from c19c1d5 to d31bf35 Compare October 14, 2024 16:39

Updated implementation and requirements listing

b0b129e

harriebird force-pushed the feature/add-weblate-fedora-messaging-schema branch from d31bf35 to b0b129e Compare October 14, 2024 16:48

abompard suggested changes Oct 14, 2024

View reviewed changes

Refined code implementation in some functions

dfd4a2f

abompard approved these changes Oct 17, 2024

View reviewed changes

harriebird force-pushed the feature/add-weblate-fedora-messaging-schema branch 3 times, most recently from bd73b1a to c866110 Compare October 22, 2024 18:16

Replaced the format with fstring

5a629b9

harriebird force-pushed the feature/add-weblate-fedora-messaging-schema branch from c866110 to 5a629b9 Compare October 22, 2024 18:27

git-pear mentioned this pull request Oct 23, 2024

Integrate Fedora messaging WeblateOrg/weblate#10054

Open

3 tasks

nijel reviewed Nov 1, 2024

View reviewed changes

nijel and others added 3 commits November 6, 2024 10:07

Add missing import of future annotations

781436c

Remove not needed constructor

f0eed24

[pre-commit.ci] auto fixes from pre-commit.com hooks

4743623

for more information, see https://pre-commit.ci

nijel reviewed Nov 6, 2024

View reviewed changes

weblate_schemas/messages.py Outdated Show resolved Hide resolved

Do not use get to fetch required parameters

0679bac

nijel reviewed Nov 6, 2024

View reviewed changes

weblate_schemas/messages.py Outdated Show resolved Hide resolved

Use summary as __str__

1c76d96

nijel enabled auto-merge (squash) November 6, 2024 09:11

nijel self-assigned this Nov 6, 2024

nijel disabled auto-merge November 6, 2024 09:43

nijel merged commit 73b608e into WeblateOrg:main Nov 6, 2024
7 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Weblate Fedora Messaging schema #364

Add Weblate Fedora Messaging schema #364

harriebird commented Oct 8, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

nijel commented Oct 8, 2024

nijel left a comment

nijel Oct 9, 2024

Zlopez Oct 10, 2024

nijel Oct 9, 2024

Zlopez Oct 10, 2024

Zlopez left a comment

Zlopez Oct 10, 2024

Zlopez Oct 10, 2024

harriebird commented Oct 13, 2024

nijel Oct 14, 2024

nijel Oct 14, 2024

nijel Oct 14, 2024

nijel Oct 14, 2024

abompard Oct 14, 2024

abompard Oct 14, 2024

abompard Oct 14, 2024

abompard Oct 14, 2024

abompard Oct 14, 2024

abompard Oct 14, 2024

nijel Nov 1, 2024

abompard Nov 4, 2024

harriebird commented Oct 14, 2024 •

edited

Loading

abompard Oct 14, 2024

abompard Oct 14, 2024

abompard Oct 14, 2024

abompard Oct 14, 2024

abompard Oct 14, 2024

abompard Oct 14, 2024

abompard Oct 14, 2024

abompard left a comment

nijel Nov 1, 2024

nijel commented Nov 6, 2024

		return "https://weblate.org/static/weblate-128.png"


		class WeblateMessage(BaseMessage):

	class WeblateMessage(BaseMessage):
	class WeblateV1Message(BaseMessage):

Add Weblate Fedora Messaging schema #364

Add Weblate Fedora Messaging schema #364

Conversation

harriebird commented Oct 8, 2024

Proposed changes

Checklist

Other information

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

nijel commented Oct 8, 2024

nijel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zlopez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harriebird commented Oct 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harriebird commented Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abompard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nijel commented Nov 6, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

harriebird commented Oct 14, 2024 •

edited

Loading