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.54k stars 2.88k forks source link

[HOLD for payment 2024-10-16] [$250] Track > Categorize it flow - Unable to search for workspace chats if the reports are not visited previously #47854

Closed lanitochka17 closed 3 weeks ago

lanitochka17 commented 2 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: 9.0.23-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: N/A Email or phone of affected tester (no customers): applausetester+kh050806@applause.expensifail.com Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Create a few workspaces
  3. Go to self DM
  4. Send a message
  5. Log out and log it
  6. Do not open any workspace chat and go to self DM
  7. Click + > Track expense > Manual
  8. Submit a track manual expense
  9. Click Categorize it
  10. Search for other existing workspace chats that do not show up in the list
  11. Note that the other existing workspace chats do not return in the search result
  12. Open all the workspace chats
  13. Repeat Step 9 and 10
  14. Note that workspace chats are only searchable after the workspace chats are visited

Expected Result:

In Step 11, user should be able to search for other existing workspace chats

Actual Result:

In Step 11, user is unable to search for other existing workspace chats The search only returns results after the workspace chats are visited (Step 14)

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/5dfaa77d-7b28-4414-b8c0-ae4f42f0283c

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01570efd314fb561ac
  • Upwork Job ID: 1828744919457771906
  • Last Price Increase: 2024-09-18
  • Automatic offers:
    • dukenv0307 | Reviewer | 104043144
    • jaydamani | Contributor | 104043145
Issue OwnerCurrent Issue Owner: @dylanexpensify
melvin-bot[bot] commented 2 months ago

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

lanitochka17 commented 2 months ago

@dylanexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

jaydamani commented 2 months ago

Edited by proposal-police: This proposal was edited at 2023-10-04T22:06:00Z.

Proposal

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

When categorizing a tracked expense, only previously visited workspaces are shown

What is the root cause of that problem?

When categorizing expense, we do an additional check to see if the workspace has categories enabled by checking if policyCategory for that workspace has at least 1 category enabled but currently, when you logout and then login, policyCategory is not being loaded for workspaces with categories enabled. It is only loaded for workspace where the category feature and all categories are disabled. This data is supposed to come from OpenApp request. When you open a report, then also policyCategory is populated which is why we start to see the workspace after it is opened once

https://github.com/Expensify/App/blob/06f5b66c8fe2d9be62bc008d67a33dcdf1b4484c/src/libs/OptionsListUtils.ts#L1974-L1974

https://github.com/Expensify/App/blob/06f5b66c8fe2d9be62bc008d67a33dcdf1b4484c/src/libs/OptionsListUtils.ts#L279-L284

image

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

Update front end to check isCategoryEnabled in the policy then update here to call an API and load categories of the selected policy. A new API will be needed from backend to load all categories of a given workspace.

What alternative solutions did you explore? (Optional)

Considering that policyCategory loads for workspace without categories but not for workspaces with categories, this likely requires a fix from backend most likely for OpenApp.

klajdipaja commented 2 months ago

Proposal

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

When submitting an expense and attempting to "Categorize" it into a workspace the "Categorize" modal does not show any options in the recent tab unless the user has opened each one of them in the current session.

What is the root cause of that problem?

The root cause of the problem is in OptionsListUtils.ts#getOptions where we filter out the reports that should be shown in the list. Currently the filtering returns only reports that have at least one enabled category in them by searching in Onyx storage for the list of ONYXKEYS.COLLECTION.POLICY_CATEGORIES and then each of them for enabled categories.

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

We should change the approach to use the areCategoriesEnabled boolean in the policy data. Lines https://github.com/Expensify/App/blob/156f0a2bd0e94558ad621caa701bd749dd124bfe/src/libs/OptionsListUtils.ts#L1974-L1977

Should be replaced with:

      const reportOptionPolicy = policies?.[`${ONYXKEYS.COLLECTION.POLICY}${reportOption.policyID}`] ?? {};
      if (reportOptionPolicy.areCategoriesEnabled) {
          recentReportOptions.push(reportOption);
      }

And after that we need to remove the connection to ONYXKEYS.COLLECTION.POLICY_CATEGORIES that we won't need anymore: https://github.com/Expensify/App/blob/156f0a2bd0e94558ad621caa701bd749dd124bfe/src/libs/OptionsListUtils.ts#L279-284

What alternative solutions did you explore? (Optional)

melvin-bot[bot] commented 2 months ago

@dylanexpensify Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

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

dukenv0307 commented 2 months ago

@dylanexpensify I think it's BE bug, OpenApp API doesn't return the category of other WS if we have many workspaces.

If it's intended behavior, do we have the API to load the category if needed?

melvin-bot[bot] commented 2 months ago

@dylanexpensify, @dukenv0307 Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 2 months ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

melvin-bot[bot] commented 2 months ago

@dylanexpensify, @dukenv0307 Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 2 months ago

@dylanexpensify @dukenv0307 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!

melvin-bot[bot] commented 2 months ago

@dylanexpensify, @dukenv0307 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

trjExpensify commented 2 months ago

@dylanexpensify I think it's BE bug, OpenApp API doesn't return the category of other WS if we have many workspaces.

If it's intended behavior, do we have the API to load the category if needed?

@dylanexpensify we need to get the answer to this to move it on. @thienlnam would be good to ask here for the Track > categorize it flow.

dylanexpensify commented 2 months ago

Apologies, assigning star to me and getting clarification!

thienlnam commented 2 months ago

This looks like an issue with createWorkspace not sending a pusher update for policy categories. I don't think that we did this on purpose to save onyx storage or anything so yeah this would require a BE fix

trjExpensify commented 2 months ago

Hm, I'm a bit confused. When you choose to Categorize it, shouldn't we check if you are a member of any workspaces and return all the newDot compatible ones in the picker? Otherwise, we proceed to create you one and show a list of categories.

thienlnam commented 2 months ago

Yeah, that should happen - but it sounds like we're not returning the categories of all the workspaces so we don't have those stored to determine which ones are compatible for the Categorize it option.

trjExpensify commented 2 months ago

Wouldn't we just be looking for workspaces with isPolicyExpenseChatEnabled===true && areCategoriesEnabled===true? https://github.com/Expensify/Web-Expensify/blob/9b27a3b68bcecd01ce955fb4dc022432438e4c43/lib/Policy.php#L4675

thienlnam commented 2 months ago

That would work, but if the category data is missing it still wouldn't show the categories. I think we just need to make sure the category data is returned and it should fix both issues

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

trjExpensify commented 1 month ago

Hm, it seems a bit expensive to store and load an entire list of categories when the first step of this flow is to simply check if you have compatible workspaces with the categories feature enabled? If you then proceed to choose a workspace, I agree we need to then load that workspace's categories, but it seems superfluous before then for the workspace picker. I might be missing something, but yeah.. πŸ€”

jaydamani commented 1 month ago

The categories are not loaded by the search flow. It expects them to be already present. Based on this comment, I think either createWorkspace is failing to load it or it is stoping the data from being loaded.

trjExpensify commented 1 month ago

What has this got to do with the Search page and/or create workspace? We're talking about clicking Categorize it and that launching a workspace selector. So when we perform that action in the track flow, why wouldn't we look for a list of eligible workspaces to return there? Otherwise, we skip opening the workspace picker and proceed to creating a new one.

thienlnam commented 1 month ago

jaydamani is saying the categories are not loaded if workspace is selected / eligible as part of the selector.

Hm, it seems a bit expensive to store and load an entire list of categories when the first step of this flow is to simply check if you have compatible workspaces with the categories feature enabled?

We load a lot of details about a policy, how big can categories get? I am not sure it's a problem to have these loaded

It sounds like you're suggesting - first check if any policies are even eligible in the first place, and then if there are some then we load the policy categories.

That would work - but if we can just already have the categories available then it would be easy unless there's a specific concern with having too many categories

trjExpensify commented 1 month ago

We load a lot of details about a policy, how big can categories get? I am not sure it's a problem to have these loaded

Categories can be pretty big, especially when connected to accounting. For example with QBO, we pull in your entire set of chart of accounts which can be 100/1000s of accounts.

thienlnam commented 1 month ago

