[$1000] Assign task- Only on multiple tap, "confirm task" button works

lanitochka17 commented 1 year ago

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:

Launch app
Tap fab
Tap assign task
Enter title
Tap next
Select any contact for assignee
Tap confirm task

Expected Result:

On single tap, " Confirm task" button must work and allow user to complete the task

Actual Result:

Only on multiple tap, "confirm task" button works

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[ ] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.3.56-2

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://github.com/Expensify/App/assets/78819774/d55d2cc1-b6e9-43d7-97f8-5e1d737eaed7

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~016637cf721a90bde2
Upwork Job ID: 1694056630083760128
Last Price Increase: 2023-08-22

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @puneetlath (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in 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

puneetlath commented 1 year ago

Seems like a bug. Making external. cc @thienlnam

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~016637cf721a90bde2

melvin-bot[bot] commented 1 year ago

Current assignee @puneetlath is 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 - @Santhosh-Sellavel (External)

saalimzafar commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The "confirm task" button functions only after being tapped multiple times to complete the task assignment.

What is the root cause of that problem?

The root cause of this problem lies in the improper event handling approach being used. The usage of onSubmit={() => onSubmit()} inside NewTaskPage.js creates unnecessary function instances during each render, leading to performance degradation and unreliable button behavior.

What changes do you think we should make in order to solve the problem?

Replace the existing onSubmit={() => onSubmit()} syntax with the more efficient onSubmit={onSubmit}. By directly assigning the onSubmit

What alternative solutions did you explore? (Optional)

NA

parteekcoder commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue

The confirm task button runs after clicking it multiple times

What is the root cause of that problem?

In the onSubmit we are calling createTask function but in the createTask function, we are making API calls to the backend to store the task. Of course, the API call will take some time depending on server availability, load, and network.

 function onSubmit() {
         //code
         Task.createTaskAndNavigate(parentReport.reportID, props.task.title, props.task.description, props.task.assignee, props.task.assigneeAccountID, props.task.assigneeChatReport);
    }

function createTaskAndNavigate(args) {
    // code
    // API Call                  // consuming time
    Navigation.dismissModal(optimisticTaskReport.reportID);          // responsible for moving to next screen
}

So API call took time and hence creates a delay in running dismissModal after it. Also, it seems that the confirm task button not working but this is not the case, it depends upon the API call we are making.

What changes do you think we should make in order to solve the problem?

As the time taken by API call depends upon many factors so we should inform the user to wait by doing this. After clicking confirm task , there should be a loading screen which informs the user that the app processing his/her request and going smoothly.

To do this we can maintain loading that will be set to true when user has triggered onSubmit button. We will show loading screen until loading state is true. When API call is over we will set loading to false which will remove loading screen from the screen and user smoothly moves ahead. Like we this screen payment page https://github.com/Expensify/App/blob/main/src/components/FullscreenLoadingIndicator.js

function FullScreenLoadingIndicator(props) {                      
    const additionalStyles = _.isArray(props.style) ? props.style : [props.style];
    return (
        <View style={[StyleSheet.absoluteFillObject, styles.fullScreenLoading, ...additionalStyles]}>
            <ActivityIndicator
                color={themeColors.spinner}
                size="large"
            />
        </View>
    );
}

https://github.com/Expensify/App/assets/100110395/1683bc82-99b7-49ac-a666-35bb0953f301

What alternative solutions did you explore? (Optional)

NA

joh42 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The user has no idea that task creation is in progress after clicking confirm task. The actual task creation is working as intended though.

What is the root cause of that problem?

There is no visual indication to the user that task creation is in progress. Specifically, the problem is that the createTaskAndNavigate action takes some time to finish. We are not waiting for the API however, since we set the task data optimistically and then navigate to the optimistic task id:

https://github.com/Expensify/App/blob/51d55de5fd22c10a3de44ce6cb36d8f36715df37/src/libs/actions/Task.js#L212

What changes do you think we should make in order to solve the problem?

Updated solution

I suggest we maintain a state in the NewTaskPage, perhaps called isWaitingForAction. Right before we initiate the createTaskAndNavigate action in onSubmit, we set the state to true, and then we pass it as isLoading={isWaitingForAction} to the FormAlertWithSubmitButton.

It's worth noting that just setting isWaitingForAction to true before calling the createTaskAndNavigate action will not actually do anything, since the component will not rerender before the action starts. I suggest we wrap the createTaskAndNavigate action in an empty setTimeout so the loading indication can start showing before the action begins.

The implementation steps would be:

Add a new state called something like isWaitingForAction to the NewTaskPage
Set isWaitingForAction to true right before we call the createTaskAndNavigate action
Wrap the call to the createTaskAndNavigate action in an instant setTimeout to make sure the component rerenders with the button loading
Pass it as isLoading={isWaitingForAction} to the FormAlertWithSubmitButton.

The result would be:

https://github.com/Expensify/App/assets/138504087/68930001-baac-4756-b266-103c9b28abd3

Old solution (the one reviewed by C+)

I suggest we start tracking the loading state of the task creation in the createTaskAndNavigate action https://github.com/Expensify/App/blob/51d55de5fd22c10a3de44ce6cb36d8f36715df37/src/pages/tasks/NewTaskPage.js#L129

This could be done like it is during signin: https://github.com/Expensify/App/blob/51d55de5fd22c10a3de44ce6cb36d8f36715df37/src/libs/actions/Session/index.js#L183-L191

IE in the optimistic data we add isLoading with a value of true. This is then set to false in both the successData and the failureData.

To add the actual loading indication, we pass props.task.isLoading as isLoading to the submit button: https://github.com/Expensify/App/blob/eaccec9ecc1136663242f06d8da65d2c0877af03/src/components/FormAlertWithSubmitButton.js#L24

In addition, we might want to prevent new submits while task creation is in progress. This could be done by either checking props.task.isLoading in the onSubmit function: https://github.com/Expensify/App/blob/51d55de5fd22c10a3de44ce6cb36d8f36715df37/src/pages/tasks/NewTaskPage.js#L112

Or by disabling the submit button while creation is in progress.

We may even want to disable the menu options (title, description etc) while task creation is in progress. This could easily be done with disabled={props.task.isLoading}

Edited: Something like this would be the steps to implement what I mentioned above:

In the createTaskAndNavigate action, write isLoading: true to ONYXKEYS.TASK at the start of the action, and write isLoading: false to the same key at the end of the action
Pass isLoading={props.task.isLoading} to the FormAlertWithSubmitButton
Optional: Pass disabled={props.task.isLoading} to the MenuItems
Optional: Add an early return to the onSubmit function when props.task.isLoading is true

What alternative solutions did you explore? (Optional)

An alternative solution would be to show a FullScreenLoadingIndicator. This is likely overkill IMO, since task creation usually takes way less than a second, and users would be subjected to what seems like multiple redirects in quick succession.

Pluto0104 commented 1 year ago

I couldn't reproduce it on android emulator. Maybe it only appears on real device?

parteekcoder commented 1 year ago

I couldn't reproduce it on android emulator. Maybe it only appears on real device?

It may or may not reproduce depending on the internet you have, server loads etc. Hence it will reproduce only in certain conditions when API response delays

Santhosh-Sellavel commented 1 year ago

I can reproduce this on a real device

Santhosh-Sellavel commented 1 year ago

@joh42's Proposal looks good to me! RC is clear, the proposed solution seems straightforward to me.

C+ Reviewed 🎀 👀 🎀

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @aldo-expensify, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Santhosh-Sellavel commented 1 year ago

@parteekcoder FullScreenLoading seems off as we can easily show the loading on the button to show the progress.

To do this we can maintain loading that will be set to true when user has triggered onSubmit button. We will show loading screen until loading state is true. When API call is over we will set loading to false which will remove loading screen from the screen and user smoothly move ahead.

Implementation details could be clearer, but how the loading state is maintained is unclear. Please try to provide at least an overview of the implementation in the future proposal, thanks!

parteekcoder commented 1 year ago

yeah sure, I will share implementation details also. Yes you are right we can go with either way first one we can show loading screen or else just show loading on the button.

I will update my proposal and let you know, thanks for pointing out

Santhosh-Sellavel commented 1 year ago

FullScreenLoading seems like a good option as we might need to disable other fields if we go with showing just loading on the button.

@aldo-expensify Let us know your thoughts, thanks!

parteekcoder commented 1 year ago

yes this is the advantage we get with loading screen so that user don't click other fields

parteekcoder commented 1 year ago

@Santhosh-Sellavel I will go with the same approach as @joh42

puneetlath commented 1 year ago

Just for my clarification. Is this happening because we are waiting for the server to respond? Because I would think that we use the optimistic without feedback design pattern here, so we'd just optimistically create the task and assume it will succeed.

Or are we doing that and the problem is just that it takes some time and there is no UI indication that something is happening?

parteekcoder commented 1 year ago

yes @puneetlath you are right see https://github.com/Expensify/App/issues/25695#issuecomment-1688918296

puneetlath commented 1 year ago

Ok got it. In that case, @thienlnam @aldo-expensify do you agree we should use the optimistic without feedback pattern here?

Santhosh-Sellavel commented 1 year ago

Ok got it. In that case, @thienlnam @aldo-expensify do you agree we should use the optimistic without feedback pattern here?

Sounds 👍

parteekcoder commented 1 year ago

@Santhosh-Sellavel I will go with this same approach here to maintain consistency also stated by @joh42

https://github.com/Expensify/App/blob/main/src/pages/home/sidebar/SidebarLinks.js

joh42 commented 1 year ago

@Santhosh-Sellavel / @puneetlath Now that I look into it, it seems like we're already creating the task optimistically. So we're not waiting for the API, but the user is redirected to a task before the server has responded. Here you can see that I'm redirected before the CreateTask request has finished:

https://github.com/Expensify/App/assets/138504087/0c27d5b6-f938-4b5c-a557-af95def5c428

What takes time is the actual createTaskAndNavigate action, but there is no UI indication showing that something is happening. As such the approach I proposed/that was adopted by another contributor will not work.

Rather, I suggest we maintain a state in the NewTaskPage, perhaps called isWaitingForAction. Right before we initiate the createTaskAndNavigate action in onSubmit, we set the state to true, and then we pass it as isLoading={isWaitingForAction} to the FormAlertWithSubmitButton.

joh42 commented 1 year ago

Also it's worth noting that just setting isWaitingForAction to true before calling the createTaskAndNavigate action will not actually do anything, since the component will not rerender before the action starts. I suggest we wrap the createTaskAndNavigate action in an empty setTimeout so the loading indication can start showing before the action begins. The result would be something like this:

https://github.com/Expensify/App/assets/138504087/68930001-baac-4756-b266-103c9b28abd3

The new implementation steps would be:

Add a new state called something like isWaitingForAction to the NewTaskPage
Set isWaitingForAction to true right before we call the createTaskAndNavigate action
Wrap the call to the createTaskAndNavigate action in an instant setTimeout to make sure the component rerenders with the button loading
Pass it as isLoading={isWaitingForAction} to the FormAlertWithSubmitButton.

I've also updated my proposal with the new implementation

saalimzafar commented 1 year ago

Anyone who can answer my question. Why it's still an issue when the latest version is different than the one reported. What if the simple update will fix the issue?

thienlnam commented 1 year ago

Why it's still an issue when the latest version is different than the one reported. What if the simple update will fix the issue?

Because the issue still exists in this new version? If the issue doesn't exist in a new version then we don't need to address it but it doesn't seem like the case here - though let me know if that's an incorrect assumption

Ok got it. In that case, @thienlnam @aldo-expensify do you agree we should use the optimistic without feedback pattern here?

I believe the pattern is correct, but we are not navigating to the task until the API call responds which is something we can fix here

joh42 commented 1 year ago

I think we are though @thienlnam?

https://github.com/Expensify/App/assets/138504087/0c27d5b6-f938-4b5c-a557-af95def5c428

saalimzafar commented 1 year ago

Version Number: 1.3.56-2 is mentioned in the issue above

And currently the version available on the play store is v1.3.57-6

And I don't know what pattern is followed in version update Just the earlier one was v1.3.56-24 [56-24] >>> [57-6]??

thienlnam commented 1 year ago

Hmm, why is createTaskAndNavigate taking so long then - maybe we can figure out how to optimize that so it doesn't take so long to execute?

Though granted, if this is not really a problem we can close it - seems like it only really applies to older android devices?

saalimzafar commented 1 year ago

The very first thing that needs to be checked is the version. If it's on the latest version only then further processing should take place. Technically it looks pointless to solve an issue which is on older version. Then what's the point of updating?? Users are often advised to keep the latest version of the application for a reason.

joh42 commented 1 year ago

I agree, it might not be the most high-value bug. On my mid-tier 2018 Android it's taking slightly over a second which is okay IMO.

Regarding the version number, the bug still exists, but it is dependent on the speed of your device.

thienlnam commented 1 year ago

Yeah agreed, will leave it up to @puneetlath / @aldo-expensify

Seems like this bug only exists on older android devices and is a result of the createTaskAndNavigate action taking a second to run which causes a delay in the navigation call to run right away.

We can either

Figure out how to speed up the method (not sure how much we can speed this up since there are a lot of things being added
Close the issue since it only really impacts slow android devices (but still works fine)
Some other solution mentioned above

Santhosh-Sellavel commented 1 year ago

I can reproduce the issue on the Samsung S23 Ultra running Android 13

saalimzafar commented 1 year ago

I can't because I recently updated the app!

puneetlath commented 1 year ago

Ok, I think the best course of action is to close this then. The solutions don't really feel ideal to me as they are more workarounds. Also, we have some discussion going in this internal channel here about whether the ideal flow is to navigate to the task or to leave the user in the current context. So rather than add more logic, I think we should just accept the current behavior on slower devices.

Expensify / App