App
App copied to clipboard
Expensify
[$1000] Reimburse- Error message is displayed after quick input of Rate and Unit
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:
- Go to URL https://staging.new.expensify.com/
- Log with any account
- Select any workspace
- Select tab Reimburse expenses then Track distance
- Quickly enter the numbers so that 000 does not appear in succession. Immediately select Unit
Expected Result:
No error message is displayed after quick input of Rate and Unit
Actual Result:
Error message is displayed after quick input of Rate and Unit
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
- [x] Android / native
- [x] Android / Chrome
- [x] iOS / native
- [x] iOS / Safari
- [x] MacOS / Chrome / Safari
- [x] MacOS / Desktop
Version Number: 1.2.85.0
Reproducible in staging?: Yes
Reproducible in production?: Yes
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: Any additional supporting documentation
https://user-images.githubusercontent.com/93399543/225386888-596eaa6c-6e2e-46ba-9f2d-266b0d3a3457.mp4
https://user-images.githubusercontent.com/93399543/225386896-b2ef94b5-fda4-465a-b882-a70c0579bff2.mp4
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Upwork Automation - Do Not Edit
- Upwork Job URL: https://www.upwork.com/jobs/~011e6f8ab17a2387c5
- Upwork Job ID: 1640484579872690176
- Last Price Increase: 2023-04-11
Triggered auto assignment to @sonialiap (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Triggered auto assignment to @mateocole (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.
Bug0 Triage Checklist (Main S/O)
- [x] This "bug" occurs on a supported platform (ensure
Platformsin OP are ✅) - [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
- [ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
- [ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
- [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync
@mateocole Whoops! This issue is 2 days overdue. Let's get this updated quick!
This looks like a bug, going to have an engineer confirm this is good to mark as external
Triggered auto assignment to @joelbettner (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.
@mateocole @kbecciv this is definitely a bug and should be fixed. That entire selector has issues. It tries to add 3 decimal places automatically to the end of the rate before I'm done typing what I want the rate to be. I think the scope of fixing this should be fixing the entire track distance selector and not just the error that we get when switching units.
Triggered auto assignment to @michaelhaxhiu (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak (External)
Current assignee @joelbettner is eligible for the External assigner, not assigning anyone new.
Proposal
Please re-state the problem that we are trying to solve in this issue.
In Reimburse, the app shows an error message if the user changes the unit soon after changing the rate.
What is the root cause of that problem?
The proposal of @khatch33 is correct for this.
What changes do you think we should make in order to solve the problem?
To prevent updating unit or rate while the API call is in progress, I disabled fields like this:
- I created a state variable called
isUpdatingUnitRateinWorkspaceReimburseViewcomponent.
isUpdatingUnitRate: false
- I set
isUpdatingUnitRateto true insetUnitandupdateRateValuefunctions before making the API call.
this.setState({isUpdatingUnitRate: true});
- I set
isUpdatingUnitRateto false incomponentDidUpdatefunction after the API call is completed.
if (this.state.isUpdatingUnitRate && prevProps.policy.lastModified !== this.props.policy.lastModified)
{
this.setState({isUpdatingUnitRate: false});
}
- I disabled the input fields for unit and rate using the
isUpdatingUnitRatevariable as a condition.
<TextInput
label={this.props.translate('workspace.reimburse.trackDistanceRate')}
placeholder={this.state.outputCurrency}
onChangeText={value => this.setRate(value)}
value={this.state.unitRateValue}
autoCompleteType="off"
autoCorrect={false}
keyboardType={CONST.KEYBOARD_TYPE.DECIMAL_PAD}
onKeyPress={this.debounceUpdateOnCursorMove}
maxLength={12}
disabled={this.state.isUpdatingUnitRate}
/>
<Picker
label={this.props.translate('workspace.reimburse.trackDistanceUnit')}
items={this.getUnitItems()}
value={this.state.unitValue}
onInputChange={value => this.setUnit(value)}
backgroundColor={themeColors.cardBG}
isDisabled={this.state.isUpdatingUnitRate}
/>
This is result video.
Proposal
Please re-state the problem that we are trying to solve in this issue.
When quickly changing both the rate and rate unit in the track distance section of the workspace reimbursement page, multiple error messages are displayed: -Your changes couldn't be saved. The workspace was modified while you were offline, please try again. -The policy was modified since you loaded it, please reload the page and try again.
@joelbettner also pointed out that the entire selector has issues (specifically adding 3 decimal places automatically to the end of the rate before typing is done) and should be fixed as a whole.
What is the root cause of that problem?
The primary problem occurs when the requests with the command UpdateWorkspaceCustomUnitRate and UpdateWorkspaceCustomUnit originating from WorkspaceReimburseView.js happen at nearly the same time. Updating the unit and the unit rate both modify the same policyID so when they both have the same lastModified value in the post request, the second request will respond with an error since the newly updated lastModified value won't match the formData. I was able to reproduce the error for both requests depending on which came second.
The unit rate change function is debounced so the error will occur almost exclusively when the unit is modified immediately after the unit rate.
As for the rate changing before the user is done typing, the debounced function is called on every input change. Every input change followed by a full second of no input change will trigger a request and display change.
What changes do you think we should make in order to solve the problem?
I agree with @joelbettner that a fix should likely go beyond the unit selection bug. However a non-workaround could include debouncing the unit update as well or unit requests being dependent on the unit rate matching the server.
If the unit rates prematurely updating is an issue, fixing it should be done together with the bug listed since it may alter the change needed. Sending requests for both values separately on every input change seems unnecessary and could lead to large accidentally changes from the user. The most intuitive solution would be to combine both input values into one request.
Proposal
Please re-state the problem that we are trying to solve in this issue.
- There is an error message when we select the unit just after editing the rate in workplace track distance screen
- The app tries to add 3 decimal places automatically after some time and it can be bad ux for the user
- This form is inconsitent with the other settings form because there is not a save button
What is the root cause of that problem?
- The call to UpdateWorkspaceCustomUnitRate and UpdateWorkspaceCustomUnit are almost at the same moment and this can cause problem in the database
- The Rate is modified on TextChanged with a debounce of 1 second
- No save button
What changes do you think we should make in order to solve the problem?
I propose that we only have one call to the API to modify the rate and the unit and that this method be called when we click on a save button that we need to add.
What alternative solutions did you explore? (Optional)
I agree with @joelbettner on this
I think the scope of fixing this should be fixing the entire track distance selector and not just the error that we get when switching units
So far we have two separate things that are wrong with selector:
- Three decimal places are added automatically to the end of the rate before you're done typing
- Error message is displayed after quick input of Rate and Unit
Are there any other issues? What should be the correct behavior for the first item? I think we shouldn't update the input unless network request was rejected, do you have any thoughts on this?
This isn't isolated to two different requests, it can also happen with one request being called twice (or more). This issue can be reproduced even when you quickly change only the unit, you don't have to change the rate. It's easier to reproduce with a slow connection
https://user-images.githubusercontent.com/9059945/228575859-3a6bbbf6-b2d6-4b2c-933e-a633a3bd1495.mov
So I don't think the root cause in @podrascanindragan47's proposal is entirely correct
error occurs if the user changes the unit while the app is still updating the rate. This is because the app sends a request to UpdateWorkspaceCustomUnit API before the previous request to UpdateWorkspaceCustomUnitRate API is completed.
However, I think the proposed solution is pretty close to what is needed
To avoid this error, the app should wait for the response from UpdateWorkspaceCustomUnitRate API before sending a request to UpdateWorkspaceCustomUnit API.
But we also need to wait for the response from UpdateWorkspaceCustomUnitRate before calling it again and the same with UpdateWorkspaceCustomUnit.
I believe there is a mechanism in the app to queue up requests, correct me if I'm wrong.
@khatch33's proposal is more detailed, and explains why this error occurs.
The root cause of this is indeed lastModified value. If you make a second request while the first one is still in progress, the first request will update the report, which will make lastModified value in the second request outdated.
a non-workaround could include debouncing the unit update as well or unit requests being dependent on the unit rate matching the server.
I don't think debouncing unit requests will fully solve the problem for requests that take more time than the debounce period, but debouncing it in addition to an actual fix seems like a good idea.
The most intuitive solution would be to combine both input values into one request.
That wouldn't work, since issue is still present even for a single network request
@ShogunFire's proposal:
I propose that we only have one call to the API to modify the rate and the unit and that this method be called when we click on a save button that we need to add.
I think not having a Save button is a design choice, there are a few screens throughout the app that have 'autosave'
That wouldn't work, since issue is still present even for a single network request
@eVoloshchak I think combining them into a single write and also increasing the debounced time would mostly solve the issue, but like you said it an error is still possible for a single request particularly with slow connection. I should have elaborated more in my proposal but waiting for a response before sending another request is what I was alluding to. However I think even with that, it would be more intuitive to combine both inputs into 1 write especially since they modify the same policyID. I actually think the best option would be a save or confirm button like @ShogunFire proposed but I figured that was a design choice so I left that out. It seems unnecessary to send requests on every edit and could lead to some pretty big accidents from the user. I also agree debouncing the unit as well doesn't truly fix the underlying issue, it would just make it much harder to reproduce the bug.
@eVoloshchak @michaelhaxhiu @joelbettner @mateocole this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!
Current assignee @michaelhaxhiu is eligible for the Bug assigner, not assigning anyone new.
@khatch33
waiting for a response before sending another request is what I was alluding to. However I think even with that, it would be more intuitive to combine both inputs into 1 write especially since they modify the same policyID
We could do that, but that wouldn't solve our problem, it occurs even for a single network request being called multiple times in a row.
It seems unnecessary to send requests on every edit and could lead to some pretty big accidents from the user. I actually think the best option would be a save or confirm button like @ShogunFire proposed
This is a valid argument, and that would also prevent our issue from happening (we can just disable the save button while another request is in progress). However, not having the save button is a design choice, so I think it's worth starting a discussion on slack (I'll do that shortly).
@podrascanindragan47, thanks for the updated proposal! Btw, it's better to post a new one instead of editing the previous comment.
Your proposal is disabling the ability to edit while another request is in progress, while ideally, we'd want to queue the requests, so the new one is sent after the previous one has finished.
Proposal
Please re-state the problem that we are trying to solve in this issue.
In Reimburse, the app shows an error message if the user changes the unit soon after changing the rate.
What is the root cause of that problem?
The proposal of @khatch33 is correct for this.
What changes do you think we should make in order to solve the problem?
Following @eVoloshchak ’s comment, I used a queue to handle the API calls. I created an updateQueue property in the WorkspaceReimburseView constructor and a processUpdateQueue method to execute the queue items one by one.
this.updateQueue = [];
processUpdateQueue(func)
{
this.updateQueue.push(func);
if(this.updateQueue.length == 1)
{
this.updateQueue[0]();
}
}
I added the queue processing logic in the componentDidUpdate function.
if(prevProps.policy.lastModified !== this.props.policy.lastModified)
{
if(this.updateQueue.length > 0)
{
this.updateQueue.shift();
}
if(this.updateQueue.length > 0)
{
this.updateQueue[0]();
}
}
I replaced the direct API calls with queue items.
this.processUpdateQueue(() => {
Policy.updateWorkspaceCustomUnit(this.props.policy.id, distanceCustomUnit, {
customUnitID: this.state.unitID,
name: this.state.unitName,
attributes: {unit: value},
}, this.props.policy.lastModified);
});
this.processUpdateQueue(() => {
Policy.updateCustomUnitRate(this.props.policy.id, currentCustomUnitRate, this.state.unitID, {
...currentCustomUnitRate,
rate: numValue * CONST.POLICY.CUSTOM_UNIT_RATE_BASE_OFFSET,
}, this.props.policy.lastModified);
});
This is result video.
Proposal
Please re-state the problem that we are trying to solve in this issue.
Reimburse- Error message is displayed after quick input of Rate and Unit
What is the root cause of that problem?
After the first request, the server is already created the new lastModified, the first request response has not been returned, the second request brings the old lastModified to request the api, then the server returns the error
What changes do you think we should make in order to solve the problem?
https://github.com/Expensify/App/pull/12162
I can reproduce this problem on two different devices. I think if we want to solve it completely, it is best to merge the two interfaces [UpdateWorkspaceCustomUnit, UpdateWorkspaceCustomUnitRate] into one interface, and we no longer need lastModified
What alternative solutions did you explore? (Optional)
I thought about adding a write lock, if there is a lock, disable all textInput
@podrascanindragan47, thanks for the updated proposal! We already have a network queue in https://github.com/Expensify/App/tree/main/src/libs/Network that you can make use of. However, it seems like this can be reproduced using two devices, which means that queuing up requests won't resolve this completely, so please hold on for now.