search

Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.12k stars 2.62k forks source link

[$500] [MEDIUM] [Feature Request] Currency selection screen should have "Recents" at top #34874

Open kavimuru opened 5 months ago

kavimuru commented 5 months ago

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: 1.4.29-0 Reproducible in staging?: y Reproducible in production?: y 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 Expensify/Expensify Issue URL: Issue reported by: @shawnborton Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1705664342497829

Action Performed:

Go to change the currency on a an expense

Expected Result:

We have a list selection pattern where the current selected item is at the top, and we have a section for recents underneath that. I would expect currency selection to work the same.

Actual Result:

There is no section for recents, and your currently selected item is not at the top.

Workaround:

unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence Snip - (2) New Expensify - Google Chrome

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01964421591b4aea1b
  • Upwork Job ID: 1750537225734946816
  • Last Price Increase: 2024-01-25
  • Automatic offers:
    • getusha | Reviewer | 0
    • DylanDylann | Contributor | 0
melvin-bot[bot] commented 5 months ago

Triggered auto assignment to @zanyrenney (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

DylanDylann commented 5 months ago

Proposal

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

zanyrenney commented 5 months ago

Yep, this looks like it should be external to me. adding the label though!

melvin-bot[bot] commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

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

zanyrenney commented 5 months ago

@getusha what do you think of the proposal from @DylanDylann ?

getusha commented 5 months ago

@DylanDylann is it possible to implement this future without having to create a new Onyx key?

DylanDylann commented 5 months ago

@getusha I think it needs to be stored in BE as well so cannot implement without creating new Onyxkey.

getusha commented 5 months ago

Probably adding a new key to the currency object will be ideal? Pulling internal engineer to take a look at this 🎀 👀 🎀

melvin-bot[bot] commented 5 months ago

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

DylanDylann commented 5 months ago

@thienlnam What do you think about this comment?

thienlnam commented 5 months ago

Yeah, if we're adding recently selected for currency selection we'll likely want another onyx key for consistency with the other recently used items. There's also a few BE changes we'll also need to make to support this update.

Though instead of keying this by policy, we should probably just create a general recentlyUsedCurrencies field for the account - currency selection is typically user-centric and independent of individual policy constraints

thienlnam commented 5 months ago

@zanyrenney Could you see which wave this would fit in to see if we should work on this now or not?

zanyrenney commented 5 months ago

sure thing! i think this should be in wave6 but I will drop it in for greg to make a call.

zanyrenney commented 5 months ago

https://expensify.slack.com/archives/C02MW39LT9N/p1706530608517779

getusha commented 5 months ago

We can proceed with @DylanDylann's proposal, i think we should first complete the backend changes first @thienlnam?

thienlnam commented 5 months ago

Yup, we'll need to do that first - let me see if someone else is interested is taking these changes on

zanyrenney commented 5 months ago

find anyone interested in taking on the changes @thienlnam

zanyrenney commented 5 months ago

?

thienlnam commented 5 months ago

You're looking at him! 😆 I'll get the internal changes up later this week

zanyrenney commented 5 months ago

woo thanks @thienlnam please update the issue with your progress 🎉

thienlnam commented 4 months ago

Got started on a draft PR - looks like we haven't been storing this anywhere since we always rely on the user's current location

Currently going through all the places that a currency could be used - scan / create / update and then adding tests

zanyrenney commented 4 months ago

Nice, thanks for the update @thienlnam can you let me know how your progress is going now?

zanyrenney commented 4 months ago

Bump @thienlnam how you going here?

thienlnam commented 4 months ago

Haven't made any new updates here - been working on Track Expense. Will likely get back to this next week

zanyrenney commented 4 months ago

waiting on @thienlnam - changing to make him owner until its able to be prioritised as there is nothing I or @getusha can do for now until then.

melvin-bot[bot] commented 4 months ago

@thienlnam, @zanyrenney, @getusha Whoops! This issue is 2 days overdue. Let's get this updated quick!

getusha commented 4 months ago

@thienlnam is OOO, will get back to this when he returns.

zanyrenney commented 4 months ago

Not sure where you got that from @getusha - i checked internally @thienlnam isn't OOO yesterday 😅

zanyrenney commented 4 months ago

@thienlnam can you please update this issue with your work / what the latest update is.

thienlnam commented 4 months ago

I was OOO on Monday for President's day. I'm back now, I've been catching up and focused on Track which is blocking some other wave 6 critical issues so I haven't made any new progress.

I don't think this is that urgent as a new feature, so I'm going to move this to a weekly - though feel free to find someone to take it over if you would like it done sooner

getusha commented 4 months ago

Not sure where you got that from @getusha

@zanyrenney noticed it from slack profile :)

zanyrenney commented 4 months ago

oh weird haha i didn't see that in our internal update for OOO, but all good, thanks!

thienlnam commented 4 months ago

BE PR is merged so we're saving all the used currencies by policy, but we're not sending any pusher updates - that's currently held on Ioni's project of Standardize how we save NVPs in onyx.

Going to update the issue title to reflect that

zanyrenney commented 4 months ago

On HOLD.

zanyrenney commented 3 months ago

Polish / waiting for BE changes.

zanyrenney commented 3 months ago

hey @thienlnam when do you think we'll be prioritising this one?

thienlnam commented 3 months ago

@iwiznia Seems like we're pretty close to sending out all account updates, when do you think that will be live?

iwiznia commented 3 months ago

close to sending out all account updates

I don't know what you mean

thienlnam commented 3 months ago

For this project https://github.com/Expensify/Expensify/issues/364560

zanyrenney commented 2 months ago

Any update further here please @thienlnam

thienlnam commented 2 months ago

Ah yeah, so the BE changes are done - but we didn't send any pusher updates in anticipation for this change which will send account NVPs automatically.

Once that is done, we can turn this external to have someone add the recents key

iwiznia commented 2 months ago

That project is done, I even sent the project done email today (removed the hold)

thienlnam commented 2 months ago

Oh nice! Okay in that case I just need to pass the onyx key to the client (doing that here), and then a contributor can start on the FE changes

DylanDylann commented 2 months ago

@thienlnam Could you check this comment? I can handle this one

zanyrenney commented 2 months ago

bump @thienlnam please can you respond to the comment above?

thienlnam commented 2 months ago

Yup, once the Auth PR is merged you can go ahead and handle the FE changes.

The onyx key will be under nvp_recentlyUsedCurrencies

melvin-bot[bot] commented 2 months ago

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

Offer link Upwork job

melvin-bot[bot] commented 2 months ago

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

thienlnam commented 2 months ago

@DylanDylann You should be ready to start on this now - the holding code is deployed

The currencies are keyed by policyID

nvp_recentlyUsedCurrencies: {
   01DC7E46150451C4: ['USD', 'CAD'],
   0426477218E500C1: ['USD', 'CAD'],
   1CAFAAA6EAD09539: ['USD', 'CAD'],
}