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

Jump to bottom

Add the Recents and Contacts headers to the single-user participants page used in the Request Money and New Chat flows #2818

Merged
merged 13 commits into from
May 24, 2021
Merged

Add the Recents and Contacts headers to the single-user participants page used in the Request Money and New Chat flows #2818

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8e6e216
Add the Recents and Contacts headers to the single-user participants …
kakajann May 12, 2021
d484819
resolve conflicts
kakajann May 13, 2021
ba7c1c5
resolve conflicts again
kakajann May 14, 2021
e41360a
use underscore isEmpty function to check if we should show the recent…
kakajann May 17, 2021
2d825e6
Merge branch 'main' of github.com:studio-504/Expensify.cash into stud…
kakajann May 17, 2021
43b5932
revert beta permissions
kakajann May 17, 2021
49426e7
test OptionsListUtils searching by email
kakajann May 17, 2021
8c05935
linter hotfix
kakajann May 17, 2021
443b2b0
Merge branch 'main' of github.com:studio-504/Expensify.cash into stud…
kakajann May 18, 2021
179d0c8
Merge branch 'main' of github.com:studio-504/Expensify.cash into stud…
kakajann May 19, 2021
e38ae64
retrigger checks
kakajann May 19, 2021
fe39481
create a var for maxRecentReports and make some comments more readable
kakajann May 21, 2021
103ebfe
move iou.recents to common.recents
kakajann May 22, 2021
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
2 changes: 2 additions & 0 deletions src/libs/OptionsListUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,8 @@ function getNewChatOptions(
return getOptions(reports, personalDetails, {}, 0, {
searchValue,
includePersonalDetails: true,
includeRecentReports: true,
maxRecentReportsToShow: 5,
excludeConcierge,
});
}
Expand Down
3 changes: 2 additions & 1 deletion src/libs/Permissions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ function canUseChronos() {
* @returns {Boolean}
*/
function canUseIOU() {
return _.contains(betas, CONST.BETAS.IOU) || canUseAllBetas();
// return _.contains(betas, CONST.BETAS.IOU) || canUseAllBetas();
return true;
}
Copy link
Contributor

@Julesssss Julesssss May 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Please revert this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

reverted


export default {
Expand Down
12 changes: 11 additions & 1 deletion src/pages/NewChatPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ class NewChatPage extends Component {
this.createNewChat = this.createNewChat.bind(this);

const {
recentReports,
personalDetails,
userToInvite,
} = getNewChatOptions(
Expand All @@ -64,6 +65,7 @@ class NewChatPage extends Component {

this.state = {
searchValue: '',
recentReports,
personalDetails,
userToInvite,
};
Expand All @@ -77,6 +79,13 @@ class NewChatPage extends Component {
getSections() {
const sections = [];

sections.push({
title: this.props.translate('iou.recents'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we're using this String for non-IOU pages, I think it should be refactored to a different group. Could you move the iou.contacts and iou.recents to common.contacts and common.recents please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I moved

data: this.state.recentReports,
shouldShow: this.state.recentReports.length > 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

!_.isEmpty would be preferred here (example)

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 just changed the code.

And I noticed that OptionsSelector ignores shouldShow condition but it requires it as prop.

indexOffset: sections.reduce((prev, {data}) => prev + data.length, 0),
});

sections.push({
title: this.props.translate('iou.contacts'),
data: this.state.personalDetails,
Expand Down Expand Up @@ -128,6 +137,7 @@ class NewChatPage extends Component {
onSelectRow={this.createNewChat}
onChangeText={(searchValue = '') => {
const {
recentReports,
personalDetails,
userToInvite,
} = getNewChatOptions(
Expand All @@ -137,12 +147,12 @@ class NewChatPage extends Component {
);
this.setState({
searchValue,
recentReports,
userToInvite,
personalDetails,
});
}}
headerMessage={headerMessage}
hideSectionHeaders
disableArrowKeysActions
hideAdditionalOptionStates
forceTextUnreadStyle
Expand Down
23 changes: 20 additions & 3 deletions src/pages/iou/steps/IOUParticipantsPage/IOUParticipantsRequest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,19 @@ class IOUParticipantsRequest extends Component {

this.addSingleParticipant = this.addSingleParticipant.bind(this);

const {personalDetails, userToInvite} = getNewChatOptions(
const {
recentReports,
personalDetails,
userToInvite,
} = getNewChatOptions(
props.reports,
props.personalDetails,
'',
true,
);

this.state = {
recentReports,
personalDetails,
userToInvite,
searchValue: '',
Expand All @@ -65,6 +70,14 @@ class IOUParticipantsRequest extends Component {
*/
getSections() {
const sections = [];

sections.push({
title: this.props.translate('iou.recents'),
data: this.state.recentReports,
shouldShow: this.state.recentReports.length > 0,
indexOffset: sections.reduce((prev, {data}) => prev + data.length, 0),
});

sections.push({
title: this.props.translate('iou.contacts'),
data: this.state.personalDetails,
Expand Down Expand Up @@ -102,19 +115,23 @@ class IOUParticipantsRequest extends Component {
value={this.state.searchValue}
onSelectRow={this.addSingleParticipant}
onChangeText={(searchValue = '') => {
const {personalDetails, userToInvite} = getNewChatOptions(
const {
recentReports,
personalDetails,
userToInvite,
} = getNewChatOptions(
this.props.reports,
this.props.personalDetails,
searchValue,
true,
);
this.setState({
searchValue,
recentReports,
userToInvite,
personalDetails,
});
}}
hideSectionHeaders
disableArrowKeysActions
hideAdditionalOptionStates
forceTextUnreadStyle
Expand Down
37 changes: 19 additions & 18 deletions tests/unit/OptionsListUtilsTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import _ from 'underscore';
import _, {result} from 'underscore';
import Onyx from 'react-native-onyx';
import * as OptionsListUtils from '../../src/libs/OptionsListUtils';
import ONYXKEYS from '../../src/ONYXKEYS';
Expand Down Expand Up @@ -199,17 +199,17 @@ describe('OptionsListUtils', () => {
// When we call getNewChatOptions() with no search value
let results = OptionsListUtils.getNewChatOptions(REPORTS, PERSONAL_DETAILS, '');

// Then no reports should be returned, only personalDetails and all the personalDetails should be returned
// minus the currently logged in user
expect(results.recentReports.length).toBe(0);
expect(results.personalDetails.length).toBe(_.size(PERSONAL_DETAILS) - 1);
// We should expect maxmimum of 5 recent reports to be returned
// minus the currently logged in user and recent reports count
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure that this is correct.

We should expect maxmimum of 5 recent reports to be returned minus the currently logged in user and recent reports count

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is two different things, no?

  • Recent reports should have a max of 5
  • personalDetails --> all the personalDetails should be returned minus the currently logged in user and recent reports count

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 got inspired by getNewGroupOptions() test and I think this is correct

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it was previously two different comments that have been merged into one?

Copy link
Contributor

Choose a reason for hiding this comment

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

The code is fine

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant here is that there should be two separate comments, but I think you have combined them into one.

// We should expect maxmimum of 5 recent reports to be returned --> this is referring to `recentReports`
// minus the currently logged in user and recent reports count --> why would the `recentReports` count be `X - 5 - logged in user`?

I think it should say this:

// We should expect maxmimum of 5 recent reports to be returned
// We should expect all personalDetails to be returned, minus the currently logged in user and recent reports count

Does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I refactored the commenting

expect(results.recentReports.length).toBe(5);
expect(results.personalDetails.length).toBe(_.size(PERSONAL_DETAILS) - 1 - 5);

// Then the result which has an existing report should also have the reportID attached
const personalDetailWithExistingReport = _.find(
results.personalDetails,
personalDetail => personalDetail.login === 'reedrichards@expensify.com',
personalDetail => personalDetail.login === 'peterparker@expensify.com',
Julesssss marked this conversation as resolved.
Show resolved Hide resolved
);
expect(personalDetailWithExistingReport.reportID).toBe(3);
expect(personalDetailWithExistingReport.reportID).toBe(2);

// When we provide a search value that does not match any personal details
results = OptionsListUtils.getNewChatOptions(REPORTS, PERSONAL_DETAILS, 'magneto');
Expand All @@ -221,26 +221,27 @@ describe('OptionsListUtils', () => {
results = OptionsListUtils.getNewChatOptions(REPORTS, PERSONAL_DETAILS, 'peterparker@expensify.com');

// Then one option will be returned and it will be the correct option
expect(results.personalDetails.length).toBe(1);
expect(results.personalDetails[0].text).toBe('Spider-Man');
expect(results.recentReports.length).toBe(1);
expect(results.recentReports[0].text).toBe('Spider-Man');

Copy link
Contributor

Choose a reason for hiding this comment

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

We should test both instead of replacing the old personalDetails test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now I expect personalDetails to be empty

Copy link
Contributor

Choose a reason for hiding this comment

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

Now I expect personalDetails to be empty

Could we add that as a test case then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, here it is.

// When we provide a search value that matches a partial display name or email
results = OptionsListUtils.getNewChatOptions(REPORTS, PERSONAL_DETAILS, 'man');

// Then several options will be returned and they will be each have the search string in their email or name
// even though the currently logged in user matches they should not show.
expect(results.personalDetails.length).toBe(3);
expect(results.personalDetails[0].text).toBe('Spider-Man');
expect(results.personalDetails[1].text).toBe('Invisible Woman');
expect(results.personalDetails[2].login).toBe('natasharomanoff@expensify.com');
expect(results.personalDetails.length).toBe(1);
expect(results.recentReports.length).toBe(2);
expect(results.personalDetails[0].login).toBe('natasharomanoff@expensify.com');
expect(results.recentReports[0].text).toBe('Invisible Woman');
expect(results.recentReports[1].text).toBe('Spider-Man');

// Test for Concierge's existence in chat options
results = OptionsListUtils.getNewChatOptions(REPORTS_WITH_CONCIERGE, PERSONAL_DETAILS_WITH_CONCIERGE);

// Concierge is included in the results by default and all the personalDetails should be returned
// minus the currently logged in user
expect(results.personalDetails.length).toBe(_.size(PERSONAL_DETAILS_WITH_CONCIERGE) - 1);
expect(results.personalDetails).toEqual(
// Concierge is included in the results by default. We should expect all the personalDetails to show
// (minus the 5 that are already showing and the currently logged in user)
expect(results.personalDetails.length).toBe(_.size(PERSONAL_DETAILS_WITH_CONCIERGE) - 1 - 5);
expect(results.recentReports).toEqual(
Julesssss marked this conversation as resolved.
Show resolved Hide resolved
expect.arrayContaining([
expect.objectContaining({login: 'concierge@expensify.com'}),
]),
Expand All @@ -250,7 +251,7 @@ describe('OptionsListUtils', () => {
results = OptionsListUtils.getNewChatOptions(REPORTS_WITH_CONCIERGE, PERSONAL_DETAILS_WITH_CONCIERGE, '', true);

// All the personalDetails should be returned minus the currently logged in user and Concierge
expect(results.personalDetails.length).toBe(_.size(PERSONAL_DETAILS_WITH_CONCIERGE) - 2);
expect(results.personalDetails.length).toBe(_.size(PERSONAL_DETAILS_WITH_CONCIERGE) - 2 - 5);
Copy link
Contributor

Choose a reason for hiding this comment

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

5 is used quite a lot, can we add a test that returns less than 5 recentReports?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kakajann can you create a var for 5, something like let maxRecentReports = 5;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

5 is used quite a lot, can we add a test that returns less than 5 recentReports?

In this line, we are testing recentReports that returns less than 5.

@kakajann can you create a var for 5, something like let maxRecentReports = 5;

Of course, I did add a file-level constant variable for maxRecentReports in my last push

expect(results.personalDetails).not.toEqual(
expect.arrayContaining([
expect.objectContaining({login: 'concierge@expensify.com'}),
Expand Down