fix(avatars): when Undefined an error promise will return #4502

divyabiyani · 2016-12-14T07:22:01Z

shane-tomlinson

Thanks for this PR @divyabiyani! I left a giant comment inline about what I believe the correct fix should be.

shane-tomlinson · 2016-12-14T11:18:56Z

app/scripts/views/mixins/avatar-mixin.js

@@ -163,6 +163,9 @@ define(function (require, exports, module) {
    },

    deleteDisplayedAccountProfileImage (account) {
+      if ( ! this._displayedProfileImage ) {


It seems to me this solution sidesteps the problem but does not fix the cause of the problem, which seems to be inconsistent state. deleteDisplayedAccountProfileImage is called from removeAvatar in avatar_change.js. removeAvatar can only be called if we believe the account has a profile image. hasProfileImage is set if the account has a profileImageUrl.

So, we believe the account has a profileImageUrl, but this._displayedProfileImage is undefined, which means the assignment to this._displayedProfileImage has not been made.

This can happen because displayAccountProfileImage is called in avatar_change.js->afterVisible. afterVisible is called after the view is already visible (with the Clear button shown to the user), this means there is a window in which the user can click the Clear button before the profile image is fetched, and before this._displayedProfileImage is set.

This behavior was introduced in #3850 to fix #3543.

We are using the cached data in this._displayedProfileImage to get the profile image ID. The image ID should already be available on the account model, which is passed in to this function.

I believe the correct fix here is to not log and display the error you added here, but rather:

Use the image ID from the account model. If there is no image ID on the account model, THEN log and display an error.

OR

Ensure this._displayedProfileImage is set by calling this.displayAccountProfileImage(account) and then deleting the avatar whenever that function call completes.

We might have to try both approaches to see which works best.

shane-tomlinson · 2016-12-14T11:20:33Z

app/scripts/lib/profile-errors.js

@@ -45,6 +45,10 @@ define(function (require, exports, module) {
    UNEXPECTED_ERROR: {
      errno: 999,
      message: t('Unexpected error')
+    },
+    NO_DISPLAYED_PROFILEIMAGE_FOUND: {
+      errno: 1049,


For any error that you add to a module, please use consecutive numbers, so this would be 1000. Errno's in each module are namespaced appropriately in our logs so we can tell the difference between errno 1000 in profile-errors and errno 1000 in auth-errors.

Also, I imagine in the end this won't be the error used. Instead it'll be something like NO_PROFILE_IMAGE_ID

shane-tomlinson · 2016-12-14T11:20:57Z

app/scripts/lib/profile-errors.js

+    },
+    NO_DISPLAYED_PROFILEIMAGE_FOUND: {
+      errno: 1049,
+      message: t('No Displayed Profile Image')


Please sentence case any errors, so the only word with a capital letter will be No

Ohk @shane-tomlinson .
I will do the necessary changes.

GitCop · 2016-12-14T14:15:40Z

There were the following issues with your Pull Request

Commit: 1c87647
Your subject line is longer than 100 characters

Guidelines are available at https://github.com/mozilla/fxa-content-server/blob/master/CONTRIBUTING.md#git-commit-guidelines

This message was auto-generated by https://gitcop.com

vladikoff · 2016-12-14T15:33:39Z

app/scripts/views/mixins/avatar-mixin.js

@@ -96,7 +97,7 @@ define(function (require, exports, module) {
      avatarWrapperEl.addClass('with-default');
    },

-    // Makes sure the account has an up-to-date image cache.
+    // Makes sure the account has an up-to-date image casche.


what happened here?

vladikoff · 2016-12-14T15:37:22Z

@divyabiyani if we go with using displayAccountProfileImage, we need to keep in mind that it is async. you need to chain .then see https://github.com/mozilla/fxa-content-server/pull/4502/files#diff-69c1b4c151f87a5c9ad048f78656cf9aR73

Also do we only call it when _displayedProfileImage is undefined?

cc @shane-tomlinson

shane-tomlinson · 2016-12-14T20:51:12Z

Also do we only call it when _displayedProfileImage is undefined?

I'd say yes, or rather, a similar approach using the image ID from the account. I'd use the image ID from the account and only fetch the profile image if the account doesn't have an image ID already.

Rationale is - something about storing and relying on _displayedProfileImage seems fishy if the account already has an image ID. The only two places _displayedProfileImage is used is in deleteDisplayedAccountProfileImage which already accepts an account that should have the profile image ID, and in hasDisplayedAccountProfileImage which is not used anywhere:

➜  scripts git:(master) pwd
/Users/stomlinson/development/fxa-content-server/app/scripts
➜  scripts git:(master) rgrep hasDisplayedAccountProfileImage .  
./views/mixins/avatar-mixin.js:    hasDisplayedAccountProfileImage () {
➜  scripts git:(master)

To me it seems the optimal fix/cleanup is:

Remove hasDisplayedAccountProfileImage and any associated tests. It's not used. No use keeping it in the code base.
Use the profile image ID from the account. If the account doesn't have an image ID, then fetch it.
In avatar-mixin.js, remove all references to this._displayedProfileImage, including the assignment. We would be getting the profile image data directly from the account and would not need the local reference. This makes us have 1 source of truth, which is (almost) always better than > 1.

vladikoff · 2016-12-14T22:48:59Z

@divyabiyani see comment above ^, let us know if something is not clear.

Try running git rebase origin/master to fetch latest updates of the repository. Let me know if there are issues.

GitCop · 2016-12-15T11:12:15Z

There were the following issues with your Pull Request

Commit: f6a9344
Your subject line is longer than 100 characters
Invalid type. Valid types are feat, fix, docs, style, refactor, perf, test, chore, revert

Guidelines are available at https://github.com/mozilla/fxa-content-server/blob/master/CONTRIBUTING.md#git-commit-guidelines

This message was auto-generated by https://gitcop.com

shane-tomlinson · 2016-12-16T14:03:15Z

app/scripts/views/mixins/avatar-mixin.js

-          this.updateProfileImage(new ProfileImage(), account);
-        });
+      if ( ! account.get('profileImageId') ) {
+        return account.fetchCurrentProfileImage()


This looks like the proper solution. I'm going to propose a slight simplification that reworks lines 161-182 to use promise chaining and eliminate duplicate code but still Do The Right Thing.

return p().then(() => { if (! account.get('profileImageId')) { return account.fetchCurrentProfileImage() .then((profileImage) => { // Cache the result to make sure we don't flash the default // image while fetching the latest profile image // this should set the profileImageId on the account. this._updateCachedProfileImage(profileImage, account); }); } // if we reach here, the account has a profile image ID already. }) // by this point, the account will have an avatar and a profileImageId .then(() => account.deleteAvatar(account.get('profileImageId'))) .then(() => this.updateProfileImage(new ProfileImage(), account));

Do you have much experience with promises? If not, they are a bit strange to get used to, but once you do, they are pretty great. A good intro is: https://developers.google.com/web/fundamentals/getting-started/primers/promises

@shane-tomlinson Hi, I have been studying about promises from past few days. @vladikoff gave me the intro link. 😄
Also, is the code fine now?

vladikoff · 2016-12-16T16:36:43Z

app/scripts/views/mixins/avatar-mixin.js

-      return account.deleteAvatar(this._displayedProfileImage.get('id'))
+      return p()
+        .then(() => {
+          if ( ! account.get('profileImageId') ) {


review nit: no space before ! or here account.get('profileImageId') ) .

Remove the space before the last parenthesis.

@pdehaan is there an ESlint rule that can help us track these?

vladikoff · 2016-12-16T16:36:54Z

app/scripts/views/mixins/avatar-mixin.js

@@ -87,10 +86,6 @@ define(function (require, exports, module) {
        });
    },

-    hasDisplayedAccountProfileImage () {
-      return this._displayedProfileImage && ! this._displayedProfileImage.isDefault();
-    },


@divyabiyani nice 👍

vladikoff · 2016-12-16T19:05:04Z

Great work @divyabiyani !

rfk added the waffle:active label Dec 14, 2016

divyabiyani changed the title ~~fix(displayedProfileImage): When Undefined an error promise will return~~ [WIP]:fix(displayedProfileImage): When Undefined an error promise will return Dec 14, 2016

divyabiyani force-pushed the divyabiyani:issue-4386 branch from 4744c18 to ecacd67 Dec 14, 2016

divyabiyani changed the title ~~[WIP]:fix(displayedProfileImage): When Undefined an error promise will return~~ fix(displayedProfileImage): When Undefined an error promise will return Dec 14, 2016

shane-tomlinson suggested changes Dec 14, 2016

View changes

shane-tomlinson reviewed Dec 14, 2016

View changes

divyabiyani changed the title ~~fix(displayedProfileImage): When Undefined an error promise will return~~ [WIP]:fix(displayedProfileImage): When Undefined an error promise will return Dec 14, 2016

vladikoff reviewed Dec 14, 2016

View changes

divyabiyani force-pushed the divyabiyani:issue-4386 branch from 9d8c3c7 to f6a9344 Dec 14, 2016

vladikoff changed the title ~~[WIP]:fix(displayedProfileImage): When Undefined an error promise will return~~ [WIP] fix(avatars): when Undefined an error promise will return Dec 14, 2016

divyabiyani force-pushed the divyabiyani:issue-4386 branch from e21c18d to a488e5f Dec 15, 2016

divyabiyani changed the title ~~[WIP] fix(avatars): when Undefined an error promise will return~~ fix(avatars): when Undefined an error promise will return Dec 16, 2016

shane-tomlinson reviewed Dec 16, 2016

View changes

divyabiyani force-pushed the divyabiyani:issue-4386 branch 2 times, most recently from 70b0bc1 to 8dd2e2c Dec 16, 2016

vladikoff reviewed Dec 16, 2016

View changes

divyabiyani force-pushed the divyabiyani:issue-4386 branch from 15f12fa to 3a8be1d Dec 16, 2016

fix(avatar): this._displayedProfileImage removed

3a8be1d

vladikoff removed the waffle:active label Dec 16, 2016

divyabiyani deleted the divyabiyani:issue-4386 branch Dec 18, 2016

mozilla / fxa-content-server Archived

fix(avatars): when Undefined an error promise will return #4502

fix(avatars): when Undefined an error promise will return #4502

divyabiyani commented Dec 14, 2016

shane-tomlinson left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GitCop commented Dec 14, 2016

This comment has been minimized.

vladikoff commented Dec 14, 2016

shane-tomlinson commented Dec 14, 2016

vladikoff commented Dec 14, 2016

GitCop commented Dec 15, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vladikoff commented Dec 16, 2016

mozilla / fxa-content-server Archived

fix(avatars): when Undefined an error promise will return #4502

fix(avatars): when Undefined an error promise will return #4502

Conversation

divyabiyani commented Dec 14, 2016

shane-tomlinson left a comment

This comment has been minimized.

shane-tomlinson Dec 14, 2016 Member

This comment has been minimized.

shane-tomlinson Dec 14, 2016 Member

This comment has been minimized.

shane-tomlinson Dec 14, 2016 Member

This comment has been minimized.

shane-tomlinson Dec 14, 2016 Member

This comment has been minimized.

divyabiyani Dec 14, 2016 Author Member

GitCop commented Dec 14, 2016

This comment has been minimized.

vladikoff Dec 14, 2016 Contributor

vladikoff commented Dec 14, 2016

shane-tomlinson commented Dec 14, 2016

vladikoff commented Dec 14, 2016

GitCop commented Dec 15, 2016

This comment has been minimized.

shane-tomlinson Dec 16, 2016 • edited Member

This comment has been minimized.

divyabiyani Dec 16, 2016 Author Member

This comment has been minimized.

vladikoff Dec 16, 2016 Contributor

This comment has been minimized.

vladikoff Dec 16, 2016 Contributor

vladikoff commented Dec 16, 2016

shane-tomlinson Dec 14, 2016
Member

shane-tomlinson Dec 14, 2016
Member

shane-tomlinson Dec 14, 2016
Member

shane-tomlinson Dec 14, 2016
Member

divyabiyani Dec 14, 2016
Author Member

vladikoff Dec 14, 2016
Contributor

shane-tomlinson Dec 16, 2016 •

edited

Member

divyabiyani Dec 16, 2016
Author Member

vladikoff Dec 16, 2016
Contributor

vladikoff Dec 16, 2016
Contributor