-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Automatically upload published and draft posts with existing remote #12594
Automatically upload published and draft posts with existing remote #12594
Conversation
This was preventing posts with remote to be marked as failed
Generated by 🚫 dangerJS |
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.
@leandroalonso Thank you for working on this. I have some questions (inline). Also, do you have a suggestion on how we could have prevented the regression from happening? Could we have added another test in #12466? Or maybe the test that was missing was for markAsFailedandDraftIfNeeded
?
WordPress/Classes/Services/PostService+MarkAsFailedAndDraftIfNeeded.swift
Show resolved
Hide resolved
WordPress/Classes/Services/PostService+MarkAsFailedAndDraftIfNeeded.swift
Outdated
Show resolved
Hide resolved
@shiki This wasn't a logic directly related to #12466 but in the end it affected it. Adding a test for this specific regression in there would imply an integration test, with multiple parts not being mocked. I would try to avoid that. I added a new test case to ensure this will not happen anymore. I'm pretty comfortable with it by now, what do you think? |
This is so it will match the file name.
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.
@leandroalonso Thank you for info.
I added a new test case to ensure this will not happen anymore. I'm pretty comfortable with it by now, what do you think?
Yeah, this seems fine. I added another one for good measure. a490330
@leandroalonso Please merge it if you agree with my changes and the build passes. 🙂 |
Fixes a regression in #12324
For tests instructions, please take a look in #12466
Update release notes:
RELEASE-NOTES.txt
if necessary.