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

(Add) Use shouldSend for notifications #4374

Merged

Conversation

Obi-Wana
Copy link
Contributor

Use shouldSend for notifications.

Copy link

what-the-diff bot commented Dec 11, 2024

PR Summary

  • Improved Notification Decision Making in TorrentHelper.php
    Adjusted method to make a smarter choice on whether to send notifications to followers.
  • Refined NewUpload notification class
    • Tweaked a method to decide whether to use the database as the notification channel, depending on certain rules.
    • Added a new method to handle the decision if a notification should be sent by checking different parameters such as relationship between sender and receiver, notification blocking options, and specific notification settings.

@Obi-Wana Obi-Wana force-pushed the use-shouldSend-for-notifications branch 27 times, most recently from cc99f55 to 8230899 Compare December 18, 2024 14:23
@Obi-Wana Obi-Wana force-pushed the use-shouldSend-for-notifications branch 2 times, most recently from f9a0b6f to 0a99fee Compare December 20, 2024 13:31
@Obi-Wana Obi-Wana force-pushed the use-shouldSend-for-notifications branch 4 times, most recently from 5854afc to 93366d5 Compare February 1, 2025 14:22
@Obi-Wana Obi-Wana requested a review from Roardom February 1, 2025 14:30
@Obi-Wana
Copy link
Contributor Author

I think it improves the code quality if you separate each check into its own if statement instead of combining the and's and the or's.

E.g. this:

        if ($this->bounty->user_id === $notifiable->id ||
            $notifiable->notification?->block_notifications == 1) {
            return false;
        }

would become this:

        if ($this->bounty->user_id === $notifiable->id) {
            return false;
        }

        if ($notifiable->notification?->block_notifications == 1) {
            return false;
        }

@Obi-Wana Obi-Wana force-pushed the use-shouldSend-for-notifications branch 3 times, most recently from da81514 to b7cf0ee Compare February 27, 2025 13:38
@Obi-Wana Obi-Wana force-pushed the use-shouldSend-for-notifications branch from b7cf0ee to d0d1877 Compare February 27, 2025 13:39
Copy link
Collaborator

@Roardom Roardom left a comment

Choose a reason for hiding this comment

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

Thank you!

@HDVinnie HDVinnie merged commit 62f5560 into HDInnovations:8.x.x Feb 28, 2025
6 checks passed
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