[$500] Web - Workspace - Workspace created in offline mode is not grayed out on Choose a workspace page

[kbecciv]

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:

1. Go offline
2. Create new workspace
3. Navigate to the Choose a workspace page

Expected Result:

Newly created workspace in offline should be grayed out

Actual Result:

Workspace created in offline mode is not grayed out

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

Bug6365353_1706909617540.Recording__2017.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0124a63474e95d7dc5
• Upwork Job ID: 1753546948780208128
• Last Price Increase: 2024-02-02

[github-actions]

👋 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.

[melvin-bot]

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

[kbecciv]

We think that this bug might be related to #wave8-collect-admins
CC @zanyrenney

[francoisl]

Sounds somewhat similar to #35702.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0124a63474e95d7dc5

[melvin-bot]

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

[ZhenjaHorbach]

Proposal

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

Web - Workspace - Workspace created in offline mode is not grayed out on Choose a workspace page

What is the root cause of that problem?

The main problem with issue is that we don't pass pending action
We can't use OfflineWithFeedback without pending action

App/src/pages/WorkspaceSwitcherPage.tsx

Lines 125 to 142 in 6a889f1

	return Object.values(policies)
	.filter((policy) => PolicyUtils.shouldShowPolicy(policy, !!isOffline))
	.map((policy) => ({
	text: policy?.name,
	policyID: policy?.id,
	brickRoadIndicator: getIndicatorTypeForPolicy(policy?.id),
	icons: [
	{
	source: policy?.avatar ? policy.avatar : ReportUtils.getDefaultWorkspaceAvatar(policy?.name),
	fallbackIcon: Expensicons.FallbackWorkspaceAvatar,
	name: policy?.name,
	type: CONST.ICON_TYPE_WORKSPACE,
	},
	],
	boldStyle: hasUnreadData(policy?.id),
	keyForList: policy?.id,
	isPolicyAdmin: PolicyUtils.isPolicyAdmin(policy),
	}));

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

To fix this issue we can update this function and add a new field

pendingAction: policy?.pendingFields?.generalSettings || policy?.pendingAction

I add pendingFields?.generalSettings because when we update the general setting we add pending action there

But I think we can update optimistic data and add pendingAction field

App/src/libs/actions/Policy.ts

Lines 828 to 855 in d122676

	value: {
	...policy,
	pendingFields: {
	generalSettings: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
	},
	// Clear errorFields in case the user didn't dismiss the general settings error
	errorFields: {
	generalSettings: null,
	},
	name,
	outputCurrency: currency,
	...(distanceUnit?.customUnitID && distanceRate?.customUnitRateID
	? {
	customUnits: {
	[distanceUnit?.customUnitID]: {
	...distanceUnit,
	rates: {
	[distanceRate?.customUnitRateID]: {
	...distanceRate,
	currency,
	},
	},
	},
	},
	}
	: {}),

Plus we have problem with OptionRow since we get crash app since we use Hoverable and PressableWithFeedback inside OfflineWithFeedback

App/src/components/OptionRow.tsx

Lines 163 to 205 in 664626c

	<Hoverable>
	{(hovered) => (
	<PressableWithFeedback
	nativeID={keyForList}
	ref={pressableRef}
	onPress={(e) => {
	if (!onSelectRow) {
	return;
	}
	setIsDisabled(true);
	if (e) {
	e.preventDefault();
	}
	let result = onSelectRow(option, pressableRef.current);
	if (!(result instanceof Promise)) {
	result = Promise.resolve();
	}
	InteractionManager.runAfterInteractions(() => {
	result?.finally(() => setIsDisabled(isOptionDisabled));
	});
	}}
	disabled={isDisabled}
	style={[
	styles.flexRow,
	styles.alignItemsCenter,
	styles.justifyContentBetween,
	styles.sidebarLink,
	!isOptionDisabled && styles.cursorPointer,
	shouldDisableRowInnerPadding ? null : styles.sidebarLinkInner,
	optionIsFocused ? styles.sidebarLinkActive : null,
	shouldHaveOptionSeparator && styles.borderTop,
	!onSelectRow && !isOptionDisabled ? styles.cursorDefault : null,
	]}
	accessibilityLabel={option.text}
	role={CONST.ROLE.BUTTON}
	hoverDimmingValue={1}
	hoverStyle={!optionIsFocused ? hoverStyle ?? styles.sidebarLinkHover : undefined}
	needsOffscreenAlphaCompositing={(option.icons?.length ?? 0) >= 2}
	onMouseDown={shouldPreventDefaultFocusOnSelectRow ? (event) => event.preventDefault() : undefined}
	>
	<View style={sidebarInnerRowStyle}>

To fix this we need delete Hoverable and use only PressableWithFeedback(In any case we only need hovered state )

Or move OfflineWithFeedback inside Hoverable or PressableWithFeedback

What alternative solutions did you explore? (Optional)

NA

[Tony-MK]

Proposal

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

Web - Workspace - Workspace created in offline mode is not grayed out on choosing a workspace page.

What is the root cause of that problem?

In the WorkspaceOverviewPage, the pendingAction for the OfflineWithFeedback component only checks for pendingFields.generalSettings. That is why it only dims when the title is edited because pendingFields.generalSettings is "update".

App/src/pages/workspace/WorkspaceOverviewPage.js

Line 109 in 3ab4e6e

<OfflineWithFeedback pendingAction={lodashGet(policy, 'pendingFields.generalSettings')}>

App/src/pages/home/sidebar/PressableAvatarWithIndicator.js

Lines 59 to 61 in 3ab4e6e

	<AvatarWithIndicator
	source={UserUtils.getAvatar(currentUserPersonalDetails.avatar, currentUserPersonalDetails.accountID)}
	tooltipText={translate('common.settings')}

Also, the SidebarUtils.getOptionData function does not check for report.pendingAction and has the value "add" when the policy was created.

Thus, the OptionRowLHNData sidebar component won't dim.

App/src/libs/SidebarUtils.ts

Line 297 in 3ab4e6e

result.pendingAction = report.pendingFields ? report.pendingFields.addWorkspaceRoom || report.pendingFields.createChat : undefined;

App/src/libs/Navigation/AppNavigator/createCustomBottomTabNavigator/BottomTabBar.tsx

Lines 66 to 67 in 3ab4e6e

	<Tooltip text={translate('common.settings')}>
	<PressableWithFeedback

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

For the WorkspaceOverviewPage, we should modify the pendingAction {lodashGet(policy, 'pendingFields.generalSettings') || lodashGet(policy, 'pendingAction')} of the OfflineWithFeedback component here and if required for the AvatarWithImagePicker over here.

For example :

App/src/pages/workspace/WorkspaceOverviewPage.js

Line 100 in 3ab4e6e

pendingAction={lodashGet(policy, 'pendingFields.avatar', null)}

pendingAction={lodashGet(policy, 'pendingFields.generalSettings') || lodashGet(policy, 'pendingAction')}

Alternatively, we could wrap the WorkspacePageWithSections component with a OfflineWithFeedback component. Then, use the policy.pendingAction for the pendingAction prop. Finally, set shouldDisableOpacity for the AvatarWithImagePicker and the inner OfflineWithFeedback component if it is present policy.pendingAction to prevent double dimming.

Lastly, for the SidebarUtils.getOptionData function, introduce report.pendingAction in the OR condition which determines result.pendingAction alongside report.pendingFields.addWorkspaceRoom and `report.pendingFields.createChat.

What alternative solutions did you explore? (Optional)

Create a pendingFields attribute for the policy in the onyx and introduce it WorkspaceOverviewPage and SidebarUtils.getOptionData. It will function like createChat and be used like pendingAction in the earlier solution.

[mountiny]

Demoting, not a blocker

[melvin-bot]

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

[mananjadhav]

@mountiny @tylerkaraszewski I just reviewed an offline issue for Delete Workspace. Both the issues need different changes, but there might be an overlap such as changing the wrapping order.

But are we planning to consolidate all the new Workspace list offline issues in one? or we can work on these separately?

[mountiny]

I think we can work on this in one issue, create and delete workspace offline and ensure the feedback is correct.

[hayata-suenaga]

okay that makes sense. then let's handle these issues separately. @mananjadhav please select a proposal when you have time 🙇

[mananjadhav]

@Tony-MK's proposal looks good to me, as it covers all cases for add workspace.

🎀 👀 🎀 C+ reviewed.

[melvin-bot]

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

[ZhenjaHorbach]

@Tony-MK's proposal looks good to me, as it covers all cases for add workspace.

🎀 👀 🎀 C+ reviewed.

Sorry, I don't mind your decision
But does it fix the problem in this issue?
Issue related to fix Choose a workspace page (This is WorkspaceSwitcherPage )
In turn, the selected proposal makes changes in a completely different place and doesn't fix the main issue

[hayata-suenaga]

@mananjadhav could you check @ZhenjaHorbach's comment?

[mananjadhav]

I'll check the comment and the proposal again to confirm.

[Tony-MK]

Hey all,

I have to say about this #35732 (comment) which I think should be part of the expected result.

In turn, the selected proposal makes changes in a completely different place

I believe that this is because my proposal included handling the add pending action for the WorkspaceOverviewPage.

Issue related to fix Choose a workspace page (This is WorkspaceSwitcherPage )

Secondly, it also handles the pending action for the WorkspaceSwitcherPage and the Sidebar when the user is on the home screen.

Finally, I believe my solution was less complicated and regression-prone than changing Onyx's data.

@Tony-MK's #35732 (comment) looks good to me, as it covers all cases for add workspace.

Therefore, my proposal should fix all the cases as @mananjadhav stated in this #35732 (comment)

I can provide the test branch if you want to review it.

However, I am happy with the C+'s and engineer's decisions.

Thanks.

[ZhenjaHorbach]

Hey all,

I have to say about #35732 (comment)

In turn, the selected proposal makes changes in a completely different place

I believe this because my proposal included handling the pending action for the WorkspaceOverviewPage.

Issue related to fix Choose a workspace page (This is WorkspaceSwitcherPage )

Secondly, it also handles the pending action for the WorkspaceSwitcherPage and the Sidebar when the user is on the home screen.

Finally, my solution was less complicated and regression-prone than changing Onyx's data.

@Tony-MK's #35732 (comment) looks good to me, as it covers all cases for add workspace.

Therefore, my proposal should fix all the as @mananjadhav stated in this #35732 (comment)

I can provide the test branch if you want to review it.

However, I am happy with the C+'s and engineer's decisions.

In my proposal I do not indicate that we are obliged to change the onyx's data

And I don't mind if your proposition is better )
It’s just not clear how your proposal fixes WorkspaceSwitcherPage
Although this issue requires it

[Tony-MK]

To fix this issue we can update this function and add a new field

pendingAction: policy?.pendingFields?.generalSettings || policy?.pendingAction

I add pendingFields?.generalSettings because when we update the general setting we add pending action there

But I think we can update optimistic data and add pendingAction field

App/src/libs/actions/Policy.ts

Lines 828 to 855 in d122676

	value: {
	...policy,
	pendingFields: {
	generalSettings: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
	},
	// Clear errorFields in case the user didn't dismiss the general settings error
	errorFields: {
	generalSettings: null,
	},
	name,
	outputCurrency: currency,
	...(distanceUnit?.customUnitID && distanceRate?.customUnitRateID
	? {
	customUnits: {
	[distanceUnit?.customUnitID]: {
	...distanceUnit,
	rates: {
	[distanceRate?.customUnitRateID]: {
	...distanceRate,
	currency,
	},
	},
	},
	},
	}
	: {}),

@ZhenjaHorbach, then it seems I did not understand this part, before mentioning the Hoverable. Could you elaborate further?

It’s just not clear how your proposal fixes WorkspaceSwitcherPage

The SidebarUtils.getOptionData function also handles the add pending action for the WorkspaceSwitcherPage.

[ZhenjaHorbach]

To fix this issue we can update this function and add a new field

pendingAction: policy?.pendingFields?.generalSettings || policy?.pendingAction

I add pendingFields?.generalSettings because when we update the general setting we add pending action there

But I think we can update optimistic data and add pendingAction field

App/src/libs/actions/Policy.ts

Lines 828 to 855 in d122676

	value: {
	...policy,
	pendingFields: {
	generalSettings: CONST.RED_BRICK_ROAD_PENDING_ACTION.UPDATE,
	},
	// Clear errorFields in case the user didn't dismiss the general settings error
	errorFields: {
	generalSettings: null,
	},
	name,
	outputCurrency: currency,
	...(distanceUnit?.customUnitID && distanceRate?.customUnitRateID
	? {
	customUnits: {
	[distanceUnit?.customUnitID]: {
	...distanceUnit,
	rates: {
	[distanceRate?.customUnitRateID]: {
	...distanceRate,
	currency,
	},
	},
	},
	},
	}
	: {}),

@ZhenjaHorbach, then it seems I did not understand this part, before mentioning the Hoverable. Could you elaborate further?

It’s just not clear how your proposal fixes WorkspaceSwitcherPage

The SidebarUtils.getOptionData function also handles the add pending action for the WorkspaceSwitcherPage.

We don't use SidebarUtils.getOptionData inside WorkspaceSwitcherPage )

This is an idea to update, but I have already proposed an option without Onyx changes

[Tony-MK]

We don't use SidebarUtils.getOptionData inside WorkspaceSwitcherPage )

@ZhenjaHorbach, we do. It works. It will also handle the home screen.

Also, I think we need to fix the WorkspaceOverviewPage as well.

[ZhenjaHorbach]

We don't use SidebarUtils.getOptionData inside WorkspaceSwitcherPage )

@ZhenjaHorbach, we do. It works. It will also handle the home screen.

Also, I think we need to fix the WorkspaceOverviewPage as well.

In WorkspaceSwitcherPage we use only OptionRow instead of OptionRowLHNData (And here we use SidebarUtils.getOptionData)
So any changes related to SidebarUtils.getOptionData will not cause any effect on OptionRow

Sorry, if I'm wrong, but can you double-check ?)
Because I think your changes won't change anything in WorkspaceSwitcherPage

[Tony-MK]

It should work because I have tested it.

However, let's hear the C+'s decision.

[mananjadhav]

Folks I was finishing up some PRs, and I'll be focusing on this one today. Please bear with me for one day.

[melvin-bot]

@tylerkaraszewski @mananjadhav @kevinksullivan this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[melvin-bot]

@tylerkaraszewski, @mananjadhav, @kevinksullivan Eep! 4 days overdue now. Issues have feelings too...

[mananjadhav]

I checked the comments/proposals again, and I can see the code has changed since we last reviewed. @Tony-MK @ZhenjaHorbach do either of you want to revise their proposals?

[mvtglobally]

Issue not reproducible during KI retests. (First week)

[kevinksullivan]

Closing out unless this is reproducible again after the mentioned changes.

[Tony-MK]

@kevinksullivan, this issue is still reproducible on the latest main branch.

Furthermore, once a workspace is created, the workspace settings page should grayed out.

Similar to when a workspace name is updated.

Could we confirm this?

Thank you

[kevinksullivan]

Hm I'm actually not sure what's going on here. I am seeing some workspaces I created previously grayed out, and some aren't while offline. What is the expected behavior?

[Tony-MK]

The expected result of graying out the options in the workspace switcher is wrong.

I suppose there was confusion about highlighting the focused option and pending action.

However, is it true that when the workspace is created, the workspace's initial page should grayed out?

Similarly, when a workspace name is updated.

Thank you.