Skip to content

Refactor + tests for Apprise custom integration#3057

Merged
dgtlmoon merged 20 commits into
dgtlmoon:masterfrom
xLinkOut:refactor+tests/apprise-rebased
Mar 27, 2025
Merged

Refactor + tests for Apprise custom integration#3057
dgtlmoon merged 20 commits into
dgtlmoon:masterfrom
xLinkOut:refactor+tests/apprise-rebased

Conversation

@xLinkOut
Copy link
Copy Markdown
Contributor

This PR aims to refactor the Apprise custom integration to improve its readability, maintainability, and code coverage by introducing new tests and adding several features. The changelog follow the Keep a Changelog format.

Added

  • Extended HTTP method support for Apprise custom handlers:
    • Added support for PATCH and HEAD methods
  • Comprehensive test suite for the Apprise custom integration

Fixed

  • Corrected a typo in authentication credentials handling, where the password was being ignored, and the username was set twice instead
  • Improved handling of parameter processing
  • In ValidateAppRiseServers, Apprise custom handlers are now correctly loaded

Changed

  • Refactored code structure:
    • Moved all Apprise-related code to the apprise_plugin folder
    • Used appropriate file names instead of putting code in __init__.py
  • Apprise assets:
    • Saved assets into constants for easy reuse
    • Directly pass assets into the AppriseAsset constructor
  • Custom handler implementation:
    • Following Apprise documentation, handlers now return a boolean (True/False) instead of raising exceptions
    • Use requests.request() instead of dynamically retrieving method aliases
    • Split custom handler function into other small fuctions for improved testability and maintainability

Finally, I've manually tested some custom URLs notification:

get://localhost:8080
get://localhost:8080?user_id=123&action=list
get://localhost:8080?search_term=hello%20world&category=test%20category
get://username:password@localhost:8080
get://localhost:8080?+content-type=application/json&+x-api-key=secret-key
get://admin:secretpass@localhost:8080?+content-type=application/xml&+x-request-id=unique-request
post://localhost:8080?+content-type=application/json
post://user:pass@localhost:8080?+content-type=application/json&+x-correlation-id=123456
post://localhost:8080?param1=value1&param2=value2&+content-type=application/json&+x-custom-header=special-value
post://localhost:8080?+content-type=application/x-www-form-urlencoded&username=testuser&password=testpass

xLinkOut added 20 commits March 25, 2025 19:34
… them directly into the constructor of AppriseAsset

Added some basic tests to validate those changes
due to a type, the "user" component was inserted twice into the auth tuple
…thentication process

chg(apprise): create headers and params dict in place
chore(apprise): more readable schema substitution

chore(apprise): renaiming
…ndlers must not raise exception but instead return a boolean based on the outcome of the sending process
@dgtlmoon
Copy link
Copy Markdown
Owner

"and adding several features"

I see PATCH and HEAD, what else was changed/added?

@dgtlmoon
Copy link
Copy Markdown
Owner

And can I ask, why? whats the background here? how did you come to this point?

)


@pytest.mark.parametrize(
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain this to me? how does this work?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pytest.mark.parametrize allows you to reuse the same test code with different parameters. For example, test_get_headers tests header parsing starting from different URLs, for which the expected result is known.
It's like doing a for loop on a single test, but in a much cleaner and more concise way (standard for pytest).

@xLinkOut
Copy link
Copy Markdown
Contributor Author

I'm using this custom handler to send callbacks to my other services with the differences detected. So I took a look at the code to better understand how it works.
As I mentioned in the other Ruff PR, I'm happy to contribute to this project with what I can. I noticed that the test coverage for this particular component was low (calculated with the coverage package), so I started writing a test suite specifically for the custom handler.
And from there, everything else followed - the tests allowed me to identify and fix bugs, and make the technical improvements I mentioned (I would say that, objectively, the code is now more maintainable in the long term, and certainly more readable and testable).
In addition, I added support for the two missing HTTP methods (PATCH in particular I find useful for interacting with other RESTful services, where you modify an existing resource). The "why" you're looking for is this, and also because this component seemed sufficiently isolated from the rest, and a good starting point to make changes to the project's code.

@dgtlmoon
Copy link
Copy Markdown
Owner

OK super nice, thanks so much! yes its better to move that code to its own file, PATCH and HEAD are also super handy!

@dgtlmoon
Copy link
Copy Markdown
Owner

Just one thing

Corrected a typo in authentication credentials handling, where the password was being ignored, and the username was set twice instead

Was there a test for this? and it wasnt caught or?

grazie!

@xLinkOut
Copy link
Copy Markdown
Contributor Author

xLinkOut commented Mar 27, 2025

I didn't find a specific test for that case, I think there isn't a process_notification test that has, as destination url, an url that includes the authentication parameters. The bug (on master) is in apprise_plugin/__init__.py:52

if results.get('user') and results.get('password'):
    auth = (unquote_plus(results.get('user')), unquote_plus(results.get('user')))

Prego!

@dgtlmoon
Copy link
Copy Markdown
Owner

If you could kindly add a test for that situation then I think this code is ready to merge :)

@xLinkOut
Copy link
Copy Markdown
Contributor Author

Sure, it's already included in test_apprise_custom_api_call.py:test_get_auth, line 26.

Four different scenarios:

  • no auth
  • auth with only username
  • auth with both
  • auth with both username and password, but encoded

If you know any other scenario(s) to test, let me know!

@dgtlmoon dgtlmoon merged commit ea9ba3b into dgtlmoon:master Mar 27, 2025
@xLinkOut xLinkOut deleted the refactor+tests/apprise-rebased branch March 28, 2025 11:46
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.

2 participants