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.

Sign up for GitHub

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

Fix split bill display / selection with new users #21113

Merged
merged 8 commits into from
Jun 20, 2023
Merged

Fix split bill display / selection with new users #21113

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cb4d866
Rename to make var more clear
Beamanator Jun 20, 2023
badbbd0
Add appropriate defaults
Beamanator Jun 20, 2023
98a739e
Make var names clearer & use accountIDs more
Beamanator Jun 20, 2023
b22d585
accountID should always exist and be unique
Beamanator Jun 20, 2023
45160f4
These are money request reports
Beamanator Jun 20, 2023
bcda417
Remove thing we might add back
Beamanator Jun 20, 2023
1f09dde
Fix default option if accountID exists
Beamanator Jun 20, 2023
e8644ea
Use accountIDs for matching selected options
Beamanator Jun 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/components/MoneyRequestConfirmationList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,15 +155,15 @@ function MoneyRequestConfirmationList(props) {
const formattedParticipantsList = _.union(formattedSelectedParticipants, formattedUnselectedParticipants);

const myIOUAmount = IOUUtils.calculateAmount(selectedParticipants.length, props.iouAmount, true);
const formattedPayeePersonalDetails = OptionsListUtils.getIOUConfirmationOptionsFromPayeePersonalDetail(
const formattedPayeeOption = OptionsListUtils.getIOUConfirmationOptionsFromPayeePersonalDetail(
payeePersonalDetails,
CurrencyUtils.convertToDisplayString(myIOUAmount, props.iouCurrencyCode),
);

sections.push(
{
title: translate('moneyRequestConfirmationList.whoPaid'),
data: [formattedPayeePersonalDetails],
data: [formattedPayeeOption],
shouldShow: true,
indexOffset: 0,
},
Expand Down Expand Up @@ -211,21 +211,21 @@ function MoneyRequestConfirmationList(props) {
const toggleOption = useCallback(
(option) => {
// Return early if selected option is currently logged in user.
if (option.login === props.session.email) {
if (option.accountID === props.session.accountID) {
return;
}

setParticipants((prevParticipants) => {
const newParticipants = _.map(prevParticipants, (participant) => {
if (participant.login === option.login) {
if (participant.accountID === option.accountID) {
return {...participant, selected: !participant.selected};
}
return participant;
});
return newParticipants;
});
},
[props.session.email],
[props.session.accountID],
);

/**
Expand Down
10 changes: 6 additions & 4 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,18 +216,19 @@ function isPersonalDetailsReady(personalDetails) {
function getParticipantsOptions(report, personalDetails) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why we're not returning isMoneyRequestReport: true here, but that's for another day

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asked here, but it's been a few months so idk if there's a good answer, just checking

const participants = lodashGet(report, 'participantAccountIDs', []);
return _.map(getPersonalDetailsForAccountIDs(participants, personalDetails), (details) => ({
keyForList: details.login,
login: details.login,
keyForList: details.login || details.displayName || String(details.accountID),
Beamanator marked this conversation as resolved.
Show resolved Hide resolved
login: details.login || details.displayName,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, why we fallback to displayName in login? This is confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user's login is not always going to be available in details - so we either need to default to displayName here orrr store displayName in each option and default to it when we render these options, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would go wrong if we keep the login field empty/undefined? (instead of falling back to displayName)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I was fixing - if there's no login we get this

Screenshot 2023-06-20 at 4 08 57 PM

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see, can we instead set a displayName field and use it in the iou pages

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relaying on accountID instead of login seems to do it.

        const displayNamesWithTooltips = ReportUtils.getDisplayNamesWithTooltips(
            (this.props.option.participantsList || this.props.option.accountID ? [this.props.option] : []).slice(0, 10),
            isMultipleParticipant,
        );

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh that seems like a better fix - though I'm still surprised isMoneyRequestReport: true, on options for MoneyRequest pages... Maybe that's a follow-up question

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will be back in 45min or so, the above fix should be the correct fix, feel free to merge if this is critical

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isMoneyRequestReport is unrelated. this is true when the report is iou report or expense report, this has nothing to do with iou, it's used for other optionrow usage (not totally sure, but that's what I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

asking in slack for someone to test on all platforms for you

accountID: details.accountID,
text: details.displayName,
firstName: lodashGet(details, 'firstName', ''),
lastName: lodashGet(details, 'lastName', ''),
alternateText: Str.isSMSLogin(details.login || '') ? LocalePhoneNumber.formatPhoneNumber(details.login) : details.login,
alternateText: Str.isSMSLogin(details.login || '') ? LocalePhoneNumber.formatPhoneNumber(details.login) : details.login || details.displayName,
icons: [
{
source: UserUtils.getAvatar(details.avatar, details.accountID),
name: details.login,
type: CONST.ICON_TYPE_AVATAR,
id: details.accountID,
},
],
payPalMeAddress: lodashGet(details, 'payPalMeAddress', ''),
Expand Down Expand Up @@ -857,12 +858,13 @@ function getSearchOptions(reports, personalDetails, searchValue = '', betas) {
function getIOUConfirmationOptionsFromPayeePersonalDetail(personalDetail, amountText) {
return {
text: personalDetail.displayName ? personalDetail.displayName : personalDetail.login,
alternateText: personalDetail.login,
alternateText: personalDetail.login || personalDetail.displayName,
icons: [
{
source: UserUtils.getAvatar(personalDetail.avatar, personalDetail.accountID),
name: personalDetail.login,
type: CONST.ICON_TYPE_AVATAR,
id: personalDetail.accountID,
},
],
descriptiveText: amountText,
Expand Down
8 changes: 4 additions & 4 deletions src/pages/iou/MoneyRequestModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const propTypes = {
}),

/** Personal details of all the users */
personalDetails: PropTypes.shape({
allPersonalDetails: PropTypes.shape({
/** Primary login of participant */
login: PropTypes.string,

Expand Down Expand Up @@ -81,7 +81,7 @@ const defaultProps = {
currentUserPersonalDetails: {
localCurrencyCode: CONST.CURRENCY.USD,
},
personalDetails: {},
allPersonalDetails: {},
iou: {
error: false,
selectedCurrencyCode: null,
Expand All @@ -108,7 +108,7 @@ function MoneyRequestModal(props) {
const [selectedOptions, setSelectedOptions] = useState(
ReportUtils.isPolicyExpenseChat(props.report)
? OptionsListUtils.getPolicyExpenseReportOptions(props.report)
: OptionsListUtils.getParticipantsOptions(props.report, props.personalDetails),
: OptionsListUtils.getParticipantsOptions(props.report, props.allPersonalDetails),
);
const [amount, setAmount] = useState(0);

Expand Down Expand Up @@ -442,7 +442,7 @@ export default compose(
iou: {
key: ONYXKEYS.IOU,
},
personalDetails: {
allPersonalDetails: {
key: ONYXKEYS.PERSONAL_DETAILS_LIST,
},
}),
Expand Down