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

Test notification using same code base #174

Closed
wants to merge 49 commits into from

Conversation

Emmo213
Copy link

@Emmo213 Emmo213 commented Aug 15, 2024

As mentioned in another pull request #167 the test notifications and regular notifications should use the same code base. These changes create a fake bird detection to allow the test notification to flow through the existing notification code. This will provide a better user experience because before it was confusing when a test notification worked but the actual notification didn't, or vice versa.

Emmo213 and others added 30 commits August 5, 2024 19:51
Base64 encode the Apprise title and body when submitting the form
Update Apprise version to 1.8.1
Update Apprise version to 1.8.1
@Emmo213
Copy link
Author

Emmo213 commented Sep 8, 2024

It's been almost a month with no discussion. Should I close this PR as unwanted?

@Nachtzuster
Copy link
Owner

I haven't had much time these last months, so anything that looks non-trivial and/or not-a-bug-fix gets pushed back.

If you're still interested I can give some pointers on how to improve this PR

@Emmo213
Copy link
Author

Emmo213 commented Sep 13, 2024

@Nachtzuster yes please. It's been working really well for me, but I'm open to suggestions as long as the end result is the same.

@Nachtzuster
Copy link
Owner

Rather that re-using scripts/birdnet_analysis.py, do it like scripts/config.php does it here:

$command = "sudo -u $user ".$home."/BirdNET-Pi/birdnet/bin/python3 ".$home."/BirdNET-Pi/scripts/species.py --threshold $threshold 2>&1";

Also, as mentioned before, no changes to unrelated files, please

@Emmo213
Copy link
Author

Emmo213 commented Sep 14, 2024

Your suggestion is to create a separate python file which invokes the existing notification logic?

@Nachtzuster
Copy link
Owner

correct

@Emmo213 Emmo213 marked this pull request as draft September 17, 2024 19:41
@Emmo213 Emmo213 marked this pull request as ready for review September 17, 2024 20:04
@Emmo213
Copy link
Author

Emmo213 commented Sep 17, 2024

correct

Moved the logic to its own file.

@Nachtzuster
Copy link
Owner

Interesting that you opted to wrap apprise() what is the reason you picked that one and not sendAppriseNotifications()

@Emmo213 Emmo213 closed this Oct 20, 2024
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