Offline Support Pages : Cancel Auto Upload - #11266
Merged
malinajirka merged 46 commits intoFeb 18, 2020
Merged
Conversation
…ic items" This reverts commit b9b8ca7.
…port' into issue-11150/page_cancel_auto_upload
|
You can test the changes on this Pull Request by downloading the APK here. |
wpmobilebot
reviewed
Feb 13, 2020
wpmobilebot
reviewed
Feb 13, 2020
Contributor
Author
|
Hi @malinajirka this PR is ready for another review from a code standpoint! As discussed, we will need to resolve the scoping issue #11303 before this can be merged so I added the do not merge label just to be safe! |
3 tasks
…page_cancel_auto_upload
Contributor
Author
|
I merged #11315 into this and it works perfectly. Thanks! |
malinajirka
approved these changes
Feb 17, 2020
malinajirka
left a comment
Contributor
There was a problem hiding this comment.
Thanks @jd-alexander! I've reviewed the changes and tested the behavior. It all LGTM.
I've left two minor comments, but I'm approving this PR. Feel free to merge this PR.
and the actions being the same isnt enforced.
Contributor
|
Thanks for the changes! ;) 🚢 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #11150
Findings
The
Post Listhas logic withinPostListItemUiStateHelperthat helps to determine which buttons should be shown based on several different conditions. This logic was adapted to create a similar behavior for thePage Listcancel auto-upload functionality.Solution
The
PageItemshad the actions hardcoded within its constructor. The actions were removed from them and added at instantiation so that logic could be run that determines if theCancelbutton should be shown or not.Issues Noticed
There's a crash that occurs at times when trying to fetch the post to determine if a page can be canceled or not, so a null check has to be done 0a035ad. I think this crash is related to the one taking place here Offline Support Pages - Update Page Status causes null post when fetching upload status #11260. I think the refactor here should resolve this.
Sometimes when a
Scheduledpost is saved, it doesn't show theCancelaction or the title doesn't get updated. I don't think that's related to this PR but it might need investigation.I added a
localIdandremoteIdto thePageItembecause there are times when either is required. I renamed theidinPageItemtolocalIdbecause it's often confusing when trying to determine which type of id should be used for a query 4caff1c. A further enhancement would be to simply change the types toLocalIdandRemoteIdso that that the compiler's type system enforces this across the featureset. I opted to leave this out of this PR. Should I create an issue for this or it can remain? since the name is already conveying the intent without changing the type.Testing
Reviewing
Only 1 reviewer is needed but anyone can review.
Submitter Checklist
RELEASE-NOTES.txtif necessary.