Remove automagic timezone changing #32371

tomneedham · 2018-08-17T12:22:48Z

Description

No automagic timezone set on login, which doesnt work when using SSO, in favour of selector in personal settings. Later, we could add the auto detection to flag up that you are in a different timezone, and maybe you want to change your settings to your current timezone.

Related Issue

Fixes User timezone is not updated when using auth methods != credentials #32165

Motivation and Context

Automagic timezone changing when you travel affects everything you see in ownCloud, including activity emails. If you don't 'login' then this wont happen, so it is unreliable. AND this only happens when using the login form, SSO does not use this.

How Has This Been Tested?

test environment:
test case 1:
test case 2:
...

Screenshots (if appropriate):

Nothing set, default when you login:

When you have a timezone set, or preset by your admin (oc_preferences table, app=core, configkey=timezone)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)
Not sure if timezone lib is used elsewhere, remove from package.json ?
Unit tests on SetTimezoneController

tomneedham · 2018-08-17T14:14:26Z

Codestyle and unit tests need to be updated - can do if this is the way we want to go... @PVince81

PVince81

Looks good overall, see comments.

I wonder if we need a "default" choice or "auto" which would fall back to the old behavior.

PVince81 · 2018-08-22T15:11:05Z

core/templates/login.php

@@ -58,7 +56,7 @@
 				<?php p($_['user_autofocus'] ? '' : 'autofocus'); ?>
 				autocomplete="on" autocapitalize="off" autocorrect="off" required>
 			<label for="password" class="infield"><?php p($l->t('Password')); ?></label>
-			<input type="submit" id="submit" class="login primary icon-confirm" title="<?php p($l->t('Log in')); ?>" value="" disabled="disabled"/>
+			<input type="submit" id="submit" class="login primary icon-confirm" title="<?php p($l->t('Log in')); ?>" value=""/>


why remove the disabled property ?

because it was enabled when the timezone was set - so without removing this it is always disabled

PVince81 · 2018-08-22T15:11:32Z

settings/Application.php

@@ -152,6 +153,15 @@ public function __construct(array $urlParams=[]) {
 				$c->query('Config')
 			);
 		});
+		$container->registerService('SetTimezoneController', function (IContainer $c) {


call it just "TimezoneController maybe, who knows what future methods it will have like for example querying all supported timezones ?

PVince81 · 2018-08-22T15:12:18Z

settings/Controller/SetTimezoneController.php

+namespace OC\Settings\Controller;
+
+use OC\AppFramework\Http;
+use \OCP\AppFramework\Controller;


remove leading backslash for consistency ?

PVince81 · 2018-08-22T15:12:54Z

settings/Controller/SetTimezoneController.php

+		IConfig $config,
+		IL10N $l,
+		IUserSession $session) {
+		parent::__construct($appName, $request);


formatting is weird, not really readable.
I think you should move the ) { on the next line

PVince81 · 2018-08-22T15:13:26Z

settings/Controller/SetTimezoneController.php

+	public function set() {
+
+		$available = \DateTimeZone::listIdentifiers();
+		if (!in_array($this->request->getParam('timezoneInput'), $available)) {


always add true in in_array to make it strict (doesn't PHPStorm mention this?)

PVince81 · 2018-08-22T15:14:56Z

settings/Panels/Personal/Profile.php

+				'core',
+				'timezone',
+				null));
+		$timezoneSelector->assign('timezones', \DateTimeZone::listIdentifiers());


this is ok for now.

will not work for Phoenix, so we'll later need to add a get method on the TimezoneController so Phoenix knows what values to use to populate. this is only to push my aadvice to rename SetTimezoneController to TimezoneController

PVince81 · 2018-08-22T15:15:19Z

settings/js/panels/profile.js

@@ -230,12 +230,25 @@ $(document).ready(function () {
 				location.reload();
 			}
 			else {
-				$('#passworderror').text(data.data.message);
+				OC.Notification.showTemporary(t('settings', 'Error saving language: {message}', { message: data.data.message }));


error saving timezone, not language ?

no, this is language - see whole file. The language save error handler was piggy backing on the password error div for its error message 🙈 which gets worse, when I was testing with LDAP so there was no password change box.

PVince81 · 2018-08-22T15:15:48Z

settings/js/panels/profile.js

 			}
 		});
 		return false;
 	});

+	$("#timezoneInput").change(function () {
+		// Serialize the data
+		var post = $("#timezoneInput").serialize();


ohhhh, never saw this serialize method before

PVince81 · 2018-08-22T15:17:28Z

@pmaier1 FYI

pmaier1 · 2018-08-22T16:04:13Z

👍 Maybe you want to rename the default timezone simply to "Default" (instead of "Server default" which I find unnecessary technical :) )

tomneedham · 2018-08-22T23:28:42Z

Actually talked to @DeepDiver1975 and he pointed out the solution for timezone addition in shibboleth. So maybe we keep it, but allow manual setting of it?

DeepDiver1975 · 2018-08-23T12:43:07Z

Actually talked to @DeepDiver1975 and he pointed out the solution for timezone addition in shibboleth. So maybe we keep it, but allow manual setting of it?

unless I finally understand the need to remove the auto-setting of timezone from the login page I prefer to keep it. We recently fixed it to solve some customer time zone issues.

Having an manual select option is fine.

codecov · 2018-08-23T13:10:06Z

Codecov Report

Merging #32371 into master will decrease coverage by 15.98%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master   #32371       +/-   ##
=============================================
- Coverage     64.01%   48.02%   -15.99%     
=============================================
  Files          1176      109     -1067     
  Lines         70233    10394    -59839     
  Branches       1267     1267               
=============================================
- Hits          44961     4992    -39969     
+ Misses        24902     5032    -19870     
  Partials        370      370

Flag	Coverage Δ	Complexity Δ
#javascript	`52.84% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`37.32% <ø> (-27.96%)`	`0 <ø> (-18666)`

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/Storage/DAV.php	`59.45% <0%> (-21.64%)`	`0% <0%> (ø)`
...ate/Files/External/Service/UserStoragesService.php
lib/private/L10N/L10N.php
apps/files_trashbin/lib/Expiration.php
...mments/tests/unit/Dav/EntityTypeCollectionTest.php
lib/base.php
apps/updatenotification/templates/admin.php
lib/private/DB/Connection.php
lib/private/Config.php
apps/federatedfilesharing/lib/TokenHandler.php
... and 1055 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b30659c...634fba6. Read the comment docs.

tomneedham · 2018-08-23T13:10:13Z

Pushed to address comments. @pmaier1 to decide if we want only automagic, automagic+manual option, or only manual? I can update this PR accoridingly

pmaier1 · 2018-08-23T13:17:42Z

Well, I agree a lot with #32165 (comment) and #32165 (comment) so I would tend to only go for the manual option. Still I want to respect @DeepDiver1975 in #32371 (comment). @DeepDiver1975 does the reasoning in the first two comments make sense to you or why would you want to keep automagic?

Depending on @DeepDiver1975's answer I'd say we either:

make the compromise and keep automagic but add the manual option additionally
just go for the manual option

Question for the compromise way: When users set the manual option, would that imply disabling automagic then?

tomneedham · 2018-08-23T13:42:16Z

Question for the compromise way: When users set the manual option, would that imply disabling automagic then?

In this case, I would suggest an 'option' in the dropdown is 'auto'. If you select 'uk/london' you automatically unselect auto.

pmaier1 · 2018-08-23T14:05:40Z

Nice, I think that's the way to go 👌

DeepDiver1975 · 2018-08-23T14:25:06Z

Okay - so to open the can of worms completly:

generally speaking the server should send any timestamps in a timezone neutral form or with the explicit timezone when
any client has to convert this timestamp to the time zone of the local client

This way the client will always get the time right - thinking about the traveling use case.

The only reason why the user needs to specify the timezone in the server is for server side generated content - like emails. In this case one does not know the timezone of the client.
Therefore we need the setting.

^ all this applies to language exactly the same way - with one additional feature: there is a http header Accept-Language which the client is sending to the server to tell the desired language. This was the server can generate content in the correct language. There is no Accept-TimeZone header ... just in case you are asking ;-)

pmaier1 · 2018-08-23T15:14:27Z

generally speaking the server should send any timestamps in a timezone neutral form or with the explicit timezone when

