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.56k stars 2.9k forks source link

[$250] Search - (edited) label disappears after refreshing Search page #50096

Closed IuliiaHerets closed 3 weeks ago

IuliiaHerets commented 1 month 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.43-1 Reproducible in staging?: Reproducible in production?: Email or phone of affected tester (no customers): applausetester+pso@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to DM.
  3. Send a message.
  4. Go to Search > Chat.
  5. Click on the message sent in Step 3.
  6. Right click on the message > Edit comment.
  7. Edit the comment and save it.
  8. Refresh Chat page.

Expected Result:

(edited) label will persist after refreshing Search page.

Actual Result:

(edited) label disappears after refreshing Search page.

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/473d0643-2756-424d-9a60-383c1205a1ac

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021843656023889583525
  • Upwork Job ID: 1843656023889583525
  • Last Price Increase: 2024-10-08
Issue OwnerCurrent Issue Owner: @CortneyOfstad
melvin-bot[bot] commented 1 month ago

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

IuliiaHerets commented 1 month ago

We think that this bug might be related to #wave-control

IuliiaHerets commented 1 month ago

@CortneyOfstad 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

huult commented 1 month ago

I believe this is a backend issue because the response from the Search API does not include isEdit in the Onyx data. When this response is saved to the snapshot, it removes isEdit from the snapshot data, causing the issue.

CortneyOfstad commented 1 month ago

Still need to test this to confirm it can be recreated! Working on that ASAP

CortneyOfstad commented 1 month ago

I cannot get this consistently recreated. Going to try again this morning and see what I can find πŸ‘

CortneyOfstad commented 1 month ago

After fresh testing, I was FINALLY able to get it to happen consistently, so getting eyes on this!

2024-10-08_09-12-30 (1)

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

hungvu193 commented 1 month ago

Sounds like a BE issue. I'll review this one in my morning πŸ₯Ή

hungvu193 commented 1 month ago

I can reproduce, reviewing shortly

hungvu193 commented 1 month ago

I think this is BE issue, isEdited didn't return from the BE. Can you please add Internal label? @CortneyOfstad

huult commented 1 month ago

Proposal

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

Search - (edited) label disappears after refreshing Search page

What is the root cause of that problem?

The backend does not return isEdited in the API response. As a result, the snapshot is updated with the response without isEdited after the Search call is successful, which causes this issue.

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

I suggest two options to fix this issue:

Option 1: We can use the message data in reportAction from Onyx to update the reportAction in snapshot_ because the reportActions from Onyx includes isEdited and is up to date. Something like that:

  1. Create SnapshotConnection.ts like ReportConnection to get all snapshot_
    
    +           let allSnapshot;
    +             Onyx.connect({
    +                   key: ONYXKEYS.COLLECTION.SNAPSHOT,
    +                   waitForCollectionCallback: true,
    +                   callback: (value) => {
    +                          allSnapshot = value;
    +                  },
    +            });

Option 2: The backend must be updated to return isEdited, if applicable.

POC https://github.com/user-attachments/assets/a5c4b50e-5a23-425f-b016-194a07f90256
huult commented 1 month ago

@hungvu193 , Could you please review my proposal? If we don't update from the backend, we can use the message from ReportAction in Onyx, which is the first option I suggest. If you agree with this option, I will continue testing and create a test branch soon. The second option is to update from the backend side.

hungvu193 commented 1 month ago

@huult I don't think looping over all snapshot data to update the reportActions is a good idea. I still think BE should fix this.

huult commented 1 month ago

@hungvu193 , Yes, thank you for your feedback. If the backend change is complex, we can address it from the frontend.

melvin-bot[bot] commented 1 month ago

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

CortneyOfstad commented 1 month ago

Hey @mallenexpensify! I am heading OoO this afternoon (10/15 to 10/23) so reassigning this in the meantime to keep it moving! Thanks!

melvin-bot[bot] commented 1 month ago

@CortneyOfstad @hungvu193 @mallenexpensify this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

mallenexpensify commented 1 month ago

@hungvu193 can you add πŸŽ€ to assign an internal engineer to confirm they agree it's a bug that needs to be fixed on the backend? (they can unassign after)

hungvu193 commented 1 month ago

πŸŽ€ πŸ‘€ πŸŽ€

melvin-bot[bot] commented 1 month ago

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

pecanoro commented 1 month ago

Yes, it seems we send a smaller version of the action compared to the normal one and isEdited is missing. I am not fully sure how search works on the BE but I am going to take a quick look

pecanoro commented 1 month ago

Found the bug

CortneyOfstad commented 3 weeks ago

I'm back from OoO – thanks for holding down the fort @mallenexpensify!

melvin-bot[bot] commented 3 weeks ago

@pecanoro, @CortneyOfstad, @hungvu193, @mallenexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

trjExpensify commented 3 weeks ago

Removing the Hot pick label because Rocio picked this up. What remains for this issue?

pecanoro commented 3 weeks ago

@trjExpensify This should be done, we can just retest and close if it's working. I think they don't get automatically closed as the ones in the internal repo

trjExpensify commented 3 weeks ago

Cool, sounds good. I'll leave that to the BZ assigned. πŸ‘

melvin-bot[bot] commented 3 weeks ago

@pecanoro, @CortneyOfstad, @hungvu193, @mallenexpensify 10 days overdue. Is anyone even seeing these? Hello?

pecanoro commented 3 weeks ago

@CortneyOfstad Can you double check the bug is gone after fixing it in the back-end?

CortneyOfstad commented 3 weeks ago

Yep! Testing now @pecanoro!

CortneyOfstad commented 3 weeks ago

Just tested and it works! The edited label stayed even after refreshing a few times!

@hungvu193 can you confirm if this needs any regression tests? Once that is confirmed, I can do the Payment Summary and get this closed out πŸ‘

Thanks!

hungvu193 commented 3 weeks ago

Heyy @CortneyOfstad, This is BE issue so I don't think there's payment summary or regression tests are needed. Feel free to close πŸ˜„