Skip to content

Add Multiple Notifiables#95

Open
aneeskhan47 wants to merge 2 commits intothomasjohnkane:masterfrom
aneeskhan47:feature/multiple-notifiables
Open

Add Multiple Notifiables#95
aneeskhan47 wants to merge 2 commits intothomasjohnkane:masterfrom
aneeskhan47:feature/multiple-notifiables

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.

4 participants