Issue/7140 add epilogue email#7177
Conversation
| } | ||
| } else { | ||
| if (hasMagicLinkLoginIntent()) { | ||
| if (hasMagicLinkLoginIntent() || hasMagicLinkSignupIntent()) { |
There was a problem hiding this comment.
Is the hasMagicLinkSignupIntent() condition needed? Can it be true while hasMagicLinkLoginIntent() is false?
There was a problem hiding this comment.
That is true that hasMagicLinkLoginIntent() must be true if hasMagicLinkSignupIntent() is true since they have the same scheme and host. I removed hasMagicLinkSignupIntent() from this conditional in 722916d.
| } | ||
|
|
||
| mDispatcher.dispatch(AccountActionBuilder.newPushSettingsAction(payload)); | ||
| } else if (changedPassword()) { |
There was a problem hiding this comment.
I notice that the check for password and username modification is via else if blocks but I wonder, can't those be changed in parallel to the display name?
There was a problem hiding this comment.
The display name and password can be changed in the same payload, but the username is different. Dispatching those two payloads simultaneously makes it difficult to know when to continue since it's not guaranteed when each call will return. That's why I chose to cascade them.
I did notice a logic error in the updateAccountOrContinue() method. I updated it in 44382b5.
| if (mAccountStore.hasAccessToken()) { | ||
| // Check signup first since login and signup have the same action and host, | ||
| // but signup has an extra parameter. | ||
| if (hasMagicLinkSignupIntent()) { |
There was a problem hiding this comment.
hasMagicLinkSignupIntent() seems to only check for the extra new_user query param without checking for the rest of the magiclink format so, maybe we can include it inside the hasMagicLinkLoginIntent() conditional below to be more secure?
…cache and injecting request cache
…T twice" This reverts commit 44382b5.
| public void onSuccess() { | ||
| endProgress(); | ||
| mPhotoUrl = GravatarUtils.fixGravatarUrl(mAccount.getAccount().getAvatarUrl(), | ||
| getResources().getDimensionPixelSize(R.dimen.avatar_sz_login_epilogue)); |
There was a problem hiding this comment.
Good call incorporating the injectCache() track @theck13 , I can see the new image be used immediately on the epilogue after setting it.
Here's a thingie: turns out that the injected image response is for a URL that is slightly different to the URL loaded in the MeFragment after the epilogue is dimissed, which causes the annoyance where the Me screen doesn't load the injected avatar.
Problem seems to be the size parameter which is different between these two screens. What do you think about handling this issue in this PR?
There was a problem hiding this comment.
Nice catch! I updated the size in SignupEpilogueFragment.java to match MeFragment.java in c75f5a7.
There was a problem hiding this comment.
Works like a charm now, thanks! 👍
|
LGTM! |
Fix
Add epilogue for email as described in #7140.
Test
a. If display name, username, or password were changed, notice progress dialog.
b. If display name, username, or password were not changed, notice no progress dialog.
Note
Some methods (i.e.
startCropActivityandstartGravatarUpload) were taken fromMeFragmentto handle uploading an avatar image. We should consider moving these methods to a utility class or library to eliminate duplicated code. We could also move methods likecreateDisplayNameFromEmailandcreateUsernameFromEmailto a utility class or library for use elsewhere.