Skip to content

Added main ToS/PP #1300

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 8 commits into from
May 17, 2018
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -20,9 +20,12 @@
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.annotation.RestrictTo;
import android.support.annotation.StringRes;
import android.support.annotation.StyleRes;
import android.text.TextUtils;

import com.firebase.ui.auth.AuthUI.IdpConfig;
import com.firebase.ui.auth.R;
import com.firebase.ui.auth.util.ExtraConstants;
import com.firebase.ui.auth.util.Preconditions;

Expand Down Expand Up @@ -128,4 +131,24 @@ public FlowParameters[] newArray(int size) {
return new FlowParameters[size];
}
};

public boolean isSingleProviderFlow() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use this in SignInKickstarter too? I like your method much better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

return providerInfo.size() == 1;
}

public @StringRes int getGlobalTermsStringResource() {
boolean hasTos = !TextUtils.isEmpty(termsOfServiceUrl);
boolean hasPp = !TextUtils.isEmpty(privacyPolicyUrl);
if(hasTos && hasPp) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you run Ctrl + Alt + L to auto-format this file? I died a little inside seeing if(. 😁

Edit: Actually, I had a bunch of other style concerns so I've just created a patch to minimize the noise. Key takeaways:

  • Whitespace matters ⬜️🌌
  • There's nothing wrong with your annotation style, but our convention is to have them above the method
  • If there's copypasta code, you're probably doing something wrong 😉. Take a look at how I got rid of your new PreambleHandler constructor and made the setup overload act as a utility for the actual implementation instead of duplicating it.
  • Consistency above all else: I'm usually a little nitpicky (sorry! 😊), but all I really care about is that new code is consistent with the rest of the codebase. Imagine trying to read a book that switches fonts every few sentences—the meaning of what you're reading would get lost in your brain going "Wait, whaaaat?" Or at least, that's what happens with me. 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been spoiled by auto formatting, sorry! Thank you for your comments, it is much appreciated.

return R.string.fui_tos_and_pp;
} else if(hasTos) {
return R.string.fui_tos_only;
} else {
return R.string.fui_pp_only;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This else implies the default is the privacy policy—is that what you want? If so, we should add checks, update the README n' docs, throw exceptions, yada yada for AuthUI to ensure the developer must supply either a tos or a pp. Otherwise, we should return -1 and handle that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll change this. We won't force them to have a ToS/Privacy policy though.

}
}

public @StringRes int getGlobalTermsFooterStringResource() {
return R.string.fui_tos_and_pp_footer;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this have if statements like above? If not, let's just inline it in the XML.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added 2 more strings to deal with ToS only & Pp only. Having this makes more sense now :)

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,21 @@
import android.view.View;
import android.view.ViewGroup;
import android.widget.EditText;
import android.widget.TextView;

import com.firebase.ui.auth.R;
import com.firebase.ui.auth.data.model.FlowParameters;
import com.firebase.ui.auth.data.model.User;
import com.firebase.ui.auth.ui.FragmentBase;
import com.firebase.ui.auth.util.ExtraConstants;
import com.firebase.ui.auth.util.ui.ImeHelper;
import com.firebase.ui.auth.util.ui.PreambleHandler;
import com.firebase.ui.auth.util.ui.fieldvalidators.EmailFieldValidator;
import com.firebase.ui.auth.viewmodel.ResourceObserver;
import com.google.firebase.auth.EmailAuthProvider;

import java.util.List;

/**
* Fragment that shows a form with an email field and checks for existing accounts with that email.
* <p>
Expand Down Expand Up @@ -96,6 +101,9 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceStat
}

view.findViewById(R.id.button_next).setOnClickListener(this);

TextView termsView = view.<TextView>findViewById(R.id.email_tos_and_pp_text);
setUpTermsOfService(termsView);
}

@Override
Expand Down Expand Up @@ -175,4 +183,14 @@ private void validateAndProceed() {
mHandler.fetchProvider(email);
}
}

private void setUpTermsOfService(TextView termsText) {
FlowParameters flowParameters = getFlowParams();
if (flowParameters.isSingleProviderFlow()) {
PreambleHandler.setup(getContext(),
flowParameters,
flowParameters.getGlobalTermsStringResource(),
termsText);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import android.support.design.widget.TextInputLayout;
import android.support.v4.app.FragmentTransaction;
import android.support.v4.view.ViewCompat;
import android.widget.TextView;

import com.firebase.ui.auth.AuthUI;
import com.firebase.ui.auth.IdpResponse;
Expand All @@ -32,6 +33,7 @@
import com.firebase.ui.auth.ui.idp.WelcomeBackIdpPrompt;
import com.firebase.ui.auth.util.ExtraConstants;
import com.firebase.ui.auth.util.data.ProviderUtils;
import com.firebase.ui.auth.util.ui.PreambleHandler;
import com.firebase.ui.auth.viewmodel.RequestCodes;
import com.google.firebase.auth.EmailAuthProvider;

Expand Down Expand Up @@ -69,6 +71,8 @@ protected void onCreate(@Nullable Bundle savedInstanceState) {
.replace(R.id.fragment_register_email, fragment, CheckEmailFragment.TAG)
.disallowAddToBackStack()
.commit();

setUpTermsOfServiceFooter();
}

@Override
Expand Down Expand Up @@ -130,4 +134,14 @@ private void setSlideAnimation() {
// Make the next activity slide in
overridePendingTransition(R.anim.fui_slide_in_right, R.anim.fui_slide_out_left);
}

private void setUpTermsOfServiceFooter() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm seeing this method a lot. I wonder if it makes sense to add a PrivacyDisclosureUtils class that includes this and the tos methods you added in FlowParameters (I'm not a huge fan of logic in a POJO). @samtstern What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a good idea to me, less repetition is better!

FlowParameters flowParameters = getFlowParams();
if(!flowParameters.isSingleProviderFlow()) {
PreambleHandler.setup(EmailActivity.this,
flowParameters,
flowParameters.getGlobalTermsFooterStringResource(),
(TextView) findViewById(R.id.email_footer_tos_and_pp_text));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import android.view.View;
import android.view.ViewGroup;
import android.widget.EditText;
import android.widget.TextView;

import com.firebase.ui.auth.AuthUI;
import com.firebase.ui.auth.IdpResponse;
Expand All @@ -22,7 +21,6 @@
import com.firebase.ui.auth.util.ExtraConstants;
import com.firebase.ui.auth.util.data.ProviderUtils;
import com.firebase.ui.auth.util.ui.ImeHelper;
import com.firebase.ui.auth.util.ui.PreambleHandler;
import com.firebase.ui.auth.util.ui.fieldvalidators.BaseValidator;
import com.firebase.ui.auth.util.ui.fieldvalidators.EmailFieldValidator;
import com.firebase.ui.auth.util.ui.fieldvalidators.NoOpValidator;
Expand Down Expand Up @@ -140,11 +138,6 @@ public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceStat
// Only show the name field if required
nameInput.setVisibility(requireName ? View.VISIBLE : View.GONE);

PreambleHandler.setup(
getContext(),
getFlowParams(),
R.string.fui_button_text_save,
view.<TextView>findViewById(R.id.create_account_text));
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O && getFlowParams().enableCredentials) {
mEmailEditText.setImportantForAutofill(View.IMPORTANT_FOR_AUTOFILL_NO);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import android.view.View;
import android.view.ViewGroup;
import android.widget.ImageView;
import android.widget.TextView;
import android.widget.Toast;

import com.firebase.ui.auth.AuthUI;
Expand All @@ -42,6 +43,7 @@
import com.firebase.ui.auth.data.remote.PhoneSignInHandler;
import com.firebase.ui.auth.data.remote.TwitterSignInHandler;
import com.firebase.ui.auth.ui.AppCompatBase;
import com.firebase.ui.auth.util.ui.PreambleHandler;
import com.firebase.ui.auth.viewmodel.ResourceObserver;
import com.firebase.ui.auth.viewmodel.idp.ProviderSignInBase;
import com.firebase.ui.auth.viewmodel.idp.SocialProviderResponseHandler;
Expand Down Expand Up @@ -106,6 +108,8 @@ protected void onFailure(@NonNull Exception e) {
}
}
});

setUpTermsOfService();
}

private void populateIdpList(List<IdpConfig> providerConfigs,
Expand Down Expand Up @@ -204,6 +208,15 @@ public void onClick(View view) {
}
}

private void setUpTermsOfService() {
TextView termsText = (TextView) findViewById(R.id.main_tos_and_pp);
FlowParameters flowParameters = getFlowParams();
PreambleHandler.setup(AuthMethodPickerActivity.this,
flowParameters,
flowParameters.getGlobalTermsStringResource(),
termsText);
}

@Override
protected void onActivityResult(int requestCode, int resultCode, Intent data) {
super.onActivityResult(requestCode, resultCode, data);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import android.support.v7.app.AlertDialog;
import android.text.TextUtils;
import android.util.Log;
import android.widget.TextView;

import com.firebase.ui.auth.IdpResponse;
import com.firebase.ui.auth.R;
Expand All @@ -36,6 +37,7 @@
import com.firebase.ui.auth.ui.AppCompatBase;
import com.firebase.ui.auth.util.ExtraConstants;
import com.firebase.ui.auth.util.FirebaseAuthError;
import com.firebase.ui.auth.util.ui.PreambleHandler;
import com.google.android.gms.tasks.OnFailureListener;
import com.google.android.gms.tasks.OnSuccessListener;
import com.google.firebase.FirebaseException;
Expand Down Expand Up @@ -103,6 +105,7 @@ protected void onCreate(@Nullable Bundle savedInstanceState) {
.replace(R.id.fragment_verify_phone, fragment, VerifyPhoneNumberFragment.TAG)
.disallowAddToBackStack()
.commit();
setUpTermsOfServiceFooter();
}

@Override
Expand Down Expand Up @@ -406,4 +409,14 @@ private SubmitConfirmationCodeFragment getSubmitConfirmationCodeFragment() {
return (SubmitConfirmationCodeFragment) getSupportFragmentManager().findFragmentByTag
(SubmitConfirmationCodeFragment.TAG);
}

private void setUpTermsOfServiceFooter() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'm definitely thinking we need a utility, this is numeros quatros I've seen. 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😅

FlowParameters flowParameters = getFlowParams();
if(!flowParameters.isSingleProviderFlow()) {
PreambleHandler.setup(PhoneActivity.this,
flowParameters,
flowParameters.getGlobalTermsFooterStringResource(),
(TextView) findViewById(R.id.email_footer_tos_and_pp_text));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import com.firebase.ui.auth.util.ExtraConstants;
import com.firebase.ui.auth.util.ui.BucketedTextChangeListener;
import com.firebase.ui.auth.util.ui.ImeHelper;
import com.firebase.ui.auth.util.ui.PreambleHandler;

/**
* Display confirmation code to verify phone numbers input in {{@link VerifyPhoneNumberFragment}}
Expand All @@ -56,7 +55,6 @@ public class SubmitConfirmationCodeFragment extends FragmentBase {
private Button mSubmitConfirmationButton;
private CustomCountDownTimer mCountdownTimer;
private PhoneActivity mVerifier;
private TextView mAgreementText;
private long mMillisUntilFinished;

public static SubmitConfirmationCodeFragment newInstance(FlowParameters flowParameters,
Expand All @@ -83,7 +81,6 @@ public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup c
mResendCodeTextView = v.findViewById(R.id.resend_code);
mConfirmationCodeEditText = v.findViewById(R.id.confirmation_code);
mSubmitConfirmationButton = v.findViewById(R.id.submit_confirmation_code);
mAgreementText = v.findViewById(R.id.create_account_tos);

final String phoneNumber = getArguments().getString(ExtraConstants.PHONE);

Expand All @@ -93,7 +90,6 @@ public View onCreateView(@NonNull LayoutInflater inflater, @Nullable ViewGroup c
setupCountDown(RESEND_WAIT_MILLIS);
setupSubmitConfirmationCodeButton();
setupResendConfirmationCodeTextView(phoneNumber);
setUpTermsOfService();
return v;
}

Expand Down Expand Up @@ -221,13 +217,6 @@ private void cancelTimer() {
}
}

private void setUpTermsOfService() {
PreambleHandler.setup(getContext(),
getFlowParams(),
R.string.fui_continue_phone_login,
mAgreementText);
}

@VisibleForTesting(otherwise = VisibleForTesting.NONE)
CustomCountDownTimer getCountdownTimer() {
return mCountdownTimer;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import android.support.annotation.RestrictTo;
import android.support.annotation.StringRes;
import android.support.design.widget.TextInputLayout;
import android.support.v4.app.FragmentActivity;
import android.text.TextUtils;
Expand All @@ -42,6 +43,7 @@
import com.firebase.ui.auth.util.GoogleApiUtils;
import com.firebase.ui.auth.util.data.PhoneNumberUtils;
import com.firebase.ui.auth.util.ui.ImeHelper;
import com.firebase.ui.auth.util.ui.PreambleHandler;
import com.firebase.ui.auth.viewmodel.RequestCodes;
import com.google.android.gms.auth.api.credentials.Credential;
import com.google.android.gms.auth.api.credentials.CredentialPickerConfig;
Expand All @@ -55,7 +57,6 @@
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
public class VerifyPhoneNumberFragment extends FragmentBase implements View.OnClickListener {
public static final String TAG = "VerifyPhoneFragment";

private Context mAppContext;

private CountryListSpinner mCountryListSpinner;
Expand Down Expand Up @@ -107,15 +108,14 @@ public void onDonePressed() {
parentActivity.setTitle(getString(R.string.fui_verify_phone_number_title));
setupCountrySpinner();
setupSendCodeButton();
setupTerms();

return v;
}

private void setupTerms() {
final String verifyPhoneButtonText = getString(R.string.fui_verify_phone_number);
final String terms = getString(R.string.fui_sms_terms_of_service, verifyPhoneButtonText);
mSmsTermsText.setText(terms);
@Override
public void onViewCreated(@NonNull View view, @Nullable Bundle savedInstanceState) {
super.onViewCreated(view, savedInstanceState);
TextView termsText = view.<TextView>findViewById(R.id.send_sms_tos);
setUpTermsOfService(termsText);
}

@Override
Expand Down Expand Up @@ -273,6 +273,34 @@ private void setCountryCode(PhoneNumber phoneNumber) {
}
}

private void setUpTermsOfService(TextView termsText) {
FlowParameters flowParameters = getFlowParams();
if (flowParameters.isSingleProviderFlow()) {
PreambleHandler.setup(getContext(),
flowParameters,
R.string.fui_verify_phone_number,
getTermsSmsStringResource(),
termsText);
} else {
final String verifyPhoneButtonText = getString(R.string.fui_verify_phone_number);
final String terms = getString(R.string.fui_sms_terms_of_service,
verifyPhoneButtonText);
mSmsTermsText.setText(terms);
}
}

private @StringRes int getTermsSmsStringResource() {
boolean hasTos = !TextUtils.isEmpty(getFlowParams().termsOfServiceUrl);
boolean hasPp = !TextUtils.isEmpty(getFlowParams().privacyPolicyUrl);
if (hasTos && hasPp) {
return R.string.fui_sms_terms_of_service_and_privacy_policy_extended;
} else if (hasTos) {
return R.string.fui_sms_terms_of_service_only_extended;
} else {
return R.string.fui_sms_privacy_policy_only_extended;
}
}

void showError(String e) {
mPhoneInputLayout.setError(e);
}
Expand Down
Loading