From b8b29e79ad5341ee5a3c533b990434cc4feb42b3 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 24 Mar 2020 11:39:09 +1000 Subject: [PATCH] FIX: Improve user timezone saving (#9230) Based on issues identified in https://meta.discourse.org/t/improved-bookmarks-with-reminders/144542/20 * Implement the resolvedTimezone() function on the user model where we return the user's timezone if it has been set, or we guess it using moment and save it to the user using an update call if it has not yet been set. This covers the cases of users who do not log out/in often who will not get their timezone set via login. This also makes sure the guess + save is done in a non-obtrusive way not on every page -- only when it is needed. * Before if a user's timezone was blank when they visited their profile page we were autofilling the dropdown with the guessed timezone from moment. However this was confusing as it would appear you have that timezone saved in the DB when you really didn't. Now we do not autofill the dropdown and added a button to automatically guess the current timezone to make everything more explicit. --- .../discourse/controllers/bookmark.js | 2 +- .../controllers/preferences/profile.js | 6 ++- .../javascripts/discourse/models/bookmark.js | 2 +- .../javascripts/discourse/models/user.js | 33 ++++++++++++++- .../discourse/routes/preferences-profile.js | 5 --- .../templates/preferences/profile.hbs | 1 + .../discourse/widgets/post-menu.js | 2 +- app/assets/stylesheets/common/base/user.scss | 4 ++ config/locales/client.en.yml | 1 + .../controllers/bookmark-test.js.es6 | 40 ++++++++++++------- test/javascripts/models/user-test.js.es6 | 38 ++++++++++++++++++ 11 files changed, 109 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/bookmark.js b/app/assets/javascripts/discourse/controllers/bookmark.js index ecafa0dd23..18a6112a25 100644 --- a/app/assets/javascripts/discourse/controllers/bookmark.js +++ b/app/assets/javascripts/discourse/controllers/bookmark.js @@ -45,7 +45,7 @@ export default Controller.extend(ModalFunctionality, { customReminderTime: null, lastCustomReminderDate: null, lastCustomReminderTime: null, - userTimezone: this.currentUser.timezone + userTimezone: this.currentUser.resolvedTimezone() }); this.loadLastUsedCustomReminderDatetime(); diff --git a/app/assets/javascripts/discourse/controllers/preferences/profile.js b/app/assets/javascripts/discourse/controllers/preferences/profile.js index 57d8759ac8..71992e99fd 100644 --- a/app/assets/javascripts/discourse/controllers/preferences/profile.js +++ b/app/assets/javascripts/discourse/controllers/preferences/profile.js @@ -73,6 +73,10 @@ export default Controller.extend({ ); }, + useCurrentTimezone() { + this.model.set("user_option.timezone", moment.tz.guess()); + }, + save() { this.set("saved", false); @@ -95,7 +99,7 @@ export default Controller.extend({ // update the timezone in memory so we can use the new // one if we change routes without reloading the user if (this.currentUser.id === this.model.id) { - this.currentUser.timezone = this.model.user_option.timezone; + this.currentUser.changeTimezone(this.model.user_option.timezone); } cookAsync(model.get("bio_raw")) diff --git a/app/assets/javascripts/discourse/models/bookmark.js b/app/assets/javascripts/discourse/models/bookmark.js index 0473c10811..bb88e7da52 100644 --- a/app/assets/javascripts/discourse/models/bookmark.js +++ b/app/assets/javascripts/discourse/models/bookmark.js @@ -113,7 +113,7 @@ const Bookmark = RestModel.extend({ formattedReminder(bookmarkReminderAt) { const currentUser = PreloadStore.get("currentUser"); return moment - .tz(bookmarkReminderAt, currentUser.timezone || moment.tz.guess()) + .tz(bookmarkReminderAt, currentUser.resolvedTimezone()) .format(I18n.t("dates.long_with_year")); }, diff --git a/app/assets/javascripts/discourse/models/user.js b/app/assets/javascripts/discourse/models/user.js index 421376609a..7b106d9206 100644 --- a/app/assets/javascripts/discourse/models/user.js +++ b/app/assets/javascripts/discourse/models/user.js @@ -840,10 +840,40 @@ const User = RestModel.extend({ !secondFactorEnabled && (enforce === "all" || (enforce === "staff" && staff)) ); + }, + + resolvedTimezone() { + if (this._timezone) { + return this._timezone; + } + + this.changeTimezone(moment.tz.guess()); + ajax(userPath(this.username + ".json"), { + type: "PUT", + dataType: "json", + data: { timezone: this._timezone } + }); + + return this._timezone; + }, + + changeTimezone(tz) { + this._timezone = tz; } }); User.reopenClass(Singleton, { + munge(json) { + // timezone should not be directly accessed, use + // resolvedTimezone() and changeTimezone(tz) + if (!json._timezone) { + json._timezone = json.timezone; + delete json.timezone; + } + + return json; + }, + // Find a `User` for a given username. findByUsername(username, options) { const user = User.create({ username: username }); @@ -852,7 +882,7 @@ User.reopenClass(Singleton, { // TODO: Use app.register and junk Singleton createCurrent() { - const userJson = PreloadStore.get("currentUser"); + let userJson = PreloadStore.get("currentUser"); if (userJson && userJson.primary_group_id) { const primaryGroup = userJson.groups.find( @@ -864,6 +894,7 @@ User.reopenClass(Singleton, { } if (userJson) { + userJson = User.munge(userJson); const store = Discourse.__container__.lookup("service:store"); return store.createRecord("user", userJson); } diff --git a/app/assets/javascripts/discourse/routes/preferences-profile.js b/app/assets/javascripts/discourse/routes/preferences-profile.js index 02eb4df866..fb9a2ff527 100644 --- a/app/assets/javascripts/discourse/routes/preferences-profile.js +++ b/app/assets/javascripts/discourse/routes/preferences-profile.js @@ -1,13 +1,8 @@ import RestrictedUserRoute from "discourse/routes/restricted-user"; -import { set } from "@ember/object"; export default RestrictedUserRoute.extend({ showFooter: true, setupController(controller, model) { - if (!model.user_option.timezone) { - set(model, "user_option.timezone", moment.tz.guess()); - } - controller.set("model", model); } }); diff --git a/app/assets/javascripts/discourse/templates/preferences/profile.hbs b/app/assets/javascripts/discourse/templates/preferences/profile.hbs index f07c8a9a38..fded99a5fd 100644 --- a/app/assets/javascripts/discourse/templates/preferences/profile.hbs +++ b/app/assets/javascripts/discourse/templates/preferences/profile.hbs @@ -14,6 +14,7 @@ onChange=(action (mut model.user_option.timezone)) class="input-xxlarge" }} + {{d-button icon="globe" label="user.use_current_timezone" action=(action "useCurrentTimezone") }}
diff --git a/app/assets/javascripts/discourse/widgets/post-menu.js b/app/assets/javascripts/discourse/widgets/post-menu.js index 95c3477ffd..c21d60f698 100644 --- a/app/assets/javascripts/discourse/widgets/post-menu.js +++ b/app/assets/javascripts/discourse/widgets/post-menu.js @@ -315,7 +315,7 @@ registerButton("bookmarkWithReminder", (attrs, state, siteSettings) => { if (attrs.bookmarkReminderAt) { let reminderAtDate = moment(attrs.bookmarkReminderAt).tz( - Discourse.currentUser.timezone + Discourse.currentUser.resolvedTimezone() ); title = "bookmarks.created_with_reminder"; titleOptions = { diff --git a/app/assets/stylesheets/common/base/user.scss b/app/assets/stylesheets/common/base/user.scss index ae62bc5fdf..c06f4fc655 100644 --- a/app/assets/stylesheets/common/base/user.scss +++ b/app/assets/stylesheets/common/base/user.scss @@ -724,3 +724,7 @@ } } } + +.timezone-input { + margin-bottom: 0.5em; +} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 4382448fea..9f72e5b3d8 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -846,6 +846,7 @@ en: clear: title: "Clear" warning: "Are you sure you want to clear your featured topic?" + use_current_timezone: "Use Current Timezone" profile_hidden: "This user's public profile is hidden." expand_profile: "Expand" collapse_profile: "Collapse" diff --git a/test/javascripts/controllers/bookmark-test.js.es6 b/test/javascripts/controllers/bookmark-test.js.es6 index 978b7cd19f..c517ad2a89 100644 --- a/test/javascripts/controllers/bookmark-test.js.es6 +++ b/test/javascripts/controllers/bookmark-test.js.es6 @@ -1,10 +1,11 @@ -import { currentUser } from "helpers/qunit-helpers"; +import { logIn } from "helpers/qunit-helpers"; +import User from "discourse/models/user"; let BookmarkController; moduleFor("controller:bookmark", { beforeEach() { - Discourse.currentUser = currentUser(); - BookmarkController = this.subject({ currentUser: Discourse.currentUser }); + logIn(); + BookmarkController = this.subject({ currentUser: User.current() }); }, afterEach() { @@ -119,7 +120,7 @@ QUnit.test( function(assert) { let dt = moment.tz( "2019-12-11T11:37:16", - BookmarkController.currentUser.timezone + BookmarkController.currentUser.resolvedTimezone() ); assert.equal( @@ -181,14 +182,23 @@ QUnit.test( } ); -QUnit.test( - "userHasTimezoneSet updates true/false based on whether the current user timezone is set globally", - function(assert) { - Discourse.currentUser.timezone = null; - BookmarkController.onShow(); - assert.equal(BookmarkController.userHasTimezoneSet, false); - Discourse.currentUser.timezone = "Australia/Brisbane"; - BookmarkController.onShow(); - assert.equal(BookmarkController.userHasTimezoneSet, true); - } -); +QUnit.test("user timezone updates when the modal is shown", function(assert) { + User.current().changeTimezone(null); + let stub = sandbox.stub(moment.tz, "guess").returns("Europe/Moscow"); + BookmarkController.onShow(); + assert.equal(BookmarkController.userHasTimezoneSet, true); + assert.equal( + BookmarkController.userTimezone, + "Europe/Moscow", + "the user does not have their timezone set and a timezone is guessed" + ); + User.current().changeTimezone("Australia/Brisbane"); + BookmarkController.onShow(); + assert.equal(BookmarkController.userHasTimezoneSet, true); + assert.equal( + BookmarkController.userTimezone, + "Australia/Brisbane", + "the user does their timezone set" + ); + stub.restore(); +}); diff --git a/test/javascripts/models/user-test.js.es6 b/test/javascripts/models/user-test.js.es6 index 20ef656cf2..4675043adf 100644 --- a/test/javascripts/models/user-test.js.es6 +++ b/test/javascripts/models/user-test.js.es6 @@ -1,5 +1,7 @@ import User from "discourse/models/user"; import Group from "discourse/models/group"; +import * as ajaxlib from "discourse/lib/ajax"; +import pretender from "helpers/create-pretender"; QUnit.module("model:user"); @@ -66,3 +68,39 @@ QUnit.test("canMangeGroup", assert => { "a group owner should be able to manage the group" ); }); + +QUnit.test("resolvedTimezone", assert => { + const tz = "Australia/Brisbane"; + let user = User.create({ timezone: tz, username: "chuck" }); + let stub = sandbox.stub(moment.tz, "guess").returns("America/Chicago"); + + pretender.put("/u/chuck.json", () => { + return [200, { "Content-Type": "application/json" }, {}]; + }); + + let spy = sandbox.spy(ajaxlib, "ajax"); + assert.equal( + user.resolvedTimezone(), + tz, + "if the user already has a timezone return it" + ); + assert.ok( + spy.notCalled, + "if the user already has a timezone do not call AJAX update" + ); + user = User.create({ username: "chuck" }); + assert.equal( + user.resolvedTimezone(), + "America/Chicago", + "if the user has no timezone guess it with moment" + ); + assert.ok( + spy.calledWith("/u/chuck.json", { + type: "PUT", + dataType: "json", + data: { timezone: "America/Chicago" } + }), + "if the user has no timezone save it with an AJAX update" + ); + stub.restore(); +});