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: getting unselected participants #32823

Merged
merged 2 commits into from
Dec 11, 2023
Merged

fix: getting unselected participants #32823

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b12afa0
fix: getting unseelected participants
sangar-1028 Dec 11, 2023
7e362f2
fix: rename participant variables
sangar-1028 Dec 11, 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
3 changes: 2 additions & 1 deletion src/components/MoneyTemporaryForRefactorRequestConfirmationList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ function MoneyTemporaryForRefactorRequestConfirmationList({

const optionSelectorSections = useMemo(() => {
const sections = [];
const unselectedParticipants = _.filter(selectedParticipantsFiltered, (participant) => !participant.selected);
const unselectedParticipants = _.filter(selectedParticipants, (participant) => !participant.selected);
Copy link
Contributor

@cubuspl42 cubuspl42 Dec 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My gosh, these names are confusing. So we have...

  • selectedParticipants
  • selectedParticipantsFiltered, filtered on participant.selected
  • unselectedParticipants, also filtered, but on the negation of the above, i.e. on !participant.selected

Also, we consider selectedParticipantsFiltered heavy enough so it needs to be memoized, but unselectedParticipants not heavy enough to be memoized.

It's great that we found the root cause and fixed it, but maybe let's minimize the risk of a similar problem in this very area.

Do you understand the difference between selectedParticipants and selectedParticipantsFiltered? What is it actually filtered on?

    /** Selected participants from MoneyRequestModal with login / accountID */
    selectedParticipants: PropTypes.arrayOf(optionPropTypes).isRequired,

It sounds like these already are the "selected participants", how can they be more selected?

We need either some renaming, or some comments, or both

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cubuspl42

selectedParticipants are the participants selected in participants page(group chat members in our case)
selectedParticipantsFiltered are participants with selected = true. User can toggle them in the confirmation page. Initially all values of selected are true
unselectedParticipants are participants with selected = false

Copy link
Contributor

Choose a reason for hiding this comment

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

So maybe let's use another term for the "outer" selected participants, like chosenParticipants or pickedParticipants?

Then we could name the "inner" selected participants symmetrically, like...

  • selectedParticipants (where selected = true)
  • unselectedParticipants (where selected = false)

I think it would be clearer to declare them close to each other.

It's important to think about why it was easy to make a mistake and what we can do so it's, well, at least a bit harder to make a similar one in the future.

I searched the code briefly and it seems that there are not many usages to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cubuspl42

Please check again and let me know your thoughts

if (hasMultipleParticipants) {
const formattedSelectedParticipants = getParticipantsWithAmount(selectedParticipantsFiltered);
let formattedParticipantsList = _.union(formattedSelectedParticipants, unselectedParticipants);
Expand Down Expand Up @@ -416,6 +416,7 @@ function MoneyTemporaryForRefactorRequestConfirmationList({
return sections;
}, [
selectedParticipantsFiltered,
selectedParticipants,
hasMultipleParticipants,
iouAmount,
iouCurrencyCode,
Expand Down