Reverting paging3 migration #14968

develric · 2021-07-01T09:32:42Z

This cherry pick changes from #14949 as an hotfix for 17.6

NOTE: code that was cherry picked from 17.7 was slightly ahead respect the original code in 17.6. This required adding the UnifiedCommentListItemDecoration that was added in 17.7. All this code is behind a feature flag, so as far as it compiles and we confirm the fix works we should be ok.

Reporting below the description from the original PR for convenience.

Fixes #14860

Migration to Paging 3 is causing some issues with Post List. We need a bit more for a partial or full migration to Paging 3.

This PR removes the Paging 3 dependency and comments out the code that is using it, since it's behind the feature flag.

To test:

Make sure that the app builds without any issue.
Go to the post list and scroll down quickly
Notice that post loading is performed correctly, the list remains visible and nothing crashes.

Regression Notes

Potential unintended areas of impact
We are removing the offending library so should be none.
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)

PR submission checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

…tion.

peril-wordpress-mobile · 2021-07-01T09:32:47Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

AliSoftware

Left a few questions while comparing the diff with the other PR.

Note that I'm no Android developer and only blindly compared the diff with other PR, or compared what was removed with what was commented, without trying to decipher the Android logic behind it, so those are merely questions and incentives to double-check, but might be false positives.

Even if there are differences in the two diffs that are correct and make sense because develop and release/17.6 differ, I think it would still be valuable to ensure the code that is marked as +added but commented out matches the code that is marked as -removed in the diff, otherwise this will lead to regressions when we uncomment them later.

AliSoftware · 2021-07-01T09:34:01Z

WordPress/src/main/java/org/wordpress/android/ui/comments/unified/CommentPagingSource.kt

