[HOLD for payment 2022-12-20] [$250] Offline default avatar doesn't update after connecting to the internet reported by @thesahindia #12712
Comments
kavimuru
added
Daily
|
Triggered auto assignment to @laurenreidexpensify ( |
laurenreidexpensify
added
the
External
|
Current assignee @laurenreidexpensify is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat ( |
melvin-bot
bot
added
the
Help Wanted
|
Triggered auto assignment to @yuwenmemon ( |
melvin-bot
bot
changed the title
|
Proposal Lines 65 to 81 in c19e245 Solution From 3e223e2e2e78ece0109bbe2c8826083c21300b6c Mon Sep 17 00:00:00 2001
From: Hans <[email protected]>
Date: Tue, 15 Nov 2022 19:05:55 +0700
Subject: [PATCH] make avatar update
---
src/components/Avatar.js | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/src/components/Avatar.js b/src/components/Avatar.js
index aa4f6dbf9..eec70dd11 100644
--- a/src/components/Avatar.js
+++ b/src/components/Avatar.js
import {Image, View} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
+import lodashGet from 'lodash/get';
import stylePropTypes from '../styles/stylePropTypes';
import Icon from './Icon';
import themeColors from '../styles/themes/default';
@@ -9,6 +10,8 @@ import CONST from '../CONST';
import * as StyleUtils from '../styles/StyleUtils';
import * as Expensicons from './Icon/Expensicons';
import getAvatarDefaultSource from '../libs/getAvatarDefaultSource';
+import {withNetwork} from './OnyxProvider';
+import networkPropTypes from './networkPropTypes';
const propTypes = {
/** Source for the avatar. Can be a URL or an icon. */
@@ -29,6 +32,9 @@ const propTypes = {
/** A fallback avatar icon to display when there is an error on loading avatar from remote URL. */
fallbackIcon: PropTypes.func,
+
+ /** Props to detect online status */
+ network: networkPropTypes.isRequired,
};
const defaultProps = {
@@ -48,6 +54,14 @@ class Avatar extends PureComponent {
};
}
+ componentDidUpdate(prevProps) {
+ if (
+ lodashGet(prevProps.network, 'isOffline') === true && lodashGet(this.props.network, 'isOffline') === false
+ ) {
+ this.setState({imageError: false});
+ }
+ }
+
render() {
if (!this.props.source) {
return null;
@@ -86,4 +100,4 @@ class Avatar extends PureComponent {
Avatar.defaultProps = defaultProps;
Avatar.propTypes = propTypes;
-export default Avatar;
+export default withNetwork()(Avatar);
--
2.31.0
Result: Screen.Recording.2022-11-15.at.19.08.07.mov |
|
Hmmm... @hungvu193 I don't think that's the behavior we're looking for here. The image avatars should still stay even if you go offline. We shouldn't remove data that we already have if that makes sense. In other words, we should only be recovering from the avatar "error state" - not purposefully sending it to an error state when we go offline. The error state should only come up if we've failed to grab the original avatar - does that make sense? |
Makes sense, I think I need to implement |
|
@yuwenmemon From fa505d1945c9dd1bf79b57488dd2639a9228d8c1 Mon Sep 17 00:00:00 2001
From: Hans <[email protected]>
Date: Tue, 15 Nov 2022 19:05:55 +0700
Subject: [PATCH] make avatar update
---
src/components/Avatar.js | 41 +++++++++++++++++++++++++++++++++++++---
1 file changed, 38 insertions(+), 3 deletions(-)
diff --git a/src/components/Avatar.js b/src/components/Avatar.js
index aa4f6dbf9..2967fd454 100644
--- a/src/components/Avatar.js
+++ b/src/components/Avatar.js
@@ -1,7 +1,8 @@
-import React, {PureComponent} from 'react';
+import React, {Component, PureComponent} from 'react';
import {Image, View} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
+import lodashGet from 'lodash/get';
import stylePropTypes from '../styles/stylePropTypes';
import Icon from './Icon';
import themeColors from '../styles/themes/default';
@@ -9,6 +10,8 @@ import CONST from '../CONST';
import * as StyleUtils from '../styles/StyleUtils';
import * as Expensicons from './Icon/Expensicons';
import getAvatarDefaultSource from '../libs/getAvatarDefaultSource';
+import {withNetwork} from './OnyxProvider';
+import networkPropTypes from './networkPropTypes';
const propTypes = {
/** Source for the avatar. Can be a URL or an icon. */
@@ -29,6 +32,9 @@ const propTypes = {
/** A fallback avatar icon to display when there is an error on loading avatar from remote URL. */
fallbackIcon: PropTypes.func,
+
+ /** Props to detect online status */
+ network: networkPropTypes.isRequired,
};
const defaultProps = {
@@ -40,7 +46,7 @@ const defaultProps = {
fallbackIcon: Expensicons.FallbackAvatar,
};
-class Avatar extends PureComponent {
+class Avatar extends Component {
constructor(props) {
super(props);
this.state = {
@@ -48,6 +54,35 @@ class Avatar extends PureComponent {
};
}
+ shouldComponentUpdate(nextProps, nextState) {
+ if (
+ lodashGet(this.props.network, 'isOffline')
+ && !lodashGet(nextProps.network, 'isOffline')
+ ) {
+ return true;
+ }
+ if (this.props.source !== nextProps.source) {
+ return true;
+ }
+ if (this.state.imageError !== nextState.imageError) {
+ return true;
+ }
+
+ return false;
+ }
+
+ componentDidUpdate(prevProps) {
+ if (!this.state.imageError) {
+ return;
+ }
+ if (
+ lodashGet(prevProps.network, 'isOffline') === true
+ && lodashGet(this.props.network, 'isOffline') === false
+ ) {
+ this.setState({imageError: false});
+ }
+ }
+
render() {
if (!this.props.source) {
return null;
@@ -86,4 +121,4 @@ class Avatar extends PureComponent {
Avatar.defaultProps = defaultProps;
Avatar.propTypes = propTypes;
-export default Avatar;
+export default withNetwork()(Avatar);
--
2.31.0
|
Proposal
I suggest that we dont unmount the user avatar or use any offline handlers because it might be inaccurate at times and it just prevent the image native handler that resumes image loading when user restored the connection |
|
I think @hungvu193 's approach is correct. We can handle the clean up in the PR. cc: @yuwenmemon 🎀 👀 🎀 C+ reviewed |
|
Thanks, @parasharrajat - Agreed. Assigning @hungvu193 |
melvin-bot
bot
removed
the
Help Wanted
|
📣 @hungvu193 You have been assigned to this job by @yuwenmemon! |
|
@hungvu193 's updated proposal might cause a regression on check the components that uses the I suggest to add the rest of the property of avatar returning |
|
I think we will not need |
|
Cool. I will create a PR and test it carefully |
|
Thanks, @hungvu193 and @parasharrajat - just a heads up that I'm going OOO until the 28th so hopefully @laurenreidexpensify will be able to help find another engineer to sign off and merge your PR when it's ready. |
|
#12808 is ready for review. |
|
PR Is merged |
|
Not yet merged. Still discussing some changes on the PR. |
|
@hungvu193 Can you please answer my questions so that I can wrap up the discussion and merge this PR asap? |
|
@parasharrajat sure, I'll get back to you shortly |
|
@parasharrajat @hungvu193 are we still waiting on anything further here? |
|
@laurenreidexpensify We're in the process of reviewing the PR here: #12808 |
|
We hurried in merging the PR and a check got missed. I still have something to confirm. Please hold the payment for this task until #12808 (comment) is done. |
|
There is no need to block the payment further. Thanks. |
melvin-bot
bot
added
Weekly
melvin-bot
bot
changed the title
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.38-6 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2022-12-20. 🎊 After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
|
BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
|
@hungvu193 @parasharrajat @thesahindia apols, I thought I had hired everyone in Upwork already 🤦🏽♀️, have sent offers now and will issue payment as soon as accepted. |
|
@laurenreidexpensify I've accepted the offer |
laurenreidexpensify
added
Weekly
|
Everyone has been paid, I'm downgrading this to weekly over the holidays so we can complete the outstanding steps in the new year when everyone is back online |
|
@nkuoch @parasharrajat bump here on completing these tasks |
|
[@parasharrajat / @nkuoch] The PR that introduced the bug has been identified. Link to the PR: [@parasharrajat / @nkuoch] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: [@parasharrajat / @nkuoch] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: I think no discussion is needed as we have started to add Offline tests to the PRs as well. @nkuoch Could you please edit the checklist #12712 and update it. Thanks. |
|
Regression Test convo here |
If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!
Action Performed:
Expected Result:
The avatar should update and you should see the default colour avatar or custom image if the profile has one
Actual Result:
It doesn't update, you have to refresh the page
Workaround:
unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.27-3
Reproducible in staging?: y
Reproducible in production?: y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
video_20221114_035336_edit.mp4
Recording.925.mp4
Expensify/Expensify Issue URL:
Issue reported by: @thesahindia
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1668379245649689
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: