Closed lanitochka17 closed 1 week ago
Triggered auto assignment to @abekkala (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.
@abekkala FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors
We think that this bug might be related to #wave-collect - Release 1
Name RHP stays open while not found view is loading
After testing, it appears that this error is not limited to the profile page; it also occurs on the description page. There is currently no logic implemented to handle the not-found page scenario. https://github.com/Expensify/App/blob/68e69623a8bca0dce5f384fa351d5cbb2214aeb4/src/pages/workspace/WorkspaceProfileDescriptionPage.tsx#L70-L78 https://github.com/Expensify/App/blob/68e69623a8bca0dce5f384fa351d5cbb2214aeb4/src/pages/workspace/WorkspaceNamePage.tsx#L56-L64
We can implement it similarly to how we're handling the currency selector in the workspace. https://github.com/Expensify/App/blob/68e69623a8bca0dce5f384fa351d5cbb2214aeb4/src/pages/workspace/WorkspaceProfileCurrencyPage.tsx#L78-L83
We can do something like this
<FullPageNotFoundView
onBackButtonPress={PolicyUtils.goBackFromInvalidPolicy}
onLinkPress={PolicyUtils.goBackFromInvalidPolicy}
shouldShow={(isEmptyObject(policy))|| !PolicyUtils.isPolicyAdmin(policy) || PolicyUtils.isPendingDeletePolicy(policy)}
subtitleKey={isEmptyObject(policy) ? undefined : 'workspace.common.notAuthorized'}
>
Result
https://github.com/Expensify/App/assets/93134676/ab2c837a-4e96-41b7-97a5-54ca52e3790f
We need to test other areas to see if the bug occurs elsewhere as well. Regarding the loading indicator, we can utilize withPolicyAndFullscreenLoading if we decide to include it. However, currently, we're fine without it.
Job added to Upwork: https://www.upwork.com/jobs/~012247865ed0caacd0
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Pujan92 (External
)
@Pujan92 we have received a proposal for this one
The profile name RHP first closes and then re appears while the not found view is loading
We don't have any logic to guard the WorkspaceNamePage
here against unauthorized access.
This is a common use cases for which a dedicated wrapper AdminPolicyAccessOrNotFoundWrapper
has been built. It was used everywhere in the same RHP cases like WorkspaceTagsSettingsPage, WorkspaceCreateTaxPage, ...
Not only does it handle showing the correct NotFound based on user's access, but it also has correct goBack
logic when we click on back button here (if the workspace is accessible, go back to the workspace profile page, otherwise go to workspace list page). Which will not be available if we use a duplicated FullPageNotFoundView
ourselves.
We should use AdminPolicyAccessOrNotFoundWrapper
it in WorkspaceNamePage
and other pages that are missing it. If we need to customize the subtitle, onLinkPress
or other props, we can pass custom param to AdminPolicyAccessOrNotFoundComponent
and in AdminPolicyAccessOrNotFoundComponent
pass it to the underlying not found page.
For example, if we want to customize to use workspace.common.notAuthorized
as the subtitle if policy
exists but the user doesn't have admin access, like here, we can add a subtitleKey
prop to AdminPolicyAccessOrNotFoundWrapper
and pass it to FullPageNotFoundView
and NotFoundPage
inside. Or we can even make this behavior (custom message if policy exists but don't have access) a default behavior of AdminPolicyAccessOrNotFoundWrapper
, and don't need the custom prop.
(Besides the above, we might need to use withPolicyAndFullscreenLoading
to wrap the WorkspaceNamePage
similar to other pages that use AdminPolicyAccessOrNotFoundWrapper
)
For pages that currently use custom FullPageNotFoundView
instead of the standardized AdminPolicyAccessOrNotFoundWrapper
, like WorkspaceProfileCurrencyPage
, we can optionally refactor to use AdminPolicyAccessOrNotFoundWrapper
Thanks for the proposals.
Indeed, AdminPolicyAccessOrNotFoundWrapper
is the wrapper created specifically for this purpose. Using AdminPolicyAccessOrNotFoundWrapper
and customizing it for the subtitle
and other required fields(if any) makes more sense to me. Considering that, I am inclined to select @nkdengineer 's proposal over @Nodebrute's proposal.
For all workspace pages add AdminPolicyAccessOrNotFoundWrapper
where it is missing and replace FullPageNotFoundView
with AdminPolicyAccessOrNotFoundWrapper
to make it consistent across all workspace related pages.
πππ C+ reviewed
Triggered auto assignment to @rlinoz, see https://stackoverflow.com/c/expensify/questions/7972 for more details.
π£ @Pujan92 π An offer has been automatically sent to your Upwork account for the Reviewer role π Thanks for contributing to the Expensify app!
π£ @nkdengineer π 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 π
I'll raise PR soon
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.67-7 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-05-06. :confetti_ball:
For reference, here are some details about the assignees on this issue:
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:
friendly bump @Pujan92
I think a checklist is not needed here as none of the specific PR caused this.
@Pujan92 please accept the offer in Upwork and I can process payment - thanks!
@nkdengineer payment sent and contract ended - thank you! π
@abekkala Accepted, Thanks!
@Pujan92 payment sent and contract ended - thank you!
@rlinoz @abekkala Be sure to fill out the Contact List!
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: 1.4.63-0 Reproducible in staging?: Y Reproducible in production?: Y 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: Slack conversation:
Issue found when executing PR https://github.com/Expensify/App/pull/36409
Action Performed:
Expected Result:
The profile name RHP should close while not found view is loading
Actual Result:
The profile name RHP first closes and then re appears while the not found view is loading
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
https://github.com/Expensify/App/assets/78819774/4f27c10f-e630-4dad-94d3-3a02b07739e1
View all open jobs on GitHub
Upwork Automation - Do Not Edit