-
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
Post / Page creation: Don't send default Post.authorID to the API #19717
Conversation
cb9c1bd
to
466c3f7
Compare
You can test the changes in Jetpack from this Pull Request by:
|
You can test the changes in WordPress from this Pull Request by:
|
nil
in the absence of a user ID and or author
da6d734
to
0484cca
Compare
nil
in the absence of a user ID and or author0484cca
to
4144e97
Compare
nil
Post.authorID to the API
nil
Post.authorID to the API8f7c973
to
8a92382
Compare
I'll continue testing the rest after your input @twstokes as I assume it might have affect on the other cases too? |
Thank you for the thorough testing @alpavanoglu! 🙇
|
|
I haven't had any issues yet, but here's what I did:
|
What's strange is when I launched the app again in the same state, a pop-up with an error appeared immediately stating that publish update has failed. I'll try the same steps again and see if it happens again, if so let you know what I can find out. |
😬 Thank you! Any logs from when the error occurred would be helpful. |
I was able to test this build during some downtime as well. If it helps for feedback, I was able to successfully test all four scenarios using the Jetpack app following the instructions for each:
I can continue to help test next week, if needed. |
Thanks a bunch @derekblank for the extra testing! 🙇 Great to know they all worked for you. |
Generated by 🚫 dangerJS |
All scenarios work as expected. LGTM! 🚀 Self-hosted site publishing ✅
Self-hosted site connected to Jetpack ✅
WordPress.com site ✅
Multi-user sites as an Admin or Editor role ✅
Multi-user sites as an Author or Contributor role ✅
|
Thanks a bunch @salimbraksa, @alpavanoglu, and @derekblank! I'll merge this and make sure it lands in the CfT as well. 🙇 |
Fixes #19589
Description
This PR resolves an issue where the app could send an invalid author ID for Posts and Pages.
Problem
When the app doesn't have a
Blog.userID
set and the user creates a new Post or Page, the app is setting the author ID to0
, which is the default value for aBasePost
's author ID in the Core Data model. This0
gets sent to the API which is an invalid author ID.The main reason
Blog.userID
might not be set is because theupdateMultiAuthor
method ofBlogService
hasn't ran, or had a failure on the server side.See technical summary
There currently exists a race where a new Post or Page can be created before the blog's user ID is populated. The call can also just outright fail, like any API call:
WordPress-iOS/WordPress/Classes/Services/BlogService.m
Lines 788 to 790 in 466c3f7
If a new Post or Page is created before the blog's user ID is populated, the post's author ID never gets set
WordPress-iOS/WordPress/Classes/Models/Blog+Post.swift
Lines 58 to 61 in bff014b
So it sticks with the default value assigned when instantiating (in this case) the Post:
WordPress-iOS/WordPress/Classes/Models/Blog+Post.swift
Line 45 in bff014b
At first glance this doesn't appear to be a problem, since:
authorID
is an optionalNSNumber
, so we assume it would default tonil
The issue is that
BasePost.authorID
defaults to0
in the Core Data model.So in these cases we're sending a
0
for the author ID instead of not sending the value, which can result in errors like:What worsens the issue is that once a Post or Page has been created by the app, the invalid author ID of
0
is stuck to it. Even if the blog's user ID is synced later on, it will only fix the issue for new Posts and Pages.Another potential solution would be to remove the default value of
0
from the Core Data model, but this could introduce risk to existing logic.Potential workarounds
Solution
If a Post or Page's author ID hasn't been set locally, we shouldn't send a value to the API.
Testing
Both the WordPress and Jetpack apps should be tested.
Self-hosted site publishing
Self-hosted site connected to Jetpack
WordPress.com site
Multi-user sites as an Admin or Editor role
This test should be ran with an Admin or Editor role.
Multi-user sites as an Author or Contributor role
This test should be ran with an Author or Contributor role.
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
RemotePost
(e905ae5)PR submission checklist:
RELEASE-NOTES.txt
if necessary.