Open IuliiaHerets opened 2 months ago
Triggered auto assignment to @joekaufmanexpensify (Bug
), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.
We think that this bug might be related to #vip-bills
Removed blocker, see https://expensify.slack.com/archives/C9YU7BX5M/p1726158576447559
Triggered auto assignment to @srikarparsi (DeployBlockerCash
), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.
:wave: 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:
@IuliiaHerets I think the uploaded video is incorrect. Can you please check?
@luacmartins fixed, sorry for this
No problem. Thank you!
This is also reproducible in prod if we just copy/paste the workspace link
Just by having the workspace link in any chat the problem also happens in prod:
https://github.com/user-attachments/assets/778e2117-0eb4-47b8-9f49-6da855676b88
We can't pay the invoice using Pay as a business in prod, so I marked it as N/A - can't check on prod
Edited by proposal-police: This proposal was edited at 2023-10-06T15:00:00Z.
The link can open workspace settings page and Members tab leads to not here page.
After deleting the workspace, we still retain some workspace information to show the invoice report's avatars and name:
These information is returned by OpenReport
:
The not found page condition here simply checks for empty/delete-pending policy
:
shouldShowPolicy
to the shouldShowNotFoundPage
above to filter these workspaces and prevent them from being accessed by URL:This condition is also used in WorkspacesListPage
to filter viewable workspace. This solution ensures if a workspace did not appear in the workspaces list, it should not be accessible via direct URL.
WorkspaceInitialPage
and WorkspaceProfilePage
:Job added to Upwork: https://www.upwork.com/jobs/~021834598579747458653
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ntdiary (External
)
The link can open the workspace settings page and Members tab leads to not here page.
After the user deletes the workspace, the policy data still exists then the not found page doesn't display because this check doesn't cover full case like we did here
We should use the same check as we did here in shouldShow
here then that can fix for all policy pages
const shouldShowPolicy = useMemo(() => {
return PolicyUtils.shouldShowPolicy(policy, isOffline, session?.email);
}, [policy, isOffline, session?.email]);
const prevShouldShowPolicy = useMemo(() => {
return PolicyUtils.shouldShowPolicy(prevPolicy, isOffline, session?.email);
}, [prevPolicy, isOffline, session?.email]);
const shouldShow = useMemo(() => {
// If the policy object doesn't exist or contains only error data, we shouldn't display it.
if (((isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors))) && isEmptyObject(policyDraft)) || shouldShowNotFoundPage) {
return true;
}
// We check isPendingDelete for both policy and prevPolicy to prevent the NotFound view from showing right after we delete the workspace
return (!isEmptyObject(policy) && !PolicyUtils.isPolicyAdmin(policy) && !shouldShowNonAdmin) || (!shouldShowPolicy && !prevShouldShowPolicy);
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [policy, shouldShowNonAdmin, shouldShowPolicy, prevShouldShowPolicy]);
Then we can update the condition here like this to display the correct subtitle
shouldShowPolicy ? 'workspace.common.notAuthorized' : undefined
We can do the same fix for WorkspaceInitialPage
https://github.com/Expensify/App/blob/b69577a55bc536a411d8c1e51fc47fc0fc00d389/src/pages/workspace/WorkspaceInitialPage.tsx#L310
We can fix this from BE side that will not return the data of deleted policy
Pending review of proposal
@ntdiary, @srikarparsi, @joekaufmanexpensify Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!
@ntdiary can you review proposals here?
@ntdiary can you review proposals here?
Ah, @joekaufmanexpensify, sorry, under review. I missed this issue. π
Taking over this issue (slack), can you please assign me here @joekaufmanexpensify @srikarparsi
We ended up rolling the dice for this one since there was a lot of interest, and @ishpaul777 was selected, so going with them!
π£ @ishpaul777 π 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 π
Pending @ishpaul777 review of proposals
Looking π
@nkdengineer I dont understand the usage for prevShouldShowPolicy in your proposal could you clarify the usage please ?
PolicyUtils.isPendingDeletePolicy(policy) && PolicyUtils.isPendingDeletePolicy(prevPolicy)
@ishpaul777 Here is the old condition and we should replace it with shouldShowPolicy
function. That is why we have prevShouldShowPolicy
variable. I create a new variable because we need to use shouldShowPolicy
to display the correct subtitle.
subtitleKey={isEmptyObject(policy) ? undefined : 'workspace.common.notAuthorized'}
Thanks for clarifying @nkdengineer. π
I dont think there are any major difference between @nkdengineer solution and @gijoe0295's solution, since @gijoe0295 Proposed this first lets assign them
π π π C+ reviewed
Current assignee @srikarparsi is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.
π£ @gijoe0295 π 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 π
@gijoe0295 is there an ETA for PR here?
Will complete in 5 hours.
PR in review
PR still in review
Still in review, found a bug that is taking sometime to fix, @gijoe0295 is still looking into it
Sounds good!
PR still in review
This one's now held for https://github.com/Expensify/react-native-onyx/pull/588 , we'll finish up the PR once that's out.
@ishpaul777 @srikarparsi I see the above PR was merged and published. Can we take this off hold now?
yeah i have asked @gijoe0295 to retest and confirm few days back https://github.com/Expensify/App/pull/49509#issuecomment-2450831699
Great. TY!
Will retest today.
@gijoe0295 Did you have a chance to do this?
@gijoe0295 has asked some help from @fabioh8010, the investigation in ongoing https://github.com/Expensify/App/pull/49509
Sounds good. TY!
If you havenβt already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Version Number: 9.0.33-1 Reproducible in staging?: Y Reproducible in production?: N/A - new feature, doesn't exist in prod Issue was found when executing this PR: https://github.com/Expensify/App/pull/48275 Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Issue reported by: Applause Internal Team
Action Performed:
Precondition:
Expected Result:
The link should lead to not here page because workspace is deleted.
Actual Result:
The link can open workspace settings page and Members tab leads to not here page.
Workaround:
Unknown
Platforms:
Screenshots/Videos
https://github.com/user-attachments/assets/a1540862-f566-4c88-b999-caf2bdf3094f
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @ntdiary