Made further changes based on feedback

api/preference.go

@@ -14,9 +14,9 @@ func InitPreference(r *mux.Router) {
 	l4g.Debug("Initializing preference api routes")
 
 	sr := r.PathPrefix("/preferences").Subrouter()
-	sr.Handle("/save", ApiAppHandler(savePreferences)).Methods("POST")
+	sr.Handle("/save", ApiUserRequired(savePreferences)).Methods("POST")
-	sr.Handle("/{category:[A-Za-z0-9_]+}", ApiAppHandler(getPreferenceCategory)).Methods("GET")
+	sr.Handle("/{category:[A-Za-z0-9_]+}", ApiUserRequired(getPreferenceCategory)).Methods("GET")
-	sr.Handle("/{category:[A-Za-z0-9_]+}/{name:[A-Za-z0-9_]+}", ApiAppHandler(getPreference)).Methods("GET")
+	sr.Handle("/{category:[A-Za-z0-9_]+}/{name:[A-Za-z0-9_]+}", ApiUserRequired(getPreference)).Methods("GET")
 }
 
 func savePreferences(c *Context, w http.ResponseWriter, r *http.Request) {
@@ -52,17 +52,21 @@ func getPreferenceCategory(c *Context, w http.ResponseWriter, r *http.Request) {
 	} else {
 		data := result.Data.(model.Preferences)
 
-		if len(data) == 0 {
+		data = transformPreferences(c, data, category)
-			if category == model.PREFERENCE_CATEGORY_DIRECT_CHANNEL_SHOW {
-				// add direct channels for a user that existed before preferences were added
-				data = addDirectChannels(c.Session.UserId, c.Session.TeamId)
-			}
-		}
 
 		w.Write([]byte(data.ToJson()))
 	}
 }
 
