Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Matrix login URL updated to accomodate newer API #970

Merged
merged 3 commits into from
Oct 15, 2023

Conversation

caronc
Copy link
Owner

@caronc caronc commented Oct 6, 2023

Description:

Related issue (if applicable): #968

Basing fix on related issue and Matrix API Login Docs

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • No lint errors (use flake8)
  • 100% test coverage

Testing

Anyone can help test this source code as follows:

# Create a virtual environment to work in as follows:
python3 -m venv apprise

# Change into our new directory
cd apprise

# Activate our virtual environment
source bin/activate

# Install the branch
pip install git+https://github.com/caronc/apprise.git@968-matrix-api-v3-support

# Test out the changes with the following command:
apprise -t "Test Title" -b "Test Message" \
  "matrix://credentials"

@caronc
Copy link
Owner Author

caronc commented Oct 6, 2023

@corentincam, I would love your feedback on this to see if it helps/works for you taking your advice.

I'm a bit surprised because i added attachment support recently and I did not have authentication issues at the time using the Matrix build i had. Is this a very recent change?

@codecov-commenter
Copy link

codecov-commenter commented Oct 6, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Files Coverage Δ
apprise/plugins/NotifyMatrix.py 99.09% <93.75%> (-0.91%) ⬇️

... and 2 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

@corentincam
Copy link

Thanks for this quick fix! Authentication and Deauthentication seem fixed, but the message sending was still broken for me...

I could make it work with a few changes though:

diff --git a/apprise/plugins/NotifyMatrix.py b/apprise/plugins/NotifyMatrix.py
index eacda09..9d5758b 100644
--- a/apprise/plugins/NotifyMatrix.py
+++ b/apprise/plugins/NotifyMatrix.py
@@ -615,7 +615,7 @@ class NotifyMatrix(NotifyBase):
                 self.image_url(notify_type)
 
             # Build our path
-            path = '/rooms/{}/send/m.room.message'.format(
+            path = '/rooms/{}/send/m.room.message/0'.format(
                 NotifyMatrix.quote(room_id))
 
             if image_url:
@@ -671,7 +671,7 @@ class NotifyMatrix(NotifyBase):
                 })
 
             # Post our content
-            postokay, response = self._fetch(path, payload=payload)
+            postokay, response = self._fetch(path, payload=payload, method='PUT')
             if not postokay:
                 # Notify our user
                 self.logger.warning(
@@ -1124,7 +1124,8 @@ class NotifyMatrix(NotifyBase):
         response = {}
 
         # fetch function
-        fn = requests.post if method == 'POST' else requests.get
+        fn = requests.post if method == 'POST' else (
+            requests.put if method == 'PUT' else requests.get)
 
         # Define how many attempts we'll make if we get caught in a throttle
         # event

Those changes probably need some adjustments to fit your coding guidelines and allow retrocompatibility, but I can confirm they work with my conduit server v0.6.0

@caronc caronc force-pushed the 968-matrix-api-v3-support branch from 601b524 to c9ef17f Compare October 14, 2023 01:42
@caronc
Copy link
Owner Author

caronc commented Oct 14, 2023

I pushed your changes, but it seems that the --attachment support broke too :(

If you test the following, i can't seem to figure out where i'm going wrong with the payload, maybe you spot it?

# you can use the `test/var/*` contents available in this package
apprise -b "message" --attach test/var/apprise-test.jpeg \
   "matrix://credentials"

The good news however is your code works (you can post to the Matrix Server); good work on your part here!

I'd like to figure out the attachments though before closing this off for good.

@caronc
Copy link
Owner Author

caronc commented Oct 15, 2023

In this latest update ?v=2 runs it with the Version 2 API and Attachments work. This latest update just made it so Attachments are gracefully skipped over on default ?v=3

.

@caronc caronc merged commit 97c7af4 into master Oct 15, 2023
12 checks passed
@corentincam
Copy link

Sorry I couldn't reply earlier, indeed the attachment doesn't seem to work out-of-the-box. I made some changes and I feel it could be doable. Not sure when I'll be able to actually work on it but I might open a PR if I figure out how to do this.

Thanks for the great work anyways!

@caronc caronc deleted the 968-matrix-api-v3-support branch December 29, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants