[HOLD for payment 2023-06-01] [HOLD for payment 2023-05-23] [$2000] Fast clicking any button 'n' times execute 'n' times #14572
Comments
kavimuru
added
Daily
slafortune
added
the
External
melvin-bot
bot
changed the title
|
Job added to Upwork: https://www.upwork.com/jobs/~01a7c5579c82436a8d |
|
Current assignee @slafortune is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mananjadhav ( |
melvin-bot
bot
added
the
Help Wanted
|
Triggered auto assignment to @roryabraham ( |
ProposalSolution We can add a callback function to the Option 1: (callback) diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js
index c269accf6..de927f481 100644
--- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -940,6 +940,7 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
Navigation.dismissModal(); // Dismiss /transition route for OldDot to NewDot transitions
}
Navigation.navigate(ROUTES.getWorkspaceInitialRoute(policyID));
+ callback();
});
}
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 5ef22eea7..d394a3240 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -93,9 +93,16 @@ class WorkspacesListPage extends Component {
constructor(props) {
super(props);
+ this.state = {
+ isDisabled: false,
+ }
+
+
this.getWalletBalance = this.getWalletBalance.bind(this);
this.getWorkspaces = this.getWorkspaces.bind(this);
this.getMenuItem = this.getMenuItem.bind(this);
+ this.createNewWorkspace = this.createNewWorkspace.bind(this);
+
}
/**
@@ -171,6 +178,11 @@ class WorkspacesListPage extends Component {
);
}
+ createNewWorkspace() {
+ this.setState({ isDisabled: true });
+ Policy.createWorkspace(undefined, undefined, undefined, undefined, () => this.setState({ isDisabled: false }));
+ }
+
render() {
const workspaces = this.getWorkspaces();
return (
@@ -195,8 +207,9 @@ class WorkspacesListPage extends Component {
<FixedFooter style={[styles.flexGrow0]}>
<Button
success
+ isDisabled={this.state.isDisabled}
text={this.props.translate('workspace.new.newWorkspace')}
- onPress={() => Policy.createWorkspace()}
+ onPress={this.createNewWorkspace}
/>
</FixedFooter>
</ScreenWrapper>
Option 2: (Promise) diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js
index c269accf6..032d4f27e 100644
--- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -934,7 +934,7 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
],
});
- Navigation.isNavigationReady()
+ return Navigation.isNavigationReady()
.then(() => {
if (transitionFromOldDot) {
Navigation.dismissModal(); // Dismiss /transition route for OldDot to NewDot transitions
diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 5ef22eea7..faf2fb523 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -93,9 +93,16 @@ class WorkspacesListPage extends Component {
constructor(props) {
super(props);
+ this.state = {
+ isDisabled: false,
+ }
+
+
this.getWalletBalance = this.getWalletBalance.bind(this);
this.getWorkspaces = this.getWorkspaces.bind(this);
this.getMenuItem = this.getMenuItem.bind(this);
+ this.createNewWorkspace = this.createNewWorkspace.bind(this);
+
}
/**
@@ -135,6 +142,8 @@ class WorkspacesListPage extends Component {
.value();
}
+ componentDidUpdate
+
/**
* Gets the menu item for each workspace
*
@@ -171,6 +180,11 @@ class WorkspacesListPage extends Component {
);
}
+ createNewWorkspace() {
+ this.setState({ isDisabled: true });
+ Policy.createWorkspace().then(() => this.setState({ isDisabled: false }));
+ }
+
render() {
const workspaces = this.getWorkspaces();
return (
@@ -195,8 +209,9 @@ class WorkspacesListPage extends Component {
<FixedFooter style={[styles.flexGrow0]}>
<Button
success
+ isDisabled={this.state.isDisabled}
text={this.props.translate('workspace.new.newWorkspace')}
- onPress={() => Policy.createWorkspace()}
+ onPress={this.createNewWorkspace}
/>
</FixedFooter>
</ScreenWrapper>
After Fix Screen.Recording.2023-01-27.at.11.31.22.PM.mov |
|
Looks like something related to 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 Feel free to drop a note in #expensify-open-source with any questions. |
ProposalWe can use a new onyx key to check if the workspace is being created and use that info to disable the button or set it to loading state Add a new key Change it's value to true when user presses the new workspace button like below at Policy.js --- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -762,6 +762,12 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
},
{
optimisticData: [
+ {
+ onyxMethod: CONST.ONYX.METHOD.SET,
+ key: ONYXKEYS.IS_CREATING_NEW_WORKSPACE,
+ value: true,
+ },
+
{
onyxMethod: CONST.ONYX.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
@@ -832,6 +838,11 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
},
],
successData: [
+ {
+ onyxMethod: CONST.ONYX.METHOD.SET,
+ key: ONYXKEYS.IS_CREATING_NEW_WORKSPACE,
+ value: false,
+ },
{
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
@@ -896,6 +907,11 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
},
],
failureData: [
+ {
+ onyxMethod: CONST.ONYX.METHOD.SET,
+ key: ONYXKEYS.IS_CREATING_NEW_WORKSPACE,
+ value: false,
+ },
{
onyxMethod: CONST.ONYX.METHOD.SET,
key: `${ONYXKEYS.COLLECTION.POLICY_MEMBER_LIST}${policyID}`,
Use the new key to set the button to loading (we can also disable it) like below at WorkspacesListPage --- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -197,6 +197,7 @@ class WorkspacesListPage extends Component {
success
text={this.props.translate('workspace.new.newWorkspace')}
onPress={() => Policy.createWorkspace()}
+ isLoading={this.props.isCreatingNewWorkspace}
/>
</FixedFooter>
</ScreenWrapper>
@@ -223,5 +224,8 @@ export default compose(
betas: {
key: ONYXKEYS.BETAS,
},
+ isCreatingNewWorkspace: {
+ key: ONYXKEYS.IS_CREATING_NEW_WORKSPACE,
+ },
}),
)(WorkspacesListPage); |
|
@Puneet-here What would be the need to create an Onyx key for this? Why not just implement it in the state of the component itself? |
Proposal:How about we debounce the button?
edit: third param can be set as https://www.freecodecamp.org/news/javascript-debounce-example/ |
ProposalProblemThis issue will exist for all the buttons used throughout the app, thus I think we should fix this in the SolutionWe should disable the button until the Note: We are applying a similar fix here #14426 diff --git a/src/components/Button.js b/src/components/Button.js
index 6e6fbade7..6854ae096 100644
--- a/src/components/Button.js
+++ b/src/components/Button.js
@@ -1,5 +1,7 @@
import React, {Component} from 'react';
-import {Pressable, ActivityIndicator, View} from 'react-native';
+import {
+ Pressable, ActivityIndicator, View, InteractionManager,
+} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../styles/styles';
import themeColors from '../styles/themes/default';
@@ -144,6 +146,9 @@ const defaultProps = {
class Button extends Component {
constructor(props) {
super(props);
+ this.state = {
+ isDisabled: props.isDisabled,
+ };
this.renderContent = this.renderContent.bind(this);
}
@@ -157,7 +162,7 @@ class Button extends Component {
// Setup and attach keypress handler for pressing the button with Enter key
this.unsubscribe = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, (e) => {
:...skipping...
diff --git a/src/components/Button.js b/src/components/Button.js
index 6e6fbade7..6854ae096 100644
--- a/src/components/Button.js
+++ b/src/components/Button.js
@@ -1,5 +1,7 @@
import React, {Component} from 'react';
-import {Pressable, ActivityIndicator, View} from 'react-native';
+import {
+ Pressable, ActivityIndicator, View, InteractionManager,
+} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../styles/styles';
import themeColors from '../styles/themes/default';
@@ -144,6 +146,9 @@ const defaultProps = {
class Button extends Component {
constructor(props) {
super(props);
+ this.state = {
+ isDisabled: props.isDisabled,
+ };
this.renderContent = this.renderContent.bind(this);
}
@@ -157,7 +162,7 @@ class Button extends Component {
// Setup and attach keypress handler for pressing the button with Enter key
this.unsubscribe = KeyboardShortcut.subscribe(shortcutConfig.shortcutKey, (e) => {
- if (!this.props.isFocused || this.props.isDisabled || this.props.isLoading || (e && e.target.nodeName === 'TEXTAREA')) {
+ if (!this.props.isFocused || this.state.isDisabled || this.props.isLoading || (e && e.target.nodeName === 'TEXTAREA')) {
return;
}
e.preventDefault();
@@ -165,6 +170,14 @@ class Button extends Component {
}, shortcutConfig.descriptionKey, shortcutConfig.modifiers, true, false, this.props.enterKeyEventListenerPriority, false);
}
+ componentDidUpdate(prevProps) {
+ if (this.props.isDisabled === prevProps.isDisabled) {
+ return;
+ }
+
+ this.setState({isDisabled: this.props.isDisabled});
+ }
+
componentWillUnmount() {
// Cleanup event listeners
if (!this.unsubscribe) {
@@ -234,6 +247,7 @@ class Button extends Component {
return (
<Pressable
onPress={(e) => {
+ this.setState({isDisabled: true});
if (e && e.type === 'click') {
e.currentTarget.blur();
}
@@ -241,7 +255,16 @@ class Button extends Component {
if (this.props.shouldEnableHapticFeedback) {
HapticFeedback.trigger();
}
- this.props.onPress(e);
+ const onPress = this.props.onPress(e);
+ InteractionManager.runAfterInteractions(() => {
+ if (!(onPress instanceof Promise)) {
+ this.setState({isDisabled: this.props.isDisabled});
+ return;
+ }
+ onPress.then(() => {
+ this.setState({isDisabled: this.props.isDisabled});
+ });
+ });
}}
onLongPress={(e) => {
if (this.props.shouldEnableHapticFeedback) {
@@ -252,9 +275,9 @@ class Button extends Component {
onPressIn={this.props.onPressIn}
onPressOut={this.props.onPressOut}
onMouseDown={this.props.onMouseDown}
- disabled={this.props.isLoading || this.props.isDisabled}
+ disabled={this.props.isLoading || this.state.isDisabled}
style={[
- this.props.isDisabled ? {...styles.cursorDisabled, ...styles.noSelect} : {},
+ this.state.isDisabled ? {...styles.cursorDisabled, ...styles.noSelect} : {},
styles.buttonContainer,
this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
this.props.shouldRemoveLeftBorderRadius ? styles.noLeftBorderRadius : undefined,
@@ -263,7 +286,7 @@ class Button extends Component {
nativeID={this.props.nativeID}
>
{({pressed, hovered}) => {
- const activeAndHovered = !this.props.isDisabled && hovered;
+ const activeAndHovered = !this.state.isDisabled && hovered;
return (
<OpacityView
shouldDim={pressed}
@@ -274,9 +297,9 @@ class Button extends Component {
this.props.large ? styles.buttonLarge : undefined,
this.props.success ? styles.buttonSuccess : undefined,
this.props.danger ? styles.buttonDanger : undefined,
- (this.props.isDisabled && this.props.success) ? styles.buttonSuccessDisabled : undefined,
- (this.props.isDisabled && this.props.danger) ? styles.buttonDangerDisabled : undefined,
- (this.props.isDisabled && !this.props.danger && !this.props.success) ? styles.buttonDisable : undefined,
+ (this.state.isDisabled && this.props.success) ? styles.buttonSuccessDisabled : undefined,
+ (this.state.isDisabled && this.props.danger) ? styles.buttonDangerDisabled : undefined,
+ (this.state.isDisabled && !this.props.danger && !this.props.success) ? styles.buttonDisable : undefined,
(this.props.success && activeAndHovered) ? styles.buttonSuccessHovered : undefined,
(this.props.danger && activeAndHovered) ? styles.buttonDangerHovered : undefined,
this.props.shouldRemoveRightBorderRadius ? styles.noRightBorderRadius : undefined,
diff --git a/src/libs/actions/Policy.js b/src/libs/actions/Policy.js
index 5a2de0e7e..a47777d0c 100644
--- a/src/libs/actions/Policy.js
+++ b/src/libs/actions/Policy.js
@@ -698,6 +698,7 @@ function generatePolicyID() {
* @param {Boolean} [makeMeAdmin] Optional, leave the calling account as an admin on the policy
* @param {String} [policyName] Optional, custom policy name we will use for created workspace
* @param {Boolean} [transitionFromOldDot] Optional, if the user is transitioning from old dot
+ * @returns {Promise} Navigation.isNavigationReady promise
*/
function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '', transitionFromOldDot = false) {
const policyID = generatePolicyID();
@@ -899,7 +900,7 @@ function createWorkspace(ownerEmail = '', makeMeAdmin = false, policyName = '',
}],
});
- Navigation.isNavigationReady()
+ return Navigation.isNavigationReady()
.then(() => {
if (transitionFromOldDot) {
Navigation.dismissModal(); // Dismiss /transition route for OldDot to NewDot transitions
|
|
hey, @kavimuru Above all, I can't really understand why all of the above proposals contain the code-panel. I don't wonna do that.
Hope my description is well-written and acceptable for you. |
Hi @sbsoso0411, this might help you understand the process.
|
This comment was marked as duplicate.
This comment was marked as duplicate.
ProposalWhen creating a new workspace, we set diff --git a/src/pages/workspace/WorkspacesListPage.js b/src/pages/workspace/WorkspacesListPage.js
index 5ef22eea79..23049e0b1a 100755
--- a/src/pages/workspace/WorkspacesListPage.js
+++ b/src/pages/workspace/WorkspacesListPage.js
@@ -195,6 +195,10 @@ class WorkspacesListPage extends Component {
<FixedFooter style={[styles.flexGrow0]}>
<Button
success
+ isLoading={_.some(
+ workspaces,
+ workspace => workspace && workspace.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
+ )}
text={this.props.translate('workspace.new.newWorkspace')}
onPress={() => Policy.createWorkspace()}
/> |
@esh-g We have established a pattern in this repo that:
This sometimes leads to extra complexity (like when we create Onyx keys that really don't need to be written to disk), but sometimes all that's needed is a bit more creative thinking to find a clean solution that fits within our app's existing patterns. So while your proposal is totally reasonable, it doesn't follow our patterns as closely as some others.
@Puneet-here this proposal shows a better understanding of our patterns, but overall I would really like to avoid adding any new Onyx keys for values that could instead be in-memory-only. We've discussed creating "ram-only" Onyx keys, but that project hasn't happened yet. So overall, I personally would prefer @fedirjh's proposal that uses the @fedirjh My only concern about your idea to use the I'm trying to think of a good way to handle this.
@rushatgabhane This might work, but overall it feels less robust than other solutions. What if you're on a slow internet connection and it takes 1600ms to create the policy?
@priyeshshah11 I like this approach a lot, like that it integrates The main problem I see with your proposal is that it returns a Promise from an action in
I just wanted to set the record straight here. There is absolutely no requirement that you include actual code in your proposal. In fact, we consider writing a proposal in plain English to be a best practice. So I applaud @sbsoso0411 for simply explaining their proposal without just posting the code. That said, if you're just joining us from Upwork, welcome! @priyeshshah11 gives good advice –read the contributing guidelines and join our slack rooms for the best contributing experience. @sbsoso0411 I think your proposal is missing some important information - when do we disable/enable the button? What is the source of truth for when the button is enabled or disabled? However...
This is a great point, and something I wished we had thought of before implementing #14426 (cc @syedsaroshfarrukhdot @eVoloshchak) In conclusion, many of the pieces of an ideal proposal are here, but I don't think any one proposal is quite right yet. Thanks for all your contributions so far, everyone! |
|
Thanks for the detailed feedback @roryabraham |
Following up on this, I was wrong and I don't think we'll have a performance issue here. I tested it with this tiny script: const _ = require('underscore');
const policies = {};
for (let i=0; i < 10000; i++) {
if (i === 9999) {
policies[i] = {pendingAction: 'add'};
} else {
policies[i] = {};
}
}
const startTime = Date.now();
const isAnyPolicyPending = _.some(policies, policy => policy && policy.pendingAction === 'add');
const endTime = Date.now();
console.log(`Search took ${endTime - startTime} milliseconds`);Even with 10000 items and a worst-case scenario where the random-generated policyID Onyx key is higher than any existing policyID, it only took like 3ms, so that should be fine. |
@roryabraham i don't think it matters how long it takes for a policy to create.
https://www.freecodecamp.org/news/javascript-debounce-example/ please correct if I'm wrong. |
melvin-bot
bot
added
the
Awaiting Payment
melvin-bot
bot
changed the title
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.14-14 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 2023-05-23. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
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:
|
|
melvin-bot
bot
added
Reviewing
melvin-bot
bot
changed the title
|
|
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.17-5 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 2023-06-01. 🎊 After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.
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:
|
|
@roryabraham can some other BZ member be assigned to this issue to issue payments & ultimately answer this as @slafortune is OOO as mentioned here. |
|
|
Thanks @s77rt ! Trying to get a clear handle on who should be getting paid on this - seems there are 2 C+ assigned. |
|
Working with @roryabraham on this. I am out of the office, but checking back on this. |
@slafortune / @roryabraham any updates here? |
|
Hey @priyeshshah11 - Looks like payment is on hold until 6/1 and also I believe that @roryabraham is still looking into this, we'll update soon |
|
all good @slafortune, just saying that originally the payment was on hold till 05/23 & then linking a second PR caused the automation to run & change that to 6/1 which shouldn't have been the case as it was only a word change & didn't change any functionality. |
|
Ok, catching up here after some OOO. I think the following payments are due:
|
|
Thanks so much @roryabraham |
|
@roryabraham thanks! for context I have assigned C+ after posting here as at the time the conversation was stale and C+ hasnt posted anything assuming they needed help. Glad we got tot he bottom of this ❤️ |
and others
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
HOLD on #17404
Action Performed:
Expected Result:
Note: The issue is reproducible with all buttons e.g. the IOU's I'll settle up elsewhere button as reported on #17167.
Actual Result:
Clicking ‘New workspace’ button ‘n’ number of times creates ‘n’ number of workspaces
If you follow the same step from 1-4 on web or mweb , then clicking the new workspace button ‘n’ number of times just creates a single workspace and not ‘n’ number of workspaces
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.59-1
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
az_recorder_20230125_172005.1.mp4
workspace.1.mp4
Expensify/Expensify Issue URL:
Issue reported by: @priya-zha
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1674643973375769
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: