Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@
import org.wordpress.android.ui.utils.PreMigrationDeepLinkData;
import org.wordpress.android.util.AppLog;
import org.wordpress.android.util.AppLog.T;
import org.wordpress.android.util.SiteUtils;
import org.wordpress.android.util.ToastUtils;
import org.wordpress.android.util.UriWrapper;
import org.wordpress.android.util.UrlUtils;
Expand All @@ -146,8 +147,6 @@
import java.util.List;
import java.util.Map;

import dagger.hilt.android.EntryPointAccessors;

import static org.wordpress.android.analytics.AnalyticsTracker.ACTIVITY_LOG_ACTIVITY_ID_KEY;
import static org.wordpress.android.analytics.AnalyticsTracker.Stat.POST_LIST_ACCESS_ERROR;
import static org.wordpress.android.analytics.AnalyticsTracker.Stat.READER_ARTICLE_DETAIL_REBLOGGED;
Expand Down Expand Up @@ -646,12 +645,8 @@ public static void viewConnectJetpackForStats(Context context, SiteModel site) {
context.startActivity(intent);
}

private static boolean shouldUseNewPostList(@Nullable SiteModel site) {
return site != null && site.hasApplicationPassword();
}

public static void viewCurrentBlogPosts(Context context, SiteModel site) {
if (shouldUseNewPostList(site)) {
if (SiteUtils.canUseWpRs(site)) {
context.startActivity(PostRsListActivity.Companion.createIntent(context));
return;
}
Expand Down Expand Up @@ -687,7 +682,7 @@ public static void viewCurrentBlogMedia(Context context, SiteModel site) {
}

public static void viewCurrentBlogPages(@NonNull Context context, @NonNull SiteModel site) {
if (shouldUseNewPagesList(site)) {
if (SiteUtils.canUseWpRs(site)) {
context.startActivity(PagesRsListActivity.Companion.createIntent(context));
AnalyticsUtils.trackWithSiteDetails(AnalyticsTracker.Stat.OPENED_PAGES, site);
return;
Expand All @@ -698,10 +693,6 @@ public static void viewCurrentBlogPages(@NonNull Context context, @NonNull SiteM
AnalyticsUtils.trackWithSiteDetails(AnalyticsTracker.Stat.OPENED_PAGES, site);
}

private static boolean shouldUseNewPagesList(@Nullable SiteModel site) {
return site != null && site.hasApplicationPassword();
}

public static void viewPostTypes(@NonNull Context context, @NonNull SiteModel site) {
SiteReference siteRef = SiteReference.Companion.create(
site.getSiteId(),
Expand Down Expand Up @@ -737,35 +728,14 @@ public static void viewPageParentForResult(@NonNull Fragment fragment, @NonNull
}

public static void viewUnifiedComments(Context context, SiteModel site) {
Intent intent;
if (shouldUseRsComments(context, site)) {
intent = CommentsRsListActivity.Companion.createIntent(context);
} else {
intent = new Intent(context, UnifiedCommentsActivity.class);
intent.putExtra(WordPress.SITE, site);
}
// Neither screen reads the site from the Intent; both resolve it from SelectedSiteRepository.
Intent intent = SiteUtils.canUseWpRs(site)
? CommentsRsListActivity.Companion.createIntent(context)
: new Intent(context, UnifiedCommentsActivity.class);
context.startActivity(intent);
AnalyticsUtils.trackWithSiteDetails(AnalyticsTracker.Stat.OPENED_COMMENTS, site);
}

/**
* Whether the wordpress-rs comments screens (list, and the notification-hosted detail) should
* be used for {@code site} instead of the legacy (FluxC) ones.
*/
public static boolean shouldUseRsComments(@NonNull Context context, @NonNull SiteModel site) {
// Match the RS posts/pages gate: use the rs comments screens only for self-hosted sites
// with an application password, and keep the legacy (FluxC) comments screens everywhere
// else (including WP.com-accessed sites).
if (!site.hasApplicationPassword()) {
return false;
}
ActivityLauncherEntryPoint entryPoint = EntryPointAccessors.fromApplication(
context.getApplicationContext(),
ActivityLauncherEntryPoint.class
);
return entryPoint.experimentalFeatures().isEnabled(Feature.RS_UNIFIED_COMMENTS);
}

public static void viewCurrentBlogThemes(Context context, SiteModel site) {
Intent intent = new Intent(context, ThemeBrowserActivity.class);
intent.putExtra(WordPress.SITE, site);
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,9 @@ class UnifiedCommentListFragment : Fragment(R.layout.unified_comment_list_fragme

private fun showCommentDetails(commentId: Long, commentStatus: CommentStatus) {
currentSnackbar?.dismiss()
// This legacy list is only reached when the RS_UNIFIED_COMMENTS flag is off or the site
// can't use wordpress-rs (see ActivityLauncher.viewUnifiedComments), so it always pairs
// with the legacy detail; the rs list launches the rs detail itself.
// This legacy list is only reached when the site can't use wordpress-rs (see
// ActivityLauncher.viewUnifiedComments), so it always pairs with the legacy detail;
// the rs list launches the rs detail itself.
commentDetails.launch(
CommentDetailsActivityRequest(
commentId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
import org.wordpress.android.ui.stats.StatsViewType;
import org.wordpress.android.ui.stats.refresh.utils.StatsLaunchedFrom;
import org.wordpress.android.util.AppLog;
import org.wordpress.android.util.SiteUtils;
import org.wordpress.android.util.StringUtils;
import org.wordpress.android.util.ToastUtils;
import org.wordpress.android.util.analytics.AnalyticsUtils;
Expand Down Expand Up @@ -407,7 +408,7 @@ private Fragment createDetailFragmentForNote(@NonNull Note note) {
SiteModel site = mSiteStore.getSiteBySiteId(note.getSiteId());
// The rs detail needs a real site from the SiteStore; the legacy fragment can fall
// back to a dummy WP.com site built from the note, so it stays the catch-all.
if (site != null && note.getCommentId() != 0 && ActivityLauncher.shouldUseRsComments(this, site)) {
if (site != null && note.getCommentId() != 0 && SiteUtils.canUseWpRs(site)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried for a while but was unable to generate a comment notification for testing this particular change. Were you able to test this flow?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcalhoun Yes, I was able to test this successfully.

fragment = UnifiedCommentDetailsFragment.newInstance(
site,
note.getCommentId(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,6 @@ class ExperimentalFeatures @Inject constructor(
"experimental_post_types",
R.string.experimental_post_types,
R.string.experimental_post_types_description
),
RS_UNIFIED_COMMENTS(
"rs_unified_comments",
R.string.experimental_rs_comments,
R.string.experimental_rs_comments_description
);
)
}
}
12 changes: 12 additions & 0 deletions WordPress/src/main/java/org/wordpress/android/util/SiteUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,18 @@ public static boolean isAccessedViaWPComRest(@NonNull SiteModel site) {
return site.getOrigin() == SiteModel.ORIGIN_WPCOM_REST;
}

/**
* Whether the wordpress-rs screens (posts, pages, comments) should be used for {@code site}
* instead of the legacy (FluxC) ones. Keep this as the single definition so those screens
* can't drift apart for the same site.
* <p>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* <p>
*

* An application password is the requirement. That includes WP.com-accessed sites, since
* Atomic and Jetpack sites can hold one (only simple WP.com sites can't).
*/
public static boolean canUseWpRs(@Nullable SiteModel site) {
return site != null && site.hasApplicationPassword();
}

public static String getSiteIconUrl(SiteModel site, int size) {
return PhotonUtils.getPhotonImageUrl(site.getIconUrl(), size, size, PhotonUtils.Quality.HIGH,
site.isPrivateWPComAtomic());
Expand Down
2 changes: 0 additions & 2 deletions WordPress/src/main/res/values/strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -1028,8 +1028,6 @@
<string name="new_stats_intro_trends_title">Trends at a Glance</string>
<string name="new_stats_intro_trends_desc">Every metric shows how it\'s trending compared to the previous period, so you always know if you\'re growing.</string>
<string name="new_stats_intro_footer">You can turn off New Stats from the overflow menu in the Stats screen.</string>
<string name="experimental_rs_comments">RS Unified Comments</string>
<string name="experimental_rs_comments_description">Show comments in a more modern UI for WordPress.com and self-hosted sites with application passwords</string>
<string name="post_rs_failed_to_load">Failed to load post</string>
<string name="page_rs_failed_to_load">Failed to load page</string>
<string name="page_rs_copy_url">Copy URL</string>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -192,4 +192,51 @@ class SiteUtilsTest {
val circularSiteImage = SiteUtils.getSiteImageType(false, CIRCULAR)
assertThat(circularSiteImage).isEqualTo(BLAVATAR_CIRCULAR)
}

@Test
fun `canUseWpRs returns false for a null site`() {
assertThat(SiteUtils.canUseWpRs(null)).isFalse()
}

@Test
fun `canUseWpRs returns false when the site has no application password`() {
val site = SiteModel()

assertThat(SiteUtils.canUseWpRs(site)).isFalse()
}

@Test
fun `canUseWpRs returns false when only one half of the credentials is stored`() {
val site = SiteModel()
site.apiRestUsernamePlain = "user"

assertThat(SiteUtils.canUseWpRs(site)).isFalse()

site.apiRestUsernamePlain = null
site.apiRestPasswordPlain = "password"

assertThat(SiteUtils.canUseWpRs(site)).isFalse()
}

@Test
fun `canUseWpRs returns true when the site has an application password`() {
val site = SiteModel()
site.apiRestUsernamePlain = "user"
site.apiRestPasswordPlain = "password"

assertThat(SiteUtils.canUseWpRs(site)).isTrue()
}

@Test
fun `canUseWpRs returns true for a WPCom site with an application password`() {
val site = SiteModel()
site.setIsWPCom(true)
site.origin = SiteModel.ORIGIN_WPCOM_REST
site.apiRestUsernamePlain = "user"
site.apiRestPasswordPlain = "password"

// Atomic and Jetpack sites can hold an application password, so being reachable over the
// WP.com REST API does not keep a site on the legacy screens.
assertThat(SiteUtils.canUseWpRs(site)).isTrue()
}
}
Loading