Hmm, okay - in that case perhaps we just load the categories after it is selected in the workspace? And then we check if the workspace is eligible not by checking the categories locally, but whether the policy has areCategoriesEnabled

melvin-bot[bot] commented 1 month ago

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

jaydamani commented 1 month ago

If it's better to load categories later, we can implement the alternate solution from here which should require a new API or something from backend to load categories for a specific policy. We would check for isCategoriesEnabled and the API would just load policyCategories for selected policy.

trjExpensify commented 1 month ago

Hmm, okay - in that case perhaps we just load the categories after it is selected in the workspace? And then we check if the workspace is eligible not by checking the categories locally, but whether the policy has areCategoriesEnabled

Yeah, I mean, by all means benchmark test it with 100s/1000s of records or something -- but it seems superfluous to the requirement of what we need to achieve in the initial step of showing a list of eligible workspaces to choose from in the first instance.

melvin-bot[bot] commented 1 month ago

@dylanexpensify, @dukenv0307 Still overdue 6 days?! Let's take care of this!

dylanexpensify commented 1 month ago

@jaydamani any movements on updated proposal? cc @dukenv0307

dukenv0307 commented 1 month ago

@trjExpensify @thienlnam seems like we want to load categories later. Do we have the API to do that?

trjExpensify commented 1 month ago

Not sure myself, maybe Jack will know.

melvin-bot[bot] commented 1 month ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

jaydamani commented 1 month ago

Updated proposal

thienlnam commented 1 month ago

Yeah, I mean, by all means benchmark test it with 100s/1000s of records or something -- but it seems superfluous to the requirement of what we need to achieve in the initial step of showing a list of eligible workspaces to choose from in the first instance.

By the way, this means that this flow will not work offline.

If we select the policy that is eligible but don't actually have the categories loaded - we'll basically get stuck on that screen so we will have to update that flow to a different offline pattern.

Do we have the API to do that?

We will have to create another endpoint to respect 1:1:1 but it would be the same as OpenPolicyCategoriesPage

trjExpensify commented 1 month ago

Hm well we should benchmark it first then to see if loading large lists of categories is problematic to invest in it.

thienlnam commented 1 month ago

Benchmark loading after the policy is selected, or from the start? This is probably only an issue for large customers that have multiple policies with 10k categories. In either solution, most accounts are probably fine.

We can go with the loading after the policy is selected it should be fine - but if the user is offline they won't be able to categorize seems fine, just pointing it out

trjExpensify commented 1 month ago

Benchmark loading after the policy is selected, or from the start?

I mean from the start. But equally, don't you have to be online for that as well, so how are we approaching offline first in the flow anywhere? πŸ˜…

thienlnam commented 1 month ago

But equally, don't you have to be online for that as well, so how are we approaching offline first in the flow anywhere? πŸ˜…

In this case, the categories would be loaded when we load the policy data. So when the user goes offline - they would have the categories available locally to categorize an expense. In any of these offline flows, the user needs to be online initially to have some data stored locally.

thienlnam commented 1 month ago

Chatted 1:1, since both do not work offline right now - let's go with

@dukenv0307 Can you choose proposals with the new solution?

dukenv0307 commented 1 month ago

Let's go with @jaydamani's proposal

We will have to create another endpoint to respect 1:1:1

but we need to hold this PR for BE change

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 1 month ago

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

thienlnam commented 1 month ago

I've been chatting on this and have the context so I can take over the issue

melvin-bot[bot] commented 1 month ago

πŸ“£ @dukenv0307 πŸŽ‰ 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 1 month ago

πŸ“£ @jaydamani πŸŽ‰ 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 1 month ago

@jaydamani You can test the solution with the OpenPolicyCategoriesPage endpoint for now

melvin-bot[bot] commented 1 month ago

@thienlnam @dylanexpensify @jaydamani @dukenv0307 this issue is now 4 weeks old, please consider:

Thanks!

jaydamani commented 1 month ago

It is working as expected. Should I raise a PR with this API or do you want to use a new one?

https://github.com/user-attachments/assets/ff778be6-85cd-4476-bf1e-623fd227f657