-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HOLD for payment 2024-10-30] [$250] Assign task - Inconsistent Button Behavior Between Submit and Assign Task After Navigating via URL #50577
Comments
Triggered auto assignment to @zanyrenney ( |
@zanyrenney FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors |
Edited by proposal-police: This proposal was edited at 2024-10-10 14:47:50 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.Assign task - Inconsistent Button Behavior Between Submit and Assign Task After Navigating via URL What is the root cause of that problem?After the first tab confirm the task the That is because when we click the confirm button it invoke this function and checking if the task?.title is not empty, since the task value is undefined so it return above error message App/src/pages/tasks/NewTaskPage.tsx Lines 117 to 121 in da700c5
And why we still kept seeing the title value in the title input, because when we open the page we set the useState title value to App/src/pages/tasks/NewTaskPage.tsx Lines 104 to 107 in da700c5
But when the task value becomes undefined the useEffect hook will be triggered since the task?.title value changed, but it do not remove the value from the title input because it will update the useState if the task?.title is not undefinedApp/src/pages/tasks/NewTaskPage.tsx Lines 104 to 107 in da700c5
What changes do you think we should make in order to solve the problem?We can remove the if condition inside the useEffect ...
setTitle(task?.title ?? ''); We can do the same for description and assignee ResultScreen.Recording.2024-10-10.at.07.45.43.movWhat alternative solutions did you explore? (Optional) |
ProposalPlease re-state the problem that we are trying to solve in this issue.Inconsistent submit button behavior between the task page and money request page. What is the root cause of that problem?From the OP video, there are actually a few problems. When we submit a money request or a task, we clear out the info from the Onyx. For the money request page, a blank screen is shown. It happens after #42413 where we show an empty view using App/src/components/SelectionList/BaseSelectionList.tsx Lines 719 to 721 in da700c5
Previously, we show a loader only if Now, the condition is inside the function which will render null if both conditions are false. App/src/components/SelectionList/BaseSelectionList.tsx Lines 484 to 492 in 36c45f4
The submit button is also disabled because the selected participants are now 0.
The task page, on the other hand, uses a local state to show the task information and only updates if the onyx value is not undefined. So, when the onyx value is cleared, the local state still contains the previous value. App/src/pages/tasks/NewTaskPage.tsx Lines 84 to 112 in da700c5
It's done like that so the local info is not cleared when the Onyx is cleared. Also, the task submit button is never disabled even if there is missing info, and instead, all of the info will be validated. App/src/pages/tasks/NewTaskPage.tsx Lines 117 to 135 in da700c5
But, we validate the data/info against the Onyx data which is already cleared, so the missing data error shows even though the page shows the local state. You might be thinking that we can just validate against the local state, but I personally don't like the idea of not synchronizing the onyx data with what we show to the user. Another downside of doing this is, that even though we show the local state on the confirmation page, pressing the title/description to edit it will show empty on the input field because the onyx is empty. Money request page: blank screen when onyx data is cleared, submit is disabled when no participant is selected What changes do you think we should make in order to solve the problem?
To fix this, we need to check for both App/src/components/SelectionList/BaseSelectionList.tsx Lines 719 to 721 in 36c45f4
I think the expected behavior would be to enable the submit button even when there is no participant selected and show an error when submitting, just like when a merchant is missing. To do that, remove the disable condition here,
and show the form error here, App/src/components/MoneyRequestConfirmationList.tsx Lines 747 to 756 in da700c5
For this, we should remove the local state and just rely on the onyx data. |
adding the external label. |
Job added to Upwork: https://www.upwork.com/jobs/~021844734228925480640 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hungvu193 ( |
Hans - please could you review the proposals above? @hungvu193 |
Sure |
Will be on my list tmr morning. |
Hi @NJ-2020, I don't think your solution will cover all the cases, ie when we have a share destination. Also removing this condition can possibly replicate this bug I also think we should also fix the expense page (we shouldn't show empty view if draft expense was submitted in other tab). Because of that I'm aligned with @bernhardoj 's proposal here 🎀 👀 🎀 C+ reviewed. |
Triggered auto assignment to @francoisl, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@francoisl @hungvu193 Can anyone remind me what's the process of getting a new translation? I currently use this: PR is here |
Let's do:
( |
@francoisl sorry, I actually need a confirmation with the new copy too since it's a new message that I add myself. |
Ah, yes |
Great, thanks! |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.52-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 2024-10-30. 🎊 For reference, here are some details about the assignees on this 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:
|
Please amend this comment in the correct format to contain all the required information above:
|
Payment Summary
BugZero Checklist (@zanyrenney)
|
Thanks for reminding. I'll complete it today |
I don't think we need regression test for this one, it was expected behavior in the past, however we decided to change it recently. |
Requested in ND. |
@zanyrenney Please confirm the payment summary when you get a chance. |
was waiting for the checklist to be completed, thanks for doing that @hungvu193 |
Payment summary $250 owed to @bernhardoj - paid through ND. Thanks! 👋🏼 |
$250 approved for @bernhardoj |
$250 approved for @hungvu193 |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: v9.0.47-1
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+tw435353535333@applause.expensifail.com
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
After navigating between pages using URLs, both the Submit and Confirm buttons should behave consistently—either both should be disabled after actions or display correct error messages.
Actual Result:
The Submit button is disabled as expected, but the Confirm button displays an incorrect error message, causing inconsistency.
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6630676_1728562512925.Screen_Recording_2024-10-10_at_5.11.48_AM.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @zanyrenneyThe text was updated successfully, but these errors were encountered: