Skip to content
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

[$1000] Web - Workspace - No loading indicator is shown on WS screen when navigating via invite link #37959

Closed
1 of 6 tasks
kbecciv opened this issue Mar 8, 2024 · 59 comments
Closed
1 of 6 tasks
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Improvement Item broken or needs improvement.

Comments

@kbecciv
Copy link

kbecciv commented Mar 8, 2024

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.49-0
Reproducible in staging?: y
Reproducible in production?: n
Issue found when executing PR: #37525
Issue reported by: Applause - Internal Team

Action Performed:

Preconditions:

  1. Has a workspace (new or existing one)
  2. Prepare join url link - use WorkspaceID and admin email for that workspace - ex. https://staging.new.expensify.com/workspace/7DE6960B26D79E36/join?email=tenema5675@bizatop.com
    For user:
  3. Open link in a new tab (for a new user / or user which already logged in)

The expected behaviour of OP (online case) is:

  • Never show loading indicator
  • Invited workspace item is immediately shown (loading, success cases)
  • If fails, gray out with RBR (failure case)

Expected Result:

A loading indicator should be shown

Actual Result:

No loading indicator is shown on all Workspace screen

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

Bug6406682_1709887302660.NoLoadingBug.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01e403ef173151cba4
  • Upwork Job ID: 1766065429321519104
  • Last Price Increase: 2024-04-23
@kbecciv kbecciv added the DeployBlockerCash This issue or pull request should block deployment label Mar 8, 2024
Copy link
Contributor

github-actions bot commented Mar 8, 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 Mar 8, 2024

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

@kbecciv
Copy link
Author

kbecciv commented Mar 8, 2024

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

@Julesssss Julesssss added Daily KSv2 Hourly KSv2 External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. and removed Hourly KSv2 DeployBlockerCash This issue or pull request should block deployment Daily KSv2 labels Mar 8, 2024
@melvin-bot melvin-bot bot changed the title Web - Workspace - No loading indicator is shown on WS screen when navigating via invite link [$500] Web - Workspace - No loading indicator is shown on WS screen when navigating via invite link Mar 8, 2024
Copy link

melvin-bot bot commented Mar 8, 2024

Job added to Upwork: https://www.upwork.com/jobs/~01e403ef173151cba4

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

melvin-bot bot commented Mar 8, 2024

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

@Julesssss
Copy link
Contributor

Removing the blocker label as this is a minor issue.

@tienifr
Copy link
Contributor

tienifr commented Mar 8, 2024

Proposal

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

No loading indicator is shown on all Workspace screen

What is the root cause of that problem?

In here, we're navigating back to all-settings page before the API request to join the workspace completes and returns the workspace data.

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

  1. We can check if policy?.isJoinRequestPending from false becomes true then we do the navigateAfterJoinRequest.
useEffect(() => {
        if (!prevPolicy?.isJoinRequestPending && policy?.isJoinRequestPending) {
            Navigation.isNavigationReady().then(() => {
                if (isUnmounted.current) {
                    return;
                }

                navigateAfterJoinRequest();
            });
        }
    }, [policy]);
  1. Then we can remove the existing navigateAfterJoinRequest logic here

We also need to decide the behavior where the user already opens the request link before, currently we'll navigate the user back to the all-settings page, if we want to keep the same behavior, we should add here

if (policy?.isJoinRequestPending) {
    Navigation.isNavigationReady().then(() => {
        if (isUnmounted.current) {
            return;
        }
        navigateAfterJoinRequest();
    });
}

(if we want to go with this, we can perhaps remove the prevPolicy?.isJoinRequestPending check above and only checks for policy?.isJoinRequestPending, should be enough)
or if we want to goBack like in the case where the user is an existing member, we can add condition here

|| policy?.isJoinRequestPending

If offline mode, I personally think the current infinite loading is fine because we have similar patterns when loading a completely new report in offline mode, or a workspace (try deeplinking to a random workspace profile whose workspace is not in Onyx, while offline). But if we want to show the full screen offline blocking view as suggested here, we can just use FullPageOfflineBlockingView.

What alternative solutions did you explore? (Optional)

In here, set isLoading to true (then back to false in success/failure data).

And in WorkspaceJoinUserPage, only run navigateAfterJoinRequest after isLoading changes from true to false (in an useEffect), we can check that the value changes from true to false by using usePrevious