+func transformPreferences(c *Context, preferences model.Preferences, category string) model.Preferences {
+	if len(preferences) == 0 && category == model.PREFERENCE_CATEGORY_DIRECT_CHANNEL_SHOW {
+		// add direct channels for a user that existed before preferences were added
+		preferences = addDirectChannels(c.Session.UserId, c.Session.TeamId)
+	}
+
+	return preferences
+}
+
 func addDirectChannels(userId, teamId string) model.Preferences {
 	var profiles map[string]*model.User
 	if result := <-Srv.Store.User().GetProfiles(teamId); result.Err != nil {

api/preference_test.go

@@ -71,13 +71,13 @@ func TestGetPreferenceCategory(t *testing.T) {
 	user2 = Client.Must(Client.CreateUser(user2, "")).Data.(*model.User)
 	store.Must(Srv.Store.User().VerifyEmail(user2.Id))
 
-	category := model.PREFERENCE_CATEGORY_TEST
+	category := model.NewId()
 
 	preferences1 := model.Preferences{
 		{
 			UserId:   user1.Id,
 			Category: category,
-			Name:     model.PREFERENCE_NAME_TEST,
+			Name:     model.NewId(),
 		},
 		{
 			UserId:   user1.Id,
@@ -86,8 +86,8 @@ func TestGetPreferenceCategory(t *testing.T) {
 		},
 		{
 			UserId:   user1.Id,
-			Category: model.PREFERENCE_CATEGORY_TEST,
+			Category: model.NewId(),
-			Name:     model.PREFERENCE_NAME_TEST,
+			Name:     model.NewId(),
 		},
 	}
 
@@ -113,7 +113,7 @@ func TestGetPreferenceCategory(t *testing.T) {
 	}
 }
 
-func TestGetDefaultDirectChannels(t *testing.T) {
+func TestTransformPreferences(t *testing.T) {
 	Setup()
 
 	team := &model.Team{DisplayName: "Name", Name: "z-z-" + model.NewId() + "a", Email: "test@nowhere.com", Type: model.TEAM_OPEN}

model/preference.go

@@ -10,8 +10,6 @@ import (
 
 const (
 	PREFERENCE_CATEGORY_DIRECT_CHANNEL_SHOW = "direct_channel_show"
-	PREFERENCE_CATEGORY_TEST                = "test" // do not use, just for testing uniqueness while there's only one real category
-	PREFERENCE_NAME_TEST                    = "test" // do not use, just for testing while there's no constant name
 )
 
 type Preference struct {
@@ -46,12 +44,11 @@ func (o *Preference) IsValid() *AppError {
 		return NewAppError("Preference.IsValid", "Invalid user id", "user_id="+o.UserId)
 	}
 
-	if len(o.Category) == 0 || len(o.Category) > 32 || !IsPreferenceCategoryValid(o.Category) {
+	if len(o.Category) == 0 || len(o.Category) > 32 {
 		return NewAppError("Preference.IsValid", "Invalid category", "category="+o.Category)
 	}
 
-	// name can either be a valid constant or an id
+	if len(o.Name) == 0 || len(o.Name) > 32 {
-	if len(o.Name) == 0 || len(o.Name) > 32 || !(len(o.Name) == 26 || IsPreferenceNameValid(o.Name)) {
 		return NewAppError("Preference.IsValid", "Invalid name", "name="+o.Name)
 	}
 
@@ -61,11 +58,3 @@ func (o *Preference) IsValid() *AppError {
 
 	return nil
 }
-
-func IsPreferenceCategoryValid(category string) bool {
-	return category == PREFERENCE_CATEGORY_DIRECT_CHANNEL_SHOW || category == PREFERENCE_CATEGORY_TEST
-}
-
-func IsPreferenceNameValid(name string) bool {
-	return name == PREFERENCE_NAME_TEST
-}

model/preference_test.go

@@ -12,7 +12,7 @@ func TestPreferenceIsValid(t *testing.T) {
 	preference := Preference{
 		UserId:   "1234garbage",
 		Category: PREFERENCE_CATEGORY_DIRECT_CHANNEL_SHOW,
-		Name:     PREFERENCE_NAME_TEST,
+		Name:     NewId(),
 	}
 
 	if err := preference.IsValid(); err == nil {
@@ -24,7 +24,7 @@ func TestPreferenceIsValid(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	preference.Category = "1234garbage"
+	preference.Category = strings.Repeat("01234567890", 20)
 	if err := preference.IsValid(); err == nil {
 		t.Fatal()
 	}
@@ -34,7 +34,7 @@ func TestPreferenceIsValid(t *testing.T) {
 		t.Fatal()
 	}
 
-	preference.Name = "1234garbage"
+	preference.Name = strings.Repeat("01234567890", 20)
 	if err := preference.IsValid(); err == nil {
 		t.Fatal()
 	}
@@ -44,11 +44,6 @@ func TestPreferenceIsValid(t *testing.T) {
 		t.Fatal()
 	}
 
-	preference.Name = PREFERENCE_NAME_TEST
-	if err := preference.IsValid(); err != nil {
-		t.Fatal()
-	}
-
 	preference.Value = strings.Repeat("01234567890", 20)
 	if err := preference.IsValid(); err == nil {
 		t.Fatal()

store/sql_preference_store.go

@@ -162,9 +162,9 @@ func (s SqlPreferenceStore) Get(userId string, category string, name string) Sto
 	go func() {
 		result := StoreResult{}
 
-		var preferences model.Preferences
+		var preference model.Preference
 
-		if _, err := s.GetReplica().Select(&preferences,
+		if err := s.GetReplica().SelectOne(&preference,
 			`SELECT
 				*
 			FROM
@@ -173,9 +173,9 @@ func (s SqlPreferenceStore) Get(userId string, category string, name string) Sto
 				UserId = :UserId
 				AND Category = :Category
 				AND Name = :Name`, map[string]interface{}{"UserId": userId, "Category": category, "Name": name}); err != nil {
-			result.Err = model.NewAppError("SqlPreferenceStore.GetByName", "We encounted an error while finding preferences", err.Error())
+			result.Err = model.NewAppError("SqlPreferenceStore.Get", "We encounted an error while finding preferences", err.Error())
 		} else {
-			result.Data = preferences[0]
+			result.Data = preference
 		}
 
 		storeChannel <- result

store/sql_preference_store_test.go

@@ -55,7 +55,7 @@ func TestPreferenceGet(t *testing.T) {
 
 	userId := model.NewId()
 	category := model.PREFERENCE_CATEGORY_DIRECT_CHANNEL_SHOW
-	name := model.PREFERENCE_NAME_TEST
+	name := model.NewId()
 
 	preferences := model.Preferences{
 		{
@@ -70,7 +70,7 @@ func TestPreferenceGet(t *testing.T) {
 		},
 		{
 			UserId:   userId,
-			Category: model.PREFERENCE_CATEGORY_TEST,
+			Category: model.NewId(),
 			Name:     name,
 		},
 		{
@@ -87,6 +87,11 @@ func TestPreferenceGet(t *testing.T) {
 	} else if data := result.Data.(model.Preference); data != preferences[0] {
 		t.Fatal("got incorrect preference")
 	}
+
+	// make sure getting a missing preference fails
+	if result := <-store.Preference().Get(model.NewId(), model.NewId(), model.NewId()); result.Err == nil {
+		t.Fatal("no error on getting a missing preference")
+	}
 }
 
 func TestPreferenceGetCategory(t *testing.T) {
@@ -111,7 +116,7 @@ func TestPreferenceGetCategory(t *testing.T) {
 		// same user/name, different category
 		{
 			UserId:   userId,
-			Category: model.PREFERENCE_CATEGORY_TEST,
+			Category: model.NewId(),
 			Name:     name,
 		},
 		// same name/category, different user
@@ -131,4 +136,11 @@ func TestPreferenceGetCategory(t *testing.T) {
 	} else if !((data[0] == preferences[0] && data[1] == preferences[1]) || (data[0] == preferences[1] && data[1] == preferences[0])) {
 		t.Fatal("got incorrect preferences")
 	}
+
+	// make sure getting a missing preference category doesn't fail
+	if result := <-store.Preference().GetCategory(model.NewId(), model.NewId()); result.Err != nil {
+		t.Fatal(result.Err)
+	} else if data := result.Data.(model.Preferences); len(data) != 0 {
+		t.Fatal("shouldn't have got any preferences")
+	}
 }

web/react/components/sidebar.jsx

@@ -310,7 +310,9 @@ export default class Sidebar extends React.Component {
             this.isLeaving.set(channel.id, true);
 
             const preference = PreferenceStore.setPreference(Constants.Preferences.CATEGORY_DIRECT_CHANNEL_SHOW, channel.teammate_id, 'false');
-            AsyncClient.savePreferences(
+
+            // bypass AsyncClient since we've already saved the updated preferences
+            Client.savePreferences(
                 [preference],
                 () => {
                     this.isLeaving.set(channel.id, false);

web/react/stores/preference_store.jsx

@@ -9,6 +9,14 @@ const UserStore = require('../stores/user_store.jsx');
 
 const CHANGE_EVENT = 'change';
 
+function getPreferenceKey(category, name) {
+    return `${category}-${name}`;
+}
+
+function getPreferenceKeyForModel(preference) {
+    return `${preference.category}-${preference.name}`;
+}
+
 class PreferenceStoreClass extends EventEmitter {
     constructor() {
         super();
@@ -28,20 +36,12 @@ class PreferenceStoreClass extends EventEmitter {
         this.dispatchToken = AppDispatcher.register(this.handleEventPayload);
     }
 
-    getKey(category, name) {
-        return `${category}-${name}`;
-    }
-
-    getKeyForModel(preference) {
-        return `${preference.category}-${preference.name}`;
-    }
-
     getAllPreferences() {
         return new Map(BrowserStore.getItem('preferences', []));
     }
 
     getPreference(category, name, defaultValue = '') {
-        return this.getAllPreferences().get(this.getKey(category, name)) || defaultValue;
+        return this.getAllPreferences().get(getPreferenceKey(category, name)) || defaultValue;
     }
 
     getPreferences(category) {
@@ -70,7 +70,7 @@ class PreferenceStoreClass extends EventEmitter {
     setPreference(category, name, value) {
         const preferences = this.getAllPreferences();
 
-        const key = this.getKey(category, name);
+        const key = getPreferenceKey(category, name);
         let preference = preferences.get(key);
 
         if (!preference) {
@@ -109,7 +109,7 @@ class PreferenceStoreClass extends EventEmitter {
             const preferences = this.getAllPreferences();
 
             for (const preference of action.preferences) {
-                preferences.set(this.getKeyForModel(preference), preference);
+                preferences.set(getPreferenceKeyForModel(preference), preference);
             }
 
             this.setAllPreferences(preferences);