Skip to content

Stop fetching personalDetails directly #9934

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

Merged
luacmartins merged 6 commits into from
Jul 20, 2022
Merged

Stop fetching personalDetails directly #9934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
82b8332
Remove fetchPersonalDetails()
marcaaron Jul 14, 2022
4a351bb
fix conflicts
marcaaron Jul 18, 2022
5853f88
remove random MY_PERSONAL_DETAILS keys
marcaaron Jul 18, 2022
0737143
Remove PersonalDetails import
marcaaron Jul 18, 2022
cd32dd7
Merge remote-tracking branch 'origin' into marcaaron-personalDetailsO…
marcaaron Jul 19, 2022
ee0dd77
DRY up tests. Create TestHelper.setPersonalDetails()
marcaaron Jul 19, 2022
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
19 changes: 0 additions & 19 deletions src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,11 @@ import MainDrawerNavigator from './MainDrawerNavigator';
// Modal Stack Navigators
import * as ModalStackNavigators from './ModalStackNavigators';
import SCREENS from '../../../SCREENS';
import Timers from '../../Timers';
import ValidateLoginPage from '../../../pages/ValidateLoginPage';
import defaultScreenOptions from './defaultScreenOptions';
import * as App from '../../actions/App';
import * as Session from '../../actions/Session';
import LogOutPreviousUserPage from '../../../pages/LogOutPreviousUserPage';
import networkPropTypes from '../../../components/networkPropTypes';
import {withNetwork} from '../../../components/OnyxProvider';

let currentUserEmail;
Onyx.connect({
Expand Down Expand Up @@ -88,9 +85,6 @@ const modalScreenListeners = {
};

const propTypes = {
/** Information about the network */
network: networkPropTypes.isRequired,

...windowDimensionsPropTypes,
};

Expand Down Expand Up @@ -122,18 +116,6 @@ class AuthScreens extends React.Component {

App.fixAccountAndReloadData();
App.setUpPoliciesAndNavigate(this.props.session);

// Refresh the personal details, timezone and betas every 30 minutes
// There is no pusher event that sends updated personal details data yet
// See https://github.com/Expensify/ReactNativeChat/issues/468
this.interval = Timers.register(setInterval(() => {
if (this.props.network.isOffline) {
return;
}
PersonalDetails.fetchPersonalDetails();
User.getBetas();

@marcaaron marcaaron Jul 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it's fine to get rid of the betas here as well. Unsure why we were also periodically refreshing these but regardless periodic refresh seems like not what we want to do longer term.

@luacmartins luacmartins Jul 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah we were holding on removing User.getBetas from the timer until AuthenticateUser is implemented - #9769. I think it's probably ok to remove it now though.

@marcaaron marcaaron Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Works for me 👍

}, 1000 * 60 * 30));

Timing.end(CONST.TIMING.HOMEPAGE_INITIAL_RENDER);

const searchShortcutConfig = CONST.KEYBOARD_SHORTCUTS.SEARCH;
Expand Down Expand Up @@ -333,7 +315,6 @@ class AuthScreens extends React.Component {
AuthScreens.propTypes = propTypes;
export default compose(
withWindowDimensions,
withNetwork(),
withOnyx({
session: {
key: ONYXKEYS.SESSION,
Expand Down
2 changes: 0 additions & 2 deletions src/libs/actions/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import CONST from '../../CONST';
import Log from '../Log';
import Performance from '../Performance';
import Timing from './Timing';
import * as PersonalDetails from './PersonalDetails';
import * as Report from './Report';
import * as BankAccounts from './BankAccounts';
import * as Policy from './Policy';
Expand Down Expand Up @@ -98,7 +97,6 @@ function getAppData(shouldSyncPolicyList = true) {

// We should update the syncing indicator when personal details and reports are both done fetching.
return Promise.all([
PersonalDetails.fetchPersonalDetails(),

@marcaaron marcaaron Jul 18, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This was one thing that I am not 100% sure about...

We won't be "waiting" for the details to happen anymore and no "syncing" stuff will be shown. Are we handling this somehow and I missed it?

@luacmartins luacmartins Jul 18, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah from the Pattern B design doc, it seems like we won't display syncing status anymore, so that's not something that we need to handle anymore.

Since we won’t use the avatar to show syncing/offline status, we can remove this usage and replace it with just this code.

@marcaaron marcaaron Jul 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice.

Report.fetchAllReports(true, true),
]);
}
Expand Down
22 changes: 0 additions & 22 deletions src/libs/actions/PersonalDetails.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,27 +117,6 @@ function formatPersonalDetails(personalDetailsList) {
return formattedResult;
}

/**
* Get the personal details for our organization
* @returns {Promise}
*/
function fetchPersonalDetails() {
return DeprecatedAPI.Get({
returnValueList: 'personalDetailsList',
})
.then((data) => {
// If personalDetailsList does not have the current user ensure we initialize their details with an empty
// object at least
const personalDetailsList = _.isEmpty(data.personalDetailsList) ? {} : data.personalDetailsList;
if (!personalDetailsList[currentUserEmail]) {
personalDetailsList[currentUserEmail] = {};
}

const allPersonalDetails = formatPersonalDetails(personalDetailsList);
Onyx.merge(ONYXKEYS.PERSONAL_DETAILS, allPersonalDetails);
});
}

/**
* Gets the first and last name from the user's personal details.
* If the login is the same as the displayName, then they don't exist,
Expand Down Expand Up @@ -327,7 +306,6 @@ function deleteAvatar(defaultAvatarURL) {
}

export {
fetchPersonalDetails,
formatPersonalDetails,
getFromReportParticipants,
getDisplayName,
Expand Down
27 changes: 3 additions & 24 deletions tests/actions/ReportTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,7 @@ describe('actions/Report', () => {
User.subscribeToUserEvents();
return waitForPromisesToResolve();
})
.then(() => TestHelper.fetchPersonalDetailsForTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN, {
[TEST_USER_LOGIN]: {
accountID: TEST_USER_ACCOUNT_ID,
email: TEST_USER_LOGIN,
firstName: 'Test',
lastName: 'User',
},
}))
.then(() => TestHelper.setPersonalDetails(TEST_USER_LOGIN, TEST_USER_ACCOUNT_ID))
.then(() => {
// This is a fire and forget response, but once it completes we should be able to verify that we
// have an "optimistic" report action in Onyx.
Expand Down Expand Up @@ -183,14 +176,7 @@ describe('actions/Report', () => {

// GIVEN a test user with initial data
return TestHelper.signInWithTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN)
.then(() => TestHelper.fetchPersonalDetailsForTestUser(TEST_USER_ACCOUNT_ID, TEST_USER_LOGIN, {
[TEST_USER_LOGIN]: {
accountID: TEST_USER_ACCOUNT_ID,
email: TEST_USER_LOGIN,
firstName: 'Test',
lastName: 'User',
},
}))
.then(() => TestHelper.setPersonalDetails(TEST_USER_LOGIN, TEST_USER_ACCOUNT_ID))
.then(() => {
global.fetch = TestHelper.getGlobalFetchMock();

Expand Down Expand Up @@ -243,14 +229,7 @@ describe('actions/Report', () => {
User.subscribeToUserEvents();
return waitForPromisesToResolve();
})
.then(() => TestHelper.fetchPersonalDetailsForTestUser(USER_1_ACCOUNT_ID, USER_1_LOGIN, {
[USER_1_LOGIN]: {
accountID: USER_1_ACCOUNT_ID,
email: USER_1_LOGIN,
firstName: 'Test',
lastName: 'User',
},
}))
.then(() => TestHelper.setPersonalDetails(USER_1_LOGIN, USER_1_ACCOUNT_ID))
.then(() => {
// When a Pusher event is handled for a new report comment
channel.emit(Pusher.TYPE.ONYX_API_UPDATE, [
Expand Down
59 changes: 31 additions & 28 deletions tests/utils/TestHelper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import Onyx from 'react-native-onyx';
import CONST from '../../src/CONST';
import * as Session from '../../src/libs/actions/Session';
import * as PersonalDetails from '../../src/libs/actions/PersonalDetails';
import HttpUtils from '../../src/libs/HttpUtils';
import ONYXKEYS from '../../src/ONYXKEYS';
import waitForPromisesToResolve from './waitForPromisesToResolve';
import * as ReportUtils from '../../src/libs/ReportUtils';

/**
* Simulate signing in and make sure all API calls in this flow succeed. Every time we add
Expand Down Expand Up @@ -51,32 +54,6 @@ function signInWithTestUser(accountID = 1, login = 'test@user.com', password = '
});
}

/**
* Fetch and set personal details with provided personalDetailsList
*
* @param {Number} accountID
* @param {String} email
* @param {Object} personalDetailsList
* @returns {Promise}
*/
function fetchPersonalDetailsForTestUser(accountID, email, personalDetailsList) {
const originalXHR = HttpUtils.xhr;
HttpUtils.xhr = jest.fn();

HttpUtils.xhr
.mockImplementationOnce(() => Promise.resolve({
accountID,
email,
personalDetailsList,
}));

PersonalDetails.fetchPersonalDetails();
return waitForPromisesToResolve()
.then(() => {
HttpUtils.xhr = originalXHR;
});
}

/**
* Use for situations where fetch() is required.
*
Expand All @@ -98,8 +75,34 @@ function getGlobalFetchMock() {
});
}

/**
* @param {String} login
* @param {Number} accountID
* @returns {Promise}
*/
function setPersonalDetails(login, accountID) {
const avatar = ReportUtils.getDefaultAvatar(login);
const details = {
accountID,
login,
avatar,
displayName: 'Test User',
firstName: 'Test',
lastName: 'User',
pronouns: '',
timezone: CONST.DEFAULT_TIME_ZONE,
payPalMeAddress: '',
phoneNumber: '',
avatarHighResolution: avatar,
};
Onyx.merge(ONYXKEYS.PERSONAL_DETAILS, {
[login]: details,
});
return waitForPromisesToResolve();
}

export {
getGlobalFetchMock,
signInWithTestUser,
fetchPersonalDetailsForTestUser,
setPersonalDetails,
};