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] Create expense - No animation when submitting expenses as a new user #54470

Open
2 of 8 tasks
vincdargento opened this issue Dec 23, 2024 · 18 comments
Open
2 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@vincdargento
Copy link

vincdargento commented Dec 23, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: v9.0.77-6
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: N/A
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Email or phone of affected tester (no customers): N/A
Issue reported by: Applause Internal Team

Action Performed:

Precondition: You should login as a new user.

  1. Open https://staging.new.expensify.com/
  2. Click on the green "+" button.
  3. Submit a manual expense

Expected Result:

A fun illustration (with short and clear directions) should be displayed to the empty state “Submits to” screen if the user does not have any recent contacts (as in PR #42413). If there are any recents in the list, the illustration shouldn't be displayed.

Actual Result:

No animation when submitting expenses as a new user, just empty page.

Workaround:

Unknown

Platforms:

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

bug.mp4

bug

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021871676942933585608
  • Upwork Job ID: 1871676942933585608
  • Last Price Increase: 2024-12-24
  • Automatic offers:
    • huult | Contributor | 105506456
Issue OwnerCurrent Issue Owner: @thesahindia
@vincdargento vincdargento added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Dec 23, 2024
Copy link

melvin-bot bot commented Dec 23, 2024

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

@abzokhattab
Copy link
Contributor

Proposal

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

When a new user submits an expense, no empty animation is shown

What is the root cause of that problem?

The issue is caused by how the EmptySelectionListContent component determines which animation and translation to display. Here’s the breakdown:

  1. In the MoneyRequestParticipantsSelector component, the iouType is passed as the contentType
  2. The EmptySelectionListContent component checks if the contentType is included in the CONTENT_TYPES list
    function EmptySelectionListContent({contentType}: EmptySelectionListContentProps) {
    const styles = useThemeStyles();
    const {translate} = useLocalize();
    if (!isContentType(contentType)) {
    return null;
    }
    const EmptySubtitle = (
    <Text style={[styles.textAlignCenter]}>
    {translate(`emptyList.${contentType}.subtitleText1`)}
  3. If the contentType is valid, the EmptySelectionListContent component uses it to fetch the corresponding translation and animation.
    const CONTENT_TYPES = [CONST.IOU.TYPE.SUBMIT, CONST.IOU.TYPE.SPLIT, CONST.IOU.TYPE.PAY];

The problem occurs because CONST.IOU.TYPE.CREATE is not included in the CONTENT_TYPES list. Without this, the EmptySelectionListContent component cannot find a matching translation or animation for the CREATE type.

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

  1. Add CONST.IOU.TYPE.CREATE to the CONTENT_TYPES list in EmptySelectionListContent.

  2. Update the translation file to include an entry for CONST.IOU.TYPE.CREATE, similar to how CONST.IOU.TYPE.SUBMIT is handled.

    App/src/languages/en.ts

    Lines 587 to 606 in ba6168a

    emptyList: {
    [CONST.IOU.TYPE.SUBMIT]: {
    title: 'Submit an expense',
    subtitleText1: 'Submit to someone and ',
    subtitleText2: `get $${CONST.REFERRAL_PROGRAM.REVENUE}`,
    subtitleText3: ' when they become a customer.',
    },
    [CONST.IOU.TYPE.SPLIT]: {
    title: 'Split an expense',
    subtitleText1: 'Split with a friend and ',
    subtitleText2: `get $${CONST.REFERRAL_PROGRAM.REVENUE}`,
    subtitleText3: ' when they become a customer.',
    },
    [CONST.IOU.TYPE.PAY]: {
    title: 'Pay someone',
    subtitleText1: 'Pay anyone and ',
    subtitleText2: `get $${CONST.REFERRAL_PROGRAM.REVENUE}`,
    subtitleText3: ' when they become a customer.',
    },
    },

    Or alternatively, replace the translation for CONST.IOU.TYPE.SUBMIT with one for CONST.IOU.TYPE.CREATE, as the submit button was changed to create.

  3. (Optional) Adjust the blockingview styles to ensure the empty animation is vertically centered in the modal.

POC

image

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

  • Verify that the EmptySelectionListContent component displays the correct animation and translation for CONST.IOU.TYPE.CREATE.

What alternative solutions did you explore? (Optional)

@FitseTLT
Copy link
Contributor

FitseTLT commented Dec 23, 2024

Edited by proposal-police: This proposal was edited at 2024-12-23 22:09:38 UTC.

Proposal

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

Create expense - No animation when submitting expenses as a new user

What is the root cause of that problem?

We are only handling split, pay, and submit in EmptySelectionListContent

const CONTENT_TYPES = [CONST.IOU.TYPE.SUBMIT, CONST.IOU.TYPE.SPLIT, CONST.IOU.TYPE.PAY];

if (!isContentType(contentType)) {
return null;
}

so blank is displayed for iouType create (create expense from global FAB) and also invoice (from send invoice)

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

We need to add cases of INVOICE and CREATE:

  1. add both types here
  2. Add the respective copies here

If style changes are needed we can apply discussing with the design team
Note: We are still using submit in submit it to someone whisper action from tracked expense so we can't remove submit.

Note: The comment here suggesting invoice should not be included is based on a wrong assumption that at the time the user send invoice there would be at least on contact to be shown in the participants list but that is wrong. If we send invoice after creating a workspace and enabling invoice before contacting anyone blank page is displayed as clearly seen in the below video.

2024-12-24.01-21-13.mp4

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can test EmptySelectionListContent displays correctly for iouTypes INVOICE and CREATE

What alternative solutions did you explore? (Optional)

@abzokhattab
Copy link
Contributor

abzokhattab commented Dec 23, 2024

According to this discussion, invoices should not be included in the list so focusing on the current issue that's why i didn't include it in the proposal... but in case it's needed after discussion, the changes should be the same as the main solution.

#42227 (comment)

We can in fact enable it for all IOU types by removing the whitelist validation. then, we should add tests to ensure that all IOU types have their respective translations.
This way, if a new IOU type is introduced without existing translation, the test will fail .. therefore we will prevent such issues to happen in the future

if (!isContentType(contentType)) {
return null;
}

@sakluger sakluger added the External Added to denote the issue can be worked on by a contributor label Dec 24, 2024
@melvin-bot melvin-bot bot changed the title Create expense - No animation when submitting expenses as a new user [$250] Create expense - No animation when submitting expenses as a new user Dec 24, 2024
Copy link

melvin-bot bot commented Dec 24, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 24, 2024
Copy link

melvin-bot bot commented Dec 24, 2024

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

@sakluger sakluger moved this to Bugs and Follow Up Issues in [#whatsnext] #expense Dec 24, 2024
@huult
Copy link
Contributor

huult commented Dec 26, 2024

Edited by proposal-police: This proposal was edited at 2024-12-26 09:02:34 UTC.

Proposal

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

No animation when submitting expenses as a new user

What is the root cause of that problem?

In this case, the iouType is create and is not included in the list of CONTENT_TYPES, so the empty page will not be displayed

listEmptyContent={<EmptySelectionListContent contentType={iouType} />}

if (!isContentType(contentType)) {
return null;
}

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

We have many similar cases, so I think we should create a common empty component to ensure consistency.

function EmptySelectionListContent({contentType}: EmptySelectionListContentProps) {

We created a new prop to determine whether to use the default empty state UI and define a common empty UI, and made the contentType prop optional

  function EmptySelectionListContent({contentType, isDefault}: EmptySelectionListContentProps) {
      const styles = useThemeStyles();
      const {translate} = useLocalize();

      if (!isContentType(contentType)) {
          if (isDefault) {
              return (
                  <View style={[styles.flex1, styles.overflowHidden, styles.minHeight65]}>
                      <BlockingView
                          icon={Illustrations.TurtleInShell}
                          iconWidth={variables.emptyListIconWidth}
                          iconHeight={variables.emptyListIconHeight}
                          title={'Nothing to Display'}
                          subtitle={'There’s currently nothing to display here.'}
                          subtitleStyle={styles.textSupporting}
                          containerStyle={styles.pb10}
                          contentFitImage="contain"
                      />
                  </View>
              );
          }
          return null;
      }
      ...
  }

And anywhere we want to use the default empty state, such as this one

listEmptyContent={<EmptySelectionListContent contentType={iouType} />}

Update to:

  listEmptyContent={
        <EmptySelectionListContent
            contentType={iouType}
            isDefault
        />
    }

or

  listEmptyContent={
        <EmptySelectionListContent
            contentType={iouType}
            isDefault={iouType === CONST.IOU.TYPE.CREATE}
        />
    }
POC
  • Screenshot 2024-12-26 at 15 55 38

I think the UI should be confirmed by the design team and updated when creating the PR (like: content, image, scrollview....)

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

Copy link

melvin-bot bot commented Dec 30, 2024

@sakluger, @thesahindia Eep! 4 days overdue now. Issues have feelings too...

@melvin-bot melvin-bot bot added the Overdue label Dec 30, 2024
@thesahindia
Copy link
Member

We have many similar cases, so I think we should create a common empty component to ensure consistency.

@huult, could you please list some of them?

@melvin-bot melvin-bot bot removed the Overdue label Dec 30, 2024
@huult
Copy link
Contributor

huult commented Dec 30, 2024

@thesahindia here you are:

  • Screenshot 2024-12-30 at 22 17 23

@thesahindia
Copy link
Member

I thought you were referring to places where we use illustrations. It seems there are currently very few cases, but creating a common component could be beneficial if we use illustrations in more scenarios in the future.

@huult
Copy link
Contributor

huult commented Dec 30, 2024

@thesahindia Yes, I will provide it later. thank you!

@thesahindia
Copy link
Member

@abzokhattab's proposal will fix this. But @huult suggested adding a common component for such cases. I prefer @huult's proposal but I will defer to the internal engineer on this.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Dec 30, 2024

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

@puneetlath
Copy link
Contributor

I also like @huult's more global solution.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 30, 2024
Copy link

melvin-bot bot commented Dec 30, 2024

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

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@huult
Copy link
Contributor

huult commented Dec 30, 2024

Thank you! I will create the pull request soon.

@abzokhattab
Copy link
Contributor

abzokhattab commented Dec 30, 2024

Thanks for the review...

I have a question: where would we use this common illustration ? as as i see all insatnaces that use EmptySelectionListContent will have the correct illustration/tranlsation the only problem is with the invoice and the create.

if you mean to apply it globally on all searches that's another thing and i think we should get an approval from the design team on that

i mean the case where isDefault will be true here will not happen unless you add this snippet in all searches

  listEmptyContent={
        <EmptySelectionListContent
            contentType={iouType}
            isDefault={iouType === CONST.IOU.TYPE.CREATE}
        />
    }

What do you think? @puneetlath @thesahindia

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 External Added to denote the issue can be worked on by a contributor
Projects
Status: Bugs and Follow Up Issues
Development

No branches or pull requests

7 participants