-        return commentListItems
-    }
+/**
+ * Temporarily commented out until migration between Paging 2 and 3 is sorted out.


nit: worth a reference to the Slack discussion, Sentry issue, and past PRs here to track it when we get back to it? 🤔

AliSoftware · 2021-07-01T09:38:07Z

WordPress/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentListFragment.kt

+    override fun onDestroyView() {
+        super.onDestroyView()
+        binding = null
+    }


I don't see this part being added in the diff of the cherry-picked commit – https://github.com/wordpress-mobile/WordPress-Android/pull/14949/files#diff-d3f669617284dbfa52ea7b4aeb770607c0f206258a309d4140a8a417153a1b02L65, but I see it was already there back then tho.
Just double-checking this re-addition is thus intentional given this was not part of the release/17.6 branch when we did 17.6 back then.

AliSoftware · 2021-07-01T09:41:26Z

...Press/src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentListViewModel.kt

-    val commentListItems = commentListItemPager.flow
-            .map { pagingData ->
-                pagingData.map { commentModel ->
-                    val toggleAction = ToggleAction(commentModel.remoteCommentId, this::clickItem)
-                    val clickAction = ClickAction(commentModel.remoteCommentId, this::toggleItem)
-                    Comment(
-                            remoteCommentId = commentModel.remoteCommentId,
-                            postTitle = commentModel.postTitle,
-                            authorName = commentModel.authorName,
-                            authorEmail = commentModel.authorEmail,
-                            body = commentModel.content,
-                            avatarUrl = "",
-                            isPending = commentModel.status == CommentStatus.UNAPPROVED.toString(),
-                            isSelected = false,
-                            clickAction = clickAction,
-                            toggleAction = toggleAction
-                    )
-                }
-                        .insertSeparators { before, after ->
-                            when {
-                                before == null -> SubHeader("Date Sub Header", -1)
-                                // TODO add the logic for determining if subheader is necessary or not
-                                else -> null
-                            }
-                        }
-            }


Side note that the diff is slightly different here from what was removed in the other PR. Probably expected given this part of the code like changed between release and develop, but just wanted to highlight this to double-check that the different code being removed did not have side effects that the original didn't have.

Also, that means that the code that is actually added and commented out on the following lines 18-73 does not match what was removed, so when we get back to this later to "revert this revert" and try to get back to migrating to paging 3, there is a risk that we would just uncomment the lines 18-73 (instead of reverting this PR) and land in a different state of the code that what was expected, introducing regressions and bugs or behavior changes that were fixed by the new code that was removed above but were still present in the code that was commented below

AliSoftware · 2021-07-01T09:44:57Z

.../src/main/java/org/wordpress/android/ui/comments/unified/UnifiedCommentListItemDecoration.kt

@@ -0,0 +1,90 @@
+package org.wordpress.android.ui.comments.unified


This whole file was not part of the other PR. Is this file addition expected there?

as mentioned in the PR description

NOTE: code that was cherry picked from 17.7 was slightly ahead respect the original code in 17.6. This required adding the UnifiedCommentListItemDecoration that was added in 17.7. All this code is behind a feature flag, so as far as it compiles and we confirm the fix works we should be ok.

being the code behind a feature flag AFAIU if it compiles it should not have an impact since it is not run and we removed the dependency on the paging 3 lib (happy to have your thoughts on this as well @ashiagr 🙇 )

Since it behind a feature flag + there are no compilation issues (and we need the new file after the latest changes since last fix) + no fast scrolling/ refresh issues, so 👍 from me.

AliSoftware · 2021-07-01T09:50:33Z

Note that the BuildKite CI failure (pipeline-pipeline-upload) is expected, as we didn't have BuildKite configured in the release branch yet back then.

develric · 2021-07-01T10:03:43Z

hey @AliSoftware 👋 , was reading your notes above; let me share my reasoning so we can double-check if it makes sense (and adapt to whatever we decide it's the best approach 🙇 ; cc @ashiagr for another opinion as well)

As you said code evolved a bit from 17.6 to 17.7
my intention was to cherry pick the original fix PR changes only adding minimal changes to the 17.6 to allow building the cherry picked changes (this for example is the case for adding UnifiedCommentListItemDecoration
All the code changes above are behind a feature flag so as far as it compiles (and we removed the dependency on paging 3) the code should not be run at all
as for the conflicts when coming back to develop; I think it's something we will need to deal with when bringing back the code to develop after the hotfix. In my understanding the code we cherry picked above will be aligned with develop and we will merge the code under the comments.unified package probably just accepting the develop version (but @khaykov will know more about it)

Does the above makes sense? (especially the last point).

Another possibility is to manual comment out the not needed code instead of cherry picking from the original fix

AliSoftware · 2021-07-01T10:15:56Z

as for the conflicts when coming back to develop; I think it's something we will need to deal with when bringing back the code to develop after the hotfix. In my understanding the code we cherry picked above will be aligned with develop and we will merge the code under the comments.unified package probably just accepting the develop version (but @khaykov will know more about it)

Actually, after taking my late morning coffee, I realized I was wrong with my suggestion above: the code that is added but commented out in this PR's diff should match what's in develop, as opposed to match what was removed in this PR, because as you said once this hotfix lands and is merged back into develop, we want the commented-out code to match what's already in develop 🙂 (And in fact if we applied my incorrect suggestion, that would lead to conflicts and could be what would risk causing regressions later when we uncomment it as it wouldn't match develop)

So nevermind, sorry for the misleading suggestion, and you were correct in your changes made then 👍 As long as the commented code added matches the commented code in develop we should be good 🙂

ashiagr

Thanks for a fast turnaround @develric. I tested the PR according to the testing steps and could not reproduce the crash. Comments load fine as well after removing the feature flag.

I agree with your reasoning in #14968 (comment), 👍 from me.

peril-wordpress-mobile · 2021-07-01T10:47:43Z

You can test the changes on this Pull Request by downloading the APKs:

peril-wordpress-mobile · 2021-07-01T10:59:25Z

	Warnings
⚠️	PR is not assigned to a milestone.

Generated by 🚫 dangerJS

khaykov and others added 4 commits July 1, 2021 11:23

Commented out code that uses Paging 3

0b79592

Removed Pagign 3 dependency.

7109a53

Fixed detekt issue.

6f37419

Cherry picking from 17.7 requires adding UnifiedCommentListItemDecora…

ed54395

…tion.

develric added Post List [Type] Crash labels Jul 1, 2021

develric requested review from AliSoftware and ashiagr July 1, 2021 09:32

AliSoftware reviewed Jul 1, 2021

View reviewed changes

ashiagr approved these changes Jul 1, 2021

View reviewed changes

develric marked this pull request as ready for review July 1, 2021 10:27

develric merged commit 3f08c96 into release/17.6.1 Jul 1, 2021

develric deleted the fix/reverting-paging3-migration branch July 1, 2021 10:50

This was referenced Jul 1, 2021

Merge hotfix 17.6.1 into trunk #14970

Merged

Merge hotfix 17.6.1 into develop #14971

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reverting paging3 migration #14968

Reverting paging3 migration #14968

develric commented Jul 1, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jul 1, 2021

AliSoftware left a comment

AliSoftware Jul 1, 2021

AliSoftware Jul 1, 2021

AliSoftware Jul 1, 2021

AliSoftware Jul 1, 2021

develric Jul 1, 2021

ashiagr Jul 1, 2021

AliSoftware commented Jul 1, 2021

develric commented Jul 1, 2021

AliSoftware commented Jul 1, 2021

ashiagr left a comment

peril-wordpress-mobile bot commented Jul 1, 2021

peril-wordpress-mobile bot commented Jul 1, 2021

		@@ -0,0 +1,90 @@
		package org.wordpress.android.ui.comments.unified

Reverting paging3 migration #14968

Reverting paging3 migration #14968

Conversation

develric commented Jul 1, 2021 • edited Loading

Regression Notes

peril-wordpress-mobile bot commented Jul 1, 2021

AliSoftware left a comment

Choose a reason for hiding this comment

AliSoftware Jul 1, 2021

Choose a reason for hiding this comment

AliSoftware Jul 1, 2021

Choose a reason for hiding this comment

AliSoftware Jul 1, 2021

Choose a reason for hiding this comment

AliSoftware Jul 1, 2021

Choose a reason for hiding this comment

develric Jul 1, 2021

Choose a reason for hiding this comment

ashiagr Jul 1, 2021

Choose a reason for hiding this comment

AliSoftware commented Jul 1, 2021

develric commented Jul 1, 2021

AliSoftware commented Jul 1, 2021

ashiagr left a comment

Choose a reason for hiding this comment

peril-wordpress-mobile bot commented Jul 1, 2021

peril-wordpress-mobile bot commented Jul 1, 2021

develric commented Jul 1, 2021 •

edited

Loading