Skip to content

[HOLD for payment 2024-02-26] [$500] Workspace - Clicking on avatar of invited workspace does not open avatar in full screen view #35731

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

Closed
6 tasks done
kbecciv opened this issue Feb 2, 2024 · 37 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@kbecciv
Copy link

kbecciv commented Feb 2, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 1.4.36.0
Reproducible in staging?: y
Reproducible in production?: n
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Action Performed:

Precondition:

  • User is invited to a workspace that has custom avatar.
  1. Go to staging.new.expensify.com.
  2. Go to settings from the bottom.
  3. Click on the invited workspace.
  4. Click on the workspace avatar.

Expected Result:

Workspace avatar will open in full screen view.

Actual Result:

Nothing happens when clicking on the avatar of invited workspace. Clicking on avatar of invited workspace does not open avatar in full screen view.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6365315_1706907042565.20240203_005005.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0188ba50d52f570fef
  • Upwork Job ID: 1753549469140381696
  • Last Price Increase: 2024-02-09
  • Automatic offers:
    • fedirjh | Reviewer | 0
    • ZhenjaHorbach | Contributor | 0
Issue OwnerCurrent Issue Owner: @fedirjh
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Feb 2, 2024
Copy link
Contributor

github-actions bot commented Feb 2, 2024

👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.

Copy link

melvin-bot bot commented Feb 2, 2024

Triggered auto assignment to @bondydaa (Engineering), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

@kbecciv
Copy link
Author

kbecciv commented Feb 2, 2024

We think that this bug might be related to #wave5-free-submitters
CC @dylanexpensify

@bondydaa
Copy link
Contributor

bondydaa commented Feb 2, 2024

Asked about this here https://expensify.slack.com/archives/C05DWUDHVK7/p1706912992107429

I don't think this is a bug or a blocker because we didn't really consider it during implementation... that being said, I do think for the read-only view, that an employee should be able to see the avatar at a larger size in the attachment modal

removing the blocker and going to put this out for external proposals.

@bondydaa bondydaa added Bug Something is broken. Auto assigns a BugZero manager. and removed DeployBlockerCash This issue or pull request should block deployment Engineering Hourly KSv2 labels Feb 2, 2024
Copy link

melvin-bot bot commented Feb 2, 2024

Triggered auto assignment to @kadiealexander (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

@melvin-bot melvin-bot bot added the Daily KSv2 label Feb 2, 2024
@bondydaa bondydaa added the External Added to denote the issue can be worked on by a contributor label Feb 2, 2024
@melvin-bot melvin-bot bot changed the title Workspace - Clicking on avatar of invited workspace does not open avatar in full screen view [$500] Workspace - Clicking on avatar of invited workspace does not open avatar in full screen view Feb 2, 2024
Copy link

melvin-bot bot commented Feb 2, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 2, 2024
Copy link

melvin-bot bot commented Feb 2, 2024

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

@bondydaa
Copy link
Contributor

bondydaa commented Feb 2, 2024

@kadiealexander no action required from ya for now.

@bondydaa
Copy link
Contributor

bondydaa commented Feb 2, 2024

what we're looking for is a way to view workspace avatars in the bigger "attachment modal" when following these steps:

Go to settings from the bottom.
Click on the invited workspace.
Click on the workspace avatar (currently does nothing, should open image in modal)

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Feb 3, 2024

Proposal

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

Workspace - Clicking on avatar of invited workspace does not open avatar in full screen view

What is the root cause of that problem?

The main problem with the issue is that we have disabled param which disables any interaction with the image

<AvatarWithImagePicker
onViewPhotoPress={() => Navigation.navigate(ROUTES.WORKSPACE_AVATAR.getRoute(policy.id))}
source={lodashGet(policy, 'avatar')}
size={CONST.AVATAR_SIZE.XLARGE}
avatarStyle={styles.avatarXLarge}
DefaultAvatar={() => (
<Avatar
containerStyles={styles.avatarXLarge}
imageStyles={[styles.avatarXLarge, styles.alignSelfCenter]}
source={policy.avatar ? policy.avatar : ReportUtils.getDefaultWorkspaceAvatar(policyName)}
fallbackIcon={Expensicons.FallbackWorkspaceAvatar}
size={CONST.AVATAR_SIZE.XLARGE}
name={policyName}
type={CONST.ICON_TYPE_WORKSPACE}
/>
)}
type={CONST.ICON_TYPE_WORKSPACE}
fallbackIcon={Expensicons.FallbackWorkspaceAvatar}
style={[styles.mb3, styles.mt5]}
isUsingDefaultAvatar={!lodashGet(policy, 'avatar', null)}
onImageSelected={(file) => Policy.updateWorkspaceAvatar(lodashGet(policy, 'id', ''), file)}
onImageRemoved={() => Policy.deleteWorkspaceAvatar(lodashGet(policy, 'id', ''))}
editorMaskImage={Expensicons.ImageCropSquareMask}
pendingAction={lodashGet(policy, 'pendingFields.avatar', null)}
errors={lodashGet(policy, 'errorFields.avatar', null)}
onErrorClose={() => Policy.clearAvatarErrors(policy.id)}
previewSource={UserUtils.getFullSizeAvatar(policy.avatar, '')}
headerTitle={translate('workspace.common.workspaceAvatar')}
originalFileName={policy.originalFileName}
disabled={readOnly}
disabledStyle={styles.cursorDefault}
/>

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

To fix this issue we can add a new property (enablePreview)

To add the ability to press on image we can make changes here

const readOnly = enablePreview && !isUsingDefaultAvatar;
disabled={isAvatarCropModalOpen || (disabled && !readOnly)}

disabled={isAvatarCropModalOpen || disabled}


Add a condition here so that instead of opening the menu
We opened the screen with the photo

For this, we can use onViewPhotoPress from the parent component which we use to navigate to a modal window with an image

Additional ways

> We can make `AttachmentModal` like wrapper for all image component to use show function from `AttachmentModal`

We can get list of events from AttachmentModal (For this we can use useRef and useImperativeHandle inside AttachmentModal)

                            onPress={() => {
                                if (disabled && readOnly) {
                                    onViewPhotoPress();
                                    return;
                                }
                                setIsMenuVisible((prev) => !prev);
                            }}

onPress={() => setIsMenuVisible((prev) => !prev)}


Update Tooltip text here and show Edit photo or View photo

text={readOnly ? translate('avatarWithImagePicker.viewPhoto') : translate('avatarWithImagePicker.editImage')}

text={translate('avatarWithImagePicker.editImage')}

Plus I notice that the close button on AttachmentModal doesn't have a selected cursor
We can fix this too

What alternative solutions did you explore? (Optional)

NA

@Tony-MK
Copy link
Contributor

Tony-MK commented Feb 3, 2024

Proposal

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

Workspace - Clicking on avatar of invited workspace does not open avatar in full screen view

What is the root cause of that problem?

In the WorkspaceOverviewPage, the AvatarWithImagePicker component disables its PressableWithoutFeedback component when the user can only read the image and not edit it.

const readOnly = !PolicyUtils.isPolicyAdmin(policy);

This is only true if the isPolicyAdmin is true.

Hence, if readOnly is true, the onViewPhotoPress function will not be called.

<Tooltip
shouldRender={!disabled}
text={translate('avatarWithImagePicker.editImage')}
>
<PressableWithoutFeedback
onPress={() => setIsMenuVisible((prev) => !prev)}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.IMAGEBUTTON}
accessibilityLabel={translate('avatarWithImagePicker.editImage')}
disabled={isAvatarCropModalOpen || disabled}
disabledStyle={disabledStyle}

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

Since we don't need to edit the avatar when the user is not an admin but can view it, I think it is reasonable to consider using the RoomHeaderAvatars component instead of the AvatarWithImagePicker component like the ReportDetailsPage.

We will get the single icon to pass into the icons prop from the getWorkspaceIcon function.

Therefore, we can use a simple condition to either render the AvatarWithImagePicker or RoomHeaderAvatars component should be rendered based on the isPolicyAdmin condition.

@melvin-bot melvin-bot bot added the Overdue label Feb 5, 2024
@trjExpensify trjExpensify moved this to Release 1: Ideal Nav & Collect Simplfied Profile, Members, Categories, Workflows (approvals) in [#whatsnext] Wave 08 - Collect Plan Admins Feb 5, 2024
@trjExpensify
Copy link
Contributor

I've popped this one in the wave8 project, as it's a byproduct of the change with ideal nav to let non-admin members view the workspace overview page. 👍 CC: @mountiny @hayata-suenaga

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Feb 12, 2024

@ZhenjaHorbach Your proposal looks overcomplicated to me. Specifically this part :

Add condition here so that instead of opening the menu We opened the screen with the photo
We have some ways: 1. We need to get list of events from AttachmentModal (For this we can use useRef and useImperativeHandle inside AttachmentModal)
2. We can make AttachmentModal like wrapper for all image component to use show function from AttachmentModal
3. Use onViewPhotoPress from parent component which we use to navigate to a modal window with an image

There are 3 options to choose from
Each of these options works well
But I can cut it

@fedirjh
Copy link
Contributor

fedirjh commented Feb 12, 2024

@ZhenjaHorbach Reviewing the proposal, I believe we can implement a simpler solution:

  1. Introduce a new prop named previewRoute to the AvatarWithImagePicker component.
  2. Within the WorkspaceProfilePage, include the prop like this: previewRoute={ROUTES.WORKSPACE_AVATAR.getRoute(policy.id)}
  3. Update the press handler to navigate to the previewRoute if it exists and the picker is disabled:
onPress={() => {
    if (previewRoute && disabled) {
        Navigation.navigate(previewRoute);
        return;
    }
    setIsMenuVisible((prev) => !prev);
}}

With this approach, we can ensure that the AvatarWithImagePicker is used for similar use cases. All that's needed is to provide a preview route and disable the picker option.

What do you think @bondydaa?

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Feb 12, 2024

@ZhenjaHorbach Reviewing the proposal, I believe we can implement a simpler solution:

  1. Introduce a new prop named previewRoute to the AvatarWithImagePicker component.
  2. Within the WorkspaceProfilePage, include the prop like this: previewRoute={ROUTES.WORKSPACE_AVATAR.getRoute(policy.id)}
  3. Update the press handler to navigate to the previewRoute if it exists and the picker is disabled:
onPress={() => {
    if (previewRoute && disabled) {
        Navigation.navigate(previewRoute);
        return;
    }
    setIsMenuVisible((prev) => !prev);
}}

With this approach, we can ensure that the AvatarWithImagePicker is used for similar use cases. All that's needed is to provide a preview route and disable the picker option.

What do you think @bondydaa?

I think there is no easier way then onViewPhotoPress instead Navigation.navigate )
Because it is responsible for where we will navigate when we click onView Photo

onViewPhotoPress={() => Navigation.navigate(ROUTES.PROFILE_AVATAR.getRoute(accountID))}

onViewPhotoPress={() => Navigation.navigate(ROUTES.WORKSPACE_AVATAR.getRoute(policy.id))}

@fedirjh
Copy link
Contributor

fedirjh commented Feb 12, 2024

I think there is no easier way then onViewPhotoPress )

This was not so clear to me when I first saw it. Let's reuse that instead of the previewRoute .

@fedirjh
Copy link
Contributor

fedirjh commented Feb 12, 2024

I think this proposal from @ZhenjaHorbach is good. We should only verify the enablePreview && onViewPhotoPress props; there is no need to check for isUsingDefaultAvatar since the default avatar should be displayed in fullscreen mode, similar to profile avatars.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented Feb 12, 2024

Current assignee @bondydaa is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@fedirjh
Copy link
Contributor

fedirjh commented Feb 12, 2024

@Tony-MK Thanks for the proposal. Your proposal looks good as well, but I think fixing AvatarWithImagePicker is the best solution for this issue.

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Feb 12, 2024
Copy link

melvin-bot bot commented Feb 12, 2024

📣 @fedirjh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

Copy link

melvin-bot bot commented Feb 12, 2024

📣 @ZhenjaHorbach 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@ZhenjaHorbach
Copy link
Contributor

📣 @ZhenjaHorbach 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

PR will be ready today or tomorrow

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 Weekly KSv2 labels Feb 13, 2024
@melvin-bot melvin-bot bot changed the title [$500] Workspace - Clicking on avatar of invited workspace does not open avatar in full screen view [HOLD for payment 2024-02-26] [$500] Workspace - Clicking on avatar of invited workspace does not open avatar in full screen view Feb 19, 2024
Copy link

melvin-bot bot commented Feb 19, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Feb 19, 2024
Copy link

melvin-bot bot commented Feb 19, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.42-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-02-26. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Feb 19, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@fedirjh] The PR that introduced the bug has been identified. Link to the PR:
  • [@fedirjh] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@fedirjh] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@fedirjh] Determine if we should create a regression test for this bug.
  • [@fedirjh] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@kadiealexander] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Feb 26, 2024
@fedirjh
Copy link
Contributor

fedirjh commented Feb 26, 2024

BugZero Checklist:

  • Link to the PR: I think this is a feature request.
  • Link to comment: N/A
  • Link to discussion: N/A
  • Determine if we should create a regression test for this bug: N/A, we have a regression test in place.

@kadiealexander
Copy link
Contributor

kadiealexander commented Feb 27, 2024

Payouts due:

Upwork job is here.

@github-project-automation github-project-automation bot moved this from Release 1: Ideal Nav & Collect Simplfied Profile, Members, Categories, Workflows (approvals) to Done in [#whatsnext] Wave 08 - Collect Plan Admins Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Development

No branches or pull requests

9 participants