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

Cancel by target #20

Merged
merged 8 commits into from
Oct 24, 2019
Merged

Conversation

atymic
Copy link
Collaborator

@atymic atymic commented Oct 17, 2019

  • update DB schema to save target id/type
  • cancel notification by target (model)
  • cancel notification by target (anon)
  • find by target (id/type)
  • getters for target id/type

cc @thomasjohnkane

atymic and others added 2 commits October 17, 2019 12:40
Just re-reading this section, I think it makes sense to clarify when the method is run.

Also extended the example a bit & fixed some of the roadmap items :)
@codecov-io
Copy link

codecov-io commented Oct 24, 2019

Codecov Report

Merging #20 into master will increase coverage by 2.68%.
The diff coverage is 98.07%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #20      +/-   ##
============================================
+ Coverage     86.98%   89.67%   +2.68%     
- Complexity       63       74      +11     
============================================
  Files             9        9              
  Lines           169      213      +44     
============================================
+ Hits            147      191      +44     
  Misses           22       22
Impacted Files Coverage Δ Complexity Δ
src/Models/ScheduledNotification.php 82% <ø> (ø) 20 <0> (ø) ⬇️
src/ScheduledNotification.php 93.39% <98.07%> (+4.68%) 39 <11> (+11) ⬆️

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 1083012...f0367f2. Read the comment docs.

@atymic atymic marked this pull request as ready for review October 24, 2019 01:34
@atymic
Copy link
Collaborator Author

atymic commented Oct 24, 2019

I just noticed that i've gone with the double l in cancelled (which is aussie english).

I'm wondering if we should keep it consistent and switch it to canceled?

@thomasjohnkane
Copy link
Owner

The American English version is two L also. I think Canada and Europe are 1. I’m fine with either, but agree it should be consistent.

@atymic
Copy link
Collaborator Author

atymic commented Oct 24, 2019

Grammarly seems to disagree
https://www.grammarly.com/blog/canceled-vs-cancelled/

I was thinking keep to american english?

Copy link
Owner

@thomasjohnkane thomasjohnkane left a comment

Choose a reason for hiding this comment

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

Tests pass and code looks good

@thomasjohnkane thomasjohnkane merged commit 0c40127 into thomasjohnkane:master Oct 24, 2019
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

3 participants