Open luacmartins opened 1 month ago
Triggered auto assignment to @stephanieelliott (NewFeature
), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.
Job added to Upwork: https://www.upwork.com/jobs/~021834524447256118011
Triggered auto assignment to Contributor-plus team member for initial proposal review - @situchan (External
)
cc @Kicu @289Adam289 @SzymczakJ @Guccio163 in case you wanna work on the frontend part of this. These changes also have a backend component that's still not available.
hello @luacmartins I would like to take care of this issue. When do you expect the backend changes to be ready? 🤔
@luacmartins, @stephanieelliott, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
📣 @situchan 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!
All yours @zfurtak
Do you have any idea when the backend will be ready @luacmartins? 😃
@zfurtak I started looking into this today. I have a draft PR to return report data in the chat queries, but I'm not sure that that's enough to display the correct report names. Do you know which data you'll need to properly display them? It seems like we need quite a bit of data for getReportName
Here's a sample data after we start returning the report data:
[
{
"key": "snapshot_2089317297",
"onyxMethod": "set",
"value": {
"data": {
"personalDetailsList": {
"844": {
"accountID": 844,
"avatar": "https://d2k5nsl2zxldvw.cloudfront.net/images/avatars/default-avatar_5.png",
"displayName": "sdf",
"firstName": "sdf",
"lastName": "",
"login": "cc2@cc.com"
}
},
"reportActions_2226999844364749": {
"6133238980486036910": {
"accountID": 844,
"actionName": "ADDCOMMENT",
"created": "2024-08-27 21:00:44.078",
"message": [
{
"html": "<a href=\"https://dev.new.expensify.com:8082/search?q=type%3Achat%20status%3Alinks%20sortBy%3Adate%20sortOrder%3Adesc&isCustomQuery=false\" target=\"_blank\" rel=\"noreferrer noopener\">https://dev.new.expensify.com:8082/search?q=type%3Achat%20status%3Alinks%20sortBy%3Adate%20sortOrder%3Adesc&isCustomQuery=false</a>",
"text": "https://dev.new.expensify.com:8082/search?q=type%3Achat%20status%3Alinks%20sortBy%3Adate%20sortOrder%3Adesc&isCustomQuery=false",
"type": "COMMENT",
"whisperedTo": []
}
],
"reportActionID": "6133238980486036910",
"reportID": "2226999844364749"
}
},
"reportActions_3613091925157067": {
"4865297248518576461": {
"accountID": 844,
"actionName": "ADDCOMMENT",
"created": "2024-09-12 22:55:20.809",
"message": [
{
"html": "<a href=\"https://www.expensify.com.dev/chat-attachments/4865297248518576461/w_efeb24490b01fb7b9b2c596b6cafafd778c09e3e.sh\" data-expensify-source=\"https://www.expensify.com.dev/chat-attachments/4865297248518576461/w_efeb24490b01fb7b9b2c596b6cafafd778c09e3e.sh\">checkDeploymentSuccess.sh</a>",
"text": "[Attachment]",
"type": "COMMENT",
"whisperedTo": []
}
],
"reportActionID": "4865297248518576461",
"reportID": "3613091925157067"
}
},
"report_2226999844364749": {
"accountID": 0,
"action": "view",
"created": "2024-03-27 17:09:53",
"currency": "USD",
"managerID": 0,
"policyID": "818714A5DBC2E598",
"reportID": "2226999844364749",
"reportName": "#admins",
"total": 0,
"type": "chat"
},
"report_3613091925157067": {
"accountID": 0,
"action": "view",
"created": "2024-02-20 18:23:08",
"currency": "USD",
"managerID": 0,
"policyID": "_FAKE_",
"reportID": "3613091925157067",
"reportName": "Chat Report",
"total": 0,
"type": "chat"
}
},
"search": {
"columnsToShow": {
"shouldShowCategoryColumn": true,
"shouldShowTagColumn": true,
"shouldShowTaxColumn": true
},
"hasMoreResults": false,
"offset": 0,
"status": "links",
"statusToShow": {
"expense": {
"all": true,
"approved": false,
"drafts": true,
"outstanding": true,
"paid": true
}
},
"type": "chat"
}
}
}
]
@luacmartins as I searched for usage of this function, it can work with only report, so in the minimalist option we only need reportID, which is already there in the Search query, so I guess backend changes aren't inevitable 🤔 this is how it looks right now:
And also I have 2 questions,
as now clicking on the message opens the desired report, do we want to add a different action to the link or should it be basically <Text>
component with blue font colour? 😃
and do we want to show it for every chat message or only these from workspaces? Because for now in my draft version it looks like below:
Hmm I think it works with the reportID only because those reports are loaded in Onyx for you. What happens if you Search for a message where you don't have the report in Onyx yet, maybe some really old one? Ie you need to rely solely on the report data returned by the Search API?
I think they would still open the report, so maybe they are just Text components with different styles. Cc @Expensify/design in case you had other plans for the "In report" link at the top.
Yes, I think we want to display the header for every chat so it gives context of where that message is coming from.
I think they would still open the report, so maybe they are just Text components with different styles. Cc @Expensify/design in case you had other plans for the "In report" link at the top.
Hmm, so clicking on the whole chat item opens the chat in the RHP... I think I might expect the report name (blue text) to take me to the chat in the inbox tab? Maybe? Curious for what others think.
Hmm I think it works with the reportID only because those reports are loaded in Onyx for you. What happens if you Search for a message where you don't have the report in Onyx yet, maybe some really old one? Ie you need to rely solely on the report data returned by the Search API?
Yeah, good point and I guess it's not the best idea to fetch the report by another API call 🤔
So what we need is a report object (OnyxEntry<Report>
)
@luacmartins
Yea, I think we need to trim that down to only the necessary fields, so we don't return a bunch of unnecessary data.
Hmm yeah I guess technically the link should take you to the report it's referencing. What if we just dropped the link style and just used our text supporting color and make it so that tapping anywhere on the search row opened up that specific message?
@shawnborton do you mean like below?
Yeah exactly. We might even consider making the room name bold? Though we don't really have a style like that anywhere else...
Separate to this conversation, I think we also need like 4px more space below the room name and above the message content. Can we try that?
This is how it looks with the changes 😊
Can you zoom out a bit and show us the whole screen?
Sure thing 👍
Let's see what the rest of the @Expensify/design thinks. That looks pretty good to me personally, though I can see where it kinda differs from the small text below report names in chat headers.
Yea, I think we need to trim that down to only the necessary fields, so we don't return a bunch of unnecessary data.
@luacmartins I analysed the function and these are the report's fields used (taking into account all different conditions):
Damn, that is a large list of things used to compute a report name... wild. We might need to simplify that a bit since some of those are their own Onyx keys, e.g. managerID will probably pull data from personalDetailsList
and iouReportID
will then pull data from another report, etc. Not sure how quite yet though.
Started a slack discussion here
Not overdue
Totally down to drop the link style and just have that presented as text like Shawn suggested.
It is looking pretty good to me too, though I'm not sure we need to have the room name bold. It's definitely not BAD being bold, just not sure it's necessary. Let's see what @dubielzyk-expensify thinks! I could definitely go either way.
Yeah I think I lean towards not having it bold to avoid too much visual clutter. It doesn't add heaps, but there's quite a few type styles already on this page and I think the non-bold keeps it a tad cleaner. No strong feelings though
Cool, down to go non-bold to start and see how it goes!
Also, in an effort to be consistent with report headers, should we use "From" instead of "In"? We use "In" for filters though, but report headers look like this:
🤔 I'm not sure I feel too strongly about that. I think I kinda like in
because from
has more meaning in the context of chat messages.
For example, in this screenshot, the message is from Zuzia but appears in the #admins
room.
But honestly I don't feel to strongly. 🤷♂️
That's a great point! Okay cool, thanks for hearing me out - sounds like we can just do nothing here.
To sum up, we don't want to have chat name in blue, either bold and In
is staying? 😊
In #admins
No blue. No bold. Keep "in"! 🚀
@zfurtak I see that some of the methods called within ReportUtils.getReportName are connected directly to Onyx data, eg. isClosedExpenseReportWithNoExpenses > hasExpenses > allTransactions
. I think we need to start by refactoring these methods to take in the data as a param, since when used with the Search page, a lot of this data might not be available in the "live" Onyx data and might just exist in the snapshot_
Onyx key.
@luacmartins so what you mean is in this PR to refactor all of these methods used by getReportName
that are connected to Onyx? :)
I think we'd need to start with that, yea. Once that's done, we'd need to compile a list of Onyx keys that need to be added to the Search type:chat response so getReportName
can properly compute the name of the report
I am currently busy working on another issue, and I'll be out of the office at the end of the week. Would it be a problem if I start working on this one on Monday?
That seems fine. I haven't been able to work on any of the backend changes yet either.
@luacmartins, @stephanieelliott, @zfurtak, @situchan Eep! 4 days overdue now. Issues have feelings too...
Haven't been able to prioritize this one yet, but it's on my list for this week.
@luacmartins, @stephanieelliott, @zfurtak, @situchan Huh... This is 4 days overdue. Who can take care of this?
@luacmartins, @stephanieelliott, @zfurtak, @situchan 6 days overdue. This is scarier than being forced to listen to Vogon poetry!
I was ooo Mon/Tue and working reduced hours today, haven't been able to prioritize this one yet.
@luacmartins, @stephanieelliott, @zfurtak, @situchan Eep! 4 days overdue now. Issues have feelings too...
@luacmartins, @stephanieelliott, @zfurtak, @situchan Eep! 4 days overdue now. Issues have feelings too...
Haven't been able to prioritize this one yet, but it's next on my list.
I'm starting to look at this one again. Gonna start by taking a look at the draft PR we have.
There's some discussion happening in the PR, but so far I've triaged the code and we'll need to refactor the following methods to no longer access Onyx directly:
Functions with direct Onyx access:
Next, I'll triage all the different objects and keys that are needed and then bring the discussion back to the team to make sure we wanna go down that path.
Problem
Coming from here, chats miss the context of the room they belong too, so users can find the same chat sent to different people.
Solution
Add the room name to the chats. This requires both backend and frontend changes.
Issue Owner
Current Issue Owner: @luacmartinsUpwork Automation - Do Not Edit