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

[$250] There is no way to identify the primary workspace, making it appear essentially random #52373

Open
1 of 8 tasks
m-natarajan opened this issue Nov 12, 2024 · 47 comments
Open
1 of 8 tasks
Assignees
Labels
Daily KSv2 Design External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@m-natarajan
Copy link

m-natarajan commented Nov 12, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!


Version Number: 9.0.60-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught on HybridApp, is this reproducible on New Expensify Standalone?:
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: @quinthar
Slack conversation (hyperlinked to channel name): ts_external_expensify_retain

Action Performed:

Prerequisite: User have multiple workspaces

  1. Go to staging.new.expensify.com
  2. Click Settings > Workspaces

Expected Result:

Add some indicator in the workspace list as to which is primary
Add some ability to make a given workspace primary

Actual Result:

There's no way to know which workspace is primary, nor control it, so it's essentially random.

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Standalone
  • Android: HybridApp
  • Android: mWeb Chrome
  • iOS: Standalone
  • iOS: HybridApp
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Snip - (1) New Expensify - Google Chrome (2)

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021857052923539231166
  • Upwork Job ID: 1857052923539231166
  • Last Price Increase: 2024-11-21
  • Automatic offers:
    • Krishna2323 | Contributor | 105026569
Issue OwnerCurrent Issue Owner: @Krishna2323
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Nov 12, 2024
Copy link

melvin-bot bot commented Nov 12, 2024

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

@Krishna2323
Copy link
Contributor

Krishna2323 commented Nov 12, 2024

Proposal


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

There is no way to identify the primary workspace, making it appear essentially random

What is the root cause of that problem?

