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

Ensure we're always including timezone in myPersonalDetails #2040

Merged
merged 4 commits into from
Mar 25, 2021
Merged

Ensure we're always including timezone in myPersonalDetails #2040

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
815c55f
ensure we're always including timezone in myPersonalDetails
NikkiWines Mar 24, 2021
07cd70c
Merge branch 'master' of github.com:Expensify/Expensify.cash into nik…
NikkiWines Mar 24, 2021
1fa1e20
skip migration is personalDetails key is empty
NikkiWines Mar 24, 2021
f3cfc16
use users old timezone selection and update to use constants where ap…
NikkiWines Mar 25, 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
8 changes: 1 addition & 7 deletions src/libs/DateUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import moment from 'moment';
import 'moment-timezone';
import _ from 'underscore';
import Onyx from 'react-native-onyx';
import Str from 'expensify-common/lib/str';
import ONYXKEYS from '../ONYXKEYS';
Expand All @@ -9,12 +8,7 @@ import CONST from '../CONST';
let timezone;
Onyx.connect({
key: ONYXKEYS.MY_PERSONAL_DETAILS,
callback: (val) => {
timezone = val ? val.timezone : CONST.DEFAULT_TIME_ZONE.selected;
if (_.isObject(timezone)) {
timezone = val.timezone.selected;
}
},
callback: (val) => { timezone = val ? val.timezone.selected : CONST.DEFAULT_TIME_ZONE.selected; },
});

/**
Expand Down
11 changes: 8 additions & 3 deletions src/libs/actions/PersonalDetails.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,16 @@ function fetch() {
returnValueList: 'personalDetailsList',
})
.then((data) => {
const allPersonalDetails = formatPersonalDetails(data.personalDetailsList);
let myPersonalDetails = {};

// If personalDetailsList is empty, ensure we set the personal details for the current user
const personalDetailsList = _.isEmpty(data.personalDetailsList)
? {[currentUserEmail]: myPersonalDetails}
: data.personalDetailsList;
const allPersonalDetails = formatPersonalDetails(personalDetailsList);
Onyx.merge(ONYXKEYS.PERSONAL_DETAILS, allPersonalDetails);

const myPersonalDetails = allPersonalDetails[currentUserEmail]
|| {avatar: getAvatar(undefined, currentUserEmail)};
myPersonalDetails = allPersonalDetails[currentUserEmail];

// Add the first and last name to the current user's MY_PERSONAL_DETAILS key
myPersonalDetails.firstName = lodashGet(data.personalDetailsList, [currentUserEmail, 'firstName'], '');
Expand Down
2 changes: 2 additions & 0 deletions src/libs/migrateOnyx.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import RenameActiveClientsKey from './migrations/RenameActiveClientsKey';
import RenamePriorityModeKey from './migrations/RenamePriorityModeKey';
import ReformatTimezone from './migrations/ReformatTimezone';

export default function () {
const startTime = Date.now();
Expand All @@ -10,6 +11,7 @@ export default function () {
const migrationPromises = [
RenameActiveClientsKey,
RenamePriorityModeKey,
ReformatTimezone,
];

// Reduce all promises down to a single promise. All promises run in a linear fashion, waiting for the
Expand Down
41 changes: 41 additions & 0 deletions src/libs/migrations/ReformatTimezone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import Onyx from 'react-native-onyx';
import _ from 'underscore';
import ONYXKEYS from '../../ONYXKEYS';
import CONST from '../../CONST';

// This migration changes the format of the timezone in the Onyx key MY_PERSONAL_DETAILS from a string to an object
export default function () {
return new Promise((resolve) => {
// Connect to the old key in Onyx to get the old value of myPersonalDetails timezone
// then update the timezone to be the default timezone and set the myPersonalDetails
// key with the updated values
const connectionID = Onyx.connect({
key: 'myPersonalDetails',
NikkiWines marked this conversation as resolved.
Show resolved Hide resolved
callback: (myPersonalDetails) => {
Onyx.disconnect(connectionID);

if (_.isUndefined(myPersonalDetails) || _.isEmpty(myPersonalDetails)) {
console.debug('[Migrate Onyx] Skipped migration ReformatTimezone: No myPersonalDetails key found');
return resolve();
}

// Fail early here because there is nothing to migrate, the timezone is already in the expected format
if (_.isObject(myPersonalDetails.timezone)) {
console.debug('[Migrate Onyx] Skipped migration ReformatTimezone');
return resolve();
}

// Replace the old timezone with the default timezone
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @NikkiWines! I'm confused as to why we're setting the old string timezone to the DEFAULT_TIME_ZONE when migrating?

My current understanding is this: the migration isn't supposed to actually change the timezone, it's just meant to change its format so that when you pick it up in DateUtil.js you can be sure that it's an object (to prevent all that conditional logic you had before when reading it from Onyx). Why would we default it to America/Los Angeles here?

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 a good point. I opted to use the default because we technically only have half the user's timezone NVP information if it's in string format. We'd be missing the automatic: true/false portion and using the default value instead just seemed like a simple solution rather than trying to guess half of their NVP value.

However, in retrospect, I think it's fine to use the old timezone value for timezone.selected and default to true, since the user can always modify that in their settings in E.cash.

const details = myPersonalDetails;
details.timezone = CONST.DEFAULT_TIME_ZONE;
Onyx.set({
[ONYXKEYS.MY_PERSONAL_DETAILS]: details,
})
.then(() => {
console.debug('[Migrate Onyx] Ran migration ReformatTimezone');
resolve();
});
},
});
});
}