Christinadobrzyn
commented
1 year ago - I am able to replicate this by following the steps above.
- No other GHs matching this issue exist
- I think this can be external
Closed kbecciv closed 1 year ago
Christinadobrzyn
commented
1 year ago 
melvin-bot[bot]
commented
1 year ago Current assignee @Christinadobrzyn is eligible for the External assigner, not assigning anyone new.
melvin-bot[bot]
commented
1 year ago Job added to Upwork: https://www.upwork.com/jobs/~010de9dc97b9a28579
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (External)
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @srikarparsi (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
bernhardoj
commented
1 year ago Proposal
When we delete a workspace, we set the optimistic value with pendingAction of delete. When we navigated back to the workspace list page, the list still has one workspace left with delete pendingAction. If we look at the implementation of OfflineWithFeedback, if the pendingAction is delete, we will hide it.
And after a while, we will receive a Pusher API update that will set the workspace item value to null.
[{"key": "policy_142AC62FE7444C5B", "onyxMethod": "set", "value": null}]
What we can do here is to change the condition to show the empty workspace view. Filter out all workspace with pendingAction of delete.
render() {
const workspaces = this.getWorkspaces();
+ const isWorkspacesEmpty = _.isEmpty(_.filter(workspaces, workspace => workspace.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE));
return (
<ScreenWrapper>
<HeaderWithCloseButton
title={this.props.translate('common.workspaces')}
shouldShowBackButton
onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS)}
onCloseButtonPress={() => Navigation.dismissModal(true)}
/>
+ {isWorkspacesEmpty ? (
- {_.isEmpty(workspaces) ? (
<BlockingView
icon={Expensicons.Building}
title={this.props.translate('workspace.emptyWorkspace.title')}
subtitle={this.props.translate('workspace.emptyWorkspace.subtitle')}
/>
) : (
<ScrollView style={styles.flex1}>
{_.map(workspaces, (item, index) => this.getMenuItem(item, index))}
</ScrollView>
)}
<FixedFooter style={[styles.flexGrow0]}>
<Button
success
text={this.props.translate('workspace.new.newWorkspace')}
onPress={() => Policy.createWorkspace()}
/>
</FixedFooter>
</ScreenWrapper>
);
}
Result
melvin-bot[bot]
commented
1 year ago Looks like something related to react-navigation may have been mentioned in this issue discussion.
As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.
Feel free to drop a note in #expensify-open-source with any questions.
tienifr
commented
1 year ago When we delete the workspace we just update the pendingAction to delete and remove this workspace when API call is successful.
In https://github.com/Expensify/App/blob/main/src/pages/workspace/WorkspacesListPage.js#L118
we get all the policies and filter by policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN => the deleted workspace still be there until the API call is successful.
we shouldn't filter out the workspace that has pendingAction === 'delete' when we're in offline
I'll filter out the workspace that has pendingAction === 'delete' when we're in online.
In https://github.com/Expensify/App/blob/main/src/pages/workspace/WorkspacesListPage.js
...
getWorkspaces() {
return _.chain(this.props.policies)
.filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+ .filter(policy => this.props.network.isOffline || policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
...
export default compose(
withLocalize,
+ withNetwork(),
...
s77rt
commented
1 year ago diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 41c6b3babe..2c037c685d 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -115,7 +115,10 @@ class WorkspacesListPage extends Component {
*/
getWorkspaces() {
return _.chain(this.props.policies)
- .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+ .filter(policy => policy
+ && policy.type === CONST.POLICY.TYPE.FREE
+ && policy.role === CONST.POLICY.ROLE.ADMIN
+ && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
.map(policy => ({
title: policy.name,
icon: policy.avatar ? policy.avatar : Expensicons.Building,
@@ -128,7 +131,6 @@ class WorkspacesListPage extends Component {
pendingAction: policy.pendingAction,
errors: policy.errors,
dismissError: () => dismissWorkspaceError(policy.id, policy.pendingAction),
- disabled: policy.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
}))
.sortBy(policy => policy.title)
.value();
@@ -164,7 +166,6 @@ class WorkspacesListPage extends Component {
badgeText={this.getWalletBalance(isPaymentItem)}
fallbackIcon={item.fallbackIcon}
brickRoadIndicator={item.brickRoadIndicator}
- disabled={item.disabled}
/>
</OfflineWithFeedback>
);I agree with the above @bernhardoj @tienifr analysis
Filter out workspaces that have pendingAction equal to CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE
Remove the unused disabled prop
OfflineWithFeedback to decide whether children should be hidden 
parasharrajat
commented
1 year ago Reviewing.
Christinadobrzyn
commented
1 year ago I'll be ooo till Jan 9th, assigning this issue to @mallenexpensify. Matt, I'm happy to take it back when I return!
syedsaroshfarrukhdot
commented
1 year ago Proposal :-
With all above proposals when we filter list on policy.pendingAction it causes a regression as shown in below video. The pendingAction changes from delete to undefined which causes deleted workspace to reappear in the list for a split second.
Regression Video :-
As pendingAction : undefined is set before removing it from the list. We can fix it in Policy.js deleteWorkspace function by passing a new value lastAction in optimisticData which isn't modified and filter list on it.
diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js
index 0f6784e7d..2bb5117eb 100644
--- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -68,6 +68,7 @@ function deleteWorkspace(policyID, reports) {
value: {
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
errors: null,
+ lastAction : CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
},
},
..._.map(reports, ({reportID}) => ({
@@ -82,6 +83,7 @@ function deleteWorkspace(policyID, reports) {
})),
];
+
// Restore the old report stateNum and statusNum
const failureData = [
..._.map(reports, ({
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 41c6b3bab..a79fce73d 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -114,8 +114,9 @@ class WorkspacesListPage extends Component {
* @returns {Array} the menu item list
*/
getWorkspaces() {
+
return _.chain(this.props.policies)
- .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN
)
+ .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN && policy.lastAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
.map(policy => ({
title: policy.name,
icon: policy.avatar ? policy.avatar : Expensicons.Building,
@@ -172,6 +173,7 @@ class WorkspacesListPage extends Component {
render() {
const workspaces = this.getWorkspaces();
return (
<ScreenWrapper>
<HeaderWithCloseButtonAfter Fix :-
We can also not filter list when we are offline if we want to
...
getWorkspaces() {
return _.chain(this.props.policies)
.filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+ .filter(policy => this.props.network.isOffline || policy.lastAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
...
export default compose(
withLocalize,
+ withNetwork(),
...After Offline Filtering Fix :-
s77rt
commented
1 year ago @syedsaroshfarrukhdot Nice catch, but this is not a regression from my proposal, this issue existed long before. Your solution does not address the root cause for that either.
The workspace will appear again for a short period only if you create the workspace and delete it immediately (in very short time), here is the order of events:
optimisticData sets the pendingAction to addoptimisticData sets the pendingAction to delete <-- Workspace no longer visiblesuccessData sets the pendingAction to null <-- Workspace is visible again (root cause)workspace to null <-- Workspace no longer existsAs you see this is an edge case and we shouldn't prioritise it, besides the final results is the workspace being deleted and not visible (self fix)
I think we still should go with my proposal, but just curious why you are adding the network.isOffline check?
syedsaroshfarrukhdot
commented
1 year ago @s77rt Yes, I agree with your analysis it is an edge case and it can be handled on filtering Workspaces on a prop which doesn't change just like in my proposal. So I believe once we are taking care of it it should be fixed along with edge case rest is up to team on what they decide how to handle it.
I am using network.isOffline to not filter out deleted workspace when in offline mode just like in the proposal video. When a workspace is deleted while offline it shouldn't be filtered unless user becomes online and actually the workspaces I deleted.
s77rt
commented
1 year ago @syedsaroshfarrukhdot Thanks for the clarification, however I still think we should go with my proposal (without adding a new prop) as that's how we deal with similar cases https://github.com/Expensify/App/blob/main/src/pages/settings/InitialSettingsPage.js#L130-L137
As for the offline case, your explanation makes sense, I will update my proposal to cover the offline cases in few minutes in both InitialSettingsPage and WorkspacesListPage
Another question: Why you are using two filters calls? it's expensive to call functions in JS and iterate over the objects twice, just wrap them in one filter for future proposals.
s77rt
commented
1 year ago diff --git a/src/pages/settings/InitialSettingsPage.js b/src/pages/settings/InitialSettingsPage.js
index c2bf6b384b..18d1067f31 100755
--- a/src/pages/settings/InitialSettingsPage.js
+++ b/src/pages/settings/InitialSettingsPage.js
@@ -131,7 +131,7 @@ class InitialSettingsPage extends React.Component {
.filter(policy => policy
&& policy.type === CONST.POLICY.TYPE.FREE
&& policy.role === CONST.POLICY.ROLE.ADMIN
- && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
+ && (this.props.network.isOffline || policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE))
.sortBy(policy => policy.name)
.pluck('avatar')
.value();
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 41c6b3babe..5fc2446513 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -23,6 +23,7 @@ import Permissions from '../../libs/Permissions';
import Button from '../../components/Button';
import FixedFooter from '../../components/FixedFooter';
import BlockingView from '../../components/BlockingViews/BlockingView';
+import {withNetwork} from '../../components/OnyxProvider';
const propTypes = {
/* Onyx Props */
@@ -115,7 +116,10 @@ class WorkspacesListPage extends Component {
*/
getWorkspaces() {
return _.chain(this.props.policies)
- .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+ .filter(policy => policy
+ && policy.type === CONST.POLICY.TYPE.FREE
+ && policy.role === CONST.POLICY.ROLE.ADMIN
+ && (this.props.network.isOffline || policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE))
.map(policy => ({
title: policy.name,
icon: policy.avatar ? policy.avatar : Expensicons.Building,
@@ -222,4 +226,5 @@ export default compose(
key: ONYXKEYS.BETAS,
},
}),
+ withNetwork(),
)(WorkspacesListPage);InitialSettingsPage and WorkspacesListPagesyedsaroshfarrukhdot
commented
1 year ago Another question: Why you are using two filters calls? it's expensive to call functions in JS and iterate over the objects twice, just wrap them in one filter for future proposals.
Agree with you will also post an updated proposal and will use one filter call to avoid it.
syedsaroshfarrukhdot
commented
1 year ago Update Proposal :-
With all above proposals when we filter list on policy.pendingAction it causes a regression as shown in below video. The pendingAction changes from delete to undefined which causes deleted workspace to reappear in the list for a split second.
Regression Video :-
As pendingAction : undefined is set before removing it from the list. We can fix it in Policy.js deleteWorkspace function by passing a new value lastAction in optimisticData which isn't modified and filter list on it.
diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js
index 0f6784e7d..eaa86a752 100644
--- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -67,6 +67,7 @@ function deleteWorkspace(policyID, reports) {
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
value: {
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
+ lastAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
errors: null,
},
},
diff --git a/src/pages/settings/InitialSettingsPage.js b/src/pages/settings/InitialSettingsPage.js
index c2bf6b384..bd64cf64d 100755
--- a/src/pages/settings/InitialSettingsPage.js
+++ b/src/pages/settings/InitialSettingsPage.js
@@ -131,7 +131,7 @@ class InitialSettingsPage extends React.Component {
.filter(policy => policy
&& policy.type === CONST.POLICY.TYPE.FREE
&& policy.role === CONST.POLICY.ROLE.ADMIN
- && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
+ && (this.props.network.isOffline || policy.lastAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE))
.sortBy(policy => policy.name)
.pluck('avatar')
.value();
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 41c6b3bab..a097a7433 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -23,6 +23,7 @@ import Permissions from '../../libs/Permissions';
import Button from '../../components/Button';
import FixedFooter from '../../components/FixedFooter';
import BlockingView from '../../components/BlockingViews/BlockingView';
+import { withNetwork } from '../../components/OnyxProvider';
const propTypes = {
/* Onyx Props */
@@ -115,7 +116,10 @@ class WorkspacesListPage extends Component {
*/
getWorkspaces() {
return _.chain(this.props.policies)
- .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+ .filter(policy => policy
+ && policy.type === CONST.POLICY.TYPE.FREE
+ && policy.role === CONST.POLICY.ROLE.ADMIN
+ && (this.props.network.isOffline || policy.lastAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE))
.map(policy => ({
title: policy.name,
icon: policy.avatar ? policy.avatar : Expensicons.Building,
@@ -208,6 +212,7 @@ WorkspacesListPage.defaultProps = defaultProps;
export default compose(
withLocalize,
+ withNetwork(),
withOnyx({
policies: {
key: ONYXKEYS.COLLECTION.POLICY,After Fix :-
Offline Fix :-
jatinsonijs
commented
1 year ago Proposal
Root Case: It is happending due to workspace takes time to delete. In deletion process pendingAction change to delete which makes workspace to hide due to condition const hideChildren = !props.network.isOffline && props.pendingAction === 'delete' && !hasErrors; in OfflineWithFeedback. But worksapce is still in list in this case WorkspacesListPage _.isEmpty(workspaces) condition will be false and message will be not display.
We have to filter policies - Filter out all delete pending policy if not offline and not have an error in policy. We must have to add error condition also otherwise it will not show workspace which have error and also in deletePending. I saw earlier this condition can happen due to some condition on workspace policy.
diff --git a/src/pages/settings/InitialSettingsPage.js b/src/pages/settings/InitialSettingsPage.js
index c2bf6b384b..0c28382cd2 100755
--- a/src/pages/settings/InitialSettingsPage.js
+++ b/src/pages/settings/InitialSettingsPage.js
@@ -130,8 +130,9 @@ class InitialSettingsPage extends React.Component {
const policiesAvatars = _.chain(this.props.policies)
.filter(policy => policy
&& policy.type === CONST.POLICY.TYPE.FREE
- && policy.role === CONST.POLICY.ROLE.ADMIN
- && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
+ && policy.role === CONST.POLICY.ROLE.ADMIN)
+ .filter(policy => this.props.network.isOffline
+ || (policy.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ? !_.isEmpty(policy.errors) : true))
.sortBy(policy => policy.name)
.pluck('avatar')
.value();
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 41c6b3babe..174469437f 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -23,6 +23,7 @@ import Permissions from '../../libs/Permissions';
import Button from '../../components/Button';
import FixedFooter from '../../components/FixedFooter';
import BlockingView from '../../components/BlockingViews/BlockingView';
+import {withNetwork} from '../../components/OnyxProvider';
const propTypes = {
/* Onyx Props */
@@ -116,6 +117,8 @@ class WorkspacesListPage extends Component {
getWorkspaces() {
return _.chain(this.props.policies)
.filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+ .filter(policy => this.props.network.isOffline
+ || (policy.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE ? !_.isEmpty(policy.errors) : true))
.map(policy => ({
title: policy.name,
icon: policy.avatar ? policy.avatar : Expensicons.Building,
@@ -208,6 +211,7 @@ WorkspacesListPage.defaultProps = defaultProps;
export default compose(
withLocalize,
+ withNetwork(),
withOnyx({
policies: {
key: ONYXKEYS.COLLECTION.POLICY,Note: We can do actions in one filter also but it will become lengthy and not easy to read.
Optionally: We can also create common function in PolicyUtil In future it will easy to change and not have to do changes on two places.
diff --git a/src/libs/PolicyUtils.js b/src/libs/PolicyUtils.js
index 3def54c19f..aad55035a2 100644
--- a/src/libs/PolicyUtils.js
+++ b/src/libs/PolicyUtils.js
@@ -54,9 +54,26 @@ function getPolicyBrickRoadIndicatorStatus(policy, policyMembers) {
return '';
}
+/**
+ * Check if policy can be display
+ * @param {Object} policy
+ * @param {boolean} isOffline
+ * @returns {boolean}
+ */
+function shouldShowPolicy(policy, isOffline) {
+ return policy
+ && policy.type === CONST.POLICY.TYPE.FREE
+ && policy.role === CONST.POLICY.ROLE.ADMIN
+ && (isOffline || (
+ policy.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE
+ ? !_.isEmpty(policy.errors) : true
+ ));
+}
+
export {
hasPolicyMemberError,
hasPolicyError,
hasCustomUnitsError,
getPolicyBrickRoadIndicatorStatus,
+ shouldShowPolicy,
};
diff --git a/src/pages/settings/InitialSettingsPage.js b/src/pages/settings/InitialSettingsPage.js
index c2bf6b384b..6d1aa97ea2 100755
--- a/src/pages/settings/InitialSettingsPage.js
+++ b/src/pages/settings/InitialSettingsPage.js
@@ -128,10 +128,7 @@ class InitialSettingsPage extends React.Component {
*/
getDefaultMenuItems() {
const policiesAvatars = _.chain(this.props.policies)
- .filter(policy => policy
- && policy.type === CONST.POLICY.TYPE.FREE
- && policy.role === CONST.POLICY.ROLE.ADMIN
- && policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE)
+ .filter(policy => PolicyUtils.shouldShowPolicy(policy, this.props.network.isOffline))
.sortBy(policy => policy.name)
.pluck('avatar')
.value();
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 41c6b3babe..5ef22eea79 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -23,6 +23,7 @@ import Permissions from '../../libs/Permissions';
import Button from '../../components/Button';
import FixedFooter from '../../components/FixedFooter';
import BlockingView from '../../components/BlockingViews/BlockingView';
+import {withNetwork} from '../../components/OnyxProvider';
const propTypes = {
/* Onyx Props */
@@ -115,7 +116,7 @@ class WorkspacesListPage extends Component {
*/
getWorkspaces() {
return _.chain(this.props.policies)
- .filter(policy => policy && policy.type === CONST.POLICY.TYPE.FREE && policy.role === CONST.POLICY.ROLE.ADMIN)
+ .filter(policy => PolicyUtils.shouldShowPolicy(policy, this.props.network.isOffline))
.map(policy => ({
title: policy.name,
icon: policy.avatar ? policy.avatar : Expensicons.Building,
@@ -208,6 +209,7 @@ WorkspacesListPage.defaultProps = defaultProps;
export default compose(
withLocalize,
+ withNetwork(),
withOnyx({
policies: {
key: ONYXKEYS.COLLECTION.POLICY,And I think causes deleted workspace reappear in the list for a split second its different issue not related to this issue. There is also another issue related to race condition. Try to create workspace and immediately turn off network. Now turn on network workspace will not create and show error "An unexpected error occurred while trying to create your Workspace. Please try again.New workspace"
mallenexpensify
commented
1 year ago @parasharrajat , can you review the proposals above when you get a chance?
parasharrajat
commented
1 year ago Sure, I will do that asap. Looks like a lot of similar proposals.
mallenexpensify
commented
1 year ago @parasharrajat to review proposals
parasharrajat
commented
1 year ago Checking the issue now.
jatinsonijs
commented
1 year ago @parasharrajat I have found some ref which may be case of this issue
Issue: https://github.com/Expensify/App/issues/13192 PR: https://github.com/Expensify/App/pull/13201
s77rt
commented
1 year ago @jatinsonijs Do you think the provided solutions re-create the issue? Are we in a loop?
s77rt
commented
1 year ago @jatinsonijs I see that your proposal have a condition for errors too. I think that will do it.
Just a note, I think you can change the third condition to:
this.props.network.isOffline || policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || !_.isEmpty(policy.errors)It's the same condition used here https://github.com/Expensify/App/blob/304d759a4792c2fa1e5294741c142c28448fe507/src/components/OfflineWithFeedback.js#L90
Overall, good catch. I like your proposal :+1:
jatinsonijs
commented
1 year ago this.props.network.isOffline || policy.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE || !_.isEmpty(policy.errors)
Hi @s77rt thanks for you comment. At starting I also thought or || condition will be enough but I guess above linked issue condition is policy is in delete pending action and also have error. In this case this condition will not work. Bcz this condition is same as linked issue. it will never check error it will return from the != deletePending
s77rt
commented
1 year ago @jatinsonijs I don't see why it won't work. We will return policies that meets at least of those requirements, if a policy has an error it will be kept and shown.
it will never check error it will return from the != deletePending
Yes, and that what is expected to happen. If a policy is not being deleted, keep it If it's being deleted (condition not met) we will check the last condition, if it has error or not...
jatinsonijs
commented
1 year ago Yes right, I had over thought on it 🙂. Both conditions are working we can use or || to simplify it.
parasharrajat
commented
1 year ago Ok, I checked the magic behind CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE. Unfortunately, It is hard to imagine without running the app. I can't sign IN :disappointed: due to known issues. But It seems that @jatinsonijs's proposal makes the most sense out of all.
Overall there are three stages of UI (based on analysis).
I will still like to try the UI first, just to be sure I are not missing anything.
jatinsonijs
commented
1 year ago @parasharrajat if you are on Mac you can try on desktop app. We don't have login issue on desktop app.
parasharrajat
commented
1 year ago Lol good idea.
srikarparsi
commented
1 year ago Hey @mallenexpensify! I'm taking a break from expensify this semester to finish up classes. Do you think you could reassign this to another engineer?
melvin-bot[bot]
commented
1 year ago Current assignees @mallenexpensify and @Christinadobrzyn are eligible for the External assigner, not assigning anyone new.
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to Contributor-plus team member for initial proposal review - @thesahindia (External)
melvin-bot[bot]
commented
1 year ago Triggered auto assignment to @NikkiWines (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.
mallenexpensify
commented
1 year ago Sorry @thesahindia there was an issue with auto-assign logic, looking into internally.
Welcome @NikkiWines !!
parasharrajat
commented
1 year ago Trying to cause API requests to fail.
jatinsonijs
commented
1 year ago @parasharrajat I think you have to get special account access which is creating error in workspace deletion. With normal account I haven't got any error.
like in linked issue Log into an account that has the necessary pre conditions applausetester@applause.expensifail.com there should be domain restricted.
parasharrajat
commented
1 year ago Finalizing it in few minutes.
jatinsonijs
commented
1 year ago It seems that @jatinsonijs's proposal makes the most sense out of all.
@parasharrajat have you test it ?, if you have any feedback plz let me know
parasharrajat
commented
1 year ago Oops, the proposal is still not approved officially. Give me a few minutes.
@jatinsonijs Do you know the password for applausetester@applause.expensifail.com? you can DM me if needed on slack. I tried two QA accounts and workspaces got deleted.
jatinsonijs
commented
1 year ago no dont know the password
On Fri, 6 Jan, 2023, 11:49 pm Rajat Parashar, @.***> wrote:
Oops, the proposal is still not approved officially. Give me a few minutes.
— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/13615#issuecomment-1373979099, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3K6ZISFLRSHSTH74MYDAH3WRBOZJANCNFSM6AAAAAAS7TS54E . You are receiving this because you were mentioned.Message ID: @.***>
parasharrajat
commented
1 year ago Trying to get some ideas on the channel.
parasharrajat
commented
1 year ago jatinsonijs
commented
1 year ago Optionally: We can also create common function in PolicyUtil In future it will easy to change and not have to do changes on two places.
@parasharrajat should I create common function in PolicyUtils.js or we are ok with do changes at 2 places ?
parasharrajat
commented
1 year ago The function is fine.
FYI, if you are new to the repo, the tagged Engineer will give the final approval and assign you the issue before you can start. Thanks.
jatinsonijs
commented
1 year ago Yeah understood, waiting for final approval.
On Sat, 7 Jan, 2023, 1:31 am Rajat Parashar, @.***> wrote:
The function is fine.
FYI, if you are new to the repo, the tagged Engineer will give the final approval and assign you the issue before you can start. Thanks.
— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/issues/13615#issuecomment-1374066282, or unsubscribe https://github.com/notifications/unsubscribe-auth/A3K6ZIRE3D3ESTPHFP7X3GLWRB22RANCNFSM6AAAAAAS7TS54E . You are receiving this because you were mentioned.Message ID: @.***>
NikkiWines
commented
1 year ago Agreed, the common function is the better route. @jatinsonijs' proposal also looks good to me. @mallenexpensify or @Christinadobrzyn can one of you hire @jatinsonijs for this role? Thank you 🙇
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
The welcome message appears immediately
Actual Result:
The welcome message does not appear immediately - it blinks
Workaround:
Unknown
Platform:
Where is this issue occurring?
Version Number: 1.2.39.0
Reproducible in staging?: Yes
Reproducible in production?: Yes
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
https://user-images.githubusercontent.com/93399543/207847494-e86ae370-5b51-4204-87b3-010443aafed4.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit