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

[HOLD for payment 2024-11-11] Remove GBR from the account settings on initial signup #47863

Closed
techievivek opened this issue Aug 22, 2024 · 43 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@techievivek
Copy link
Contributor

techievivek commented Aug 22, 2024

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.
Screenshot 2024-08-22 at 7 29 17 PM

Problem

This step is an unnecessary diversion during the initial onboarding setup.
Screenshot 2024-08-22 at 7 29 10 PM
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.

Issue OwnerCurrent Issue Owner: @JmillsExpensify
@techievivek techievivek added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Aug 22, 2024
@techievivek techievivek self-assigned this Aug 22, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Aug 22, 2024
Copy link

melvin-bot bot commented Aug 22, 2024

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

Copy link

melvin-bot bot commented Aug 22, 2024

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
Copy link
Contributor

FitseTLT commented Aug 22, 2024

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

if (hasLoginListInfo(loginList)) {
return CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
}

What alternative solutions did you explore? (Optional)

@bernhardoj
Copy link
Contributor

bernhardoj commented Aug 22, 2024

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.

const contactMethodBrickRoadIndicator = UserUtils.getLoginListBrickRoadIndicator(loginList);

There are 2 cases. 1 if there is an error which will show the RBR.

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

And the other one if one of the contact methods is not validated yet which will show the GBR.

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

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.

if (hasLoginListInfo(loginList)) {
return CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
}

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
Copy link
Contributor

cretadn22 commented Aug 22, 2024

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

if (hasLoginListInfo(loginList)) {
return CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
}

and remove () => !!loginList && UserUtils.hasLoginListInfo(loginList)

const infoCheckingMethods: CheckingMethod[] = [() => !!loginList && UserUtils.hasLoginListInfo(loginList), () => SubscriptionUtils.hasSubscriptionGreenDotInfo()];

What alternative solutions did you explore? (Optional)

@Krishna2323
Copy link
Contributor

Krishna2323 commented Aug 22, 2024

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.

function hasLoginListInfo(loginList: OnyxEntry<LoginList>): boolean {
return !Object.values(loginList ?? {}).every((field) => field.validatedDate);
}
/**
* Gets the appropriate brick road indicator status for a given loginList.
* Error status is higher priority, so we check for that first.
*/
function getLoginListBrickRoadIndicator(loginList: OnyxEntry<LoginList>): LoginListIndicator {
if (hasLoginListError(loginList)) {
return CONST.BRICK_ROAD_INDICATOR_STATUS.ERROR;
}
if (hasLoginListInfo(loginList)) {
return CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
}
return undefined;
}

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.

const login = loginList?.[loginName];
const pendingAction = login?.pendingFields?.deletedLogin ?? login?.pendingFields?.addedLogin ?? undefined;
if (!login?.partnerUserID && !pendingAction) {
return null;
}
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) {
indicator = CONST.BRICK_ROAD_INDICATOR_STATUS.INFO;
}

        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

Monosnap.screencast.2024-08-22.19-50-33.mp4

@Krishna2323
Copy link
Contributor

Proposal Updated

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

@ntdiary
Copy link
Contributor

ntdiary commented Aug 23, 2024

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

Copy link

melvin-bot bot commented Aug 23, 2024

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

@Krishna2323
Copy link
Contributor

@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
Copy link
Contributor

FitseTLT commented Aug 23, 2024

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
Copy link
Contributor

ntdiary commented Aug 23, 2024

@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? 😂
image


@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. 😄

Copy link

melvin-bot bot commented Aug 26, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 26, 2024
@ntdiary
Copy link
Contributor

ntdiary commented Aug 27, 2024

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? 🙂

@melvin-bot melvin-bot bot removed the Overdue label Aug 27, 2024
@techievivek
Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link

melvin-bot bot commented Aug 30, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 30, 2024
@techievivek
Copy link
Contributor Author

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

@JmillsExpensify
Copy link

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
Copy link
Contributor Author

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

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 11, 2024
@Krishna2323
Copy link
Contributor

@ntdiary, PR ready for review ^

@techievivek
Copy link
Contributor Author

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

@melvin-bot melvin-bot bot added Monthly KSv2 and removed Weekly KSv2 labels Oct 18, 2024
Copy link

melvin-bot bot commented Oct 18, 2024

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
Copy link
Contributor

ntdiary commented Oct 21, 2024

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

Copy link

melvin-bot bot commented Oct 31, 2024

⚠️ 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.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 labels Nov 4, 2024
@melvin-bot melvin-bot bot changed the title Remove GBR from the account settings on initial signup [HOLD for payment 2024-11-11] Remove GBR from the account settings on initial signup Nov 4, 2024
Copy link

melvin-bot bot commented Nov 4, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Nov 4, 2024
Copy link

melvin-bot bot commented Nov 4, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.56-9 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-11-11. 🎊

For reference, here are some details about the assignees on this issue:

  • @ntdiary requires payment through NewDot Manual Requests
  • @Krishna2323 requires payment (Needs manual offer from BZ)

Copy link

melvin-bot bot commented Nov 4, 2024

@ntdiary @JmillsExpensify The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

Copy link

melvin-bot bot commented Nov 11, 2024

Payment Summary

Upwork Job

BugZero Checklist (@JmillsExpensify)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@JmillsExpensify
Copy link

Payment summary:

@ntdiary
Copy link
Contributor

ntdiary commented Nov 12, 2024

BugZero Checklist:

  • [Contributor] Classify the bug:
Bug classification

Source of bug:

  • 1a. Result of the original design (eg. a case wasn't considered)
  • 1b. Mistake during implementation
  • 1c. Backend bug
  • 1z. Other: new feature

Where bug was reported:

  • 2a. Reported on production
  • 2b. Reported on staging (deploy blocker)
  • 2c. Reported on a PR
  • 2z. Other:

Who reported the bug:

  • 3a. Expensify user
  • 3b. Expensify employee
  • 3c. Contributor
  • 3d. QA
  • 3z. Other:
  • [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

    Link to comment: N/A

  • [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

    Link to discussion: N/A

  • [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again. *
    Based on this comment, it seems that we already have a test in place, just need to update it. :)

Regression Test Proposal Template
  • [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

    Link to issue:

Regression Test Proposal

Precondition:

Test:

Do we agree 👍 or 👎

@Krishna2323
Copy link
Contributor

@JmillsExpensify, can you please send me an offer on Upwork? Thanks

@JmillsExpensify
Copy link

Sure thing! Doing that now.

@JmillsExpensify
Copy link

Offer sent.

@Krishna2323
Copy link
Contributor

Offer accepted :)

@JmillsExpensify
Copy link

All paid out. Thanks everyone! Pending NewDot payment can be requested, so I'm closing this one out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
No open projects
Status: Polish
Development

No branches or pull requests

9 participants