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

Fix sent_at typo in fillable array #50

Merged
merged 3 commits into from
Sep 10, 2020
Merged

Conversation

azarzag
Copy link
Contributor

@azarzag azarzag commented May 4, 2020

No description provided.

@codecov-io
Copy link

codecov-io commented May 4, 2020

Codecov Report

Merging #50 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #50   +/-   ##
=========================================
  Coverage     97.89%   97.89%           
  Complexity       82       82           
=========================================
  Files            10       10           
  Lines           238      238           
=========================================
  Hits            233      233           
  Misses            5        5           
Impacted Files Coverage Δ Complexity Δ
src/Models/ScheduledNotification.php 100.00% <ø> (ø) 22.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 45946c6...f7c1bd8. Read the comment docs.

@atymic
Copy link
Collaborator

atymic commented May 5, 2020

@thomasjohnkane looks like travis is borked. I can't fix it as I don't have privs.

@azarzag
Copy link
Contributor Author

azarzag commented May 10, 2020

@atymic and @thomasjohnkane any idea when this PR will be merged?

Notification rescheduling is completely broken since the sent_at value is never reset.

@atymic
Copy link
Collaborator

atymic commented May 11, 2020

@thomasjohnkane can you fix Travis? I can't merge this without permissions (as the checks are not passing).

@@ -31,7 +31,7 @@ class ScheduledNotification extends Model
'notification_type',
'notification',
'send_at',
'sent',
'sent_at',
Copy link
Collaborator

Choose a reason for hiding this comment

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

while this should be corrected, we don't used mass assignment when rescheduling:
https://github.com/thomasjohnkane/snooze/blob/master/src/Models/ScheduledNotification.php#L147

So the cause of the bug must be elsewhere. Could you open an issue with more details?

@atymic
Copy link
Collaborator

atymic commented Jul 16, 2020

cc @thomasjohnkane

@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2020

Codecov Report

Merging #50 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #50   +/-   ##
=========================================
  Coverage     97.99%   97.99%           
  Complexity       77       77           
=========================================
  Files            10       10           
  Lines           249      249           
=========================================
  Hits            244      244           
  Misses            5        5           
Impacted Files Coverage Δ Complexity Δ
src/Models/ScheduledNotification.php 100.00% <ø> (ø) 20.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 568e530...0112b5c. Read the comment docs.

@atymic atymic merged commit 3d2f890 into thomasjohnkane:master Sep 10, 2020
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

4 participants