-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
[HOLD for payment 2023-03-13] [$4000] Able to change timezone through direct link even when the automatically determine location switch is on #14340
Comments
I can replicate the reported behavior. Setting the timezone via the link does not update the timezone set on my profile or in the timezone selection list. Looks like making the change via the link does not override the "automatically determine toggle" |
Asking team about expected behavior here |
Team agreed that the behavior described here isn't ideal. Suggested solution is when |
Job added to Upwork: https://www.upwork.com/jobs/~01b8d0920a25d93b4f |
Triggered auto assignment to @kevinksullivan ( |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr ( |
Current assignee @tylerkaraszewski is eligible for the External assigner, not assigning anyone new. |
Proposal diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 7a89ac8ce..625ad8347 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -33,6 +33,7 @@ class TimezoneSelectPage extends Component {
this.saveSelectedTimezone = this.saveSelectedTimezone.bind(this);
this.filterShownTimezones = this.filterShownTimezones.bind(this);
+ this.isAutomaticTimezone = lodashGet(props.currentUserPersonalDetails, 'timezone.automatic', CONST.DEFAULT_TIME_ZONE.automatic);
this.currentSelectedTimezone = lodashGet(props.currentUserPersonalDetails, 'timezone.selected', CONST.DEFAULT_TIME_ZONE.selected);
this.allTimezones = _.chain(moment.tz.names())
@@ -55,6 +56,13 @@ class TimezoneSelectPage extends Component {
};
}
+ componentDidMount() {
+ if (!this.isAutomaticTimezone) {
+ return;
+ }
+ Navigation.navigate(ROUTES.SETTINGS_TIMEZONE);
+ }
+
/**
* @param {Object} timezone
* @param {String} timezone.text |
Looks like something related to As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our Feel free to drop a note in #expensify-open-source with any questions. |
Proposal Check the timezone iff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 7a89ac8ce7..fe31c60405 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -34,7 +34,13 @@ class TimezoneSelectPage extends Component {
this.saveSelectedTimezone = this.saveSelectedTimezone.bind(this);
this.filterShownTimezones = this.filterShownTimezones.bind(this);
- this.currentSelectedTimezone = lodashGet(props.currentUserPersonalDetails, 'timezone.selected', CONST.DEFAULT_TIME_ZONE.selected);
+ const timezoneDetails = lodashGet(props.currentUserPersonalDetails, 'timezone', CONST.DEFAULT_TIME_ZONE);
+ this.currentSelectedTimezone = timezoneDetails.selected;
+
+ if (timezoneDetails.automatic) {
+ Navigation.navigate(ROUTES.SETTINGS_TIMEZONE);
+ }
+
this.allTimezones = _.chain(moment.tz.names()) Screen.Recording.2023-01-27.at.2.41.47.AM.mp4 |
Proposal1st. First, we need to introduce a new parameter to the navigate function whether we want to replace the route or not. Then, just like the same thing the others done above is to check whether the timezone is set to automatic or not. If yes, we will navigate to timezone initial page with the replace set to true. diff --git a/src/libs/Navigation/Navigation.js b/src/libs/Navigation/Navigation.js
index 3635a7840..f2d38e207 100644
--- a/src/libs/Navigation/Navigation.js
+++ b/src/libs/Navigation/Navigation.js
@@ -132,7 +132,7 @@ function isDrawerRoute(route) {
* Main navigation method for redirecting to a route.
* @param {String} route
*/
-function navigate(route = ROUTES.HOME) {
+function navigate(route = ROUTES.HOME, replace = false) {
if (!canNavigate('navigate', {route})) {
// Store intended route if the navigator is not yet available,
// we will try again after the NavigationContainer is ready
@@ -159,7 +159,7 @@ function navigate(route = ROUTES.HOME) {
return;
}
- linkTo(navigationRef.current, route);
+ linkTo(navigationRef.current, route, replace);
}
/**
diff --git a/src/libs/Navigation/linkTo.js b/src/libs/Navigation/linkTo.js
index 04977e6a6..3c6af3a2a 100644
--- a/src/libs/Navigation/linkTo.js
+++ b/src/libs/Navigation/linkTo.js
@@ -1,10 +1,12 @@
import {
getStateFromPath,
getActionFromState,
+ StackActions,
} from '@react-navigation/core';
import linkingConfig from './linkingConfig';
-export default function linkTo(navigation, path) {
+export default function linkTo(navigation, path, replace) {
const normalizedPath = !path.startsWith('/') ? `/${path}` : path;
if (navigation === undefined) {
throw new Error("Couldn't find a navigation object. Is your component inside a screen in a navigator?");
@@ -27,7 +29,11 @@ export default function linkTo(navigation, path) {
root = current;
}
- const action = getActionFromState(state, linkingConfig.config);
+ let action = getActionFromState(state, linkingConfig.config);
+
+ if (replace) {
+ action = {...action, type: 'REPLACE'};
+ }
if (action !== undefined) {
root.dispatch(action);
diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 7a89ac8ce..28b6b214e 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -34,7 +34,11 @@ class TimezoneSelectPage extends Component {
this.saveSelectedTimezone = this.saveSelectedTimezone.bind(this);
this.filterShownTimezones = this.filterShownTimezones.bind(this);
- this.currentSelectedTimezone = lodashGet(props.currentUserPersonalDetails, 'timezone.selected', CONST.DEFAULT_TIME_ZONE.selected);
+ this.currentTimezone = lodashGet(props.currentUserPersonalDetails, 'timezone', CONST.DEFAULT_TIME_ZONE);
+ this.currentSelectedTimezone = this.currentTimezone.selected;
+ if (this.currentTimezone.automatic) {
+ Navigation.navigate(ROUTES.SETTINGS_TIMEZONE, true);
+ }
this.allTimezones = _.chain(moment.tz.names())
.filter(timezone => !timezone.startsWith('Etc/GMT'))
.map(timezone => ({
@@ -82,7 +86,7 @@ class TimezoneSelectPage extends Component {
onBackButtonPress={() => Navigation.navigate(ROUTES.SETTINGS_TIMEZONE)}
onCloseButtonPress={() => Navigation.dismissModal(true)}
/>
- <OptionsSelector
+ {!this.currentTimezone.automatic && <OptionsSelector
textInputLabel={this.props.translate('timezonePage.timezone')}
value={this.state.timezoneInputText}
onChangeText={this.filterShownTimezones}
@@ -90,7 +94,7 @@ class TimezoneSelectPage extends Component {
optionHoveredStyle={styles.hoveredComponentBG}
sections={[{data: this.state.timezoneOptions}]}
shouldHaveOptionSeparator
- />
+ />}
</ScreenWrapper>
);
}
2nd. diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 7a89ac8ce..b4d8052db 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -34,7 +34,12 @@ class TimezoneSelectPage extends Component {
this.saveSelectedTimezone = this.saveSelectedTimezone.bind(this);
this.filterShownTimezones = this.filterShownTimezones.bind(this);
- this.currentSelectedTimezone = lodashGet(props.currentUserPersonalDetails, 'timezone.selected', CONST.DEFAULT_TIME_ZONE.selected);
+ this.currentTimezone = lodashGet(props.currentUserPersonalDetails, 'timezone', CONST.DEFAULT_TIME_ZONE);
+ this.currentSelectedTimezone = this.currentTimezone.selected;
+ if (this.currentTimezone.automatic) {
+ Navigation.goBack();
+ Navigation.navigate(ROUTES.SETTINGS_TIMEZONE);
+ }
this.allTimezones = _.chain(moment.tz.names())
.filter(timezone => !timezone.startsWith('Etc/GMT'))
.map(timezone => ({
@@ -74,6 +79,10 @@ class TimezoneSelectPage extends Component {
}
I also add !this.currentTimezone.automatic to not render the page to prevent keyboard blinking as shown from the video below. 329549.t.mp4After applying the condition, here is how it looks. 329550.t.mp4 |
PreProposalThe TimezoneSelectPage component suffers from stale data issue and proposals won't work as intended.The stale data is on Although technically this is a different issue it seems necessary to fix the reported bug. This is how I'd fix the stale data issue: diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 7a89ac8ce7..634e947be9 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -31,28 +31,51 @@ class TimezoneSelectPage extends Component {
constructor(props) {
super(props);
+ this.getCurrentTimezone = this.getCurrentTimezone.bind(this);
+ this.getAllTimezones = this.getAllTimezones.bind(this);
this.saveSelectedTimezone = this.saveSelectedTimezone.bind(this);
this.filterShownTimezones = this.filterShownTimezones.bind(this);
- this.currentSelectedTimezone = lodashGet(props.currentUserPersonalDetails, 'timezone.selected', CONST.DEFAULT_TIME_ZONE.selected);
- this.allTimezones = _.chain(moment.tz.names())
+ this.timezone = this.getCurrentTimezone();
+ this.allTimezones = this.getAllTimezones();
+
+ this.state = {
+ timezoneInputText: this.timezone.selected,
+ timezoneOptions: this.allTimezones,
+ };
+ }
+
+ componentDidUpdate(prevProps) {
+ if (lodashGet(prevProps.currentUserPersonalDetails, 'timezone') === lodashGet(this.props.currentUserPersonalDetails, 'timezone')) {
+ return;
+ }
+
+ this.timezone = this.getCurrentTimezone();
+ this.allTimezones = this.getAllTimezones();
+ this.setState({
+ timezoneInputText: this.timezone.selected,
+ timezoneOptions: this.allTimezones,
+ });
+ }
+
+ getCurrentTimezone() {
+ return lodashGet(this.props.currentUserPersonalDetails, 'timezone', CONST.DEFAULT_TIME_ZONE);
+ }
+
+ getAllTimezones() {
+ return _.chain(moment.tz.names())
.filter(timezone => !timezone.startsWith('Etc/GMT'))
.map(timezone => ({
text: timezone,
keyForList: timezone,
// Include the green checkmark icon to indicate the currently selected value
- customIcon: timezone === this.currentSelectedTimezone ? greenCheckmark : undefined,
+ customIcon: timezone === this.timezone.selected ? greenCheckmark : undefined,
// This property will make the currently selected value have bold text
- boldStyle: timezone === this.currentSelectedTimezone,
+ boldStyle: timezone === this.timezone.selected,
}))
.value();
-
- this.state = {
- timezoneInputText: this.currentSelectedTimezone,
- timezoneOptions: this.allTimezones,
- };
}
/** IMPORTANT: The proposals below are based on that proposal being applied first. NOTE: There is also another issue with Proposal 1diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 634e947be9..0fe9581ed3 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -37,6 +37,9 @@ class TimezoneSelectPage extends Component {
this.filterShownTimezones = this.filterShownTimezones.bind(this);
this.timezone = this.getCurrentTimezone();
+ if (this.timezone.automatic) {
+ Navigation.navigate(ROUTES.SETTINGS_TIMEZONE);
+ }
this.allTimezones = this.getAllTimezones();
this.state = {
@@ -51,6 +54,9 @@ class TimezoneSelectPage extends Component {
}
this.timezone = this.getCurrentTimezone();
+ if (this.timezone.automatic) {
+ Navigation.navigate(ROUTES.SETTINGS_TIMEZONE);
+ }
this.allTimezones = this.getAllTimezones();
this.setState({
timezoneInputText: this.timezone.selected, DetailsAfter applying the PreProposal we can just follow it with any of the above proposals However it won't cover all the cases. One case that I have in mind is cache related where you will still get the timezones list. You can experience this issue by following those steps:
Kooha-2023-01-28-15-32-56.mp4Proposal 2diff --git a/src/libs/actions/PersonalDetails.js b/src/libs/actions/PersonalDetails.js
index 9f0247a75f..5fadb808d5 100644
--- a/src/libs/actions/PersonalDetails.js
+++ b/src/libs/actions/PersonalDetails.js
@@ -198,8 +198,7 @@ function updateAutomaticTimezone(timezone) {
}
/**
- * Updates user's 'selected' timezone, then navigates to the
- * initial Timezone page.
+ * Updates user's 'selected' timezone.
*
* @param {String} selectedTimezone
*/
@@ -220,7 +219,6 @@ function updateSelectedTimezone(selectedTimezone) {
},
}],
});
- Navigation.navigate(ROUTES.SETTINGS_TIMEZONE);
}
/**
diff --git a/src/pages/settings/Profile/TimezoneSelectPage.js b/src/pages/settings/Profile/TimezoneSelectPage.js
index 634e947be9..79c6844ded 100644
--- a/src/pages/settings/Profile/TimezoneSelectPage.js
+++ b/src/pages/settings/Profile/TimezoneSelectPage.js
@@ -83,7 +83,15 @@ class TimezoneSelectPage extends Component {
* @param {String} timezone.text
*/
saveSelectedTimezone({text}) {
- PersonalDetails.updateSelectedTimezone(text);
+ if (this.timezone.automatic) {
+ PersonalDetails.updateAutomaticTimezone({
+ automatic: false,
+ selected: text,
+ });
+ } else {
+ PersonalDetails.updateSelectedTimezone(text);
+ }
+ Navigation.navigate(ROUTES.SETTINGS_TIMEZONE);
}
/** DetailsThis proposal is based on the initially mentioned expected results. Kooha-2023-01-28-15-40-37.mp4Proposal 1 vs. Proposal 2Personally, I think we should go with proposal 2 for two reasons:
|
Proposal
So this is the expected results:
To achieve the expected results (1, 2, and 4), we should reset the diff --git a/src/libs/actions/PersonalDetails.js b/src/libs/actions/PersonalDetails.js
index 9f0247a75f..d076c50e71 100644
--- a/src/libs/actions/PersonalDetails.js
+++ b/src/libs/actions/PersonalDetails.js
@@ -205,6 +205,7 @@ function updateAutomaticTimezone(timezone) {
*/
function updateSelectedTimezone(selectedTimezone) {
const timezone = {
+ automatic: false,
selected: selectedTimezone,
};
API.write('UpdateSelectedTimezone', {
The expected result 3 : This is a separate issue that should be addressed separately.RCAWhen deep linking any page in the settings stack, the first screen displayed will become the initial screen and remain mounted until the settings modal is dismissed. This problem occurs on every screen within the settings stack. It is recommended to create a separate issue for this and hold it prior to releasing the new navigation. TestsThese tests demonstrate that when we deep link certain pages and then click the back button, the initial page will remain mounted until we close the settings modal. I have only included a few pages to illustrate the issue, but every page within the settings modal has this problem. Test 1
|
Thank you for the proposals! @Pujan92 @bernhardoj Thanks for noticing the keyboard visibility while redirecting to the settings timezone page! For your first proposal, I don't think we want to change anything in Also, could you explain why putting the navigation inside the constructor rather than in the For the keyboard appearance issue, we can automatically set the |
Thanks for the payment @kevinksullivan, just wondering whether this qualifies for the bonus? As all the work by me & the review from @mollfpr was done within 3 business days, we were just waiting for @tylerkaraszewski's review for quite some time. |
12k total (including C+ review and timeline bonus) is huge bounty for this issue 😮 |
@priyeshshah11 @tylerkaraszewski @mollfpr @kevinksullivan Linked PR to this issue causing regression and broke the scroll to initial focused option functionality. Screen.Recording.2023-03-24.at.8.12.52.PM.movSharing possible solution
Screen.Recording.2023-03-24.at.8.13.09.PM.mov |
@tylerkaraszewski, @kevinksullivan, @mollfpr, @priyeshshah11 Huh... This is 4 days overdue. Who can take care of this? |
@tylerkaraszewski, @kevinksullivan, @mollfpr, @priyeshshah11 6 days overdue. This is scarier than being forced to listen to Vogon poetry! |
Is there any action required by me? |
Was just checking our policy on regressions beyond 7 days. It looks like this should still qualify for the bonus. Sent another offer to get this squared away @priyeshshah11 @mollfpr |
Thanks @kevinksullivan but the offers shows as withdrawn for me and I can't accept/apply. Could you please resend? |
Thanks @kevinksullivan accepted! |
@priyeshshah11 hm, upwork seems to be throwing an error each time I try to invite you. I'll try again. |
I don't know what's going on here. It makes it seem as though it worked and then doesn't show you in the list. Want to try applying and I'll accept the proposal from there? Sorry about this @priyeshshah11 |
@kevinksullivan Got the offer and accepted now! Thanks 👍 |
@tylerkaraszewski, @kevinksullivan, @mollfpr, @priyeshshah11 Whoops! This issue is 2 days overdue. Let's get this updated quick! |
@kevinksullivan - this issue shows as overdue but I'm not sure what action we're waiting on. A PR from @priyeshshah11 ? |
Just waiting to finish payment @tylerkaraszewski |
@priyeshshah11 it looks like the offer is still pending... Does it show as accepted on your end? |
@kevinksullivan accepted & requested payment |
sheesh, sorry about all the back and forth @priyeshshah11 . It seems I finally got this to work. Closing out! |
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Action Performed:
Expected Result:
The controls should be disabled, with only the ability to go back
Actual Result:
After updating the timezone directly through the link, the new timezone is displayed on the timezone initial page, but
The old timezone is displayed on the profile page
The old timezone is selected on the timezone list page
The automatically determine location switch doesn't work
Workaround:
unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Version Number: 1.2.54-2
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:
Screen.Recording.2023-01-16.at.5.32.35.PM.mov
Recording.1305.mp4
Expensify/Expensify Issue URL:
Issue reported by: @adeel0202
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1673873060216069
View all open jobs on GitHub
Upwork Automation - Do Not Edit
The text was updated successfully, but these errors were encountered: