Skip to content
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

[$250] In additional details page can't clear the first name/ last name field reported by @thesahindia #12807

Closed
kavimuru opened this issue Nov 17, 2022 · 26 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Nov 17, 2022

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:

  1. Make sure you have a name set at profile page
  2. Visit https://staging.new.expensify.com/enable-payments
  3. Try deleting the whole name

Expected Result:

User should be able to clear the name completely .

Actual Result:

You can't clear the name completely.

Workaround:

unknown

Platform:

Where is this issue occurring?

  • Web
  • iOS
  • Android
  • Desktop App
  • Mobile Web

Version Number: 1.2.28-2
Reproducible in staging?: Y
Reproducible in production?: Y
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

Screen.Recording.2022-05-10.at.7.45.53.PM.mov
Recording.954.mp4

Expensify/Expensify Issue URL:
Issue reported by: @thesahindia
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1652192776856909

View all open jobs on GitHub

Upwork Automation - Do Not Edit

@kavimuru kavimuru added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

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

@hungvu193
Copy link
Contributor

hungvu193 commented Nov 17, 2022

Proposal
In AdditionalDetailsStep.js the problem came from the way we calculated the value for first name and last name fields.
In this case, when we tried to delete the first name field and when it became empty, the getFirstName() function will return the firstName from PersonalDetails.extractFirstAndLastNameFromAvailableDetails(this.props.currentUserPersonalDetails) and made us couldn't edit the field anymore because the prop this.props.walletAdditionalDetailsDraft.legalFirstName was still empty so it always return firstName.

    getFirstName() {
        const {firstName} = PersonalDetails.extractFirstAndLastNameFromAvailableDetails(this.props.currentUserPersonalDetails);
        return this.props.walletAdditionalDetailsDraft.legalFirstName || firstName;
    }

  <TextInput
      containerStyles={[styles.mt4]}
      label={this.props.translate(this.fieldNameTranslationKeys.legalFirstName)}
      onChangeText={val => this.clearErrorAndSetValue('legalFirstName', val)}
      value={this.getFirstName()}
      errorText={this.getErrorText('legalFirstName')}
  />

Solution
Give the TextInput defaultValue prop and let the value prop to the prop that we updated in Onyx.

                                <TextInput
                                    containerStyles={[styles.mt4]}
                                    label={this.props.translate(this.fieldNameTranslationKeys.legalFirstName)}
                                    onChangeText={val => this.clearErrorAndSetValue('legalFirstName', val)}
-                                 value={this.getFirstName()}
+                                  value={this.props.walletAdditionalDetailsDraft.legalFirstName}
+                                  defaultValue={this.getFirstName()}
                                    errorText={this.getErrorText('legalFirstName')}
                                />



                             <TextInput
                                    containerStyles={[styles.mt4]}
                                    label={this.props.translate(this.fieldNameTranslationKeys.legalLastName)}
                                    onChangeText={val => this.clearErrorAndSetValue('legalLastName', val)}
-                                   value={this.getLastName()}
+                                  value={this.props.walletAdditionalDetailsDraft.legalLastName}
+                                  defaultValue={this.getLastName()}
                                    errorText={this.getErrorText('legalLastName')}
                                />

Result:

Screen.Recording.2022-11-17.at.18.10.15.mov

@MitchExpensify MitchExpensify added the External Added to denote the issue can be worked on by a contributor label Nov 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

Current assignee @MitchExpensify is eligible for the External assigner, not assigning anyone new.

@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

Job added to Upwork: https://www.upwork.com/jobs/~013826c459732f8dcc

@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 17, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 17, 2022

Triggered auto assignment to @tylerkaraszewski (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@melvin-bot melvin-bot bot changed the title In additional details page can't clear the first name/ last name field reported by @thesahindia [$250] In additional details page can't clear the first name/ last name field reported by @thesahindia Nov 17, 2022
@Emz27
Copy link

Emz27 commented Nov 18, 2022

Proposal

  • use state for textinput values where the api values are the default values, we dont need to listen to api values after that to prevent slow/lag on input change

I have this kind of bug in our project where we used redux form in our textinputs, redux form was slow thats why it keeps repeating the values

@s77rt
Copy link
Contributor

s77rt commented Nov 18, 2022

Proposal

diff --git a/src/pages/EnablePayments/AdditionalDetailsStep.js b/src/pages/EnablePayments/AdditionalDetailsStep.js
index 1a90a8a35..808042118 100644
--- a/src/pages/EnablePayments/AdditionalDetailsStep.js
+++ b/src/pages/EnablePayments/AdditionalDetailsStep.js
@@ -73,17 +73,7 @@ const defaultProps = {
         idNumber: '',
         errorCode: '',
     },
-    walletAdditionalDetailsDraft: {
-        legalFirstName: '',
-        legalLastName: '',
-        addressStreet: '',
-        addressCity: '',
-        addressState: '',
-        addressZip: '',
-        phoneNumber: '',
-        dob: '',
-        ssn: '',
-    },
+    walletAdditionalDetailsDraft: {},
     ...withCurrentUserPersonalDetailsDefaultProps,
 };
 
