-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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-29] [$250] Switch to OD - Exit survey reason is not cleared after dismissing RHP and page refresh #50215
Comments
Triggered auto assignment to @anmurali ( |
@anmurali 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 |
ProposalPlease re-state the problem that we are trying to solve in this issue.The exit survey reason is not cleared after dismissing the RHP and refreshing the page. What is the root cause of that problem?We don't reset the draft reason when dismissing the modal here . What changes do you think we should make in order to solve the problem?
function clearDraftReason() {
Onyx.set(ONYXKEYS.FORMS.EXIT_SURVEY_REASON_FORM_DRAFT, null);
}
useEffect(() => {
ExitSurvey.clearDraftReason();
}, []);
POCScreen.Recording.2024-10-04.at.12.17.31.movWhat alternative solutions did you explore? (Optional) |
Edited by proposal-police: This proposal was edited at 2024-10-11 03:01:16 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
and
we always use What changes do you think we should make in order to solve the problem?
to:
since previously, it used the data from draft.
App/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx Lines 64 to 67 in 4ffbd42
to:
What alternative solutions did you explore? (Optional)
or
For the bug "The draft value saves for a response is not shown when we refresh the page and go to the response page"
As we can see, the data for
App/src/components/Form/FormProvider.tsx Line 92 in abdfb72
With it, we only set the |
@anmurali Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Job added to Upwork: https://www.upwork.com/jobs/~021843834311761874490 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @suneox ( |
It feels like a nice to have, but sure, let's fix it. |
Thanks for all the proposals. In this flow, we have implemented a draft functionality, and we need to retain this feature. We’re still looking for a proposal that maintains the draft functionality—we just need to ensure that we can start a new flow clearly. |
Proposal updated
|
Triggered auto assignment to @arosiclair, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
But i think we use the clearDraft on mounting in multiple locations: we have this pattern in multiple locations, so that is why i preferred clearing using
|
@abzokhattab Most of your reference code clears specific data upon mount but it doesn’t address clearing draft data or only clears draft data with specific logic. Therefore, it isn't a common pattern for clear draft data when starting a new flow. |
Thanks for the prompt review i've cleaned the referenced examples ... but i think this approach makes the code more modular and keeps the clearing logic within the relevant component, reducing side effects and aligning with patterns already used in similar components like CreateReportFieldsPage and SubscriptionSize, ... . It keeps the codebase cleaner since the clearing logic is added on the component itself not by another component . but lets see what @arosiclair thinks Thanks again for your feedback!! |
Edited by proposal-police: This proposal was edited at 2024-10-11 09:39:50 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?We are only meant to clear the draft values when the user actual goes to Screen.Recording.2024-10-11.at.3.03.34.PM.movSo here the actual bug is that when we fill out the Screen.Recording.2024-10-11.at.3.05.46.PM.movthe values should only be cleared when we click confirm and go to Here is the step where we set the values to null: App/src/pages/settings/ExitSurvey/ExitSurveyConfirmPage.tsx Lines 84 to 87 in 285bde8
App/src/libs/actions/ExitSurvey.ts Lines 32 to 54 in abdfb72
What changes do you think we should make in order to solve the problem?So to solve the actual bug here, we should display the draft value of the --- a/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx
+++ b/src/pages/settings/ExitSurvey/ExitSurveyResponsePage.tsx
@@ -114,6 +115,7 @@ function ExitSurveyResponsePage({route, navigation}: ExitSurveyResponsePageProps
<InputWrapper
InputComponent={TextInput}
inputID={INPUT_IDS.RESPONSE}
+ value={draftResponse}
label={translate(`exitSurvey.responsePlaceholder`)}
accessibilityLabel={translate(`exitSurvey.responsePlaceholder`)}
role={CONST.ROLE.PRESENTATION} This way we actually address the bug.
What alternative solutions did you explore? (Optional) |
@suneox @arosiclair can you please review my proposal, I think the acutal bug here is that the draft value is not displayed for response, I have provided the solution along with video as well please check. Even in the video attached in the bug description, you can see that the tester doesn't get the draft value for response they have to go back select another option and come back that is when they see the draft value Also, the convention to pass App/src/components/AddressForm.tsx Line 182 in abdfb72
|
@twilight294 Sure I'll start review your proposal within today |
Thank you 😄 |
Proppsal updated
|
Thanks for the update. Let’s ensure we clarify and follow the expected behavior: The latest proposals from @abzokhattab and @twilight294 still doesn't match the expected result. cc: @arosiclair |
@suneox that is what i am trying to put forward here, the actual bug is different then mentioned in the OP :)), If you check the card shipping missingDetails RHP flow, you can observe the same behaviour |
@twilight294 I’m not sure about this. Let’s wait for the internal team to make a decision. If the expected behavior doesn't reset values when starting a new flow, maybe we can go with your solution. |
@suneox Do you have any feedback about my updated solution? |
@truph01 Your solution about reload page doesn't show draftValue can override the value or defaultValue has passed into the InputWrapper in other places after draftValue is loaded. Since this issue only occurs on this specific page, and we don't have context on another page so we should avoid making changes that could impact almost all forms. We need to wait for internal team feedback on the expected result |
But we only want to set the value for defaultValue after the draftValue has loaded, right? Otherwise, while the draftValue is still loading, its value will be undefined, leading to the bug we mentioned in this issue.
A similar bug can be reproduced in Screen.Recording.2024-10-13.at.23.04.22.mov
Sure, I just need to share my solution about that bug since you commented "If the expected behavior doesn't reset values when starting a new flow, maybe we can go with your solution." |
I agree with this. Let's reset the form if the user cancels. The draft can persist if they refresh without canceling. |
📣 @suneox 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @truph01 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app! Offer link |
PR is ready |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.51-4 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-29. 🎊 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:
|
Checklist
|
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.44-0
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers): applausetester+shsb22cp111@applause.expensifail.com
Issue reported by: Applause Internal Team
Action Performed:
Expected Result:
The previously selected choice and the filled text is cleared once the field is dismissed and the page is refreshed
Actual Result:
Step 6. The previously selected answer is displayed as selected with a checkmark
Step 9. The previously filled-out answer is displayed
Workaround:
Unknown
Platforms:
Screenshots/Videos
Bug6623606_1727983107927.2024-10-03_22_02_19.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @anmuraliThe text was updated successfully, but these errors were encountered: