From 357186ab7ebd100c38d4776e9e05e9d2d988f570 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Mon, 7 Feb 2022 16:58:27 +1000
Subject: [PATCH] FIX: User option fields definition was being mutated on save
 (#15837)

In the commit d8bf2810ffe49619077371d854fc12cba81f80fa we hoisted
the userOptionFields array to a module-level variable, but kept
the code inside save() the same. This causes an issue where if
save() is called twice on the same user with some array of user
option fields, the userOptionFields array is mutated, which means
the second save is likely not saving the fields intended.

This commit fixes the issue by not mutating the array. We cannot
change them into consts though, because we have an API to add more
items to the array.
---
 .../javascripts/discourse/app/models/user.js  | 11 ++++-
 .../discourse/tests/acceptance/user-test.js   | 46 +++++++++++++++++++
 .../discourse/tests/unit/models/user-test.js  |  2 +-
 3 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/models/user.js b/app/assets/javascripts/discourse/app/models/user.js
index 83f36723b66..4f9cca7a4d5 100644
--- a/app/assets/javascripts/discourse/app/models/user.js
+++ b/app/assets/javascripts/discourse/app/models/user.js
@@ -334,13 +334,16 @@ const User = RestModel.extend({
       userFields.filter((uf) => !fields || fields.indexOf(uf) !== -1)
     );
 
+    let filteredUserOptionFields = [];
     if (fields) {
-      userOptionFields = userOptionFields.filter(
+      filteredUserOptionFields = userOptionFields.filter(
         (uo) => fields.indexOf(uo) !== -1
       );
+    } else {
+      filteredUserOptionFields = userOptionFields;
     }
 
-    userOptionFields.forEach((s) => {
+    filteredUserOptionFields.forEach((s) => {
       data[s] = this.get(`user_option.${s}`);
     });
 
@@ -379,6 +382,10 @@ const User = RestModel.extend({
       }
     });
 
+    return this._saveUserData(data, updatedState);
+  },
+
+  _saveUserData(data, updatedState) {
     // TODO: We can remove this when migrated fully to rest model.
     this.set("isSaving", true);
     return ajax(userPath(`${this.username_lower}.json`), {
diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-test.js
index 32375c88b35..121ccb9e486 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/user-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/user-test.js
@@ -1,3 +1,7 @@
+import EmberObject from "@ember/object";
+import User from "discourse/models/user";
+import selectKit from "discourse/tests/helpers/select-kit-helper";
+import sinon from "sinon";
 import {
   acceptance,
   exists,
@@ -131,3 +135,45 @@ acceptance("User Routes - Moderator viewing warnings", function (needs) {
     assert.ok($("div.alert-info").length, "has the permissions alert");
   });
 });
+
+acceptance("User - Saving user options", function (needs) {
+  needs.user({
+    admin: false,
+    moderator: false,
+    username: "eviltrout",
+    id: 1,
+    user_option: EmberObject.create({}),
+  });
+
+  needs.settings({
+    disable_mailing_list_mode: false,
+  });
+
+  needs.pretender((server, helper) => {
+    server.put("/u/eviltrout.json", () => {
+      return helper.response(200, { user: {} });
+    });
+  });
+
+  test("saving user options", async function (assert) {
+    const spy = sinon.spy(User.current(), "_saveUserData");
+
+    await visit("/u/eviltrout/preferences/emails");
+    await click(".pref-mailing-list-mode input[type='checkbox']");
+    await click(".save-changes");
+
+    assert.ok(
+      spy.calledWithMatch({ mailing_list_mode: true }),
+      "sends a PUT request to update the specified user option"
+    );
+
+    await selectKit("#user-email-messages-level").expand();
+    await selectKit("#user-email-messages-level").selectRowByValue(2); // never option
+    await click(".save-changes");
+
+    assert.ok(
+      spy.calledWithMatch({ email_messages_level: 2 }),
+      "is able to save a different user_option on a subsequent request"
+    );
+  });
+});
diff --git a/app/assets/javascripts/discourse/tests/unit/models/user-test.js b/app/assets/javascripts/discourse/tests/unit/models/user-test.js
index fe80430aa2f..5b8a6a47262 100644
--- a/app/assets/javascripts/discourse/tests/unit/models/user-test.js
+++ b/app/assets/javascripts/discourse/tests/unit/models/user-test.js
@@ -1,8 +1,8 @@
 import * as ajaxlib from "discourse/lib/ajax";
 import { module, test } from "qunit";
+import sinon from "sinon";
 import Group from "discourse/models/group";
 import User from "discourse/models/user";
-import sinon from "sinon";
 
 module("Unit | Model | user", function () {
   test("staff", function (assert) {