Skip to content

Add info text argument to startFeedback #3971

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

Merged
merged 4 commits into from
Aug 8, 2022
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
3 changes: 2 additions & 1 deletion firebase-appdistribution-api/api.txt
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ package com.google.firebase.appdistribution {
method public boolean isTesterSignedIn();
method @NonNull public com.google.android.gms.tasks.Task<java.lang.Void> signInTester();
method public void signOutTester();
method public void startFeedback();
method public void startFeedback(int);
method public void startFeedback(@NonNull CharSequence);
method @NonNull public com.google.firebase.appdistribution.UpdateTask updateApp();
method @NonNull public com.google.firebase.appdistribution.UpdateTask updateIfNewReleaseAvailable();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,27 @@ public interface FirebaseAppDistribution {
* <li>If tester is not signed in, presents the tester with a Google Sign-in UI
* <li>Starts a full screen activity for the tester to compose and submit the feedback
* </ol>
*
* @param infoTextResourceId string resource ID of text to display to the tester before collecting
* feedback data (e.g. Terms and Conditions)
*/
void startFeedback(int infoTextResourceId);

/**
* Takes a screenshot, and starts an activity to collect and submit feedback from the tester.
*
* <p>Performs the following actions:
*
* <ol>
* <li>Takes a screenshot of the current activity
* <li>If tester is not signed in, presents the tester with a Google Sign-in UI
* <li>Starts a full screen activity for the tester to compose and submit the feedback
* </ol>
*
* @param infoText text to display to the tester before collecting feedback data (e.g. Terms and
* Conditions)
*/
void startFeedback();
void startFeedback(@NonNull CharSequence infoText);

/** Gets the singleton {@link FirebaseAppDistribution} instance. */
@NonNull
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,12 @@ public UpdateTask updateApp() {
}

@Override
public void startFeedback() {
delegate.startFeedback();
public void startFeedback(int infoTextResourceId) {
delegate.startFeedback(infoTextResourceId);
}

@Override
public void startFeedback(@NonNull CharSequence infoText) {
delegate.startFeedback(infoText);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,12 @@ public UpdateTask updateApp() {
}

@Override
public void startFeedback() {
public void startFeedback(int infoTextResourceId) {
return;
}

@Override
public void startFeedback(@NonNull CharSequence infoText) {
return;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@

import android.graphics.Bitmap;
import android.os.Bundle;
import android.text.method.LinkMovementMethod;
import android.view.View;
import android.widget.EditText;
import android.widget.ImageView;
import android.widget.TextView;
import android.widget.Toast;
import androidx.annotation.Nullable;
import androidx.appcompat.app.AppCompatActivity;
Expand All @@ -33,17 +35,21 @@ public class FeedbackActivity extends AppCompatActivity {

public static final String RELEASE_NAME_EXTRA_KEY =
"com.google.firebase.appdistribution.FeedbackActivity.RELEASE_NAME";
public static final String INFO_TEXT_EXTRA_KEY =
"com.google.firebase.appdistribution.FeedbackActivity.INFO_TEXT";
public static final String SCREENSHOT_FILENAME_EXTRA_KEY =
"com.google.firebase.appdistribution.FeedbackActivity.SCREENSHOT_FILE_NAME";

private FeedbackSender feedbackSender;
private String releaseName;
private CharSequence infoText;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, how do you choose CharSequence instead of a plain ole' String?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that's what Context.getText() returns: https://developer.android.com/reference/android/content/Context#getText(int)

Context.getString() returns a String but it does not retain any html/styles, which we need.

@Nullable private File screenshotFile;

@Override
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
releaseName = getIntent().getStringExtra(RELEASE_NAME_EXTRA_KEY);
infoText = getIntent().getCharSequenceExtra(INFO_TEXT_EXTRA_KEY);
if (getIntent().hasExtra(SCREENSHOT_FILENAME_EXTRA_KEY)) {
screenshotFile = getFileStreamPath(getIntent().getStringExtra(SCREENSHOT_FILENAME_EXTRA_KEY));
}
Expand All @@ -53,9 +59,14 @@ protected void onCreate(Bundle savedInstanceState) {

private void setupView() {
setContentView(R.layout.activity_feedback);

TextView infoTextView = this.findViewById(R.id.infoText);
infoTextView.setText(infoText);
infoTextView.setMovementMethod(LinkMovementMethod.getInstance());

Bitmap thumbnail = readThumbnail();
if (thumbnail != null) {
ImageView screenshotImageView = (ImageView) this.findViewById(R.id.thumbnail);
ImageView screenshotImageView = this.findViewById(R.id.thumbnail);
screenshotImageView.setImageBitmap(thumbnail);
} else {
View screenshotErrorLabel = this.findViewById(R.id.screenshotErrorLabel);
Expand All @@ -73,7 +84,7 @@ private Bitmap readThumbnail() {

public void submitFeedback(View view) {
setSubmittingStateEnabled(true);
EditText feedbackText = (EditText) findViewById(R.id.feedbackText);
EditText feedbackText = findViewById(R.id.feedbackText);
feedbackSender
.sendFeedback(releaseName, feedbackText.getText().toString(), screenshotFile)
.addOnSuccessListener(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.AUTHENTICATION_FAILURE;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.HOST_ACTIVITY_INTERRUPTED;
import static com.google.firebase.appdistribution.FirebaseAppDistributionException.Status.UPDATE_NOT_AVAILABLE;
import static com.google.firebase.appdistribution.impl.FeedbackActivity.INFO_TEXT_EXTRA_KEY;
import static com.google.firebase.appdistribution.impl.FeedbackActivity.RELEASE_NAME_EXTRA_KEY;
import static com.google.firebase.appdistribution.impl.FeedbackActivity.SCREENSHOT_FILENAME_EXTRA_KEY;
import static com.google.firebase.appdistribution.impl.TaskUtils.safeSetTaskException;
Expand All @@ -44,7 +45,6 @@
import com.google.firebase.appdistribution.UpdateStatus;
import com.google.firebase.appdistribution.UpdateTask;
import java.util.concurrent.Executor;
import java.util.concurrent.Executors;

/**
* This class is the "real" implementation of the Firebase App Distribution API which should only be
Expand All @@ -63,6 +63,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
private final SignInStorage signInStorage;
private final ReleaseIdentifier releaseIdentifier;
private final ScreenshotTaker screenshotTaker;
private final Executor taskExecutor;

private final Object updateIfNewReleaseTaskLock = new Object();

Expand Down Expand Up @@ -95,7 +96,8 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
@NonNull SignInStorage signInStorage,
@NonNull FirebaseAppDistributionLifecycleNotifier lifecycleNotifier,
@NonNull ReleaseIdentifier releaseIdentifier,
@NonNull ScreenshotTaker screenshotTaker) {
@NonNull ScreenshotTaker screenshotTaker,
@NonNull Executor taskExecutor) {
this.firebaseApp = firebaseApp;
this.testerSignInManager = testerSignInManager;
this.newReleaseFetcher = newReleaseFetcher;
Expand All @@ -105,6 +107,7 @@ class FirebaseAppDistributionImpl implements FirebaseAppDistribution {
this.releaseIdentifier = releaseIdentifier;
this.lifecycleNotifier = lifecycleNotifier;
this.screenshotTaker = screenshotTaker;
this.taskExecutor = taskExecutor;
lifecycleNotifier.addOnActivityDestroyedListener(this::onActivityDestroyed);
lifecycleNotifier.addOnActivityPausedListener(this::onActivityPaused);
lifecycleNotifier.addOnActivityResumedListener(this::onActivityResumed);
Expand Down Expand Up @@ -306,12 +309,12 @@ private UpdateTask updateApp(boolean showDownloadInNotificationManager) {
}

@Override
public void startFeedback() {
startFeedback(Executors.newSingleThreadExecutor());
public void startFeedback(int infoTextResourceId) {
startFeedback(firebaseApp.getApplicationContext().getString(infoTextResourceId));
}

@VisibleForTesting
public void startFeedback(Executor taskExecutor) {
public void startFeedback(@NonNull CharSequence infoText) {
screenshotTaker
.takeScreenshot()
.onSuccessTask(
Expand All @@ -327,16 +330,19 @@ public void startFeedback(Executor taskExecutor) {
.onSuccessTask(taskExecutor, unused -> releaseIdentifier.identifyRelease())
.onSuccessTask(
taskExecutor,
releaseName -> launchFeedbackActivity(releaseName, screenshotFilename)))
releaseName ->
launchFeedbackActivity(releaseName, infoText, screenshotFilename)))
.addOnFailureListener(
taskExecutor, e -> LogWrapper.getInstance().e("Failed to launch feedback flow", e));
}

private Task<Void> launchFeedbackActivity(String releaseName, String screenshotFilename) {
private Task<Void> launchFeedbackActivity(
String releaseName, CharSequence infoText, String screenshotFilename) {
return lifecycleNotifier.consumeForegroundActivity(
activity -> {
Intent intent = new Intent(activity, FeedbackActivity.class);
intent.putExtra(RELEASE_NAME_EXTRA_KEY, releaseName);
intent.putExtra(INFO_TEXT_EXTRA_KEY, infoText);
intent.putExtra(SCREENSHOT_FILENAME_EXTRA_KEY, screenshotFilename);
activity.startActivity(intent);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.google.firebase.platforminfo.LibraryVersionComponent;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.Executors;

/**
* Registers FirebaseAppDistribution.
Expand Down Expand Up @@ -92,7 +93,8 @@ private FirebaseAppDistribution buildFirebaseAppDistribution(ComponentContainer
signInStorage,
lifecycleNotifier,
releaseIdentifier,
new ScreenshotTaker(firebaseApp, lifecycleNotifier));
new ScreenshotTaker(firebaseApp, lifecycleNotifier),
Executors.newSingleThreadExecutor());

if (context instanceof Application) {
Application firebaseApplication = (Application) context;
Expand Down
23 changes: 17 additions & 6 deletions firebase-appdistribution/src/main/res/layout/activity_feedback.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="match_parent"
android:padding="24dp"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this just generated XML changes? I kind of just skimmed this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These add the info text to our proof-of-concept UX.

tools:context=".FeedbackActivity">

<TextView
Expand Down Expand Up @@ -34,28 +35,29 @@
android:id="@+id/submitButton"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginTop="32dp"
android:text="Submit"
android:layout_marginTop="24dp"
android:onClick="submitFeedback"
android:text="Submit"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintHorizontal_bias="0.5"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@+id/feedbackText" />
app:layout_constraintTop_toBottomOf="@+id/infoText" />
<TextView
android:id="@+id/loadingLabel"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:layout_marginTop="44dp"
android:layout_marginTop="24dp"
android:text="Submitting feedback..."
android:visibility="invisible"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@+id/feedbackText" />
<!-- Width and height should match bitmap created in setupView() -->
app:layout_constraintTop_toBottomOf="@+id/infoText" />
<ImageView
android:id="@+id/thumbnail"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:scaleType="centerInside"
android:layout_marginTop="24dp"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
Expand All @@ -72,5 +74,14 @@
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@+id/submitButton"
app:layout_constraintVertical_bias="0.403" />
<TextView
android:id="@+id/infoText"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:ems="5"
android:layout_marginTop="24dp"
app:layout_constraintEnd_toEndOf="parent"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toBottomOf="@+id/feedbackText" />

</androidx.constraintlayout.widget.ConstraintLayout>
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import static com.google.firebase.appdistribution.impl.ErrorMessages.NETWORK_ERROR;
import static com.google.firebase.appdistribution.impl.ErrorMessages.RELEASE_NOT_FOUND_ERROR;
import static com.google.firebase.appdistribution.impl.ErrorMessages.UPDATE_CANCELED;
import static com.google.firebase.appdistribution.impl.FeedbackActivity.INFO_TEXT_EXTRA_KEY;
import static com.google.firebase.appdistribution.impl.FeedbackActivity.RELEASE_NAME_EXTRA_KEY;
import static com.google.firebase.appdistribution.impl.FeedbackActivity.SCREENSHOT_FILENAME_EXTRA_KEY;
import static com.google.firebase.appdistribution.impl.TestUtils.assertTaskFailure;
Expand Down Expand Up @@ -122,6 +123,7 @@ public class FirebaseAppDistributionServiceImplTest {
private FirebaseAppDistributionImpl firebaseAppDistribution;
private TestActivity activity;
private FirebaseApp firebaseApp;
private ExecutorService taskExecutor = Executors.newSingleThreadExecutor();

@Mock private InstallationTokenResult mockInstallationTokenResult;
@Mock private TesterSignInManager mockTesterSignInManager;
Expand Down Expand Up @@ -162,7 +164,8 @@ public void setup() throws FirebaseAppDistributionException {
mockSignInStorage,
mockLifecycleNotifier,
mockReleaseIdentifier,
mockScreenshotTaker));
mockScreenshotTaker,
taskExecutor));

when(mockTesterSignInManager.signInTester()).thenReturn(Tasks.forResult(null));
when(mockSignInStorage.getSignInStatus()).thenReturn(true);
Expand Down Expand Up @@ -632,11 +635,9 @@ public void updateApp_withApkReleaseAvailable_returnsSameApkTask() {

@Test
public void startFeedback_signsInTesterAndStartsActivity() throws InterruptedException {
ExecutorService testExecutor = Executors.newSingleThreadExecutor();
when(mockReleaseIdentifier.identifyRelease()).thenReturn(Tasks.forResult("release-name"));

firebaseAppDistribution.startFeedback(testExecutor);
TestUtils.awaitAsyncOperations(testExecutor);
firebaseAppDistribution.startFeedback("Some terms and conditions");
TestUtils.awaitAsyncOperations(taskExecutor);

ArgumentCaptor<Intent> argument = ArgumentCaptor.forClass(Intent.class);
verify(activity).startActivity(argument.capture());
Expand All @@ -645,5 +646,7 @@ public void startFeedback_signsInTesterAndStartsActivity() throws InterruptedExc
.isEqualTo("release-name");
assertThat(argument.getValue().getStringExtra(SCREENSHOT_FILENAME_EXTRA_KEY))
.isEqualTo(TEST_SCREENSHOT_FILE_NAME);
assertThat(argument.getValue().getStringExtra(INFO_TEXT_EXTRA_KEY))
.isEqualTo("Some terms and conditions");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ class MainActivity : AppCompatActivity() {
}

feedbackButton.setOnClickListener {
firebaseAppDistribution.startFeedback()
firebaseAppDistribution.startFeedback(R.string.terms_and_conditions)
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<resources>
<string name="app_name">App Distro Sample</string>
<string name="terms_and_conditions"><b>Before giving feedback</b> you might want to check out the <a href="http://google.com">Terms and Conditions</a></string>
</resources>