Remove GBR from the account settings on initial signup

techievivek commented 2 months ago

Based on the discussion here:https://expensify.slack.com/archives/C07HPDRELLD/p1724334488573109?thread_ts=1724252245.683699&cid=C07HPDRELLD, we've decided to remove the GBR from the account settings that direct users to validate their login.

Problem

This step is an unnecessary diversion during the initial onboarding setup.

Account validation is only required for setting up 2FA and for wallet and bank account-related processes, where we have the validation path integrated into those specific flows.

Solution

Remove the GBR from the account settings that users encounter when signing up on NewDot.

melvin-bot[bot] commented 2 months ago

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

melvin-bot[bot] commented 2 months ago

Triggered auto assignment to @JmillsExpensify (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.

FitseTLT commented 2 months ago

Proposal

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

Remove GBR from the account settings on initial signup

What is the root cause of that problem?

New change : Remove GBR for initial sign up

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

Remove this code https://github.com/Expensify/App/blob/bf5d7431ac4ea662215a3ae05268f880e63f7ccd/src/libs/UserUtils.ts#L60-L62

What alternative solutions did you explore? (Optional)

bernhardoj commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-22 14:15:43 UTC.

Proposal

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

We don't want to show the GBR for new unvalidated user.

What is the root cause of that problem?

The GBR will show if the brick road indicator is returned from getLoginListBrickRoadIndicator. https://github.com/Expensify/App/blob/ed04ac47da2194e279d4a4a6225280f755148bda/src/pages/settings/Profile/ProfilePage.tsx#L71

There are 2 cases. 1 if there is an error which will show the RBR. https://github.com/Expensify/App/blob/ed04ac47da2194e279d4a4a6225280f755148bda/src/libs/UserUtils.ts#L56-L63

And the other one if one of the contact methods is not validated yet which will show the GBR. https://github.com/Expensify/App/blob/ed04ac47da2194e279d4a4a6225280f755148bda/src/libs/UserUtils.ts#L48-L50

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

We can remove the validated status check here (and on other components), so GBR won't show if there is a non-validated contact method. https://github.com/Expensify/App/blob/ed04ac47da2194e279d4a4a6225280f755148bda/src/libs/UserUtils.ts#L60-L62

login A: validated login B: non-validated

Before: GBR shows

After: No GBR

OR

We can modify hasLoginListInfo to always return false if the login list is only 1, otherwise, use the current logic.

cretadn22 commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-22 14:23:06 UTC.

Proposal

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

Remove GBR from the account settings on initial signup

What is the root cause of that problem?

New feature

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

Remove these lines https://github.com/Expensify/App/blob/ed04ac47da2194e279d4a4a6225280f755148bda/src/libs/UserUtils.ts#L60-L62

and remove () => !!loginList && UserUtils.hasLoginListInfo(loginList) https://github.com/Expensify/App/blob/ed04ac47da2194e279d4a4a6225280f755148bda/src/components/Indicator.tsx#L66

What alternative solutions did you explore? (Optional)

Krishna2323 commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-09-03 14:02:45 UTC.

Proposal

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

Remove GBR from the account settings on initial signup

What is the root cause of that problem?

When hasLoginListInfo is true, we show the RBR. https://github.com/Expensify/App/blob/ed04ac47da2194e279d4a4a6225280f755148bda/src/libs/UserUtils.ts#L48-L64

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

We should accept the current session in getLoginListBrickRoadIndicator, then pass it to hasLoginListInfo.

function getLoginListBrickRoadIndicator(loginList: OnyxEntry<LoginList>, session: OnyxEntry<Session>): LoginListIndicator {
if (hasLoginListError(loginList)) {
    return CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
}
if (hasLoginListInfo(loginList, session)) {
    return CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
}
return undefined;
}

In hasLoginListInfo, we should filter out the current user and the run .every.

function hasLoginListInfo(loginList: OnyxEntry<LoginList>, session: OnyxEntry<Session>): boolean {
return !Object.values(loginList ?? {})
    .filter((field) => session?.email !== field.partnerUserID)
    .every((field) => field.validatedDate);
}

If we also want to hide the GBR on contact methods page then we can check if the login is a default contact method or not and if so, we will not add the GBR.

https://github.com/Expensify/App/blob/2d1f8cd1cb072f0b9c9672effa65199727553ab9/src/pages/settings/Profile/Contacts/ContactMethodsPage.tsx#L48-L67

        const isDefaultContactMethod = session?.email === login?.partnerUserID;
        let description = '';
        if (session?.email === login?.partnerUserID) {
            description = translate('contacts.getInTouch');
        } else if (login?.errorFields?.addedLogin) {
            description = translate('contacts.failedNewContact');
        } else if (!login?.validatedDate) {
            description = translate('contacts.pleaseVerify');
        }
        let indicator;
        if (Object.values(login?.errorFields ?? {}).some((errorField) => !isEmptyObject(errorField))) {
            indicator = CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
        } else if (!login?.validatedDate && !isDefaultContactMethod) {
            indicator = CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
        }

Optional: We can subscribe to onyx to get the current session.

What alternative solutions did you explore? (Optional) 

Result

https://github.com/user-attachments/assets/a7f80b54-a2c1-4f3e-9d9d-fcb845ebd109

Krishna2323 commented 2 months ago

Proposal Updated

No change in solution or RCA, just added the result video.

ntdiary commented 2 months ago

seems like a simple problem, I would approve @bernhardoj's proposal, because they are point out the hasLoginListInfo function, and then suggested removing it from other components.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 2 months ago

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

Krishna2323 commented 2 months ago

@ntdiary, I guess we only need to remove the GBR which is shown on initial signup. Removing hasLoginListInfo will never show the GRB when user has another unvalidated account.

we've decided to remove the GBR from the account settings that direct users to validate their login.

FitseTLT commented 2 months ago

seems like a simple problem,

@ntdiary Yeah it is a simple and straightforward problem with simple fix that's why I am giving clear simple proposal I don't know why you are not selecting my proposal which has the same solution as the selected proposal but was the first. cc @techievivek

ntdiary commented 2 months ago

@ntdiary, I guess we only need to remove the GBR which is shown on initial signup. Removing hasLoginListInfo will never show the GRB when user has another unvalidated account.

@Krishna2323, interesting guess, I previously thought the three GBRs in the OP would be completely removed. I can't see the Slack discussion in the OP, so maybe @techievivek can help clarify this behavior further. Also, if the Contact Method Page remains as it is, this means that when there is only one GBR on this page, the three GBRs above won't be displayed, However, if I add a new contact method, those three GBRs will appear. Is this behavior expected? 😂

@ntdiary Yeah it is a simple and straightforward problem with simple fix that's why I am giving clear simple proposal I don't know why you are not selecting my proposal which was the first.

@FitseTLT, because your proposal only mentioned modifying the getLoginListBrickRoadIndicator function and didn't mention the Avatar Indicator. When I reviewed this issue, there were already four proposals, so I chose the first one that seemed more comprehensive and meaningful to me. Of course, this is just my personal opinion. In the past, some issues were assigned to whoever responded first, but I'm not sure if that applies here, we can leave it up to @techievivek to decide. 😄

melvin-bot[bot] commented 2 months ago

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

ntdiary commented 2 months ago

Hi @techievivek, when you have some time, could you clarify this case: If a user successfully signs up and then adds a new contact method, should we display the three GBRs in the OP? 🙂

techievivek commented 2 months ago

That's a good question. I'm not entirely sure what the expected behavior should be. On the one hand, since the user's primary login still needs to be verified, it doesn't make sense to display the GBR to direct the user to verify another login, as verifying this login would make it their primary login. However, on the other hand, we might still want to show the GBR because the user has voluntarily added this login method and may want to validate it.

CC @JmillsExpensify @danielrvidal for thoughts on the above.

danielrvidal commented 2 months ago

If someone adds a secondary login I think it makes sense to add the GBR to all the logins so they know they need to verify them. By that chance, they are taking actions to use us, and thus, we're not as worried about making sure they only see GBRs for onboarding. Curious if @JmillsExpensify agrees on that one?

melvin-bot[bot] commented 2 months ago

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

techievivek commented 2 months ago

Not overdue, reached out to Jason for his thoughts on above point.

JmillsExpensify commented 2 months ago

Apologies for missing this. I agree with ya'll that we should show GBR for the case where a user has explicitly added a secondary login, as they can't use that until it's verified.

techievivek commented 2 months ago

Ok, thanks for confirming the behavior here. @ntdiary we should be good to move forward now.

ntdiary commented 2 months ago

Ok, thanks for confirming the behavior here. @ntdiary we should be good to move forward now.

@techievivek, Thank you! Then, to summarize, if I understand correctly, when the user signs up for the first time, we want all four GBRs in the image below to be removed. And if the user explicitly adds a second contact method, these four GBRs will be shown, right? 😄 (or more precisely, there will be five because of the two contact methods)

If that's right, @Krishna2323's proposal is the closest, but the contact methods page still isn't meeting the expected result.

Krishna2323 commented 2 months ago

@ntdiary, I think we should keep the GBR on the contact method page even for the primary login to validate.

I have updated my proposal if we want to hide the GBR on the contact method page also.

ntdiary commented 1 month ago

I have updated my proposal if we want to hide the GBR on the contact method page also.

I think we can move forward with @Krishna2323's proposal.

cc @techievivek

Additionally, just a tiny suggestion, I think using every could be enough? :)

!Object.values(loginList ?? {})
        .filter((field) => session?.email !== field.partnerUserID)
        .every((field) => field.validatedDate);

==>

!Object.values(loginList ?? {})
        .every((field) => field.validatedDate || (session?.email === field.partnerUserID));

melvin-bot[bot] commented 1 month ago

@JmillsExpensify @ntdiary @techievivek 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!

techievivek commented 1 month ago

Then, to summarize, if I understand correctly, when the user signs up for the first time, we want all four GBRs in the image below to be removed. And if the user explicitly adds a second contact method, these four GBRs will be shown, right? 😄 (or more precisely, there will be five because of the two contact methods)

Yes, we only want to remove the GBR during the initial signup. Once the user starts exploring the app and adding things explicitly, it should function as normal.

techievivek commented 1 month ago

@Krishna2323 Assigned the GH to you, can you get started with the PR?

Krishna2323 commented 1 month ago

@techievivek, thanks for assigning. I will try my best to raise the PR today, but if not, I will definitely raise it by tomorrow.

Krishna2323 commented 1 month ago

@ntdiary, PR ready for review ^

techievivek commented 1 month ago

Looks like we haven't made a progress for a while here, any updates?

melvin-bot[bot] commented 2 weeks ago

This issue has not been updated in over 15 days. @JmillsExpensify, @ntdiary, @techievivek, @Krishna2323 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

ntdiary commented 1 week ago

PR #49445 that we’ve been waiting for was merged today, so we'll continue to push forward with our PR #49004 later. :)

melvin-bot[bot] commented 2 days ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Expensify / App