Fix TransactionTooLargeException when opening HistoryDetailActivity #16105
Conversation
…n IDs to avoid TransactionTooLargeException (the new parameters are used to fetch the revisions)
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
👀 CI |
|
You can test the changes on this Pull Request by downloading the APKs: |
There was a problem hiding this comment.
Thanks for tackling this one, @renanferrari 🙇 It works well! I left couple of comments in the code.
|
|
||
| ActivityLauncher.viewHistoryDetailForResult(this, mRevision, revisions); | ||
| private long[] getRevisionsIds(@NonNull final List<Revision> revisions) { |
There was a problem hiding this comment.
If I understand this correctly, number of returned ID's will be same as the number of revisions, so maybe we don't need to extract them into idsList first?
Can we simplify it into something like this (not tested)?:
private long[] getRevisionsIds(@NonNull final List<Revision> revisions) {
final long[] idsArray = new long[revisions.size()];
for (int i = 0; i < revisions.size(); i++) {
final Revision current = revisions.get(i);
idsArray[i] = current.getRevisionId();
}
return idsArray;
}
| final ArrayList<Revision> revisions = new ArrayList<>(); | ||
| for (int i = 0; i < revisionModels.size(); i++) { | ||
| final RevisionModel current = revisionModels.get(i); | ||
| final Revision revision = new Revision(current.getRevisionId(), current.getDiffFromVersion(), |
There was a problem hiding this comment.
Revision has constructor that takes a RevisionModel. Wonder if we can utilize it?
|
|
||
| private fun removeRevisionsFromLocalDB(post: PostModel, revisions: List<RevisionModel>) { | ||
| revisions.forEach { | ||
| postStore.deleteLocalRevision(it, site, post) |
There was a problem hiding this comment.
If we only remove the revisions received from the endpoint, there is a chance we might have some revisions not removed, if for some reason they are not in the response. What do you think about removing all the revision for specific post?
| final long[] previousRevisionsIds = getArguments().getLongArray(EXTRA_PREVIOUS_REVISIONS_IDS); | ||
| final List<RevisionModel> revisionModels = new ArrayList<>(); | ||
| for (final long revisionId : previousRevisionsIds) { | ||
| revisionModels.add(mPostStore.getRevisionById(revisionId)); |
There was a problem hiding this comment.
This is probably not a significant issue, but I wonder if performance might suffer when accessing DB from main thread for a post wit a lot of changes.
…ViewModel instead of just the ones returned by remote
…nModel instead of passing every field
Fixes #16012
Depends on: wordpress-mobile/WordPress-FluxC-Android#2310
To test:
1 - Log in
2 - Select a site
3 - On "My Site", tap on "Posts"
4 - Tap on a post that was updated at least once
5 - Tap on overflow menu (three vertical dots on top right)
6 - Tap on "History"
7 - Tap on any Revision on list. Revision details screen should open, instead of the app crashing.
Regression Notes
Potential unintended areas of impact
Revision details ViewPager (introduced a bug while developing and then fixed it)
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing opening history details screen with different sites, moving the ViewPager from beginning to end.
What automated tests I added (or what prevented me from doing so)
I've tried to update
HistoryViewModelTestwith my changes, but couldn't do it becauseOnRevisionsFetchedvisibility is limited. If there are any ideas to have this code tested the way it is, please let me know.PR submission checklist:
RELEASE-NOTES.txtif necessary.