[HOLD for payment 2023-11-02] [$500] Dev - Home address - AddressPage (and many more) – Missing displayName prop leads to console error (testID undefined) #28334

kbecciv · 2023-09-27T17:10:27Z

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:

Open the profile popup via LHN
Click the profile image icon to open up profile settings
Click 'Personal details'
Click 'Home address'

Expected Result:

The testID property for the page should be assigned a valid value.

Actual Result:

The testID property is undefined. As a result, end-to-end testing is compromised, due to many pages missing testIDs.

Workaround:

Unknown

Platforms:

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

Version Number: Dev 1.3.74-0
Reproducible in staging?: n
Reproducible in production?: n
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

bug-1.mov

bug-1-ios-web.mov

Expensify/Expensify Issue URL:
Issue reported by: @Samueljh1
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695735346255799

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~010af2a5f37c5f7f2f
Upwork Job ID: 1707080289779253248
Last Price Increase: 2023-09-27
Automatic offers:

samueljh1 | Reporter | 27506510

Issue Owner

Current Issue Owner: @garrettmknight

melvin-bot · 2023-09-27T17:10:34Z

Job added to Upwork: https://www.upwork.com/jobs/~010af2a5f37c5f7f2f

melvin-bot · 2023-09-27T17:10:39Z

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

ikevin127 · 2023-09-27T17:28:24Z

Proposal - Updated

@roryabraham:

As stated above, I want to see a proposal for this issue which fixes it by:

1. Removing the custom lint rule from eslint-config-expensify that prevents setting `displayName` on class components

2. Adding a new lint rule that requires all React components to have their `displayName` property set.

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

Removing the custom lint rule from eslint-config-expensify that prevents setting displayName on class components.
Adding a new lint rule that requires all React components to have their displayName property set.

What is the root cause of that problem?

The testID property is undefined for multiple pages and due to it being a required prop we get the reported console errors whenever displayName is not set for a component.

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

As mentioned by the quote at the start of the updated proposal we need the following changes:

Removing the custom lint rule from eslint-config-expensify that prevents setting displayName on class components:

module.exports = {
    rules: {
        'rulesdir/no-negated-variables': 'error',
        'rulesdir/no-api-in-views': 'error',
        'rulesdir/prefer-actions-set-data': 'error',
        'rulesdir/prefer-onyx-connect-in-libs': 'error',
        'rulesdir/no-thenable-actions-in-views': 'error',
        'rulesdir/prefer-early-return': 'error',
        'rulesdir/no-inline-named-export': 'error',
        'rulesdir/prefer-underscore-method': 'error',
        'rulesdir/no-useless-compose': 'error',
        'rulesdir/prefer-import-module-contents': 'error',
        'rulesdir/no-multiple-api-calls': 'error',
        'rulesdir/no-multiple-onyx-in-file': 'error',
        'rulesdir/no-call-actions-from-actions': 'error',
        'rulesdir/no-api-side-effects-method': 'error',
-       'rulesdir/display-name-property': 'error',
        'rulesdir/prefer-localization': 'error',
        'rulesdir/onyx-props-must-have-default': 'error',
        'no-restricted-imports': ['error', {
            paths: [{
                name: 'react-native',
                importNames: ['Button', 'Text', 'TextInput', 'Picker'],
                message: 'Please use an Expensify component from src/components/ instead.',
            }, {
                name: 'react-native',
                importNames: ['SafeAreaView'],
                message: 'Please use SafeAreaView from react-native-safe-area-context',
            }],
        }],
    },
};

Adding a new lint rule that requires all React components to have their displayName property set:

        ...
        {
            files: ['*.js', '*.jsx'],
            settings: {
                'import/resolver': {
                    node: {
                        extensions: ['.js', '.website.js', '.desktop.js', '.native.js', '.ios.js', '.android.js', '.config.js', '.ts', '.tsx'],
                    },
                },
            },
            rules: {
                'import/extensions': [
                    'error',
                    'ignorePackages',
                    {
                        js: 'never',
                        jsx: 'never',
                        ts: 'never',
                        tsx: 'never',
                    },
                ],
                'no-restricted-imports': [
                    'error',
                    {
                        paths: restrictedImportPaths,
                        patterns: restrictedImportPatterns,
                    },
                ],
                curly: 'error',
+                "react/display-name": [
+                    "error",
+                    {
+                        ignoreTranspilerName: false
+                    }
+                ],
+            },
+        },
        ...

The video below demonstrates:

removal of the display name rule from eslint-config-expensify
addition of the react/display-name rule to the .eslintrc.js for .js / .jsx components
the testID error still occurs on PersonalDetailsInitialPage component when the display name is not set
the npm run lint linter results requiring all React components to have their displayName property set

What alternative solutions did you explore? (Optional)

N/A

Videos

web.mp4

ZhenjaHorbach · 2023-09-27T17:30:24Z

I'm not sure if this problem is still relevant

This PR #28089 should have fixed errors like this

HardikChoudhary24 · 2023-09-27T17:44:29Z

Proposal

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

AddressPage (and many more) – Missing displayName prop leads to console error (testID undefined)

What is the root cause of that problem?

The root cause of this issue is the absence of AddressPage.displayName="AddressPage" for the AddressPage and similarly for other pages.

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Lines 132 to 135 in e74b4d2

    
           <ScreenWrapper 
        
               includeSafeAreaPaddingBottom={false} 
        
               testID={AddressPage.displayName} 
        
           >

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

To address this problem, we can resolve it by simply adding AddressPage.displayName="AddressPage" for Address Page Component and similarly for each components missing the displayName.
This will solve the issue perfectly.

What alternative solutions did you explore? (Optional)
N/A

ZhenjaHorbach · 2023-09-27T17:49:40Z

Proposal

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

AddressPage (and many more) – Missing displayName prop leads to console error (testID undefined)

What is the root cause of that problem?

The root cause of this issue is the absence of testID ={AddressPage.displayName} where AddressPage.displayName="AddressPage" for the AddressPage and similarly for other pages.

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 132 in 76c1559

<ScreenWrapper includeSafeAreaPaddingBottom={false}>

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

To address this problem, we can resolve it by simply adding testID ={AddressPage.displayName} and setting AddressPage.displayName="AddressPage" for each components missing the testID prop.
This will solve the issue perfectly.

What alternative solutions did you explore? (Optional) N/A

Check main branch)

HardikChoudhary24 · 2023-09-27T17:52:37Z

Proposal

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

AddressPage (and many more) – Missing displayName prop leads to console error (testID undefined)

What is the root cause of that problem?

The root cause of this issue is the absence of testID ={AddressPage.displayName} where AddressPage.displayName="AddressPage" for the AddressPage and similarly for other pages.

App/src/pages/settings/Profile/PersonalDetails/AddressPage.js

Line 132 in 76c1559

<ScreenWrapper includeSafeAreaPaddingBottom={false}>

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

To address this problem, we can resolve it by simply adding testID ={AddressPage.displayName} and setting AddressPage.displayName="AddressPage" for each components missing the testID prop.
This will solve the issue perfectly.

What alternative solutions did you explore? (Optional) N/A

Check main branch)

Yes, I have Updated my proposal.

Samueljh1 · 2023-09-28T20:58:05Z

Proposal

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

The testID property for the affected pages (e.g., AddressPage) should be assigned a valid value, but it is instead undefined for many pages.

What is the root cause of that problem?

Some pages have missing definitions for the displayName prop, which is commonly used to set the testID prop.

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

Add a definition for XXXPage.displayName for all pages using a ScreenWrapper, to ensure testID is not undefined.

What alternative solutions did you explore? (Optional)

roryabraham · 2023-09-29T09:31:29Z

Sorry, I caused this bug. I think to fix it we should remove the lint rule that prevents setting displayName on class components, and instead enforce that all components have a displayName set.

melvin-bot · 2023-10-03T11:23:10Z

@sobitneupane Huh... This is 4 days overdue. Who can take care of this?

roryabraham · 2023-10-04T02:08:56Z

Assigning myself since I caused all these console errors in #27978

roryabraham · 2023-10-04T02:10:51Z

As stated above, I want to see a proposal for this issue which fixes it by:

Removing the custom lint rule from eslint-config-expensify that prevents setting displayName on class components
Adding a new lint rule that requires all React components to have their displayName property set.

sobitneupane · 2023-10-04T08:28:03Z

We haven't received any proposal that meets the expectation.

ikevin127 · 2023-10-04T08:49:58Z

Updated my proposal.
Of course it will require me to add the displayName for all those 77 components that are missing it.

melvin-bot · 2023-10-04T10:08:05Z

📣 @sobitneupane Please request via NewDot manual requests for the Reviewer role ($500)

melvin-bot · 2023-10-04T10:08:08Z

📣 @ikevin127 You have been assigned to this job!
Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs!
Keep in mind: Code of Conduct | Contributing 📖

melvin-bot · 2023-10-04T10:08:10Z

📣 @Samueljh1 We're missing your Upwork ID to automatically send you an offer for the Reporter role.
Once you apply to the Upwork job, your Upwork ID will be stored and you will be automatically hired for future jobs!

roryabraham · 2023-10-04T10:11:17Z

@ikevin127 the video looks good so I assigned you, but I'm a bit confused about one thing:

from the docs, ignoreTranspilerName defaults to false and the setting it to true seems to be what makes the displayName property required. Are the docs wrong or did you mean to include ignoreTranspilerName=true in your proposal? Or am I missing something?
Please also create a follow-up PR in eslint-config-expensify to remove the unused custom rule once it's no longer used in this codebase.

Thanks!

roryabraham · 2023-10-04T10:12:26Z

Or if ignoreTranspilerName should indeed be false, I think that should be the default according to the docs, so we don't need to specify it.

melvin-bot · 2023-10-26T22:31:15Z

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot · 2023-10-26T22:31:17Z

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.91-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

fix/28334: Missing displayName prop leads to console error (testID undefined) #28813

If no regressions arise, payment will be issued on 2023-11-02. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

@ikevin127 requires payment
@Samueljh1 requires payment

melvin-bot · 2023-11-03T15:26:19Z

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

melvin-bot · 2023-11-03T15:26:21Z

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

melvin-bot · 2023-11-03T15:27:29Z

📣 @mananjadhav Please request via NewDot manual requests for the Contributor role ($500)

melvin-bot · 2023-11-03T15:27:33Z

📣 @Samueljh1 🎉 An offer has been automatically sent to your Upwork account for the Reporter role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

roryabraham · 2023-11-03T15:27:59Z

@garrettmknight we were missing a BZ assignee here, my fault! let's issue payments for this issue. Thanks!

situchan · 2023-11-03T15:31:01Z

There was regression fixed in #30397

garrettmknight · 2023-11-03T15:32:04Z

I can help out

garrettmknight · 2023-11-03T16:42:51Z

@ikevin127 can you apply to this job posting in Upwork so I can pay you out?

garrettmknight · 2023-11-03T16:44:48Z

@Samueljh1 can you accept this offer so I can pay you out?

garrettmknight · 2023-11-03T16:44:56Z

Summary of payments:

Urgency bonus: No
Regression penalty: Yes
Reporter: @Samueljh1 $50 paid via upwork
Contributor: @ikevin127 $250 paid via upwork
Reviewer: @mananjadhav $250

JmillsExpensify · 2023-11-03T21:30:28Z

$250 payment approved for @mananjadhav based on comment above.

Samueljh1 · 2023-11-04T03:29:25Z

@Samueljh1 can you accept this offer so I can pay you out?

Done, thanks

garrettmknight · 2023-11-06T16:29:26Z

Everyone's paid - closing.

