Skip to content

Removing sign in tester from checkForNewRelease #3397

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.

Sign up for GitHub

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 5 commits into from
Feb 9, 2022
Merged

Removing sign in tester from checkForNewRelease #3397

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3129d6d
Removing sign in tester from checkForNewRelease
Feb 3, 2022
1522061
Fixing tests
Feb 3, 2022
c402427
removing comment
Feb 3, 2022
998d9f7
Fixing message
Feb 8, 2022
eedbeeb
Fixing tests
Feb 8, 2022
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
10 changes: 8 additions & 2 deletions ...stribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,9 +252,15 @@ public synchronized Task<AppDistributionRelease> checkForNewRelease() {
LogWrapper.getInstance().v("Response in progress");
return cachedCheckForNewReleaseTask;
}
if (!isTesterSignedIn()) {
return Tasks.forException(
new FirebaseAppDistributionException(
Constants.ErrorMessages.AUTHENTICATION_ERROR, AUTHENTICATION_FAILURE));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the status works but I don't think the message is accurate. Could it be something like "Tester not signed in"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is "Failed to authenticate the tester" incorrect? The sentence seems pretty accurate. If the tester is not signed in you cannot authenticate them. This message is also used in getExceptionForHttpResponse in the testerAPiClient and the tester sign in manager. Changing it too "Tester not signed in" could work for those, but it doesn't fit as well as the current message.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, maybe it's not technically inaccurate, but it's not very helpful. It's true we couldn't authenticate the user, but it's so vague that we're not helping them understand why or resolve the problem.

In the other cases you mention (we get back a 401, or we fail to get a FID) we might not even understand the reasons ourselves, so vague is probably OK. In this case though, we know the reason: the tester wasn't signed in. So I think we should say that in the error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is your thought to make a new status that is "Tester not signed in" or to change the message that is being shown in the other cases. In option two the other message is "Failed to authorize the tester". Which I am ok using for the generic cases if we don't make a new message

}

cachedCheckForNewReleaseTask =
signInTester()
Copy link

Choose a reason for hiding this comment

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

Could you remove the comment that starts on line 155? With this PR, we no longer need to qualify why we call signIn before calling checkForNewRelease

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

.onSuccessTask(unused -> this.newReleaseFetcher.checkForNewRelease())
this.newReleaseFetcher
.checkForNewRelease()
.onSuccessTask(
appDistributionReleaseInternal -> {
setCachedNewRelease(appDistributionReleaseInternal);
Expand Down
9 changes: 4 additions & 5 deletions ...bution/src/test/java/com/google/firebase/appdistribution/FirebaseAppDistributionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,13 +188,12 @@ public void checkForNewRelease_whenCheckForNewReleaseFails_throwsError() {
}

@Test
public void checkForNewRelease_callsSignInTester() {
when(mockNewReleaseFetcher.checkForNewRelease())
.thenReturn(Tasks.forResult(TEST_RELEASE_NEWER_AAB_INTERNAL.build()));
public void checkForNewRelease_testerIsNotSignedIn_taskFails() {
when(firebaseAppDistribution.isTesterSignedIn()).thenReturn(false);

firebaseAppDistribution.checkForNewRelease();
Task<AppDistributionRelease> task = firebaseAppDistribution.checkForNewRelease();

verify(mockTesterSignInManager, times(1)).signInTester();
assertTaskFailure(task, AUTHENTICATION_FAILURE, AUTHENTICATION_ERROR);
}

@Test
Expand Down