Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.56k stars 2.9k forks source link

[HOLD for payment 2022-05-20] [$4,000] Web - Workspace- New capability to set tracking rate and unit persists after page refresh #7573

Closed kbecciv closed 2 years ago

kbecciv commented 2 years ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

Precondition: Add the VBA into your testing account

  1. Launch the app
  2. Log in with expensiafail account
  3. Go to Setting
  4. Select any Workspace
  5. Go to Reimburse expenses
  6. Refresh the pages

Expected Result:

New capability to set tracking rate and unit appears when go to reimburse expense page

Actual Result:

Tracking rate and unit does not appear and appears for a brief moment when refreshing the page.

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.1.36.0

Reproducible in staging?: Yes

Reproducible in production?: No

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/152598163-121bfbea-ed3d-44a1-9159-4f4408e5eab8.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause

Slack conversation:

Issue was found when executing: https://github.com/Expensify/App/pull/6839

View all open jobs on GitHub

MelvinBot commented 2 years ago

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

Julesssss commented 2 years ago

Hi @kbecciv can you explain why we should not see the mileage tracking? I can't see any requirement for the rate to be hidden:

New capability to set tracking rate and unit is not seen after refresh the page

kavimuru commented 2 years ago

Hi @Julesssss

As per #6839 "set tracking rate and unit" option should appear in ""Reimburse expenses" page. It appears if the account does not have VBA added, and if the account has VBA it does not appear in ""Reimburse expenses" page and appears for a brief moment when refreshing the page. So expected and actual results should be corrected. Attaching a new video here with accounts which has VBA account and which does not have it

https://user-images.githubusercontent.com/43996225/152815273-dc090daf-17f7-4eb4-9e30-8e7c17ee2af3.mp4

.

Julesssss commented 2 years ago

Okay, thanks for explaining. I'm having an unrelated issue adding a VBA account currently, but I think we can start to review proposals.

MelvinBot commented 2 years ago

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

Julesssss commented 2 years ago

Not overdue

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

Triggered auto assignment to @Beamanator (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

stephanieelliott commented 2 years ago

Posted to Upwork: https://www.upwork.com/jobs/~0152b595bdb97e852c

Beamanator commented 2 years ago

@stephanieelliott FYI I don't think you need to unassign the Engineer manually, in this case I believe @Julesssss was ok with staying assigned b/c he also triaged the issue on the engineering side.

@Julesssss do you want to make this issue? Feel free to swap if you want it 👍

Julesssss commented 2 years ago

Yeah, I swapped back, all hail the FairSelection algorithm

stephanieelliott commented 2 years ago

Price 2x'd to $500

Beamanator commented 2 years ago

@Julesssss it looks like you're still not assigned 😅 I'll assign you if another week goes by 😈

Julesssss commented 2 years ago

I was so focused on writing that comment that I forgot to assign myself 😅

stephanieelliott commented 2 years ago

2x'd this on Upwork https://www.upwork.com/jobs/~0152b595bdb97e852c

MelvinBot commented 2 years ago

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

stephanieelliott commented 2 years ago

I'm about to go OOO, so reapplied the label to assign this to another CM team member (thanks @NicMendonca)

NicMendonca commented 2 years ago

doubled again -- https://www.upwork.com/jobs/~0152b595bdb97e852c

NicMendonca commented 2 years ago

doubled -- https://www.upwork.com/jobs/~0152b595bdb97e852c

NicMendonca commented 2 years ago

Forgot to change title ☝️

NicMendonca commented 2 years ago

doubled again - https://www.upwork.com/jobs/~0152b595bdb97e852c

OneDevStopShop commented 2 years ago

I have been looking into this. It seems that

Tracking rate and unit does not appear and appears for a brief moment when refreshing the page.

Since the pre-req is to add a VBA, the reason why it does not show up is simply because for the VBA view we do not have the sections for tracking distance and rates.

NoVBAView: https://github.com/Expensify/App/blob/4b5abf63ee24c2031945a3c119330cc86f45985b/src/pages/workspace/reimburse/WorkspaceReimburseNoVBAView.js#L159-L188

VBAView: https://github.com/Expensify/App/blob/4b5abf63ee24c2031945a3c119330cc86f45985b/src/pages/workspace/reimburse/WorkspaceReimburseVBAView.js#L20-L65

The solution is to copy over the section and state to the VBAView.

NicMendonca commented 2 years ago

@OneDevStopShop thanks for that proposal! Before we review, can you please link your Upwork profile here? Thanks!

OneDevStopShop commented 2 years ago

Hi, this is my profile: https://www.upwork.com/freelancers/~010b263ae646bc0273 Thank you

rushatgabhane commented 2 years ago

Sorry for the delay, I'll review the proposal tomorrow.

@onedevstopshop in the meantime, could you please help me understand why this issue isn't applicable on native (mobile devices)?

OneDevStopShop commented 2 years ago

@rushatgabhane I do not see why this would not be applicable on mobile devices as well. The code just is not there and so it should not show up unless there was an issue with determining VBA

rushatgabhane commented 2 years ago

Okay, so I'm just thinking out loud if I understand this issue.

rushatgabhane commented 2 years ago

Hi I am having trouble with Upwork and cannot complete jobs at this time. Please find someone new for the job and sorry for the inconvenience https://github.com/Expensify/App/issues/7917#issuecomment-1093009100

@OneDevStopShop I like your proposal, but when do you reckon you can get your Upwork profile fixed? It'll be unfortunate if your efforts are in vain.

rushatgabhane commented 2 years ago

@OneDevStopShop any updates?

rushatgabhane commented 2 years ago

@OneDevStopShop's Upwork profile has been deleted and they don't meet the prereq for working on E/App jobs

@Julesssss I'd suggest that we use @OneDevStopShop's proposal and

Julesssss commented 2 years ago

Ah, thanks for pointing that out.

Sure, would you like to implement it @rushatgabhane? If @OneDevStopShop is able to get an Upwork profile up I think we would still happily pay out a bonus for making the proposal.

rushatgabhane commented 2 years ago

would you like to implement it.

@Julesssss yes !! (only because this should be a 5 min fix)

Borrowed Proposal

Move the track distance section, and it's related methods from NoVBAView to VBAView. This isn't already the case because it's proly a mistake/misunderstanding.

https://github.com/Expensify/App/blob/4b5abf63ee24c2031945a3c119330cc86f45985b/src/pages/workspace/reimburse/WorkspaceReimburseNoVBAView.js#L159-L188

https://github.com/Expensify/App/blob/4b5abf63ee24c2031945a3c119330cc86f45985b/src/pages/workspace/reimburse/WorkspaceReimburseVBAView.js#L20

Julesssss commented 2 years ago

Hey @rushatgabhane. Just one thing, I noticed you'll be out to study. Did you still want to work on this as a contributor? No worries if not, it's completely up to you :)

