Add support for privacy policy URLs in sign-up #727
Conversation
|
@TheCraftKid this looks great! Can you provide a quick screenshot for me? |
|
The code looks great but tests are broken, I think there is somewhere in tests where we are using the |
|
Hey, I fixed the tests, but I'm trying to build the app to get a screenshot, and keep getting this error: |
There was a problem hiding this comment.
@samtstern Not so fast, I think there's a bit of broken code. 😨
@TheCraftKid Awesome, start! 😄 There's a little work to do, but it's almost there!
| android:layout_width="wrap_content" | ||
| android:layout_height="wrap_content" | ||
| android:checked="true" | ||
| android:text="@string/google_privacy_label"/> |
There was a problem hiding this comment.
Could you add another button like the Firebase Analytics privacy policy or something? Otherwise a single radio button is kinda useless... 😄
There was a problem hiding this comment.
I was looking for some kind of Fireabse privacy policy, but I could only find terms of service. That's a good idea.
| } | ||
|
|
||
| return GOOGLE_PRIVACY_POLICY_URL; | ||
| } |
There was a problem hiding this comment.
See my comment in the xml layout. If you don't want to add another privacy policy, we could just have it return null. Otherwise, this doesn't make much sense...
| int start = mAgreementText.length(); | ||
| spannableStringBuilder.setSpan(foregroundColorSpan, start, start + link.length(), 0); | ||
|
|
||
| mAgreementText.append(" and the "); |
There was a problem hiding this comment.
Instead of doing this which isn't translatable, could we instead combine the setUpTermsOfService and setUpPrivacyPolicy methods to make the two clickable bits of text in one go? This should simplify things and fix the bug below.
|
|
||
| mAgreementText.append(" and the "); | ||
| mAgreementText.append(spannableStringBuilder); | ||
| mAgreementText.setOnClickListener(new View.OnClickListener() { |
There was a problem hiding this comment.
I don't think this will work. Try clicking the Terms of Service link and it will open the privacy policy instead. Oops! 😄 You'll need to make only the bits of text clickable instead of the entire view like so: https://stackoverflow.com/a/10697453/4548500.
There was a problem hiding this comment.
Hmmm, there's another bug I just thought about: if the dev provides a PP but not a TOS, the agreement text will be something like and the privacy policy. I'm thinking about how we can rewrite this in a sustainable way.
There was a problem hiding this comment.
I was debating whether to combine setting the terms of service and privacy policy text in a single method, but I was a bit afraid to touch anything else. We can use two template strings in strings.xml for the possibility that either link doesn't exist.
There was a problem hiding this comment.
Yeah, that's what I did. Since you mentioned having issues with the Spans, I went ahead and created a patch for you: Fix_various_bugs.zip.
If you want to give it a go yourself, hack at it! 😄 Otherwise, just extract the zip and then apply the patch in Android Studio by calling up the shortcut: Ctrl/Cmd + Shift + A -> "Apply patch".
There was a problem hiding this comment.
Wow, thanks! With the string templates, I'm curious why you didn't use the normal Java ones. Is there any technical problem with doing so?
There was a problem hiding this comment.
I needed a way to manually inject the strings so that I could get their start and end indices:
private void replaceTarget(String target, @StringRes int replacementRes, String url) {
int targetIndex = mBuilder.toString().indexOf(target);
if (targetIndex != -1) {
String replacement = mContext.getString(replacementRes);
mBuilder.replace(targetIndex, targetIndex + target.length(), replacement);
int end = targetIndex + replacement.length();
mBuilder.setSpan(mLinkSpan, targetIndex, end, 0);
mBuilder.setSpan(new CustomTabsSpan(url), targetIndex, end, 0);
}
}If you have any idea how to improve this mess, go for it! 😄
|
@TheCraftKid For your build error, make sure the Firebase test project you setup has a matching package: https://stackoverflow.com/a/39301178/4548500. Here's mine: |
| mBuilder.getChars(0, mBuilder.length(), currentPreambleChars, 0); | ||
| String currentPreamble = String.valueOf(currentPreambleChars); | ||
|
|
||
| int targetIndex = currentPreamble.indexOf(target); |
There was a problem hiding this comment.
@TheCraftKid whoops, forgot to mention that I updated my patch. Turns out the SpannableStringBuilder#toString() method does exactly what I just did so all of that can be replaced with mBuilder.toString().indexOf(target);
| import com.firebase.ui.auth.R; | ||
| import com.firebase.ui.auth.ui.FlowParameters; | ||
|
|
||
| public class PreambleHandler { |
There was a problem hiding this comment.
@samtstern LGTM now, but can you think of ways to improve this?
|
@TheCraftKid and @SUPERCILEX thank you both for getting this all ready. I won't actually have time to verify and merge this until after the long weekend, so just warning you in advance of my silence. But we're gonna get this in. |
|
@samtstern Awesome! 😄 @TheCraftKid Could you push the code simplification with This char[] currentPreambleChars = new char[mBuilder.length()];
mBuilder.getChars(0, mBuilder.length(), currentPreambleChars, 0);
String currentPreamble = String.valueOf(currentPreambleChars);
int targetIndex = currentPreamble.indexOf(target);should turn into this int targetIndex = mBuilder.toString().indexOf(target); |
This addresses and resolves #690.
(Sorry it took a bit, I had to find a way to add URL to the existing text spans.)