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

Jump to bottom
Merged
merged 5 commits into from
Feb 9, 2022
Merged

Removing sign in tester from checkForNewRelease #3397

merged 5 commits into from
Feb 9, 2022

Conversation

manny-jimenez
Copy link
Contributor

No description provided.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Feb 3, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-appdistribution

    Overall coverage changed from 76.42% (7f09987) to 76.43% (3692aa5) by +0.02%.

    FilenameBase (7f09987)Merge (3692aa5)Diff
    FirebaseAppDistribution.java94.44%94.47%+0.03%

Test Logs

Notes

  • Commit (3692aa5) is created by Prow via merging PR base commit (7f09987) and head commit (eedbeeb).
  • Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/9Ziet9UkLe.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Feb 3, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-appdistribution

    TypeBase (7f09987)Merge (3692aa5)Diff
    aar123 kB123 kB-25 B (-0.0%)
    apk (release)1.55 MB1.55 MB-160 B (-0.0%)

Test Logs

Notes

  • Commit (3692aa5) is created by Prow via merging PR base commit (7f09987) and head commit (eedbeeb).

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/OTAb1vVn3L.html

rachaprince
rachaprince approved these changes Feb 3, 2022
View reviewed changes
...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java
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

lfkellogg
lfkellogg reviewed Feb 4, 2022
View reviewed changes
...pdistribution/src/main/java/com/google/firebase/appdistribution/FirebaseAppDistribution.java Outdated
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

@manny-jimenez manny-jimenez merged commit 10296e1 into master Feb 9, 2022
@manny-jimenez manny-jimenez deleted the fad-remove-signin-from-checkForNewRelease branch February 9, 2022 14:52
qdpham13 pushed a commit that referenced this pull request Feb 15, 2022
@manny-jimenez @qdpham13
Removing sign in tester from checkForNewRelease (#3397)
7a495f2 
* Removing sign in tester from checkForNewRelease

* Fixing tests

* removing comment

* Fixing message

* Fixing tests

Co-authored-by: Manny Jimenez <[email protected]>
jeremyjiang-dev pushed a commit that referenced this pull request Mar 9, 2022
@manny-jimenez @jeremyjiang-dev
Removing sign in tester from checkForNewRelease (#3397)
d9ecb0e 
* Removing sign in tester from checkForNewRelease

* Fixing tests

* removing comment

* Fixing message

* Fixing tests

Co-authored-by: Manny Jimenez <[email protected]>
@firebase firebase locked and limited conversation to collaborators Mar 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lfkellogg lfkellogg lfkellogg left review comments

@rachaprince rachaprince Awaiting requested review from rachaprince

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@manny-jimenez @google-oss-bot @rachaprince @lfkellogg