@@ -121,12 +111,12 @@ class AdditionalDetailsStep extends React.Component {
 
     getFirstName() {
         const {firstName} = PersonalDetails.extractFirstAndLastNameFromAvailableDetails(this.props.currentUserPersonalDetails);
-        return this.props.walletAdditionalDetailsDraft.legalFirstName || firstName;
+        return this.props.walletAdditionalDetailsDraft.legalFirstName ?? firstName;
     }
 
     getLastName() {
         const {lastName} = PersonalDetails.extractFirstAndLastNameFromAvailableDetails(this.props.currentUserPersonalDetails);
-        return this.props.walletAdditionalDetailsDraft.legalLastName || lastName;
+        return this.props.walletAdditionalDetailsDraft.legalLastName ?? lastName;
     }
 
     /**

Details

  1. walletAdditionalDetailsDraft props are not required and should not be initialised with empty string, keep them undefined.
  2. In getFirstName and getLastName use nullish coalescing instead of short-circuting.
Kooha-2022-11-18-09-36-55.mp4

@thesahindia
Copy link
Member

I believe it's a good opportunity to refactor this page to use Form and the issue can be solved in that. Let's update the issue description unless someone disagrees.

@hungvu193
Copy link
Contributor

I agree, I remember we already have plan to refactor existing form to use Form component right?

@thesahindia
Copy link
Member

thesahindia commented Nov 18, 2022

Just adding some other ways to reach additional details page (Because the mentioned steps only work when you have a bank account)

Change the code at ModalStackNavigators.js and visit search page

 const SearchModalStackNavigator = createModalStackNavigator([{
     getComponent: () => {
-        const SearchPage = require('../../../pages/SearchPage').default;
-        return SearchPage;
+        const EnablePaymentsPage = require('../../../pages/EnablePayments/EnablePaymentsPage').default;
+        return EnablePaymentsPage;
     },
     name: 'Search_Root',

On web you can just visit http://localhost:8080/enable-payments

@melvin-bot

This comment was marked as off-topic.

@sobitneupane
Copy link
Contributor

sobitneupane commented Nov 18, 2022

I believe it's a good opportunity to refactor this page to use Form and the issue can be solved in that. Let's update the issue description unless someone disagrees.

Waiting for decision on this before reviewing proposal.

cc: @tylerkaraszewski @MitchExpensify

@melvin-bot melvin-bot bot added the Overdue label Nov 21, 2022
@MitchExpensify
Copy link
Contributor

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Nov 21, 2022
@melvin-bot
Copy link

melvin-bot bot commented Nov 25, 2022

@tylerkaraszewski, @sobitneupane, @MitchExpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot
Copy link

melvin-bot bot commented Nov 29, 2022

@tylerkaraszewski, @sobitneupane, @MitchExpensify 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@joelbettner joelbettner self-assigned this Nov 29, 2022
@joelbettner joelbettner removed the External Added to denote the issue can be worked on by a contributor label Nov 29, 2022
@madmax330 madmax330 removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 30, 2022
@tylerkaraszewski tylerkaraszewski removed their assignment Nov 30, 2022
@melvin-bot melvin-bot bot removed the Overdue label Nov 30, 2022
@tylerkaraszewski
Copy link
Contributor

Letting @joelbettner take this one.

@joelbettner
Copy link
Contributor

I've been tackling this. I'm refactoring the page to use our Form functionality.

@melvin-bot melvin-bot bot added the Overdue label Dec 5, 2022
@joelbettner
Copy link
Contributor

I'm working on the tests for this in the PR.

@melvin-bot melvin-bot bot removed the Overdue label Dec 5, 2022
@joelbettner joelbettner added the Reviewing Has a PR in review label Dec 6, 2022
@joelbettner
Copy link
Contributor

PR has been fully tested and is in review.

@joelbettner
Copy link
Contributor

Test are complete and the PR is ready for review.

@MitchExpensify
Copy link
Contributor

MitchExpensify commented Dec 12, 2022

This was handled internally, and @sobitneupane held on any reviews, so I believe the only payment due is the reporting bonus to @thesahindia - Invited you to the Upwork Job here for that payment!

Also reassigning a new CM as this will need the payment completed once the invite is accepted

@thesahindia
Copy link
Member

@davidcardoza, I have accepted the offer. Please settle this at upwork so that we can close this issue.

@davidcardoza
Copy link
Contributor

Done

@joelbettner
Copy link
Contributor

This issue still needs to be open because the refactor PR is still in review.

@joelbettner joelbettner reopened this Dec 14, 2022
@joelbettner
Copy link
Contributor

This was taken care of by the PR mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests