-
Notifications
You must be signed in to change notification settings - Fork 117
Posts connector test implemented. #1121
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
Conversation
b3d65fb to
b5dd19e
Compare
9e0f058 to
62c6d36
Compare
62c6d36 to
42bfceb
Compare
dero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kidunot89 Good progress here! Added a few comments and a question. May have also potentially found a bug in Stream, but want to confirm with you first as I'm new to the codebase.
42bfceb to
c1622e9
Compare
|
@dero All requested changes have been made. Please review this again. |
dero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you.
I created #1176 to separately track the issue with log misinterpreting post status transitions from new as posts being updated instead of post being created.
| } | ||
|
|
||
| if ( 'auto-draft' === $old && 'auto-draft' !== $new ) { | ||
| if ( in_array( $old, $start_statuses, true ) && ! in_array( $new, $start_statuses, true ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also update the tests to confirm that changes 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kidunot89 This looks great, thanks!
dero
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now! 👍
| } | ||
|
|
||
| if ( 'auto-draft' === $old && 'auto-draft' !== $new ) { | ||
| if ( in_array( $old, $start_statuses, true ) && ! in_array( $new, $start_statuses, true ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kidunot89 This looks great, thanks!
|
Was this change related to this failing unit test? https://travis-ci.com/github/xwp/stream/jobs/393230336#L512 I wonder why the Travis checks didn't run for this pull request. |
|
No, but it's small issue, I'll fix it another PR. |
Partially fixes #1093
Summary checklist
transition_post_statuscallback testdeleted_postcallback test