-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add support for viewing group avatar photo #48757
Conversation
Signed-off-by: Tsaqif <[email protected]>
src/libs/actions/Report.ts
Outdated
@@ -842,8 +845,6 @@ function openReport( | |||
if (avatar) { | |||
parameters.file = avatar; | |||
} | |||
|
|||
clearGroupChat(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am moving this to the NewChatConfirmPage
because there is a glitch when completing the new chat group flow. The draft is removed even though the page is still displayed, causing an issue where the avatar and participants are removed.
bug video:
bug-d.mp4
src/libs/actions/Report.ts
Outdated
InteractionManager.runAfterInteractions(() => { | ||
if (shouldDismissModal) { | ||
Navigation.dismissModalWithReport(report); | ||
} else { | ||
Navigation.navigateWithSwitchPolicyID({route: ROUTES.HOME}); | ||
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report?.reportID ?? '-1'), actionType); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am adding InteractionManager.runAfterInteractions
because there is a bug when on new chat confirm page -> view photo of the avatar -> refresh the page -> going offline -> complete the new group chat flow, the report name changes to CONST.REPORT.DEFAULT_REPORT_NAME
. This issue arises due to the following line:
App/src/libs/actions/Report.ts
Line 775 in 4af620b
reportName: ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportName ?? CONST.REPORT.DEFAULT_REPORT_NAME, |
The problem occurs because openReport
from the report screen is triggered before the new group report data is stored in Onyx.
Bug:
bug-d.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see #48757 (comment).
Also is this related to the avatar or it's an existing bug?
@s77rt There is a bug on Android where the avatar is not displayed when viewing a report avatar until the screen is tapped. This issue also occurs in the main when uploading a custom avatar to a workspace and then viewing the photo. Bug:bug-d.mp4 |
src/pages/ReportAvatar.tsx
Outdated
const rootGroupReport = ReportUtils.getRootParentReport(report); | ||
const source = | ||
UserUtils.getFullSizeAvatar(report?.avatarUrl, 0) ?? | ||
UserUtils.getFullSizeAvatar(rootGroupReport?.avatarUrl, 0) ?? | ||
ReportUtils.getDefaultGroupAvatar(ReportUtils.getRootParentReport(report)?.reportID ?? '') ?? | ||
''; | ||
return { | ||
source, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When opening a url of thread report the adding avatar in the end of url the current staging display:
Bug:
bug-d.mp4
So, I am using using the avatar of root of Group Chat for thread of group chat.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the report is not a group we shouldn't use the group avatar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should we display for the thread in the group chat?
to test: in the thread of group chat add at the end. For example:
staging.new.expensify.com/r/1234567/avatar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not work on this here. It's not directly related to group chat avatars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
src/libs/actions/Report.ts
Outdated
InteractionManager.runAfterInteractions(() => { | ||
if (shouldDismissModal) { | ||
Navigation.dismissModalWithReport(report); | ||
} else { | ||
Navigation.navigateWithSwitchPolicyID({route: ROUTES.HOME}); | ||
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(report?.reportID ?? '-1'), actionType); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we adding the InteractionManager
. here? We should wait for interaction only after we register them. Doing so without any registered handler is a workaround
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of this bug:
I am adding InteractionManager.runAfterInteractions
because there is a bug when on new chat confirm page -> view photo of the avatar -> refresh the page -> going offline -> complete the new group chat flow, the report name changes to CONST.REPORT.DEFAULT_REPORT_NAME
. This issue arises due to the following line:
App/src/libs/actions/Report.ts
Line 775 in 4af620b
reportName: ReportConnection.getAllReports()?.[`${ONYXKEYS.COLLECTION.REPORT}${reportID}`]?.reportName ?? CONST.REPORT.DEFAULT_REPORT_NAME, |
The problem occurs because openReport
from the report screen is triggered before the new group report data is stored in Onyx.
Bug:
bug-d.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use dismissModal
in onModalClose, we can revert this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this bug related to the new feature (group avatar view) or an existing bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s because we’re adding a new feature View photo
in the NewChatConfirmPage.
The test step is:
On the new chat confirm page -> add avatar -> View photo
of the avatar -> ReportAvatar will open -> refresh the page -> Go offline -> Close the report avatar -> complete the new group chat flow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure what went wrong here.
The problem occurs because openReport from the report screen is triggered before the new group report data is stored in Onyx
Why this does not happen when we proceed with the flow normally? (without the avatar view feature)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After merging main, it works now.
I am reverting this change.
@@ -694,6 +694,7 @@ function updateGroupChatAvatar(reportID: string, file?: File | CustomRNImageMani | |||
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`, | |||
value: { | |||
avatarUrl: file?.uri ?? '', | |||
avatarFileName: file ? file?.name ?? '' : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avatarFileName: file ? file?.name ?? '' : null, | |
avatarFileName: file?.name ?? '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we set avatarFileName
to an empty string when file
is undefined, the avatarFileName
will remain an empty string. For example, when removing the avatar, the avatarFileName
will stay as an empty string, but it should be set to null
to effectively remove the avatarFileName
.
To test this, go to the avatar picker’s popover menu and select "Remove Avatar." Then, check the Onyx value of avatarFileName
. While the avatarUrl
will be removed, the avatarFileName
will remain an empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While the avatarUrl will be removed, the avatarFileName will remain an empty string
How is that the url is being removed but the filename is not? given that both fallback to an empty string, or is it because the former is cleared from BE onyx response?
Either way, let's set both to null
src/pages/ReportAvatar.tsx
Outdated
const avatarURL = ReportUtils.getWorkspaceAvatar(report); | ||
const policyName = ReportUtils.getPolicyName(report, true, policy); | ||
const isPolicyRelatedReport = !!policyName; | ||
const [shouldShowDefaultGroupAvatar, setShouldShowDefaultGroupAvatar] = useState(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't use a state here. This info should be concluded from the draft data (avatarUri field)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To check whether the blob URI is available, I am using FileUtils.readFileAsync
, which is an asynchronous operation. If an error is thrown, it means the blob URI isn't available. That is why we must use useState
.
Is there any alternative? I am open to other solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not available clear the onyx value (the source of truth)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I understand now.
src/pages/ReportAvatar.tsx
Outdated
if (isNewGroupDraftAvatar) { | ||
Navigation.goBack(); | ||
Navigation.isNavigationReady().then(() => { | ||
Navigation.navigate(ROUTES.NEW_CHAT_CONFIRM); | ||
}); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just dismiss the modal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the user refreshes the ReportAvatar page on the NewChatConfirmPage, dismissModal
will open the home/report screen instead of the NewChatConfirmPage. If the user starts a new chat again, the new group draft will be cleared. Should we dismiss the modal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. I think we should keep the user on the same page. But why we are doing this navigation that way? Why not Navigation.goBack(ROUTES.NEW_CHAT_CONFIRM)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve tried it, but it still doesn’t work; the report screen opens instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why this happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also happens in the main.
When the user opens the expense policy report -> goes to report details -> clicks on the workspace avatar -> the report/workspace avatar will be displayed -> Refresh the report avatar page -> close the report avatar modal -> notice that the home/report screen is displayed instead of the report details page
(which is the parameter of the goBack).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please investigate the root cause of this one. And will passing shouldEnforceFallback=true
fix the issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@s77rt It causes other bugs. This is a video from ReportDetail, but it will also reproduce in NewChatConfirmPage:
bug video:
bug-d.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please investigate the root cause of this one.
@s77rt I'm not sure; the ReportAvatar page might not be supported yet. There is no entry for the ReportAvatar screen in getAdaptedState
.
I think need some modifications in getAdaptedState
function
src/pages/ReportAvatar.tsx
Outdated
isWorkspaceAvatar | ||
source={attachment.source} | ||
onModalClose={onModalClose} | ||
isWorkspaceAvatar={isPolicyRelatedReport && !isNewGroupDraftAvatar} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which cases both can be true
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
src/pages/NewChatConfirmPage.tsx
Outdated
@@ -151,11 +163,12 @@ function NewChatConfirmPage({newGroupDraft, allPersonalDetails}: NewChatConfirmP | |||
}} | |||
size={CONST.AVATAR_SIZE.XLARGE} | |||
avatarStyle={styles.avatarXLarge} | |||
shouldDisableViewPhoto | |||
shouldDisableViewPhoto={!avatarFile} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shouldDisableViewPhoto
prop should be removed and we should relay only on isUsingDefaultAvatar
to disable the view photo option.
Signed-off-by: Tsaqif <[email protected]>
Signed-off-by: Tsaqif <[email protected]>
Signed-off-by: Tsaqif <[email protected]>
src/pages/ReportAvatar.tsx
Outdated
newGroupDraft: { | ||
key: ONYXKEYS.NEW_GROUP_CHAT_DRAFT, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the hook instead useOnyx
Signed-off-by: Tsaqif <[email protected]>
@tsa321 I have been testing this and I keep seeing bugs related to the group draft avatar which is increasing the work complexity without adding significant value to the UX. For now let's remove that functionality for the group draft (the |
Signed-off-by: Tsaqif <[email protected]>
Signed-off-by: Tsaqif <[email protected]>
@s77rt PR is ready for review |
Signed-off-by: Tsaqif <[email protected]>
src/pages/ReportAvatar.tsx
Outdated
const attachment = useMemo(() => { | ||
if (isPolicyRelatedReport) { | ||
return { | ||
source: UserUtils.getFullSizeAvatar(ReportUtils.getWorkspaceAvatar(report), 0), | ||
headerTitle: policyName, | ||
originalFileName: policy?.originalFileName ?? policy?.id ?? report?.policyID ?? '', | ||
isWorkspaceAvatar: true, | ||
}; | ||
} | ||
|
||
if (ReportUtils.isGroupChat(report) && !ReportUtils.isThread(report)) { | ||
return { | ||
source: report?.avatarUrl ? UserUtils.getFullSizeAvatar(report.avatarUrl, 0) : ReportUtils.getDefaultGroupAvatar(report?.reportID ?? ''), | ||
headerTitle: ReportUtils.getReportName(report), | ||
originalFileName: report?.avatarFileName ?? '', | ||
isWorkspaceAvatar: false, | ||
}; | ||
} | ||
|
||
return { | ||
source: UserUtils.getFullSizeAvatar(ReportUtils.getWorkspaceAvatar(report), 0), | ||
headerTitle: unavailableWorkspace, | ||
originalFileName: policy?.originalFileName ?? policy?.id ?? report?.policyID, | ||
isWorkspaceAvatar: true, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have three cases? Can we change this to:
- If report is related to policy use the old logic
- Otherwise use report.avatarUrl
Signed-off-by: Tsaqif <[email protected]>
@s77rt done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Getting close
src/pages/ReportAvatar.tsx
Outdated
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps | ||
}, [report, policy]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to disable eslint here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
const attachment = useMemo(() => { | ||
if (ReportUtils.isGroupChat(report) && !ReportUtils.isThread(report)) { | ||
return { | ||
source: report?.avatarUrl ? UserUtils.getFullSizeAvatar(report.avatarUrl, 0) : ReportUtils.getDefaultGroupAvatar(report?.reportID ?? ''), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
report?.avatarUrl
should be enough as you are nut supposed to be able to access this page if report does not have a custom avatar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can still access it using a URL, for example: dev.new.expensify.com:8082/r/12345678/avatar
.
src/pages/ReportAvatar.tsx
Outdated
onModalClose={() => { | ||
Navigation.goBack(ROUTES.REPORT_WITH_ID_DETAILS.getRoute(report?.reportID ?? '-1')); | ||
}} | ||
isWorkspaceAvatar | ||
isWorkspaceAvatar={attachment.isWorkspaceAvatar} | ||
maybeIcon | ||
// In the case of default workspace avatar, originalFileName prop takes policyID as value to get the color of the avatar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this comment to where we set originalFileName
for the workspace related reports
src/pages/ReportAvatar.tsx
Outdated
@@ -23,21 +23,38 @@ type ReportAvatarProps = ReportAvatarOnyxProps & StackScreenProps<AuthScreensPar | |||
function ReportAvatar({report = {} as Report, route, policies, isLoadingApp = true}: ReportAvatarProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While at it, can you please migrate the withOnyx
props to use the hook instead useOnyx
src/types/onyx/Report.ts
Outdated
/** The filename of the avatar */ | ||
avatarFileName?: string; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB. Can you move this to be below avatarUrl
Signed-off-by: Tsaqif <[email protected]>
Signed-off-by: Tsaqif <[email protected]>
@s77rt done |
Reviewer Checklist
Screenshots/Videos |
@tsa321 Please remove the outdated testing steps |
@s77rt I have updated the test steps. |
Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM, thanks! 🙇
@tsa321 conflicts here if you can fix them then we are ready to merge. Thanks! |
Signed-off-by: Tsaqif <[email protected]>
@marcaaron I have resolved the conflict. The check errors are from main. |
Signed-off-by: Tsaqif <[email protected]>
I'd like to get a follow up going potentially to just review the necessary changes to fix the eslint warnings as they seem unrelated, potentially complex, and this one is ready to merge in it's current state. There's more context on it here, but I am not sure if the refactoring effort needs to be combined into this PR. We might still ask you to do it. @roryabraham probably has more thoughts on this. Edit: Gonna merge and left some reasoning here -> https://expensify.slack.com/archives/C01GTK53T8Q/p1726541893211989?thread_ts=1725905735.105989&cid=C01GTK53T8Q |
@marcaaron looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
This was an eslint warning that we will fix in a follow up PR. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/marcaaron in version: 9.0.37-0 🚀
|
🚀 Deployed to production by https://github.com/grgia in version: 9.0.38-4 🚀
|
Details
Fixed Issues
$ #39850
PROPOSAL: #39850 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android-native-d.mp4
Android: mWeb Chrome
android-mweb_d.mp4
iOS: Native
ios-native_d.mp4
iOS: mWeb Safari
ios-msfari_d.mp4
MacOS: Chrome / Safari
macos-web-d.mp4
macos-web-offline_d.mp4
MacOS: Desktop
macos-desktop_d.mp4