Closed marcaaron closed 6 months ago
Thanks Melv but we're good. This is still in planning.
almost ready to come off hold. I have been focusing on Wave work the past week.
We are also going to be incorporating some of the components from the Simplified Collect project so waiting to see where that stuff lands so we don't end up building the new invite flow more than once.
Not really much blocking this one at this point so I'm going to remove the HOLD. The backend parts for creating a Group Chat should all be on production now. I think we can probably start engaging with somebody external to help implement this.
@waterim is taking this one on https://expensify.slack.com/archives/C03UK30EA1Z/p1707785405223319
Hello, Im Artem from Callstack and already working on this!
Quick heads up, I'm going OOO this week. I will be able to help with reviews and any questions when I return.
My update: working on this, hopefully first half of the next week will finish with initial version for reviews
Sounds great @waterim. Let me know if you have any questions about anything at all so we can work through it!
@marcaaron sure, thank you! Today reaolved few issues and everything goes well, but still a lot stuff to finish :)
@marcaaron Have one question, downloaded all SVGs for default avatar from here , but they have size: [40px, 40px], do we export SVG in a different way? Cause I can see that default avatars for users and workspaces with sizing [80px, 80px], but workspace avatars on the same figma are sized to [40,40] too
Also one more question reagarding this part:
To do this, we will modify the navigateAndOpenReport() method here to handle the: memberRoles (if any) reportName
It already handles the logins. So, we need to add arguments for roles and reportName to that method. We must also modify the logic here when we have the group chat case since we won’t ever need to get an existing chat and will always be creating one.
The optimisticReport returned from ReportUtils.buildOptimisticChatReport() must be modified to update the participants field (see refactor plan here) with the corresponding role for each participant. Finally, the optimistic report is passed here to be created in the server. We must also pass the groupChatAdminLogins.
Im not sure if I can understand it correctly, memberRoles and reportName, but where to pass those fields?
And regarding this part: The optimisticReport returned from ReportUtils.buildOptimisticChatReport() must be modified to update the participants field
. As I can see this part is not implemented yet, what should I do with that?
And regarding this part Finally, the optimistic report is passed here to be created in the server. We must also pass the groupChatAdminLogins.
.
@marcaaron Maybe I should create a draft with ongoing changes to understand if Im on the right way, because doc is not very detailed and thats easy to miss something. What do you think?
Cause I can see that default avatars for users and workspaces with sizing [80px, 80px], but workspace avatars on the same figma are sized to [40,40] too
Let me see if I can get @dubielzyk-expensify to answer this one. I would guess we want these to be consistent with whatever we are doing now. They are SVGs so resizing should be no problem as they are vector graphics? Maybe I did not fully understand the question.
Maybe I should create a draft with ongoing changes to understand if Im on the right way
That sounds perfect. It will be easier to discuss some of these changes in the context of a PR and I can point you directly to the places in the code we need to update.
For now, I'll answer your questions so far.
memberRoles and reportName, but where to pass those fields
When we are creating a Group Chat we will call the API with these parameters and some others.
(not shown, but we can also call it with an accountIDList
as in this example - more on this below)
Existing method where we call this API is here (use staging.expensify.com
or expensify.com
for testing).
The design has changed a bit. For the API we need to just take any "admins" and make sure they are in the list of groupChatAdminLogins
.
API would then return something like this for the report
object:
{
"onyxMethod": "merge",
"key": "report_3166052379363217",
"value": {
"reportID": "3166052379363217",
"reportName": "Testing",
"type": "chat",
"chatType": "group",
"ownerAccountID": 0,
"managerID": 0,
"policyID": "_FAKE_",
"participants": {
"17356": {
"hidden": true,
"role": "admin"
},
"17357": {
"hidden": true,
"role": "member"
},
"17358": {
"hidden": true,
"role": "member"
}
},
"participantAccountIDs": [
17356,
17358,
17357
],
"visibleChatMemberAccountIDs": [],
"isPinned": false,
"lastReadTime": null,
"lastMentionedTime": null,
"lastReadSequenceNumber": 0,
"lastVisibleActionCreated": "2024-02-28 03:23:15.981",
"lastVisibleActionLastModified": "2024-02-28 03:23:15.981",
"lastMessageText": "",
"lastActorAccountID": 0,
"notificationPreference": "hidden",
"stateNum": 0,
"statusNum": 0,
"oldPolicyName": "",
"visibility": null,
"isOwnPolicyExpenseChat": false,
"lastMessageHtml": "",
"iouReportID": null,
"hasOutstandingChildRequest": false,
"policyName": null,
"hasParentAccess": null,
"parentReportID": null,
"parentReportActionID": null,
"writeCapability": "all",
"description": "",
"isDeletedParentAction": null,
"reportFields": null,
"total": 0,
"unheldTotal": 0,
"currency": "USD",
"submitterPayPalMeAddress": "",
"chatReportID": null,
"isWaitingOnBankAccount": false,
"nonReimbursableTotal": 0,
"isCancelledIOU": false
}
}
So we need our optimistic report here to have roughly the same participants
object.
And regarding this part Finally, the optimistic report is passed here to be created in the server. We must also pass the groupChatAdminLogins. .
This was not worded correctly. But anyone who is an "admin" should get passed to groupChatAdminsLogins
(for now it's only the report creator - so not too much to worry about for this PR).
The trickiest part for this will be updating the participants
accountID when we do not yet have them. We do something like this:
Which can also have a generated accountID for a user that may or may not exist yet:
So, this needs to be referenced in the participants
object. Then the API should remove any of these "fake users". Sounds complicated, but we can focus on users that exist for now and figure a few of the details out as we go if that works for you.
I think hopefully seeing the API request/response for this part will fill in some of the missing pieces. But please do not hesitate to let me know if anything else is unclear. Thanks very much! 🙇
Let me see if I can get @dubielzyk-expensify to answer this one. I would guess we want these to be consistent with whatever we are doing now. They are SVGs so resizing should be no problem as they are vector graphics? Maybe I did not fully understand the question.
@waterim The .svgs are vector graphics so they scale nicely to 80x80. It's only .pngs we need to export larger assets for. So yeah, you should be able to use the svg assets fine :)
cc @Expensify/design
@marcaaron @dubielzyk-expensify Fixed svgs, they had hardcoded width height and thats why code didn't change sizing correctly, now thats fine :)
@marcaaron Thank you for details so much, trying to figure it out, will post a draft today too for better understanding for me and you about the situation
@Expensify/design tiny nitpick—but the admin badge should be using our new (condensed) badge styles right?
Absolutely. And I wonder if it should be aligned to the right side of the row as well? That would match other places I think.
@shawnborton like this?
Yup exactly what I was thinking! Whatcha think?
I'm for it.
Will there ever be a situation where there's a check selector AND a badge? Actually it doesn't matter. It still looks totally normal.
[!NOTE] I don't know if the above mock is even possible, I just wanted to make sure the badge wouldn't look weird in case it is.
Totally agree, I don't think it's possible but even if it is, it's not terrible!
Cool, thank you, actually that was my concern regarding this design which I have in a doc, but you resolved it even before my question :)
After I merged main I found a weird bug related to new chat creation page, probably the issue with OptionsSelector itself. The issue is with selecting and deselecting people.
Here is a video with the bug from https://staging.new.expensify.com/new/chat
https://github.com/Expensify/App/assets/39777589/ffb6b74e-f32b-4a77-a1ca-88d52fcfb5c6
Do I need to report it or ticket already exists for this? @marcaaron @shawnborton
Hmm I'm not sure if a bug for that exists, feel free to go ahead and report it!
Agree. This seems very unexpected.
@waterim apologies, I must amend something that I've said WRT to the participant accountIDs as I did not notice until now 😅
(not shown, but we can also call it with an accountIDList as in this example - more on this below)
It's true that we can call the API with an accountIDList
- but I think we actually need to create a new request param like optimisticAccountIDList
so we can check if any have changed. I'm working on this backend change now so it will be ready for us to test when we need it.
@marcaaron Created a draft with ongoing changes, working on the create flow
The backend change we need to clear optimistic accountIDs should be live soon. For now we should have all the pieces in place to test creating group chats with known participants.
@waterim what's your ETA for when the PR will be ready for review? Anything I can do to help? Thanks!
@marcaaron i think Wednesday is a most likely possible day for an open PR. i was busy on Friday, today got back to the task, resolving last your comment and would be able to proceed with creation!
Sounds great, thanks!
@marcaaron, @waterim Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
I think we are about there with the review changes. I will dedicate tomorrow to some final testing.
And we can start working on this very soon -> https://github.com/Expensify/App/issues/34927
@marcaaron, @waterim Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
This got delayed slightly as we needed a few last minute backend changes to address a bug with the Splits feature. We should be ready to move forward as soon as all of these PRs are deployed:
https://github.com/Expensify/Web-Expensify/pull/41329 - merged + on production https://github.com/Expensify/Web-Expensify/pull/41290 - merged + on production https://github.com/Expensify/Auth/pull/10217 - merged + on production
All the PRs are on production now so we should be unblocked.
This PR was merged 🎉 (but not yet deployed)
On to the next one...
⚠️ Looks like this issue was linked to a Deploy Blocker here
If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.
If a regression has occurred and you are the assigned CM follow the instructions here.
If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.
Reviewing
label has been removed, please complete the "BugZero Checklist".
The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.58-8 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:
If no regressions arise, payment will be issued on 2024-04-09. :confetti_ball:
For reference, here are some details about the assignees on this issue:
@marcaaron Can you please assign me here?
Done! Thanks for your help on this one.
Issue is ready for payment but no BZ is assigned. @joekaufmanexpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!
[Upwork Job]()
Job added to Upwork: https://www.upwork.com/jobs/~0174ced808f29e748c
Current assignee @s77rt is eligible for the External assigner, not assigning anyone new.
@marcaaron @s77rt just to confirm, this regression was valid, correct?
Yes that's correct
Okay, sounds good. In that case, the only payment we need to issue here is $250 to @s77rt via upwork. Payment was originally $500, but becomes $250 with 50% regression reduction.
Holding on:
Group Chats Design Doc
Changes described in detail here.
Goals
Current UI:
New UI (with differences noted):
API call:
chatType
togroup
when callingOpenReport
so the backend will know we are creating a "Group Chat" and add a constant for this.%firstName%
,%firstName%
, …. one for each member, with the first member alphabetically appearing first (leftmost)Group
chat contains only one member because everyone else has left, then the default name will be%firstName%’s group chat
groupChatAdminLogins
. For now, it will include the creator of the chat only.Migration notes:
reportID
(as this won't ever change) similar to how the default avatars are generated.This will all need to be performed in one App PR. We will hard deprecate older versions after it is deployed to production by updating the minimum app version so there is a hard dependency on this PR getting approved and merged:
Other things to note:
'hidden'
until a comment is left. We must ensure this behavior is preserved.Implementation notes:
Our existing screen for this flow is: NewChatSelectorPage, but the Chat tab section is the NewChatPage. Its Modal stack navigator is here.
We need to: Modify the “Create group” button so that it says “Next”. Add an additional page to the NewChatSelectorPage stack navigator. Create a new page which we will call NewChatConfrmPage Add the appropriate routes for this page at /new/chat/confirm
Switching the button to “Next” here when we have tapped the “Add to group” option. Navigating to the new page
We need to add logic to navigate the user to the confirm page when they have selected “Add to group” here and have some staging participants.
We’ll also need some way for this new page to access those participants. The selectedOptions are currently stored here and house the members being added to the group. We will move them to a new Onyx key called newGroupChat that will be exclusively used to hold the state for a Group chat that is in the process of being created. This will allow the user to refresh the page and pick up where they left off (which is not possible today) and also allow us to share this state between all pages in the flow.
Start group button By this point, the newGroupChat Onyx state should have the following required data for the OpenReport API call: optimisticReportID optimistic accountIDs for any participants groupChatAdminLogins - should only contain the creator reportName - empty string (for now)
onPress we will navigate to the report just like we do already for Group DMs.
To do this, we will modify the navigateAndOpenReport() method here to handle the: memberRoles (if any) reportName
It already handles the logins. So, we need to add arguments for roles and reportName to that method. We must also modify the logic here when we have the group chat case since we won’t ever need to get an existing chat and will always be creating one.
The optimisticReport returned from ReportUtils.buildOptimisticChatReport() must be modified to update the participants field (see refactor plan here) with the corresponding role for each participant. Finally, the optimistic report is passed here to be created in the server. We must also pass the memberRoles as it will contain the link between any optimistic accountID and the login/email.
Group chat avatars will also need to display correctly in the Search function, Report header, details page, etc. We must modify the methods that get the avatars from personal details or workspace and first check for the report.avatarUrl. We can modify the existing ReportUtils.getIcons() method to first check the report.avatarUrl when we have a “Group chat” and fallback to a default avatar based off of the reportID.
Upwork Automation - Do Not Edit