New feature

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


  • We can add one more badge to indicate the active policy.
  • We can get activePolicyID from useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID); and use the condition activePolicyID === policyID to display the badge.
  • We might need to create translations for the badge text and we can discuss the icon in the PR.
    {!!isJoinRequestPending && (
    <View style={[styles.flexRow, styles.gap2, styles.alignItemsCenter, styles.justifyContentEnd]}>
    <Badge
    text={translate('workspace.common.requested')}
    textStyles={styles.textStrong}
    badgeStyles={[styles.alignSelfCenter, styles.badgeBordered]}
    icon={Expensicons.Hourglass}
    />
    </View>
    )}
  • Then we can add an option to make a policy default in WorkspacesListPage and create util function for updating the onyx values (NVP_ACTIVE_POLICY_ID) and making the api call. We might only want to show the Make default option only if isOwner is true, we can use the condition if (isOwner) { or we can add the option without any condition.
    if (isOwner) {
    threeDotsMenuItems.push({
    icon: Expensicons.Trashcan,
    text: translate('workspace.common.delete'),
    onSelected: () => {
    setPolicyIDToDelete(item.policyID ?? '-1');
    setPolicyNameToDelete(item.title);
    setIsDeleteModalOpen(true);
    },
    shouldCallAfterModalHide: true,
    });
    }
  • We can use theme.success for badge border.

Note

I don't think there is an api endpoint used in new dot to update the NVP_ACTIVE_POLICY_ID, so we need to add the api endpoint and the types for that.

What alternative solutions did you explore? (Optional)

Result

@maddylewis maddylewis moved this to Product (CRITICAL) in [#whatsnext] #retain Nov 12, 2024
@twisterdotcom twisterdotcom added NewFeature Something to build that is a new item. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Nov 12, 2024
Copy link

melvin-bot bot commented Nov 12, 2024

Current assignee @twisterdotcom is eligible for the NewFeature assigner, not assigning anyone new.

Copy link

melvin-bot bot commented Nov 12, 2024

Triggered auto assignment to @dubielzyk-expensify (Design), see these Stack Overflow questions for more details.

@twisterdotcom
Copy link
Contributor

Adding Design to show exactly how we want this to appear based on the outcome of the Slack thread here: https://expensify.slack.com/archives/C07NZ8B1VTQ/p1731356570733819?thread_ts=1731353082.160469&cid=C07NZ8B1VTQ

@dubielzyk-expensify
Copy link
Contributor

dubielzyk-expensify commented Nov 13, 2024

Extending @dannymcclain's mock to add in the rest of the things asked in the Slack thread:

image
image

Tooltip on badge hover for extra clarification:

image

cc @jamesdeanexpensify for copy 👀 on the tooltip

cc @Expensify/design

@shawnborton
Copy link
Contributor

Looks great to me!

@daledah
Copy link
Contributor

daledah commented Nov 13, 2024

Edited by proposal-police: This proposal was edited at 2024-11-13 10:29:53 UTC.

Proposal

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

There's no way to know which workspace is primary, nor control it, so it's essentially random.

What is the root cause of that problem?

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

A. Display the badge "Default" and its tooltip "This workspace will be used when sending receipts to [email protected]"

  • In WorkspacesListRow, introduce new param, isDefault?: boolean.

  • Then in here, display the badge and tooltip if isDefault is true:

            {!!isDefault && (
                <Tooltip
                    maxWidth={200}
                    text={'This workspace will be used when sending receipts to [email protected]'}
                >
                    <View style={[styles.flexRow, styles.gap2, styles.alignItemsCenter, styles.justifyContentEnd]}>
                        <Badge
                            text={'Default'}
                            textStyles={styles.textStrong}
                            badgeStyles={[styles.alignSelfCenter, styles.badgeBordered]}
                        />
                    </View>
                </Tooltip>
            )}
    const [activePolicyID] = useOnyx(ONYXKEYS.NVP_ACTIVE_POLICY_ID);

and when using WorkspacesListRow, use activePolicyID as:

                                isDefault={item.policyID === activePolicyID}
  • Result:

image

B. Display the option "Set as default workspace"

  • In WorkspacesListPage, add the "Set as default workspace" if the current policyID is not activePolicyID and current user is owner. We can consider remove the isOwner from this condition because the activePolicyID is specific for each account. But it can be decided later:
            if (item.policyID !== activePolicyID && isOwner) {
                threeDotsMenuItems.push({
                    icon: Expensicons.Star,
                    text: 'Set as default workspace',
                    onSelected: () => setPolicyAsDefault(item.policyID)
                });
            }
  • Result:

image

C. Handle setPolicyAsDefault

  • Note: BE needs to create the API to do it to keep the consistency.

let activePolicyID: OnyxEntry<string>;
Onyx.connect({
    key: ONYXKEYS.NVP_ACTIVE_POLICY_ID,
    callback: (val) => {
        activePolicyID = val;
    },
});
function setPolicyAsDefault(policyID: string) {
    const optimisticData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: ONYXKEYS.NVP_ACTIVE_POLICY_ID,
            value: policyID,
        },
    ];

    const failureData: OnyxUpdate[] = [
        {
            onyxMethod: Onyx.METHOD.MERGE,
            key: ONYXKEYS.NVP_ACTIVE_POLICY_ID,
            value: activePolicyID,
        },
    ];

    const parameters = {
        policyID,
    };

    API.write('api-to-set-policy-as-default', parameters, {
        optimisticData,
        failureData,
    });
}

D. Always move the "Default" workspace to top.

  • We can update this sort logic:

.sort((a, b) => localeCompare(a.title, b.title));

so that if the report has item.policyID === activePolicyID, at is always moved to top.

What alternative solutions did you explore? (Optional)

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 13, 2024
@dannymcclain
Copy link
Contributor

Lovely, thanks @dubielzyk-expensify!

@jamesdeanexpensify
Copy link
Contributor

Getting final votes on copy here.

@jamesdeanexpensify
Copy link
Contributor

jamesdeanexpensify commented Nov 13, 2024

For the tooltip copy when you hover over the Default badge, let's go with:

Receipts sent to [email protected] will appear in this workspace.

@dubielzyk-expensify
Copy link
Contributor

image

@jamesdeanexpensify
Copy link
Contributor

Looks good!

@twisterdotcom twisterdotcom added the External Added to denote the issue can be worked on by a contributor label Nov 14, 2024
Copy link

