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

Feature/disable scheduling #73

Merged

Conversation

gborcherds
Copy link
Contributor

Feel free to not accept this, but I have need of being able to turn off the automatic scheduling. I have shared models across multiple repos and I only need the scheduling to run in one repo.

@codecov-io
Copy link

codecov-io commented Mar 1, 2021

Codecov Report

Merging #73 (96bafc5) into master (512fb64) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #73      +/-   ##
============================================
- Coverage     97.26%   97.24%   -0.03%     
- Complexity       80       81       +1     
============================================
  Files            10       10              
  Lines           256      254       -2     
============================================
- Hits            249      247       -2     
  Misses            7        7              
Impacted Files Coverage Δ Complexity Δ
src/ServiceProvider.php 90.90% <100.00%> (+0.43%) 5.00 <0.00> (+1.00)
src/Serializer.php 84.61% <0.00%> (-1.10%) 7.00% <0.00%> (ø%)
src/Events/NotificationSent.php 100.00% <0.00%> (ø) 1.00% <0.00%> (ø%)
src/Events/NotificationInterrupted.php 100.00% <0.00%> (ø) 1.00% <0.00%> (ø%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 512fb64...96bafc5. Read the comment docs.

src/ServiceProvider.php Outdated Show resolved Hide resolved
@gborcherds
Copy link
Contributor Author

Thanks, I've made those changes.

config/snooze.php Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@atymic atymic left a comment

Choose a reason for hiding this comment

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

See comments, looks good though :)

@gborcherds
Copy link
Contributor Author

Your description is much clearer than mine, but is exactly what I wanted. Appreciate you doing this.

@gborcherds
Copy link
Contributor Author

Also appreciate the package, our app uses this to send over 200 scheduled notifications per day. Feel free to merge in when you can.

@atymic
Copy link
Collaborator

atymic commented Mar 2, 2021

(you didn't tick the box to allow me to change your PR, so you'll need to commit those changes :))

gborcherds and others added 2 commits March 2, 2021 16:12
Co-authored-by: atymic <atymicq@gmail.com>
@OrkhanAlikhanov
Copy link

I think this is related to my issue #78
To work around this, I wanted to disable scheduling which is not possible either :/

@OrkhanAlikhanov
Copy link

@gborcherds How do you update vendor files? Using something like cweagans/composer-patches ?

@atymic atymic merged commit b938359 into thomasjohnkane:master Jun 28, 2021
@ricuss
Copy link
Contributor

ricuss commented Aug 16, 2021

@atymic @gborcherds This PR introduced a situation where when snooze is disabled, but a pruneAge is set, then the package is in an error state.

The culprit is this bit of code in the provider:

 if (! config('snooze.disabled')) {
                $frequency = config('snooze.sendFrequency', 'everyMinute');
                $schedule = $this->app->make(Schedule::class);
                $schedule->command('snooze:send')->{$frequency}();
            }

            if (config('snooze.pruneAge') !== null) {
                $schedule->command('snooze:prune')->daily();
            }

The $schedule variable isn't available if snooze is disabled, but it will try to use it when pruneAge is not null.

I've made a pr to address this here #84

atymic added a commit to atymic/laravel-snooze that referenced this pull request Mar 20, 2024
Co-authored-by: atymic <atymicq@gmail.com>
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

5 participants