kbecciv added the External Added to denote the issue can be worked on by a contributor label Sep 27, 2023

melvin-bot bot changed the title ~~Dev - Home address - AddressPage (and many more) – Missing displayName prop leads to console error (testID undefined)~~ [$500] Dev - Home address - AddressPage (and many more) – Missing displayName prop leads to console error (testID undefined) Sep 27, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 27, 2023

melvin-bot bot assigned sobitneupane Sep 27, 2023

melvin-bot bot added the Daily KSv2 label Sep 27, 2023

situchan mentioned this issue Sep 29, 2023

[No QA] Use displayName for testID #27978

Merged

59 tasks

melvin-bot bot added the Overdue label Oct 3, 2023

roryabraham self-assigned this Oct 4, 2023

melvin-bot bot removed the Overdue label Oct 4, 2023

roryabraham assigned ikevin127 Oct 4, 2023

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 4, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 26, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Oct 26, 2023

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 2, 2023

roryabraham added the Bug Something is broken. Auto assigns a BugZero manager. label Nov 3, 2023

melvin-bot bot assigned garrettmknight Nov 3, 2023

roryabraham assigned mananjadhav and unassigned garrettmknight Nov 3, 2023

garrettmknight self-assigned this Nov 3, 2023

melvin-bot bot added the Overdue label Nov 6, 2023

garrettmknight closed this as completed Nov 6, 2023

melvin-bot bot removed the Overdue label Nov 6, 2023

[HOLD for payment 2023-11-02] [$500] Dev - Home address - AddressPage (and many more) – Missing displayName prop leads to console error (testID undefined) #28334

[HOLD for payment 2023-11-02] [$500] Dev - Home address - AddressPage (and many more) – Missing displayName prop leads to console error (testID undefined) #28334

Comments

kbecciv commented Sep 27, 2023 • edited by roryabraham Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

melvin-bot bot commented Sep 27, 2023

melvin-bot bot commented Sep 27, 2023

ikevin127 commented Sep 27, 2023 • edited Loading

Proposal - Updated

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

Videos

ZhenjaHorbach commented Sep 27, 2023

HardikChoudhary24 commented Sep 27, 2023 • edited Loading

Proposal

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

What is the root cause of that problem?

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

ZhenjaHorbach commented Sep 27, 2023

Proposal

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

What is the root cause of that problem?

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

HardikChoudhary24 commented Sep 27, 2023

Proposal

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

What is the root cause of that problem?

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

Samueljh1 commented Sep 28, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

roryabraham commented Sep 29, 2023

melvin-bot bot commented Oct 3, 2023

roryabraham commented Oct 4, 2023

roryabraham commented Oct 4, 2023

sobitneupane commented Oct 4, 2023

ikevin127 commented Oct 4, 2023

melvin-bot bot commented Oct 4, 2023

melvin-bot bot commented Oct 4, 2023

melvin-bot bot commented Oct 4, 2023

roryabraham commented Oct 4, 2023

roryabraham commented Oct 4, 2023

melvin-bot bot commented Oct 26, 2023

melvin-bot bot commented Oct 26, 2023

melvin-bot bot commented Nov 3, 2023

melvin-bot bot commented Nov 3, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Nov 3, 2023

melvin-bot bot commented Nov 3, 2023

roryabraham commented Nov 3, 2023

situchan commented Nov 3, 2023

garrettmknight commented Nov 3, 2023

garrettmknight commented Nov 3, 2023

garrettmknight commented Nov 3, 2023

garrettmknight commented Nov 3, 2023 • edited Loading

JmillsExpensify commented Nov 3, 2023

Samueljh1 commented Nov 4, 2023

garrettmknight commented Nov 6, 2023

kbecciv commented Sep 27, 2023 •

edited by roryabraham

Loading

ikevin127 commented Sep 27, 2023 •

edited

Loading

HardikChoudhary24 commented Sep 27, 2023 •

edited

Loading

garrettmknight commented Nov 3, 2023 •

edited

Loading