FEATURE: Add timezone to core user_options (PR #8380)

[martin]

Based on internal discussions, in preparation for improvements to bookmarks that will include reminders, we are moving the timezone for a user from a UserCustomField provided by to a core field on UserOption. The user can set this in their profile.

As well as the manual setting, whenever a user logs in or accepts the initial Discourse invitation, we guess their timezone using moment.tz.guess(), and save it against their user IF they have not already set a timezone in their profile.

GitHub

[discoursebot]

You’ve signed the CLA, martin-brennan. Thank you! This pull request is ready for review.

[jjaffeux]

Looks good to only I’m not sure the timezone_guess naming is needed. I think we can go with timezone everywhere.

[jjaffeux]

Also it would be nice to make a PR to discourse-calendar to ensure they work together and don’t do the same thing two times (might need some migration?).

Might we worth checking discourse-group-timezones too, but I can do this if you want.

[eviltrout]

app/models/user.rb

@@ -670,6 +670,16 @@ def update_visit_record!(date)
     create_visit_record!(date) unless visit_record_for(date)
   end
 
+  def update_timezone_if_missing(timezone)
+    return if timezone.blank?
+
+    # we only want to update the user's timezone if they have not set it themselves
+    UserOption

There’s no need to select the timezone before updating it here. You could do this:

UserOption.where(user_id: self.id, timezone: nil).update_all(timezone: timezone)

It will be one SQL query instead of two.

[eviltrout]

Looking good so far. I’ve suggested a couple of changes.

[eviltrout]

db/migrate/20191120015344_add_timezone_to_user_options.rb

@@ -0,0 +1,19 @@
+# frozen_string_literal: true
+
+class AddTimezoneToUserOptions < ActiveRecord::Migration[6.0]
+  def up
+    add_column :user_options, :timezone, :string, index: true

Curious why you’d like to index the timezone column here. Wouldn’t we always want to retrieve it by user_id?

[eviltrout]

spec/models/user_spec.rb

@@ -917,6 +917,37 @@ def assert_good(username)
     end
   end
 
+  describe "update_timezone_if_missing" do
+    let(:timezone) { nil }
+
+    it "does nothing if timezone is nil" do
+      UserOption.expects(:select).never

We prefer to avoid stubbing. For example I suggested an update above instead of a select, which would allow this test to pass even if it wasn’t working.

It would be better if you made a user object with at timezone, called this method and then checked that the timezone didn’t change.

[martin]

Ah thank you for that, was trying to golf it to the smallest query possible. Will change

[martin]

No problem

[martin]

Also it would be nice to make a PR to discourse-calendar to ensure they work together and don’t do the same thing two times (might need some migration?).

Might we worth checking discourse-group-timezones too, but I can do this if you want.

@jjaffeux I will make a PR for discourse-calendar today, that is part of my plan. I checked on discourse-group-timezones already and all it does is expect there to be a timezone at the group level, which discourse-calendar handles, so no changes need to be made there. Will test all the changes in tandem locally as well before I mark as non-wip

[martin]

Yeah I suppose you are right, the only thing it would be useful for is retrieving all the users in a timezone in a query, which I’m not sure we need right now. I can remove it and we can always put it back in later if we need

[martin]

Weird that Travis qunit failed here, the module that failed passes locally for me.

[SamSaffron]

A few questions:

1. Do we want any level of validation on timezone, technically this allows users to set timezone as “Mars” or some giant string?

2. Since it is clear we want to validate, what do we do when an invalid timezone is sent to the API? What if the timezones in moment are out of sync with the timezones rails knows about?

3. It looks like this is only set on login, will this be set when you first sign up and simply activate your account?

Long term though, what happens when timezone on laptop is out of sync with the user setting, eg you travel to the Spain. Moment.js will know this happened but the server will not… manual dealing with this is fine for v1 but maybe long term we want to think about this

[martin]

@SamSaffron they are great points. I can definitely validate the timezone, I was wondering about that as well. I think if we are setting the timezone for the user on login and it’s a bogus timezone we just leave it nil for them. And if they try to save their own bogus timezone from their profile, just pop a 422 error.

RE: moment.js issue, I think it’s a slim chance that Rails and moment.js timezone lists will become out of sync, they both use the IANA timezone list IANA — Time Zone Database. BUT if they do we could be logging when an invalid timezone is encountered for a user, so we can track down these mismatches when they do occur.

It looks like this is only set on login, will this be set when you first sign up and simply activate your account?

I’m also setting it on accept invitation, but I will set it on the registration form too.

I definitely think we will need to consider the moving around issue at some point. I guess a deferred modal on login that pops up and says “Hey, your timezone is set to Australia/Brisbane on Discourse, but it looks like you are Olé-ing in Spain right now, do you want to set your timezone to Spain/Madrid?”

[martin]

So, when a user saves the timezone in their profile page and somehow give it garbage this is caught in UserOption's validator and shown to the user:

Any invalid timezone detected is also logged with Rails.logger.warn so we can debug if need be. As said if they are setting their timezone on login/invitation and its invalid (somehow, moment timezone sync issues perhaps) then we just log the issue and do nothing to their timezone.

Also, I now set the user’s timezone on signup!