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

[HOLD] [$1000] Fetch more attachments #16315

Closed
luacmartins opened this issue Mar 20, 2023 · 37 comments
Closed

[HOLD] [$1000] Fetch more attachments #16315

luacmartins opened this issue Mar 20, 2023 · 37 comments
Assignees
Labels
Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item.

Comments

@luacmartins
Copy link
Contributor

luacmartins commented Mar 20, 2023

Problem

Coming from #9279 (comment), we implemented an image carousel component but we do not fetch more attachments when we reach the end of client loaded reportActions, even if the chat report has more attachments.

Why is this important?

Users might reach the end of loaded reportActions and think they don't have any more attachments in the report, which causes confusion.

Solution

Create a new API command to return more attachments for the chat and call it within the attachment carousel component so that we load more attachments when we reach the end, similar to what we do for reportActions.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01edaff85b84a90ff7
  • Upwork Job ID: 1637860885039116288
  • Last Price Increase: 2023-03-23
Issue OwnerCurrent Issue Owner: @marcaaron
Issue OwnerCurrent Issue Owner: @marcaaron
@luacmartins luacmartins added Daily KSv2 NewFeature Something to build that is a new item. labels Mar 20, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Mar 20, 2023
@luacmartins luacmartins added the Internal Requires API changes or must be handled by Expensify staff label Mar 20, 2023
@melvin-bot melvin-bot bot unlocked this conversation Mar 20, 2023
@MelvinBot
Copy link

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

@MelvinBot
Copy link

Triggered auto assignment to Contributor Plus for review of internal employee PR - @aimane-chnaif (Internal)

@MelvinBot
Copy link

@aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added the Overdue label Mar 23, 2023
@aimane-chnaif
Copy link
Contributor

@luacmartins can you please assign engineer here? I am only the assignee.

@melvin-bot melvin-bot bot removed the Overdue label Mar 23, 2023
@luacmartins luacmartins self-assigned this Mar 23, 2023
@luacmartins
Copy link
Contributor Author

Done!

@luacmartins luacmartins added the External Added to denote the issue can be worked on by a contributor label Mar 23, 2023
@melvin-bot melvin-bot bot changed the title Fetch more attachments [$1000] Fetch more attachments Mar 23, 2023
@MelvinBot
Copy link

Triggered auto assignment to @mallenexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot
Copy link

Current assignee @aimane-chnaif is eligible for the External assigner, not assigning anyone new.

@MelvinBot
Copy link

Current assignee @luacmartins is eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added the Overdue label Mar 27, 2023
@luacmartins luacmartins removed the External Added to denote the issue can be worked on by a contributor label Mar 27, 2023
@luacmartins
Copy link
Contributor Author

This needs to be internal since we need to create a new API command for it.

@melvin-bot melvin-bot bot removed the Overdue label Mar 27, 2023
@luacmartins
Copy link
Contributor Author

No updates yet

@melvin-bot melvin-bot bot added the Overdue label Apr 3, 2023
@luacmartins
Copy link
Contributor Author

I'm ooo this week, so no updates

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 3, 2023
@melvin-bot melvin-bot bot added the Overdue label Jul 25, 2023
@luacmartins
Copy link
Contributor Author

I'm still working on other roadmap issues and won't be able to tackle this any time soon. I asked if anyone else is interested in working on this here

@melvin-bot melvin-bot bot removed the Overdue label Jul 25, 2023
@melvin-bot melvin-bot bot added the Overdue label Aug 3, 2023
@luacmartins
Copy link
Contributor Author

I'm focused on wave4 issues. I'll bump the slack thread again to look for volunteers to take this on.

@melvin-bot melvin-bot bot removed the Overdue label Aug 3, 2023
@marcaaron marcaaron added Monthly KSv2 and removed Weekly KSv2 labels Aug 3, 2023
@marcaaron marcaaron self-assigned this Aug 3, 2023
@marcaaron marcaaron changed the title [$1000] Fetch more attachments [HOLD] [$1000] Fetch more attachments Aug 3, 2023
@marcaaron
Copy link
Contributor

Gonna pick this up from @luacmartins for now. If anyone wants to work on this with more urgency feel free to take the HOLD off and assign yourself.

@melvin-bot melvin-bot bot added the Overdue label Sep 4, 2023
@mallenexpensify
Copy link
Contributor

@marcaaron 'll get to before long

@melvin-bot melvin-bot bot removed the Overdue label Sep 5, 2023
@melvin-bot melvin-bot bot added the Overdue label Oct 6, 2023
@marcaaron
Copy link
Contributor

One thing I've also noticed about the current feature is that it is also very slow when fetching the full size attachments in the carousel for a chat with a large history. Experienced this during Offshore when checking posts in #social.

So, I would maybe propose something like this:

  • Only load a max of 3-5 attachments into the carousel at once (or figure out whatever makes it slow - I'd guess it's loading too many full size attachments)
  • Load the "next" full size attachment when we move in either direction (left or right)
  • Until we have comment linking we'll need an API that does something like:
    • When going "left"...
    • Check if there is an attachment in the report history
    • If so, return all reportActions between the oldest one we have and the attachment comment

I have some other thoughts - but I think it's still not quite a near-term priority so leaving the HOLD.

@melvin-bot melvin-bot bot removed the Overdue label Oct 6, 2023
@melvin-bot melvin-bot bot added the Overdue label Nov 7, 2023
@marcaaron
Copy link
Contributor

No updates.

@melvin-bot melvin-bot bot removed the Overdue label Nov 8, 2023
@melvin-bot melvin-bot bot added the Overdue label Dec 11, 2023
@marcaaron
Copy link
Contributor

Still not a priority. But feels like maybe it could move into a wave as a general chat improvement. The vsb room seems to be the most active channel for that type of stuff so I'll try to raise this there.

@melvin-bot melvin-bot bot removed the Overdue label Dec 12, 2023
@mallenexpensify
Copy link
Contributor

@mallenexpensify
Copy link
Contributor

Per discussion here we're going to close this to focus on near-term roadmap issues. We can reopen this when the time is right

@aimane-chnaif
Copy link
Contributor

@mallenexpensify does team track all closed issues which will be reopened after wave?

@mallenexpensify
Copy link
Contributor

@aimane-chnaif , currently, no. We'll be able to find any issues and would either reopen or link to/from them in the future. We discussed a few options for how to potentially track but none of them gained traction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item.
Projects
None yet
Development

No branches or pull requests

6 participants