melvin-bot bot commented Nov 14, 2024

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

@melvin-bot melvin-bot bot changed the title There is no way to identify the primary workspace, making it appear essentially random [$250] There is no way to identify the primary workspace, making it appear essentially random Nov 14, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 14, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Nov 22, 2024
@grgia
Copy link
Contributor

grgia commented Nov 22, 2024

Assigned, thank you!

Copy link

melvin-bot bot commented Nov 22, 2024

📣 @Krishna2323 🎉 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 📖

@Krishna2323
Copy link
Contributor

@grgia, what will the api endpoint to update the active policy id? For now I added UPDATE_ACTIVE_POLICY: 'UpdateActivePolicy', in the WRITE_COMMANDS file.

@grgia, we need your help 🙇🏻. Could you please check this comment when you have some time? Thanks!

@grgia
Copy link
Contributor

grgia commented Nov 29, 2024

@Krishna2323 I was under the impression no BE changes were required for this

@Krishna2323
Copy link
Contributor

@grgia, we don't have the API endpoint to update the default policy in NewDot, so we need the endpoint. It might already exist in the backend.

@grgia
Copy link
Contributor

grgia commented Dec 3, 2024

update the default policy in NewDot

@Krishna2323, are you saying we need to add a new endpoint UpdateActivePolicy?

I can look into the existing oldDot command if needed.

What would be returned?

@Krishna2323
Copy link
Contributor

Krishna2323 commented Dec 4, 2024

are you saying we need to add a new endpoint UpdateActivePolicy?

@grgia, yeah, we don't have any endpoint to update active policyID in NewDot.

What would be returned?

it should accept policyID and return:

{
    "jsonCode": 200,
    "requestID": "8ec7939cea8159d6-DEL",
    "onyxData": [
        {
            "key": "nvp_expensify_activePolicyID",
            "onyxMethod": "merge",
            "value": "newActivePolicyID"
        }
    ],
    "previousUpdateID": 3240252142,
    "lastUpdateID": 3240911729
}

@Krishna2323
Copy link
Contributor

@grgia, please let us know the next steps when you are available. Thanks!

@grgia
Copy link
Contributor

grgia commented Dec 9, 2024

Ah gotcha, so we just need the API call for setting the NVP? I can make a BE issue

@Krishna2323
Copy link
Contributor

so we just need the API call for setting the NVP?

yes

@grgia
Copy link
Contributor

grgia commented Dec 11, 2024

BE issue created here - https://github.com/Expensify/Expensify/issues/452492
I may be able to knock this one out today

cc @Krishna2323

@Krishna2323
Copy link
Contributor

Thanks @grgia 🙌🏻

@grgia
Copy link
Contributor

grgia commented Dec 11, 2024

Hey @Krishna2323 I was looking into it and it seems like we could use the SetNameValuePair command here, can you take a look at that option before I go further? I can still push a PR if not

@Krishna2323
Copy link
Contributor

@grgia SetNameValuePair is working well for active policy updates🎉. Can you confirm if there will be any issues when changing the value type to boolean | string?

type SetNameValuePairParams = {
name: string;
value: boolean;
};
export default SetNameValuePairParams;

@grgia
Copy link
Contributor

grgia commented Dec 12, 2024

@Krishna2323 why would we set the ActivePolicyID as a boolean?

@grgia
Copy link
Contributor

grgia commented Dec 12, 2024

Oh 🤦 lol we're adding string. Yes you can send a string to this command

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Dec 18, 2024
@grgia
Copy link
Contributor

grgia commented Jan 3, 2025

Hey @Krishna2323 can you please prioritize this one so we can wrap it up? thanks!

@grgia grgia added Daily KSv2 and removed Weekly KSv2 Reviewing Has a PR in review labels Jan 3, 2025
@Krishna2323
Copy link
Contributor

@grgia, the PR was already ready for your final review, I just resolved a minor conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Design External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
Status: Product (CRITICAL)
Development

No branches or pull requests

10 participants