rushatgabhane commented 2 years ago

~~@Julesssss thanks for asking, yes I still want to work on this as a contributor. I have a diff from the time I tested the proposal, so I can handle this one.~~

PR coming up! (if everything else is good)

rushatgabhane commented 2 years ago

Oh no, gotta run! @Julesssss I won't be able to submit a PR for this issue. Please make it internal, or open to all contributors. Thanks for your patience and understanding!

Julesssss commented 2 years ago

No worries. Good luck with your exams!

melvin-bot[bot] commented 2 years ago

Current assignee @NicMendonca is eligible for the External assigner, not assigning anyone new.

Julesssss commented 2 years ago

Hey @NicMendonca, we need to figure out how to re-roll for an external contributor somehow. Any ideas? Else I guess we can ask in the Slack channel to see if anyone wants to be manually assigned.

Beamanator commented 2 years ago

I think you can try reapplying Exported (even though that'll also post in #expensify-open-source) :D

melvin-bot[bot] commented 2 years ago

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

melvin-bot[bot] commented 2 years ago

Current assignee @Julesssss is eligible for the Exported assigner, not assigning anyone new.

Julesssss commented 2 years ago

Lol. Poor effort bot.

Julesssss commented 2 years ago

Sweet.

FYI @parasharrajat, we had a proposal approved but the contributor disappeared. So we're back to awaiting proposals.

mateusbra commented 2 years ago

PROPOSAL(updated)

We need to merge both components NoVBAView to VBAView: WorkspaceReimburseView is going to be:

import React from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import lodashGet from 'lodash/get';
import _ from 'underscore';
import TextInput from '../../../components/TextInput';
import Picker from '../../../components/Picker';
import Text from '../../../components/Text';
import styles from '../../../styles/styles';
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
import * as Expensicons from '../../../components/Icon/Expensicons';
import * as Illustrations from '../../../components/Icon/Illustrations';
import Section from '../../../components/Section';
import Navigation from '../../../libs/Navigation/Navigation';
import ROUTES from '../../../ROUTES';
import CopyTextToClipboard from '../../../components/CopyTextToClipboard';
import * as Link from '../../../libs/actions/Link';
import compose from '../../../libs/compose';
import ONYXKEYS from '../../../ONYXKEYS';
import * as Policy from '../../../libs/actions/Policy';
import withFullPolicy from '../withFullPolicy';
import CONST from '../../../CONST';
import Button from '../../../components/Button';

const propTypes = {
    /** The policy ID currently being configured */
    policyID: PropTypes.string.isRequired,

    /** Does the user has a VBA into its account? */
    hasVBA: PropTypes.bool.isRequired,

    /** Policy values needed in the component */
    policy: PropTypes.shape({
        customUnit: PropTypes.shape({
            id: PropTypes.string,
            name: PropTypes.string,
            value: PropTypes.string,
            rate: PropTypes.shape({
                id: PropTypes.string,
                name: PropTypes.string,
                value: PropTypes.number,
            }),
        }),
        outputCurrency: PropTypes.string,
    }).isRequired,

    ...withLocalizePropTypes,
};

class WorkspaceReimburseView extends React.Component {
    constructor(props) {
        super(props);
        this.state = {
            unitID: lodashGet(props, 'policy.customUnit.id', ''),
            unitName: lodashGet(props, 'policy.customUnit.name', ''),
            unitValue: lodashGet(props, 'policy.customUnit.value', 'mi'),
            rateID: lodashGet(props, 'policy.customUnit.rate.id', ''),
            rateName: lodashGet(props, 'policy.customUnit.rate.name', ''),
            rateValue: this.getRateDisplayValue(lodashGet(props, 'policy.customUnit.rate.value', 0) / 100),
            outputCurrency: lodashGet(props, 'policy.outputCurrency', ''),
        };

        this.unitItems = [
            {
                label: this.props.translate('workspace.reimburse.kilometers'),
                value: 'km',
            },
            {
                label: this.props.translate('workspace.reimburse.miles'),
                value: 'mi',
            },
        ];

        this.debounceUpdateOnCursorMove = this.debounceUpdateOnCursorMove.bind(this);
        this.updateRateValueDebounced = _.debounce(this.updateRateValue.bind(this), 1000);
    }

    getRateDisplayValue(value) {
        const numValue = parseFloat(value);
        if (Number.isNaN(numValue)) {
            return '';
        }

        return numValue.toFixed(3);
    }

    setRate(value) {
        const isInvalidRateValue = value !== '' && !CONST.REGEX.RATE_VALUE.test(value);

        this.setState(prevState => ({
            rateValue: !isInvalidRateValue ? value : prevState.rateValue,
        }), () => {
            // Set the corrected value with a delay and sync to the server
            this.updateRateValueDebounced(this.state.rateValue);
        });
    }

    setUnit(value) {
        this.setState({unitValue: value});

        Policy.setCustomUnit(this.props.policyID, {
            customUnitID: this.state.unitID,
            customUnitName: this.state.unitName,
            attributes: {unit: value},
        }, null);
    }

    debounceUpdateOnCursorMove(event) {
        if (!_.contains(['ArrowLeft', 'ArrowRight'], event.key)) {
            return;
        }

        this.updateRateValueDebounced(this.state.rateValue);
    }

    updateRateValue(value) {
        const numValue = parseFloat(value);

        if (_.isNaN(numValue)) {
            return;
        }

        this.setState({
            rateValue: numValue.toFixed(3),
        });

        Policy.setCustomUnitRate(this.props.policyID, this.state.unitID, {
            customUnitRateID: this.state.rateID,
            name: this.state.rateName,
            rate: numValue.toFixed(3) * 100,
        }, null);
    }

    render() {
        return (
            <>
                <Section
                    title={this.props.translate('workspace.reimburse.captureReceipts')}
                    icon={Illustrations.ReceiptYellow}
                    menuItems={[
                        {
                            title: this.props.translate('workspace.reimburse.viewAllReceipts'),
                            onPress: () => Link.openOldDotLink(`expenses?policyIDList=${this.props.policyID}&billableReimbursable=reimbursable&submitterEmail=%2B%2B`),
                            icon: Expensicons.Receipt,
                            shouldShowRightIcon: true,
                            iconRight: Expensicons.NewWindow,
                        },
                    ]}
                >
                    <View style={[styles.mv4, styles.flexRow, styles.flexWrap]}>
                        <Text>
                            {this.props.translate('workspace.reimburse.captureNoVBACopyBeforeEmail')}
                            <CopyTextToClipboard
                                text="receipts@expensify.com"
                                textStyles={[styles.textBlue]}
                            />
                            <Text>{this.props.translate('workspace.reimburse.captureNoVBACopyAfterEmail')}</Text>
                        </Text>
                    </View>
                </Section>

                <Section
                    title={this.props.translate('workspace.reimburse.trackDistance')}
                    icon={Illustrations.GpsTrackOrange}
                >
                    <View style={[styles.mv4]}>
                        <Text>{this.props.translate('workspace.reimburse.trackDistanceCopy')}</Text>
                    </View>
                    <View style={[styles.flexRow, styles.alignItemsCenter]}>
                        <View style={[styles.rateCol]}>
                            <TextInput
                                label={this.props.translate('workspace.reimburse.trackDistanceRate')}
                                placeholder={this.state.outputCurrency}
                                onChangeText={value => this.setRate(value)}
                                value={this.state.rateValue}
                                autoCompleteType="off"
                                autoCorrect={false}
                                keyboardType={CONST.KEYBOARD_TYPE.DECIMAL_PAD}
                                onKeyPress={this.debounceUpdateOnCursorMove}
                            />
                        </View>
                        <View style={[styles.unitCol]}>
                            <Picker
                                label={this.props.translate('workspace.reimburse.trackDistanceUnit')}
                                items={this.unitItems}
                                value={this.state.unitValue}
                                onInputChange={value => this.setUnit(value)}
                            />
                        </View>
                    </View>
                </Section>

                {!this.props.hasVBA ? (
                    <Section
                        title={this.props.translate('workspace.reimburse.unlockNextDayReimbursements')}
                        icon={Illustrations.JewelBoxGreen}
                    >
                        <View style={[styles.mv4]}>
                            <Text>{this.props.translate('workspace.reimburse.unlockNoVBACopy')}</Text>
                        </View>
                        <Button
                            text={this.props.translate('workspace.common.bankAccount')}
                            onPress={() => Navigation.navigate(ROUTES.getWorkspaceBankAccountRoute(this.props.policyID))}
                            icon={Expensicons.Bank}
                            style={[styles.mt4]}
                            iconStyles={[styles.mr5]}
                            shouldShowRightIcon
                            extraLarge
                            success
                        />
                    </Section>
                )
                    : (
                        <Section
                            title={this.props.translate('workspace.reimburse.fastReimbursementsHappyMembers')}
                            icon={Illustrations.BankUserGreen}
                            menuItems={[
                                {
                                    title: this.props.translate('workspace.reimburse.reimburseReceipts'),
                                    onPress: () => Link.openOldDotLink(`reports?policyID=${this.props.policyID}&from=all&type=expense&showStates=Archived&isAdvancedFilterMode=true`),
                                    icon: Expensicons.Bank,
                                    shouldShowRightIcon: true,
                                    iconRight: Expensicons.NewWindow,
                                },
                            ]}
                        >
                            <View style={[styles.mv4]}>
                                <Text>{this.props.translate('workspace.reimburse.fastReimbursementsVBACopy')}</Text>
                            </View>
                        </Section>
                    )}
            </>
        );
    }
}

WorkspaceReimburseView.propTypes = propTypes;
WorkspaceReimburseView.displayName = 'WorkspaceReimburseView';

export default compose(
    withFullPolicy,
    withLocalize,
    withOnyx({
        policy: {
            key: ({policyID}) => `${ONYXKEYS.COLLECTION.POLICY}${policyID}`,
        },
    }),
)(WorkspaceReimburseView);

SCREEN RECORDING:

https://user-images.githubusercontent.com/56851391/166076977-92c50eb5-9ad5-47ac-a64d-6c0704848638.mp4

parasharrajat commented 2 years ago

I don't like the proposal. It is not following DRY.

My proposal be:

  1. Merge both WorkspaceReimburseVBAView and WorkspaceReimburseNoVBAView as 95% code is same.
  2. Pass down the hasVBA from the WorkspaceReimbursePage to the new merged component.
  3. Now based on hasVBA switch between the last section's details.

if we want to follow the similar pattern same as other pages.

  1. Move sections into there on small components.
  2. So the whole code to manage the trackDistance will be into a component.
  3. Use the required sections from step 2 into WorkspaceReimburseVBAView and WorkspaceReimburseNoVBAView components.
parasharrajat commented 2 years ago

@Julesssss We would need your review here. Thanks.

Julesssss commented 2 years ago

I'm totally with you here @parasharrajat. But just to be 100% sure, you're suggesting these changes are made to @mateusbra's proposal, right?

Julesssss commented 2 years ago

I think I'd prefer to merge the two similar Components.

parasharrajat commented 2 years ago

I am fine either way. Either @mateusbra can do this or I can create the PR. Your call? It doesn't look time-consuming.

:ribbon: :eyes: :ribbon: C+ reviewed

mateusbra commented 2 years ago

I think I'd prefer to merge the two similar Components.

I updated my proposal in order to cover the requested changes, I'll use it if I'm chosen to create the PR.