-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
My Site Dashboard [Phase 2]: Navigate to Edit Post [HACK - DO NOT MERGE] #15720
Conversation
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
You can test the changes on this Pull Request by downloading the APKs: |
This newly data class will now be holding the extra 'postCardType' information, in addition to the 'postId' and it is going to be used to differentiate clicking on draft from scheduled posts.
This update differential between navigating to: - Edit draft post, when a draft post item is clicked - Edit scheduled post, when a scheduled post item is clicked
This is a partial solution and will help the user navigate to the appropriate tab within the 'Posts' screen. On subsequent commit, the user will be further navigated to the 'Edit Post' screen through the 'Posts' screen.
This 'targetPostId' will be utilised to find the post, from the list of posts, based on the remote post id and then automatically click on it.
This 'targetPostId' is now converted to a 'RemotePostId' and passed into the view model. The view model will then be responsible to find the item in the list, upon a data updated event and then trigger the navigation event, just like it is currently for 'scrollToPosition'.
This functionality is a copy-paste of the existing 'scrollToPosition' functionality, which also happens when the 'onDataUpdated(...)' get triggered. The difference here is that now the 'RemotePostId' is used to find the item in the list, instead of the 'LocalPostId'. Also, another difference is that since the 'onDataUpdated(...)' can get triggered multiple times, the 'navigateToRemotePostId' is only reset when the item is found. This can happen either when 'onDataUpdated(...)' is triggered for the first time (database) or afterwards in a subsequent trigger (network).
This now connect the 'PostListFragment' with the 'PostListViewModel', listening to the 'navigateToPost' event and triggering the necessary action to automatically navigate to the 'Edit Post' screen from the 'Posts' screen.
Since 'onDataUpdated(...)' can get triggered multiple times, reset the [navigateToRemotePostId] only when the item is found and the navigation trigger is actually invoked. Also, due to the way paging library works (see 'PagedList'), and since this screen utilizes this solution, the 'findItemListItem(...)' process will return a valid result/index twice for a specific tab: - The first result might either be a 'PostListItemUiState' or an 'EndListIndicatorItem', and - The second result will always be an 'EndListIndicatorItem'. Since both results will return a valid index, it is safer to use the second result to make sure that no more 'onDataUpdated(...)' will occur for that specific tab. To achieve that the 'navigateToEditPost' logic is being utilised. Also, by doing that the paging library gets some additional time to load the data, before it is being automatically selected right afterwards. PS: The database vs network trigger is not relevant since the paging library, through its data source solution, will be handling all that internally. NOTE: This whole solution is hacky and done this way to help us achieve a quick result, without needing to update the paging library or the underneath list store related implementation.
In addition to the 'PostListViewModel.navigateToPost(...) and its logic, which includes: - The 'PostListViewModel.navigateToEditPost' to skip the first result, and - The [PostListViewModel.navigateToRemotePostId] to reset the navigation trigger. An extra delay of 'NAVIGATE_TO_EDIT_POST_DELAY_MS' milliseconds it added to make sure that the list is submitted to the [PostListAdapter] paging related adapter (see [PagedListAdapter]). This will make sure that the newly submitted list updated the existing and potentially stale list. Otherwise, and due to the fact that the list submission can take some additional time, selecting the item might launch the 'EditPostActivity' with a stale item and thus this might overwrite any remote changes. PS: A delay of half a second would have been sufficient enough, based on the testing that was done, but doubling this number to 1 second seemed safer to to make sure that no there will be less changes for the stale item to get selected. NOTE: This whole solution is hacky and done this way to help us achieve a quick result, without needing to update the paging library or the underneath list store related implementation.
664aaa1
to
d1967ae
Compare
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.
@ParaskP7
This is quite a complex problem, so bravo for finding a solution so quickly. 🥇
I’ve tested according to your instructions and so far, so good. I can't wait to see the next installment that eliminates the post list.
We probably want to add a general testing scenario for configuration changes. I rotated my device after tapping a post card item and I never transitioned from the post list to the edit view (remained on the post list). I may have been premature with this one, but I decides to test it after I was reading the "hack area" comments related to navigateToEditPost
. I’ll be sure to repeat the test against subsequent PR’s.
Otherwise, and due to the fact that the list submission can take some additional time, selecting the item might launch the 'EditPostActivity' with a stale item and thus this might overwrite any remote changes.
^^ I haven’t been able to come up with a better solution. I wonder if we should use firebase remote config for the delay value for the testing/beta period to see how it performs. Then if it needs to be adjusted, we can do so without creating a new build. Something to consider.
We talked about guarding this entire implementation with a feature flag … I didn’t see that in here explicitly (yes, we just talked about it today - :)) . It is indirectly guarded by the msd flag and by the postId = null logic. Am I correct in stating that future logic will add a flag so we can switch from “post item tap → post list → highlight row” and this solution?
Nice work. 🙇
@@ -321,7 +321,8 @@ class MySiteFragment : Fragment(R.layout.my_site_fragment), | |||
PagePostCreationSourcesDetail.POST_FROM_MY_SITE | |||
) | |||
// TODO: ashiagr this is unhandled right now as mocked post is being used which cannot be opened in the editor |
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.
Can this todo be removed now?
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.
👋 @zwarm !
Thank you, but this TODO
is actually removed already, just in a subsequent commit. Can you please verify that?
🙏
@@ -609,7 +622,7 @@ public static void viewCurrentBlogPostsOfType(Context context, SiteModel site, P | |||
if (postListType == null) { | |||
context.startActivity(PostsListActivity.buildIntent(context, site)); | |||
} else { | |||
context.startActivity(PostsListActivity.buildIntent(context, site, postListType, false, null)); | |||
context.startActivity(PostsListActivity.buildIntent(context, site, postListType, false, null, postId)); | |||
} | |||
AnalyticsUtils.trackWithSiteDetails(AnalyticsTracker.Stat.OPENED_POSTS, site); |
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.
NOTE: This is what I was talking about earlier today re: tracking "opened posts" . Should we be tracking the open post list when we are automagically opening edit? Perhaps we need to track it some other way, because who knows, we MAY actually land on the posts view depending upon the scenario.
Nothing to do with this comment, just wanted us to think about it. :)
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.
👋 @zwarm !
Yeap, totally. As I said, let's think and talk about it more at any point you like while you will be implementing this kind of tracking event logic, I believe there are multiple ways to do what we want.
👍
Great job @ParaskP7! Although hacky, it works and serves the purpose! 👏 It was only after I extensively tested that I found two extreme edge cases when a (dashboard card) post couldn't be found on the posts screen on Scenario 1: Local draft post
Note that it happens only the first time. The navigation works if we return to the dashboard and re-click that post. local_draft_post.mp4Scenario 2: Author's post not yet laid out on the posts screen As we always display the author's most recent posts on the dashboard draft posts card, most of the time they will be present on the top of the posts list and can be found during this navigation. There's a possibility that a few posts from a contributor are present before the author's post on the posts screen and the author post is not yet laid out. In that case, the author's post won't be found during this navigation and the flow will stop at the posts screen. author_post_not_yet_laid_out.mp4Not yet sure if auto-scrolling to that position and then performing a click can help. I'll continue to test and let you know if I come across anything else. 🙇♀️ |
👋 @zwarm !
🙇 + 🙏 Now let's see what I have missed... 😅
Yes, this can be found in this #15723 PR here. I just added you as a reviewer as well, just in case.
Hmmm... let me test this and thanks for figuring out an inconsistency with the flow! So, what you are saying is that you:
Are the above described steps correct? If so, I wonder if we need to care for that since the user will be getting a full screen post loading dialog at this point anyway. Maybe we can lock the screen orientation when the full screen post loading dialog is shown to prevent that from even happening. Is that worth it, wdyt?
Good idea! However, I wonder and I would be kind against that because I don't want to start creating a tool for us to bend and improve on this hacky solution. Instead, what I want is for us to drop this solution and revert to a simpler one if we figure out that our user are having issues with it. Adding ad-hoc delays to see how it performs and controlling that from
I just added a
This need to be discussed and agreed upon, for now it is just an idea, thus I added it as a suggestion. But, I am personally in favor of it. However, I would wait up until we end up completing our initial hacky solution and then we can move forward with thinking about this. Because, who knows, maybe we will end up dropping this hacky solution after all and thus this will leave us with the next best thing to be doing. Wdyt? 🤔 |
👋 @ashiagr !
🙇 + 🙏 Now let's see what I have missed... 😅
So many thanks for going through this extensive testing and for the two scenarios you found that might cause us trouble. I'll check both and come back to you on that.
Let me check on that and I'll report back to you. 🙏
Yes, if a problem happens it will happen only the first time, when you return back to the
Let me check on that and I'll report back to you. 🙏
This is so very interesting, I wouldn't expect this to happen. What I would expect instead is for the top 3 drafts and scheduled on the dashboard feed and Let me dig a bit deeper and I'll come back to you on that.
Yeap, not sure as well, but at least it will not cause any data loses. 🤔 PS: I just added a Suggestions section in the parent #15709 issue, with the title:
Thank you so much, that would be great! Actually everything you done so far helps enormously, so please keep it up! 🙇 |
This is indeed an interesting find. I also tested from a contributor's account and noticed that they could only see author's draft posts on the dashboard draft post card and could not see drafts created by them even if they were more recent. contributor_drafts.mp4I think we should discuss this scenario whether contributors should see their draft or author's and vice-versa: |
👋 @ashiagr !
Once again, thank you for testing and finding out that this scenario doesn't work. I actually tested it myself and verified the same thing. Then, I started digging deeper and I found the solution. I also found why this happens. So, the solution is mainly to do the below two things, one is optional but I totally recommend to keep things simple:
Below, I am going to provide some logs so that you understand what is happening and with those I am going to explain why the above change will work on all 3 below scenarios:
Below, lots are happening, don't get overwhelmed and please feel free to reach out to me so I can explain in more detail. 1st Scenario: In this scenario you will see that the 1st call of
2nd Scenario: In this scenario you will see that the 1st call of
In this scenario you will see that the 1st call of At this point you will notice 3 calls, which I have missed previously, thinking that there will be always 2 calls. However, this new solution, by moving the
|
👋 @ashiagr !
As per your latest comment, and this one from myself on this other #15723 PR, I think we not need to step back a bit and reassess how we want to deal with this new find of yours and Chris's. No matter, this should not block this PR as nothing can be done as part of this PR to address this issue. Wdyt? |
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.
👋 @ParaskP7 !
Scenario 1: Local draft post
Thanks for fixing the issue and also including a detailed explanation in #15720 (comment).
I've tested the solution, it works like a charm. 🎉
Scenario 2: Author's post not yet laid out on the posts screen
I think we not need to step back a bit and reassess how we want to deal with this new find of yours and Chris's.
I agree with the above. Let's reassess how to deal with it. Can we add it to our TODO
list?
I'm approving the PR, feel free to merge when you're ready.
👋 @ashiagr ! 👋 @ParaskP7 !
Thank you for testing and verifying that everything now work as expected, including this extra scenario. 👍
Yes, I agree, actually this and its subsequent PR are now on hold, see comment here. We will reassess the solution as a whole and take any appropriate steps afterwards, but for now and in order to wrap up the project, we will keep it simple and just show the draft/scheduled posts screen when a user taps a draft/scheduled post.
Thank for approving, but as per the above comment I am going to close this and its subsequent PR. Then, I am going to create a new PR which just navigates to the draft/scheduled posts screen when a user taps a draft/scheduled post, keeping all the commits that I did to do so from this PR, up until the launcher changes. PS: I'll keep the navigation action functionality separate, include the remote |
As per this comment, I am now closing this PR. PS: I am going to create a new PR which just navigates to the draft/scheduled posts screen when a user taps a draft/scheduled post, keeping all the commits that I did to do so from this PR, up until the launcher changes. |
This navigation is temporary and as such not utilizing the 'action.postId' in order to navigate to the 'Edit Post' screen. Instead, it fallbacks to navigating to the 'Posts' screen and targeting a specific tab. Previously, another issue was opened in order to help us create a solution to navigate straight to 'Edit Post', just as the user expected. However, as this solution was implemented and problems were popping-up, this solution was paused indefinitely and until it further reassessment. Useful links: - Issue: https://github.com/wordpress-mobile/WordPress-Android/issues /15709 - PR: #15720 - PR: #15723 - Decision: https://github.com/wordpress-mobile/WordPress-Android/pull/ 15723#issuecomment-996762177
Parent: #15709
This PR implements this functionality that will allow the user to navigate to the
Edit Post
screen, from theMy Site
screen, through thePosts
screen.PS: The PR is marked as
HACK
because of the twohack
related commits that are trying to bend the paging framework to allow such navigation. @ashiagr and @zwarm I am adding you both as reviewers since I want both your feedback on this PR, its solution and more specifically I want to to focus on the twohack
related commits. Maybe you can recommend another way of doing that.Prerequisite:
My Site
->Me
->App Settings
->Debug Settings
configuration.MySiteDashboardPhase2FeatureConfig
feature.To test:
My Site
tab, your draft or scheduled posts, if any, are shown.Posts
screen is shown for a second or two.Edit Post
screen is launched with the content of the draft or scheduled post that was selected.The above is testing the happy path. In addition to that please make sure to test any or all other scenarios, ultimately trying to break the solution. For example:
Edit Post
screen, then you will get the remote updates, instead of the now stale/out-dated local state of the post.Offline
mode and try to test the solution. You might need to comment out theCardsSource.postState(...)
for when an error occurs duringcardsStore.fetchCards(selectedSite)
(see line 83). Otherwise, when entering theOffline
mode, the dashboard feed will disappear. PS: This scenario will be also handling via theHandle offline mode
subtask, but it would be great to test it now anyway.NOTE: It is expected that the
Posts
screen is shown. A subsequent PR, which will handle the full screen post loading, will make sure that the loading experience will be seamless and that the user will never notice the intermediatePosts
screen.Regression Notes
Data Loss (⚠️ ): Due to the nature of the solution, the user can enter into a situation where by clicking on a draft or scheduled post, the
Edit Post
screen will open a stale version of the post, or an unwanted version. At which point, if the user clicks back on a draft post, publishes this draft post, or rescheduled/publishes a scheduled post, then they might experience unintentional data loss.Extensive manual testing. See
To test
sections above.N/A
PR submission checklist:
RELEASE-NOTES.txt
if necessary.