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] QAB - Quick action shows the old title "Track distance" for distance expense in self DM #55456

Open
6 of 8 tasks
lanitochka17 opened this issue Jan 19, 2025 · 59 comments
Open
6 of 8 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 19, 2025

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: 9.0.87-0
Reproducible in staging?: Y
Reproducible in production?: Y
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): [email protected]
Issue reported by: Applause - Internal Team

Action Result:

  1. Go to staging.new.expensify.com
  2. Go to self DM
  3. Click + > Create expense > Manual
  4. Create a manual expense in self DM
  5. Open FAB
  6. Note that Quick action shows "Create expense"
  7. Create a scan expense in self DM
  8. Open FAB
  9. Note that Quick action shows "Create expense for receipt"
  10. Create a distance expense in self DM
  11. Open FAB

Expected Result:

Since "track expense" is renamed to "create expense", "Track distance" in Quick action should be renamed to "Create expense for distance", which is coherent with Quick action for "Create expense" (Step 6) and "Create expense for receipt" (Step 9)

Actual Result:

Quick action still shows the old title "Track distance" for distance expense in self DM

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence
Bug6717987_1737263039838.20250119_125142.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021881123172239037305
  • Upwork Job ID: 1881123172239037305
  • Last Price Increase: 2025-02-04
  • Automatic offers:
    • etCoderDysto | Contributor | 105938059
Issue OwnerCurrent Issue Owner: @mananjadhav
@lanitochka17 lanitochka17 added Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 labels Jan 19, 2025
Copy link

melvin-bot bot commented Jan 19, 2025

Triggered auto assignment to @jliexpensify (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.

@etCoderDysto
Copy link
Contributor

etCoderDysto commented Jan 19, 2025

🚨 Edited by proposal-police: This proposal was edited at 2025-01-19 14:36:34 UTC.

🚨 Edited by proposal-police: This proposal was edited at 2025-01-19 14:36:34 UTC.

Proposal

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

QAB - Quick action shows the old title "Track distance" for distance expense in self DM

Bug 1

  • Update recordDistance copy to "Track distance"
  • A quick refactor to clean up this code, as maintaining distinct trackManual, trackScan, trackDistance options here seem unnecessary now with the consolidation to one "Create expense" flow.

Bug 2

  • Update the per diem QAB copy to "Create per diem". Note: currently we are not displaying perdiem qab action after a perdiem expense is created.

What is the root cause of that problem?

Bug1

  • We are getting fab title here using titleKey

    const titleKey = getQuickActionTitle(quickAction?.action ?? ('' as QuickActionName));
    return titleKey ? translate(titleKey) : '';

  • getQuickActionTitle returns translation keys that is based on the quick action type. Example: if the quick action type is manual request quickAction.requestMoney will be returned

    const getQuickActionTitle = (action: QuickActionName): TranslationPaths => {
    switch (action) {
    case CONST.QUICK_ACTIONS.REQUEST_MANUAL:
    return 'quickAction.requestMoney';

  • then key will be used to get the translation copies. Currently, we are using separate keys for track expense actions and request actions. CONST.QUICK_ACTIONS.REQUEST_MANUAL and CONST.QUICK_ACTIONS.TRACK_MANUAL will return different translation keys

    App/src/languages/en.ts

    Lines 838 to 844 in 86b9c79

    quickAction: {
    scanReceipt: 'Scan receipt',
    recordDistance: 'Record distance',
    requestMoney: 'Create expense',
    splitBill: 'Split expense',
    splitScan: 'Split receipt',
    splitDistance: 'Split distance',

Bug 2 (per diem)
When creating perdiem request, we are not updating quick action with the new perdiem request as shown below. For manual and scan request, we update quick action here, but we are not handling the case for perdiem request. BE also doesn't create quick action for perdiem requsts.

The call stack for perdiem: submitPerDiemExpense -> getPerDiemExpenseInformation -> buildOnyxDataForMoneyRequest

App/src/libs/actions/IOU.ts

Lines 1340 to 1348 in b59f23b

if (!isOneOnOneSplit && !isPerDiemRequest) {
optimisticData.push({
onyxMethod: Onyx.METHOD.SET,
key: ONYXKEYS.NVP_QUICK_ACTION_GLOBAL_CREATE,
value: {
action: newQuickAction,
chatReportID: chat.report?.reportID,
isFirstQuickAction: isEmptyObject(quickAction),
},

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

Bug 1

  • Change the getQuickActionTitle switch case to return scanReceipt, recordDistance and requestMoney translation keys for money request and track expense here
case CONST.QUICK_ACTIONS.REQUEST_MANUAL || CONST.QUICK_ACTIONS.TRACK_MANUAL:
            return 'quickAction.requestMoney';
        case CONST.QUICK_ACTIONS.REQUEST_SCAN || CONST.QUICK_ACTIONS.TRACK_SCAN:
            return 'quickAction.scanReceipt';
        case CONST.QUICK_ACTIONS.REQUEST_DISTANCE ||  CONST.QUICK_ACTIONS.TRACK_DISTANCE:
            return 'quickAction.recordDistance';
  • Update recordDistance key copy to "Track distance"
  • Remove trackManual, trackScan, and trackDistance copies from en.ts and es.ts

App/src/languages/en.ts

Lines 838 to 841 in 86b9c79

quickAction: {
scanReceipt: 'Scan receipt',
recordDistance: 'Record distance',
requestMoney: 'Create expense',

Bug 2:

  1. Add a variable for per diem here
PER_DIEM: 'perDiem'
  1. Here we should assign newQuickAction = CONST.QUICK_ACTIONS.PER_DIEM when the request type is perdiem

let newQuickAction: ValueOf<typeof CONST.QUICK_ACTIONS> = isScanRequest ? CONST.QUICK_ACTIONS.REQUEST_SCAN : CONST.QUICK_ACTIONS.REQUEST_MANUAL;

  1. Then here we should remove && !isPerDiemRequest logic to updated onyx quick action with perdiem request type

    App/src/libs/actions/IOU.ts

    Lines 1340 to 1345 in b59f23b

    if (!isOneOnOneSplit && !isPerDiemRequest) {
    optimisticData.push({
    onyxMethod: Onyx.METHOD.SET,
    key: ONYXKEYS.NVP_QUICK_ACTION_GLOBAL_CREATE,
    value: {
    action: newQuickAction,

  2. Here return a new translation key for per diem quick action

        case CONST.QUICK_ACTIONS.PER_DIEM:
            return 'quickAction.perDiemRequest';
  1. Add a copy in en.ts and es.ts for quickAction.perDiemRequest translation key

  2. we should update backend to to mirror the front end

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

We can add test that validates the quick action type is perdiem after creating perdiem expense

What alternative solutions did you explore? (Optional)

Copy link
Contributor

⚠️ @etCoderDysto Thanks for your proposal. Please update it to follow the proposal template, as proposals are only reviewed if they follow that format (note the mandatory sections).

@jliexpensify jliexpensify added the External Added to denote the issue can be worked on by a contributor label Jan 19, 2025
@melvin-bot melvin-bot bot changed the title QAB - Quick action shows the old title "Track distance" for distance expense in self DM [$250] QAB - Quick action shows the old title "Track distance" for distance expense in self DM Jan 19, 2025
Copy link

melvin-bot bot commented Jan 19, 2025

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 19, 2025
Copy link

melvin-bot bot commented Jan 19, 2025

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

@jliexpensify jliexpensify changed the title [$250] QAB - Quick action shows the old title "Track distance" for distance expense in self DM [$125] QAB - Quick action shows the old title "Track distance" for distance expense in self DM Jan 19, 2025
Copy link

melvin-bot bot commented Jan 19, 2025

Upwork job price has been updated to $125

@jliexpensify
Copy link
Contributor

@Expensify/design I think this one makes sense, but I'd like to confirm this is ok for the rename:

"Track distance" in Quick action should be renamed to "Create expense for distance", which is coherent with Quick action for "Create expense" (Step 6) and "Create expense for receipt" (Step 9)

@shawnborton
Copy link
Contributor

Yeah, I think it makes sense to map the QAB actions to the Global create actions. What if we used our dot separator pattern for the QAB labels? Like so:

  • If the user does Create expense > Manual, then QAB should say Create expense • Manual
  • If the user does Create expense > Scan, then QAB should say Create expense • Scan
  • If the user does Create expense > Distance, then QAB should say Create expense • Distance
  • If the user does Create expense > Per diem, then the QAB should say Create expense • Per diem

Thoughts @Expensify/design @trjExpensify @JmillsExpensify @jamesdeanexpensify ?

@dubielzyk-expensify
Copy link
Contributor

Note that Quick action shows "Create expense for receipt"

I don't mind that at all and I recon it's probably the right way to go especially from a systems perspective. I will say I really like the human and straight forward string of Scan receipt. Not sure if it can be replicated easily across the board, but if we go the less structured way, I wouldn't mind seeing something like:

  • Manual -> Create expense
  • Scan -> Scan receipt
  • Distance -> Track distance (Add distance!? )
  • Per diem -> Per diem request (!?)

@shawnborton
Copy link
Contributor

I see what you are saying but I don't love the variability of the words we're using there... some are expenses, some are requests, some track, some create, etc...

I could get down with something like:

  • Create manual expense
  • Scan receipt
  • Create distance expense
  • Create per diem expense

Thoughts?

@dubielzyk-expensify
Copy link
Contributor

Yeah that's a fair point. I guess I just think words such as manual are less friendly etc. I appreciate the riff 🙏

I think the above works well for me, but keen to hear what the others think.

@etCoderDysto
Copy link
Contributor

  • Create manual expense
  • Scan receipt
  • Create distance expense
  • Create per diem expense

@shawnborton Does this change apply for quick actions created my track expense and money request or are we making the change for track expense only?

Currently we are displaying the copy below for money request(create expense) types

        scanReceipt: 'Scan receipt',
        recordDistance: 'Record distance',
        requestMoney: 'Create expense',

And this copy for track expense

        trackManual: 'Create expense',
        trackScan: 'Create expense for receipt',
        trackDistance: 'Track distance',

@shawnborton
Copy link
Contributor

Part of me thinks we'd use the same language everywhere, the only difference is that a "tracked" expense ends up in your personal space, which would be denoted by the icon on the right side of the QAB row.

From my personal space, I don't even see the langauge for Track anymore anyways:

Image

Maybe @trjExpensify knows how that works now though? Where and when would you see Track language in the app now?

@etCoderDysto
Copy link
Contributor

Yes, the subtitle (name(you)) also can be serve as an identifier that the action is track expense.

Image

@shawnborton
Copy link
Contributor

Yes, the subtitle (name(you)) also can be serve as an identifier that the action is track expense.

Love that, yup that makes sense.

@trjExpensify
Copy link
Contributor

I think we should keep the Scan receipt label, as we did that specifically for an element of familiarity of migrated users and "fire and forget" to open the camera.

I think probably "Track distance" is the most colloquial term for the action there, we just couldn't use it before because of the whole "Track expense" flow, hence why we rolled with "Record distance". So I would vote for the below personally:

Manual: Create expense
Scan: Scan receipt
Distance: Track distance

@dannymcclain
Copy link
Contributor

I initially really loved Shawn's idea of using the dot separator pattern, but after reading through the thread I think I've come around to what Tom is saying above. Now that we're banishing the idea of "tracking an expense" being tied to your personal space, I think what Tom has laid out makes the most sense/will jive with users the best.

@shawnborton
Copy link
Contributor

Yeah same, I can totally get down with what Tom is suggesting. I think it feels much more natural and succinct than what I was suggesting.

@trjExpensify what are you thinking for per diem though?

@trjExpensify
Copy link
Contributor

@trjExpensify what are you thinking for per diem though?

In the doc it just had Per diem for the QAB label, I'm okay with leaving that one, but could also get behind Create per diem if we want.

@shawnborton
Copy link
Contributor

I like Create per diem so we create some consistency with using a verb at the beginning across all of the items.

@trjExpensify
Copy link
Contributor

Okay cool, based on the above the copy for trackManual and trackDistance are good, but the trackScan one has incorrectly been changed to Create expense for receipt which isn't ideal. I've asked @JKobrynski to update that in his PR here.

As for this issue, I think we can focus on these things:

  1. Update recordDistance copy to "Track distance"
  2. Update the per diem QAB copy to "Create per diem" (though I can't even see it as a quickAction here?)
  3. A quick refactor to clean up this code, as maintaining distinct trackManual, trackScan, trackDistance options here seem unnecessary now with the consolidation to one "Create expense" flow. (Per the above point, if the destination for the quick action is the selfDM, that's highlighted in the second line of the quick action button).

How does that sound to everyone?

@shawnborton
Copy link
Contributor

Works for me 👍

@etCoderDysto
Copy link
Contributor

Sorry for not making it clear. I will clean it up in a minute.

@mananjadhav
Copy link
Collaborator

Thanks. Looking forward to it.

@etCoderDysto
Copy link
Contributor

@mananjadhav I have tried to make my proposal clear. I hope it is clear now. It might be confusing since we are trying to handle two bugs. 1. Updating, and consolidating the qab copy for manual, scan, and distance money request as well as track expense, and 2. Creating QAB support for perdiem requests. Currently, when creating perdiem request quick action will not be updated with perdiem quick action and retain the previous quick action type.

Updated proposal

@mananjadhav
Copy link
Collaborator

Thanks for the revisions. This looks better. @etCoderDysto's proposal looks good to me.

🎀 👀 🎀 C+ reviewed.

Copy link

melvin-bot bot commented Jan 29, 2025

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

@arosiclair
Copy link
Contributor

A quick refactor to clean up this code, as maintaining distinct trackManual, trackScan, trackDistance options here seem unnecessary now with the consolidation to one "Create expense" flow.

What are we cleaning up exactly? From what I understand there are still 3 different QAB options we display.

@etCoderDysto
Copy link
Contributor

@arosiclair yes, we have 3 different QAB options - manual, scan and distance. But, depending on if the action is money request or track expense action, we are displaying different copies for the three quick action types.

money request actions copy

scanReceipt: 'Scan receipt',
recordDistance: 'Record distance',
requestMoney: 'Create expense',

track expense actions copy

trackManual: 'Create expense',
trackScan: 'Create expense for receipt',
trackDistance: 'Track distance',

The refactor is to remove the copy for track actions from here, and use the money request actions copy for track expense actions too. That means if a user creates track manual expense they will see 'Create expense' as qab title which is reference by requestMoney key.

@trjExpensify
Copy link
Contributor

Yeah, Andrew we used to have Submit expense and Track expense flows. Now we just have the one Create expense flow. So the distinct label names for track (trackManual, trackScan and trackDistance) are redundant now.

If you use Create expense and..

  • choose manual (requestMoney), you should have a QAB labelled Create expense.
  • choose scan (scanReceipt), you should have a QAB labelled Scan receipt
  • choose distance (RecordDistance), you should have a QAB labelled Track distance.*

*Yes, this used to be the QAB label name for distance in the Track flow, but we like it better than "Record distance", which we only chose because the "Track" flow existed.

@arosiclair
Copy link
Contributor

Alright proposal LGTM then 👍

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jan 31, 2025
Copy link

melvin-bot bot commented Jan 31, 2025

📣 @etCoderDysto 🎉 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 📖

Copy link

melvin-bot bot commented Feb 2, 2025

@mananjadhav @arosiclair @jliexpensify @etCoderDysto this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

@jliexpensify
Copy link
Contributor

Not overdue, this is being worked on!

Copy link

melvin-bot bot commented Feb 4, 2025

@mananjadhav, @arosiclair, @jliexpensify, @etCoderDysto Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@melvin-bot melvin-bot bot added the Overdue label Feb 4, 2025
@mananjadhav
Copy link
Collaborator

@etCoderDysto What's the ETA on the PR?

@melvin-bot melvin-bot bot removed the Overdue label Feb 4, 2025
@etCoderDysto
Copy link
Contributor

@mananjadhav sorry for the delay. I am working on the PR. PR will be ready for review by tomorrow.

@mananjadhav
Copy link
Collaborator

@jliexpensify @trjExpensify I am assuming based on the new context of including the per diem and code refactor can we bump the pay back to the original price of $250?

@jliexpensify jliexpensify changed the title [$125] QAB - Quick action shows the old title "Track distance" for distance expense in self DM [$250] QAB - Quick action shows the old title "Track distance" for distance expense in self DM Feb 4, 2025
Copy link

melvin-bot bot commented Feb 4, 2025

Upwork job price has been updated to $250

@jliexpensify
Copy link
Contributor

I think that's reasonable.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Feb 7, 2025
@etCoderDysto
Copy link
Contributor

@mananjadhav PR is ready for review.

I have some issues to fix while the pr is reviewed, and a BE support for per diem qab is added. I have commented about the issues here. I will fix them while BE support is being added for per diem qab

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. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: No status
Development

No branches or pull requests

9 participants