Out of scope but I see currently the case where there's an error of JoinWorkspaceViaInviteLink is not explicitly handled, instead the user just navigates to the all-settings page. If we want to keep the same behavior, in the useEffect in 1 above, if there's policyJoinMember.errors (we need to connect to the policyJoinMember_${policyId} Onyx Key), we will also trigger the navigateAfterJoinRequest. If we want to show an error to the user, we will need a design of where and how we want to show the error.

.

@brandonhenry
Copy link
Contributor

brandonhenry commented Mar 8, 2024

Proposal

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

The WorkspacesListPage component does not show a loading screen when a workspace join request is in progress. Instead, it displays the "Create a new workspace" section, which should be hidden during the loading state.

What is the root cause of that problem?

The root cause of the problem is that the WorkspacesListPage component does not have access to the status of the join request. It is not aware of whether a join request is currently pending, so it cannot determine when to show the loading screen and hide the "Create a new workspace" section.

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

To solve the problem, we can make the following changes to the WorkspacesListPage component:

  1. Update the WorkspaceListPageOnyxProps type to include the policyJoinMember key:

    type WorkspaceListPageOnyxProps = {
        // ...
        policyJoinMember: OnyxCollection<{
            policyID: string;
            inviterEmail: string;
            errors?: OnyxCommon.OnyxError;
        }>;
    };

    Link to code

  2. Update the withOnyx call to include the policyJoinMember key:

    export default withPolicyAndFullscreenLoading(
        withOnyx<WorkspaceListPageProps, WorkspaceListPageOnyxProps>({
            // ...
            policyJoinMember: {
                key: ONYXKEYS.COLLECTION.POLICY_JOIN_MEMBER,
            },
        })(WorkspacesListPage),
    );

    Link to code

  3. Modify the WorkspacesListPage component to show a loading screen when a join request is in progress and there are no workspaces:

    function WorkspacesListPage({
        policies,
        allPolicyMembers,
        reimbursementAccount,
        reports,
        policyJoinMember,
    }: WorkspaceListPageProps) {
        // ...
    
       if (!isEmptyObject(policyJoinMember) && isEmptyObject(workspaces)){
         return (
             <ScreenWrapper 
                 includeSafeAreaPaddingBottom={false}
                 shouldEnablePickerAvoiding={false}
                 shouldEnableMaxHeight
                 testID={WorkspacesListPage.displayName}
                 shouldShowOfflineIndicatorInWideScreen
             >
                 <HeaderWithBackButton
                         title={translate('common.workspaces')}
                         shouldShowBackButton={isLessThanMediumScreen}
                         onBackButtonPress={() => Navigation.goBack()}
                     />
                 <FullScreenLoadingIndicator style={[styles.flex1, styles.pRelative]} />
             </ScreenWrapper>
         )
     }
    
     // ... (existing code for empty / current workspace list)

    Link to code

This achieves what we are looking for in the WorkspacesListPage component

Screen.Recording.2024-03-08.at.5.20.55.PM.mov

What alternative solutions did you explore? (Optional)

An alternative solution could be to pass the join request status as a prop to the WorkspacesListPage component from the parent component or screen. However, this would require additional changes to the parent component and may not be as clean as accessing the status directly from the Onyx state using withOnyx.

cc. @mananjadhav @Julesssss

@ahmedGaber93
Copy link
Contributor

ahmedGaber93 commented Mar 9, 2024

Proposal

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

Web - Workspace - No loading indicator is shown on WS screen when navigating via invite link

What is the root cause of that problem?

we already have FullScreenLoadingIndicator inside WorkspaceJoinUserPage.tsx.
but after we calling the api, we redirect immeditally to WorkspacesListPage screen before getting the api response with workspace Policy date, and the onxy optimisticData not enough because it related to POLICY_JOIN_MEMBER not POLICY and not have workspace data.

PolicyAction.inviteMemberToWorkspace(policyID, inviterEmail);
isJoinLinkUsed = true;
Navigation.isNavigationReady().then(() => {

const optimisticMembersState = {policyID, inviterEmail};

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

we should wait to the api response then navigate by the two steps below:

  1. add condition if(policy?.isJoinRequestPending) surround navigate method here.

  2. the step one not work alone because inside this useEffect while the policy change from {} to backend response, the isJoinLinkUsed is changed to true, and this will make useEffect return early and not navigate.

    useEffect(() => {
    if (!policy || !policies || isUnmounted.current || isJoinLinkUsed) {
    return;
    }

isJoinLinkUsed is designed to call the api only one time not multipes times when deps change, so to fix this we need to remove it from the begining of useEffect and put it surround the api call

// if (!policy || !policies || isUnmounted.current || isJoinLinkUsed) {
if (!policy || !policies || isUnmounted.current) {

// PolicyAction.inviteMemberToWorkspace(policyID, inviterEmail);
if (!isJoinLinkUsed) {
    PolicyAction.inviteMemberToWorkspace(policyID, inviterEmail);
    isJoinLinkUsed = true;
}
result
Screen.Recording.2024-03-09.at.11.25.36.AM.mov

handle offline mode

this requirement depond on what design team decide, but mainly we have a lot of option.

  1. when user is offline the screen will infinite loop and no offline indicator is displayed.
    we can fix this by display the offline indicator by add shouldShowOfflineIndicatorInWideScreen to ScreenWarrper here.
    and use const {isOffline} = useNetwork(); to get network state and use it in useEffect deps to request api and complete proccess when back online, we need also to add !isOffline in this condition if (!isJoinLinkUsed) { in the main proposal .
    in this case the screen still loading but we show offline indicator in the bottom of screen.
result

Screenshot 2024-03-17 at 10 26 53 PM

  1. return early FullPageOfflineBlockingView or its content depond on this condition (isOffline && !isPolicyMember && !!policy?.isJoinRequestPending)

  2. can call the api and conditionly navigate to WorkspacesListPage(only in offline) but in this case the pending workspace item not displayed util the network back online.

What alternative solutions did you explore? (Optional)

we can optionaly handle errors by POLICY_JOIN_MEMBER and also add case for offline mode.

@brandonhenry
Copy link
Contributor

not sure, but i feel like ahmed's proposal may cause regressions.

@melvin-bot melvin-bot bot added the Overdue label Mar 11, 2024
@mananjadhav
Copy link
Collaborator

Still going through the proposals, but I am still haven't checked why it's not reproducible on production only on staging. Can someone highlight that in the proposals?

@melvin-bot melvin-bot bot removed the Overdue label Mar 11, 2024
@mananjadhav
Copy link
Collaborator

Taking this back as per message

@melvin-bot melvin-bot bot removed the Overdue label Mar 23, 2024
@Julesssss
Copy link
Contributor

Hey @mananjadhav, any thoughts on my comment? #37959 (comment)

Copy link

melvin-bot bot commented Mar 29, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Mar 29, 2024
@Julesssss
Copy link
Contributor

Okay, so we're still seeking proposals again

@melvin-bot melvin-bot bot removed the Overdue label Apr 2, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Apr 5, 2024
Copy link

melvin-bot bot commented Apr 5, 2024

@mananjadhav, @Julesssss Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@Julesssss
Copy link
Contributor

Still waiting...

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Apr 8, 2024
@Julesssss
Copy link
Contributor

I'll be back in a week to check for proposals again

@melvin-bot melvin-bot bot removed the Overdue label Apr 11, 2024
Copy link

melvin-bot bot commented Apr 12, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@melvin-bot melvin-bot bot added the Overdue label Apr 15, 2024
Copy link

melvin-bot bot commented Apr 16, 2024

@mananjadhav, @Julesssss Huh... This is 4 days overdue. Who can take care of this?

Copy link

melvin-bot bot commented Apr 18, 2024

@mananjadhav, @Julesssss Still overdue 6 days?! Let's take care of this!

Copy link

melvin-bot bot commented Apr 19, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

Copy link

melvin-bot bot commented Apr 22, 2024

@mananjadhav, @Julesssss 10 days overdue. Is anyone even seeing these? Hello?

@Julesssss
Copy link
Contributor

I'm going to bump to $1000 as nobody has commented in a month

@melvin-bot melvin-bot bot removed the Overdue label Apr 23, 2024
@Julesssss Julesssss changed the title [$500] Web - Workspace - No loading indicator is shown on WS screen when navigating via invite link [$1000] Web - Workspace - No loading indicator is shown on WS screen when navigating via invite link Apr 23, 2024
Copy link

melvin-bot bot commented Apr 23, 2024

Upwork job price has been updated to $1000

@brandonhenry
Copy link
Contributor

any feedback on my proposal? I have accurate root cause and solution: #37959 (comment)

Cc. @Julesssss @mananjadhav @mkhutornyi

@Julesssss
Copy link
Contributor

Hey @brandonhenry. Looking back I think this was the last comment about your proposal.

@Julesssss
Copy link
Contributor

Anyway, after retesting I believe the current behaviour is desired, and this issue can be closed. We're matching this expected behaviour:

test1.mov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Improvement Item broken or needs improvement.
Projects
None yet
Development

No branches or pull requests