The server can do that still, right? If I'm not mistaken the only question to answer here is: Do I want to set a fixed timezone on the server or do I want it to dynamically change on login. Clients can then still convert it as they wish to.

Not sure if I get all implications right.

When using automagic timezone the drawback is that server-generated things like mails use the timezone that was detected on last login. This implies that the timing of these actions follows users when travelling.
When using a fixed timezone the drawback is that the time in the webUI will not match when I'm in a different timezone as long as I don't change it it in the settings

I'm missing some examples to get a better understanding of the implications.

PVince81 · 2018-08-23T15:41:02Z

Hmmm, is the web UI even affected by the timezone change ? I'm not sure.

Maybe the setting could be called "Timezone to use in email communication" or something related to server-only things ?

tomneedham · 2018-08-23T15:43:47Z

Maybe the setting could be called "Timezone to use in email communication" or something related to server-only things ?

By this point, we are creating a mess for end users......

tomneedham · 2018-10-01T10:26:59Z

Closing this as this only affects users of custom user backends - and their specific implementations.

tomneedham added the 3 - To Review label Aug 17, 2018

tomneedham self-assigned this Aug 17, 2018

ownclouders added Type:Bug p3-medium Normal priority labels Aug 17, 2018

tomneedham force-pushed the user-timezone branch from c6aaba3 to 0ef1b17 Compare August 17, 2018 12:23

PVince81 reviewed Aug 22, 2018

View reviewed changes

tomneedham force-pushed the user-timezone branch from 0ef1b17 to 4059ab4 Compare August 23, 2018 13:09

Remove automagic timezone setting on login

634fba6

tomneedham force-pushed the user-timezone branch from 4059ab4 to 634fba6 Compare August 23, 2018 13:11

tomneedham closed this Oct 1, 2018

lock bot locked as resolved and limited conversation to collaborators Oct 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove automagic timezone changing #32371

Remove automagic timezone changing #32371

tomneedham commented Aug 17, 2018

tomneedham commented Aug 17, 2018

PVince81 left a comment

PVince81 Aug 22, 2018

tomneedham Aug 22, 2018

PVince81 Aug 22, 2018

PVince81 Aug 22, 2018

PVince81 Aug 22, 2018

PVince81 Aug 22, 2018

PVince81 Aug 22, 2018

PVince81 Aug 22, 2018

tomneedham Aug 22, 2018

PVince81 Aug 22, 2018

PVince81 commented Aug 22, 2018

pmaier1 commented Aug 22, 2018

tomneedham commented Aug 22, 2018

DeepDiver1975 commented Aug 23, 2018

codecov bot commented Aug 23, 2018 •

edited

Loading

tomneedham commented Aug 23, 2018

pmaier1 commented Aug 23, 2018 •

edited

Loading

tomneedham commented Aug 23, 2018

pmaier1 commented Aug 23, 2018

DeepDiver1975 commented Aug 23, 2018

pmaier1 commented Aug 23, 2018 •

edited

Loading

PVince81 commented Aug 23, 2018

tomneedham commented Aug 23, 2018

tomneedham commented Oct 1, 2018

Remove automagic timezone changing #32371

Remove automagic timezone changing #32371

Conversation

tomneedham commented Aug 17, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

tomneedham commented Aug 17, 2018

PVince81 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Aug 22, 2018

pmaier1 commented Aug 22, 2018

tomneedham commented Aug 22, 2018

DeepDiver1975 commented Aug 23, 2018

codecov bot commented Aug 23, 2018 • edited Loading

Codecov Report

tomneedham commented Aug 23, 2018

pmaier1 commented Aug 23, 2018 • edited Loading

tomneedham commented Aug 23, 2018

pmaier1 commented Aug 23, 2018

DeepDiver1975 commented Aug 23, 2018

pmaier1 commented Aug 23, 2018 • edited Loading

PVince81 commented Aug 23, 2018

tomneedham commented Aug 23, 2018

tomneedham commented Oct 1, 2018

codecov bot commented Aug 23, 2018 •

edited

Loading

pmaier1 commented Aug 23, 2018 •

edited

Loading

pmaier1 commented Aug 23, 2018 •

edited

Loading