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 Multiple Notifiables #95

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aneeskhan47
Copy link

@aneeskhan47 aneeskhan47 commented Oct 30, 2021

Hi,

i was raised an issue #94 using my other account @aneesdev.. i made this pr to be able to create many scheduled notifications just like laravel Notification facade. so there is createMany method for multiple notifiables.

    $enrolledUsers = User::whereRelation('enrolledCourses', 'course_id', $course_id)->get();

    ScheduledNotification::createMany(
        $enrolledUsers, // Target
        new NewLessonNotification($lesson), // Notification
        Carbon::createFromFormat('d/m/Y h:i a', $request->scheduled_at) // Send At
   ); 

@aneeskhan47
Copy link
Author

@thomasjohnkane

@aneeskhan47
Copy link
Author

@atymic

@aneesdev
Copy link

aneesdev commented Apr 2, 2022

@thomasjohnkane package dead?

@aneeskhan47
Copy link
Author

@atymic will this be merged ever or not?

@atymic
Copy link
Collaborator

atymic commented Mar 20, 2024

Could you bring this up to date and add some tests to ensure no BC breaks please?

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes are missing coverage. Please review.

Project coverage is 87.66%. Comparing base (7efbf33) to head (57774df).

❗ Current head 57774df differs from pull request most recent head 8090880. Consider uploading reports for the commit 8090880 to get more accurate results

Files Patch % Lines
src/ScheduledNotification.php 0.00% 28 Missing ⚠️

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

Additional details and impacted files
@@             Coverage Diff              @@
##             master      #95      +/-   ##
============================================
- Coverage     96.42%   87.66%   -8.77%     
- Complexity       88       97       +9     
============================================
  Files            10       10              
  Lines           280      308      +28     
============================================
  Hits            270      270              
- Misses           10       38      +28     

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

@aneeskhan47
Copy link
Author

@atymic it's been 2 years 😅... but I updated the original PR description, there is a new method createMany which does the same as the create method on the ScheduledNotification class but for multiple notifiables.

as for the Breaking change, I think there will be none as this just introduces the new createMany method and does not modify any existing create method, plus the existing tests seems to be passing 😃

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.

None yet

4 participants