Skip to content

Commit

Permalink
Rename FollowedTagsChanged to FollowedTagsFetched
Browse files Browse the repository at this point in the history
Upon investigation I realized that most places using this event were actually
using it as a callback for the end of the fetch task rather than something that
is ONLY emitted when the tags CHANGE, which was what the name implied.

That only worked because of the bug mentioned in commit 3017bab that did not
properly compare the local and server tags and considered all fetches as
requiring a local database write, which in turn triggered the event.

The constant triggering of that FollowedTagsChanged event was also the cause of
bugs #20009 and #20010 since part of the code that runs there was actually only
supposed to run some times.

To fix that I renamed it to FollowedTagsFetched to better represent what it does
and figured out which parts of the code inside `ReaderPostListFragment` event
listener were supposed to run and in which scenario. I also added a `didChange`
field in the `FollowedTagsFetched` event for completeness but that part might
not be needed in the future, since the only code using that is legacy code
related to the old subfilter which should be removed in the future.
  • Loading branch information
Thomas Horta committed Feb 19, 2024
1 parent 4eafa94 commit 62827ff
Show file tree
Hide file tree
Showing 13 changed files with 69 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,27 @@ private ReaderEvents() {
throw new AssertionError();
}

public static class FollowedTagsChanged {
public static class FollowedTagsFetched {
private final boolean mDidSucceed;
private final boolean mDidChange;

public FollowedTagsChanged(boolean didSucceed) {
public FollowedTagsFetched(boolean didSucceed) {
mDidSucceed = didSucceed;
mDidChange = true;
}

public FollowedTagsFetched(boolean didSucceed, boolean didChange) {
mDidSucceed = didSucceed;
mDidChange = didChange;
}

public boolean didSucceed() {
return mDidSucceed;
}

public boolean didChange() {
return mDidChange;
}
}

public static class TagAdded {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
import org.wordpress.android.ui.mysite.jetpackbadge.JetpackPoweredBottomSheetFragment;
import org.wordpress.android.ui.pages.SnackbarMessageHolder;
import org.wordpress.android.ui.prefs.AppPrefs;
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsFetched;
import org.wordpress.android.ui.reader.ReaderEvents.TagAdded;
import org.wordpress.android.ui.reader.ReaderTypes.ReaderPostListType;
import org.wordpress.android.ui.reader.actions.ReaderActions;
Expand Down Expand Up @@ -934,14 +935,16 @@ private void resetPostAdapter(ReaderPostListType postListType) {

@SuppressWarnings("unused")
@Subscribe(threadMode = ThreadMode.MAIN)
public void onEventMainThread(ReaderEvents.FollowedTagsChanged event) {
public void onEventMainThread(FollowedTagsFetched event) {
if (getPostListType() == ReaderPostListType.TAG_FOLLOWED) {
// reload the tag filter since tags have changed
reloadTags();
if (event.didChange()) {
// reload the tag filter since tags have changed or we just opened the fragment
reloadTags();
}

// update the current tag if the list fragment is empty - this will happen if
// the tag table was previously empty (ie: first run)
if (isPostAdapterEmpty()) {
if (isPostAdapterEmpty() && (ReaderBlogTable.hasFollowedBlogs() || !mHasUpdatedPosts)) {
updateCurrentTag();
}
}
Expand Down Expand Up @@ -1942,12 +1945,7 @@ private void showBookmarksSavedLocallyDialog(ShowBookmarkedSavedOnlyLocallyDialo
private final ReaderInterfaces.DataLoadedListener mDataLoadedListener = new ReaderInterfaces.DataLoadedListener() {
@Override
public void onDataLoaded(boolean isEmpty) {
if (!isAdded()) return;

if (!mHasUpdatedPosts) {
fetchInitialData();
return;
}
if (!isAdded() || !mHasUpdatedPosts) return;

if (isEmpty) {
if (getPostListType() != ReaderPostListType.SEARCH_RESULTS
Expand All @@ -1973,20 +1971,20 @@ public void onDataLoaded(boolean isEmpty) {
}
};

private void fetchInitialData() {
if (getPostListType() == ReaderPostListType.TAG_FOLLOWED) {
reloadTags();

// update the current tag if the list fragment is empty - this will happen if
// the tag table was previously empty (ie: first run)
if (isPostAdapterEmpty()) {
updateCurrentTag();
}
}

if (mReaderViewModel != null) mReaderViewModel.loadTabs();
if (mSubFilterViewModel != null) mSubFilterViewModel.loadSubFilters();
}
// private void fetchInitialData() {
// if (getPostListType() == ReaderPostListType.TAG_FOLLOWED) {
// reloadTags();
//
// // update the current tag if the list fragment is empty - this will happen if
// // the tag table was previously empty (ie: first run)
// if (isPostAdapterEmpty()) {
// updateCurrentTag();
// }
// }
//
// if (mReaderViewModel != null) mReaderViewModel.loadTabs();
// if (mSubFilterViewModel != null) mSubFilterViewModel.loadSubFilters();
// }

private boolean isBookmarksList() {
return getPostListType() == ReaderPostListType.TAG_FOLLOWED
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import org.wordpress.android.ui.LocaleAwareActivity;
import org.wordpress.android.ui.RequestCodes;
import org.wordpress.android.ui.prefs.AppPrefs;
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsFetched;
import org.wordpress.android.ui.reader.actions.ReaderActions;
import org.wordpress.android.ui.reader.actions.ReaderBlogActions;
import org.wordpress.android.ui.reader.actions.ReaderTagActions;
Expand Down Expand Up @@ -204,7 +205,7 @@ protected void onResume() {

@SuppressWarnings("unused")
@Subscribe(threadMode = ThreadMode.MAIN)
public void onEventMainThread(ReaderEvents.FollowedTagsChanged event) {
public void onEventMainThread(FollowedTagsFetched event) {
AppLog.d(AppLog.T.READER, "reader subs > followed tags changed");
getPageAdapter().refreshFollowedTagFragment();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import org.wordpress.android.models.ReaderTagList;
import org.wordpress.android.models.ReaderTagType;
import org.wordpress.android.ui.reader.ReaderConstants;
import org.wordpress.android.ui.reader.ReaderEvents;
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsFetched;
import org.wordpress.android.ui.reader.actions.ReaderActions.ActionListener;
import org.wordpress.android.ui.reader.utils.ReaderUtils;
import org.wordpress.android.util.AppLog;
Expand Down Expand Up @@ -51,7 +51,7 @@ public static boolean deleteTag(final ReaderTag tag,
private static boolean deleteTagsLocallyOnly(ActionListener actionListener, ReaderTag tag) {
ReaderTagTable.deleteTag(tag);
ReaderActions.callActionListener(actionListener, true);
EventBus.getDefault().post(new ReaderEvents.FollowedTagsChanged(true));
EventBus.getDefault().post(new FollowedTagsFetched(true));

return true;
}
Expand Down Expand Up @@ -130,7 +130,7 @@ public static boolean addTags(@NonNull final List<ReaderTag> tags,
private static boolean saveTagsLocallyOnly(ActionListener actionListener, ReaderTagList newTags) {
ReaderTagTable.addOrUpdateTags(newTags);
ReaderActions.callActionListener(actionListener, true);
EventBus.getDefault().post(new ReaderEvents.FollowedTagsChanged(true));
EventBus.getDefault().post(new FollowedTagsFetched(true));

return true;
}
Expand All @@ -147,7 +147,7 @@ private static boolean saveTagsLocallyAndRemotely(ActionListener actionListener,
if (actionListener != null) {
ReaderActions.callActionListener(actionListener, true);
}
EventBus.getDefault().post(new ReaderEvents.FollowedTagsChanged(true));
EventBus.getDefault().post(new FollowedTagsFetched(true));
};

RestRequest.ErrorListener errorListener = volleyError -> {
Expand All @@ -159,7 +159,7 @@ private static boolean saveTagsLocallyAndRemotely(ActionListener actionListener,
if (actionListener != null) {
ReaderActions.callActionListener(actionListener, true);
}
EventBus.getDefault().post(new ReaderEvents.FollowedTagsChanged(true));
EventBus.getDefault().post(new FollowedTagsFetched(true));
return;
}

Expand All @@ -171,7 +171,7 @@ private static boolean saveTagsLocallyAndRemotely(ActionListener actionListener,
if (actionListener != null) {
ReaderActions.callActionListener(actionListener, false);
}
EventBus.getDefault().post(new ReaderEvents.FollowedTagsChanged(false));
EventBus.getDefault().post(new FollowedTagsFetched(false));
};

ReaderTagTable.addOrUpdateTags(newTags);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import org.wordpress.android.models.discover.ReaderDiscoverCards
import org.wordpress.android.modules.IO_THREAD
import org.wordpress.android.modules.UI_THREAD
import org.wordpress.android.ui.reader.ReaderEvents.FetchDiscoverCardsEnded
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsChanged
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsFetched
import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResult.CHANGED
import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResult.FAILED
import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResult.HAS_NEW
Expand Down Expand Up @@ -211,7 +211,7 @@ class ReaderDiscoverDataProvider @Inject constructor(

@Suppress("unused", "UNUSED_PARAMETER")
@Subscribe(threadMode = BACKGROUND)
fun onFollowedTagsChanged(event: FollowedTagsChanged) {
fun onFollowedTagsFetched(event: FollowedTagsFetched) {
launch {
refreshCards()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ package org.wordpress.android.ui.reader.repository.usecases.tags

import org.greenrobot.eventbus.Subscribe
import org.greenrobot.eventbus.ThreadMode
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsChanged
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsFetched
import org.wordpress.android.ui.reader.repository.ReaderRepositoryCommunication
import org.wordpress.android.ui.reader.repository.ReaderRepositoryCommunication.Error.NetworkUnavailable
import org.wordpress.android.ui.reader.repository.ReaderRepositoryCommunication.Error.RemoteRequestFailure
Expand Down Expand Up @@ -48,7 +48,7 @@ class FetchFollowedTagsUseCase @Inject constructor(

@Suppress("unused")
@Subscribe(threadMode = ThreadMode.BACKGROUND)
fun onFollowedTagsChanged(event: FollowedTagsChanged) {
fun onFollowedTagsFetched(event: FollowedTagsFetched) {
val result = if (event.didSucceed()) {
Success
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import org.greenrobot.eventbus.Subscribe
import org.greenrobot.eventbus.ThreadMode
import org.wordpress.android.fluxc.store.AccountStore
import org.wordpress.android.models.ReaderTag
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsChanged
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsFetched
import org.wordpress.android.ui.reader.actions.ReaderTagActions
import org.wordpress.android.ui.reader.repository.ReaderRepositoryCommunication
import org.wordpress.android.ui.reader.repository.ReaderRepositoryCommunication.Error.NetworkUnavailable
Expand Down Expand Up @@ -43,7 +43,7 @@ class FollowInterestTagsUseCase @Inject constructor(

@Suppress("unused")
@Subscribe(threadMode = ThreadMode.BACKGROUND)
fun onFollowedTagsChanged(event: FollowedTagsChanged) {
fun onFollowedTagsFetched(event: FollowedTagsFetched) {
val result = if (event.didSucceed()) {
Success
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.wordpress.android.ui.prefs.AppPrefs;
import org.wordpress.android.ui.reader.ReaderConstants;
import org.wordpress.android.ui.reader.ReaderEvents;
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsFetched;
import org.wordpress.android.ui.reader.ReaderEvents.InterestTagsFetchEnded;
import org.wordpress.android.ui.reader.services.ServiceCompletionListener;
import org.wordpress.android.util.AppLog;
Expand Down Expand Up @@ -175,6 +176,7 @@ public void run() {
localTopics.addAll(ReaderTagTable.getCustomListTags());
localTopics.addAll(ReaderTagTable.getDiscoverPostCardsTags());

boolean didChangeFollowedTags = false;
if (!localTopics.isSameList(serverTopics)) {
AppLog.d(AppLog.T.READER, "reader service > followed topics changed "
+ "updatedDisplayNames [" + displayNameUpdateWasNeeded + "]");
Expand All @@ -190,9 +192,9 @@ public void run() {
ReaderTagTable.replaceTags(serverTopics);
}
// broadcast the fact that there are changes
// TODO thomashortadev this was being sent EVERY time
EventBus.getDefault().post(new ReaderEvents.FollowedTagsChanged(true));
didChangeFollowedTags = true;
}
EventBus.getDefault().post(new FollowedTagsFetched(true, didChangeFollowedTags));
AppPrefs.setReaderTagsUpdatedTimestamp(new Date().getTime());

taskCompleted(UpdateTask.TAGS);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ class SubFilterViewModel @Inject constructor(

@Suppress("unused", "UNUSED_PARAMETER")
@Subscribe(threadMode = ThreadMode.MAIN)
fun onEventMainThread(event: ReaderEvents.FollowedTagsChanged) {
fun onEventMainThread(event: ReaderEvents.FollowedTagsFetched) {
AppLog.d(T.READER, "Subfilter bottom sheet > followed tags changed")
loadSubFilters()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ class ReaderViewModel @Inject constructor(

@Suppress("unused", "UNUSED_PARAMETER")
@Subscribe(threadMode = MAIN)
fun onTagsUpdated(event: ReaderEvents.FollowedTagsChanged) {
fun onTagsUpdated(event: ReaderEvents.FollowedTagsFetched) {
loadTabs()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import org.mockito.Mock
import org.mockito.junit.MockitoJUnitRunner
import org.mockito.kotlin.whenever
import org.wordpress.android.BaseUnitTest
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsChanged
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsFetched
import org.wordpress.android.ui.reader.repository.ReaderRepositoryCommunication.Error.NetworkUnavailable
import org.wordpress.android.ui.reader.repository.ReaderRepositoryCommunication.Error.RemoteRequestFailure
import org.wordpress.android.ui.reader.repository.ReaderRepositoryCommunication.Success
Expand Down Expand Up @@ -61,12 +61,12 @@ class FetchFollowedTagsUseCaseTest : BaseUnitTest() {
}

@Test
fun `Success returned when FollowedTagsChanged event is posted with true`() = test {
fun `Success returned when FollowedTagsFetched event is posted with success`() = test {
// Given
whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true)
val event = FollowedTagsChanged(true)
val event = FollowedTagsFetched(true)
whenever(readerUpdateServiceStarterWrapper.startService(contextProvider.getContext(), EnumSet.of(TAGS)))
.then { useCase.onFollowedTagsChanged(event) }
.then { useCase.onFollowedTagsFetched(event) }

// When
val result = useCase.fetch()
Expand All @@ -76,12 +76,12 @@ class FetchFollowedTagsUseCaseTest : BaseUnitTest() {
}

@Test
fun `RemoteRequestFailure returned when FollowedTagsChanged event is posted with false`() = test {
fun `RemoteRequestFailure returned when FollowedTagsFetched event is posted with failure`() = test {
// Given
whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true)
val event = FollowedTagsChanged(false)
val event = FollowedTagsFetched(false)
whenever(readerUpdateServiceStarterWrapper.startService(contextProvider.getContext(), EnumSet.of(TAGS)))
.then { useCase.onFollowedTagsChanged(event) }
.then { useCase.onFollowedTagsFetched(event) }

// When
val result = useCase.fetch()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import org.wordpress.android.models.ReaderPost
import org.wordpress.android.models.discover.ReaderDiscoverCard.ReaderPostCard
import org.wordpress.android.models.discover.ReaderDiscoverCards
import org.wordpress.android.ui.reader.ReaderEvents.FetchDiscoverCardsEnded
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsChanged
import org.wordpress.android.ui.reader.ReaderEvents.FollowedTagsFetched
import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResult.FAILED
import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResult.HAS_NEW
import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResult.UNCHANGED
Expand Down Expand Up @@ -249,7 +249,11 @@ class ReaderDiscoverDataProviderTest : BaseUnitTest() {
@Test
fun `when followed tags change the discover feed gets refreshed`() = test {
// Act
dataProvider.onFollowedTagsChanged(FollowedTagsChanged(true))
dataProvider.onFollowedTagsFetched(
FollowedTagsFetched(
true
)
)
// Assert
verify(fetchDiscoverCardsUseCase).fetch(REQUEST_FIRST_PAGE)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ class ReaderViewModelTest : BaseUnitTest() {
}

@Test
fun `Tags are reloaded when FollowedTagsChanged event is received`() = testWithNonEmptyTags {
fun `Tags are reloaded when FollowedTagsFetched event is received`() = testWithNonEmptyTags {
// Arrange
var state: ReaderUiState? = null
viewModel.uiState.observeForever {
Expand Down

0 comments on commit 62827ff

Please sign in to comment.