pills: Streamline input pills (for user groups).

The main point of this change is to streamline the core
code for input pills, and we use also modify user groups.

The main change to input_pill.js is that you now
configure a function called `create_item_from_text`, and
that can return an arbitrary object, and it just needs
a field called `display_value`.

Other changes:
    * You now call `input.create(opts)` to create the
      widget.
    * There is no longer a cache, because we can
      write smarter code in typeahead `source` functions
      that exclude ids up front.
    * There is no value/optinalKey complexity, because
      the calling code can supply arbitrary objects and
      do their own external data management on the pill
      items.
    * We eliminate `prependPill`.
    * We eliminate `data`, `keys`, and `values`, and just
      have `items`.

.eslintrc.json

@@ -38,6 +38,7 @@
         "ui_util": false,
         "lightbox": false,
         "input_pill": false,
+        "user_pill": false,
         "stream_color": false,
         "people": false,
         "user_groups": false,

docs/subsystems/input-pills.md

@@ -15,95 +15,62 @@ A pill container should have the following markup:
 
 The pills will automatically be inserted in before the ".input" in order.
 
-## Basic Example
+## Basic Usage
 
 ```js
-var pc = input_pill($("#input_container"));
+var pill_containter = $("#input_container");
+var pills = input_pill.create({
+    container: pill_container,
+    create_item_from_text: user_pill.create_item_from_email,
+    get_text_from_item: user_pill.get_email_from_item,
+});
 ```
 
-## Advanced Example
+You can look at `static/js/user_pill.js` to see how the above
+methods are implemented.  Essentially you just need to convert
+from raw data (like an email) to structured data (like an object
+with display_value, email, and user_id for a user), and vice
+versa.  The most important field to supply is `display_value`.
+For user pills `pill_item.display_value === user.full_name`.
 
-```html
+## Typeahead
-<div class="pill-container" id="input_container">
+
-    <div class="input" contenteditable="true"></div>
+Pills almost always work in conjunction with typeahead, and
-</div>
+you will want to provide a `source` function to typeahead
-<button>Submit</button>
+that can exclude items from the prior pills.  Here is an
-```
+example snippet from our user group settings code.
 
 ```js
-var pc = input_pill($("#input_container").eq(0));
+source: function () {
+    return user_pill.typeahead_source(pills);
+},
+```
 
-// this is a map of user emails to their IDs.
+And then in `user_pill.js`...
-var map = {
+
-    "user@gmail.com": 112,
+```js
-    "example@zulip.com": 18,
+exports.typeahead_source = function (pill_widget) {
-    "test@example.com": 46,
+    var items = people.get_realm_persons();
-    "oh@oh.io": 2,
+    var taken_user_ids = exports.get_user_ids(pill_widget);
+    items = _.filter(items, function (item) {
+        return taken_user_ids.indexOf(item.user_id) === -1;
+    });
+    return items;
 };
-
-// when a user tries to create a pill (by clicking enter), check if the map
-// contains an entry for the user email entered, and if not, reject the entry.
-// otherwise, return the ID of the user as a key.
-pc.onPillCreate(function (value, reject) {
-    var key = map[value];
-
-    if (typeof key === "undefined") reject();
-
-    return key;
-});
-
-// this is a submit button
-$("#input_container + button").click(function () {
-    // log both the keys and values.
-    // the keys would be the human-readable values, and the IDs the optional
-    // values that are returned in the `onPillCreate` method.
-    console.log(pc.keys(), pc.values());
-});
 ```
 
-### `onPillCreate` method
+### `onPillCreate` and `onPillRemove` methods
 
-The `onPillCreate` method can have a few different key actions. The function can
+You can get notifications from the pill code that pills have been
-work as a validator, where if the `reject` function is called, it will disable
+created/remove.
-the pill from being added to the list. You can provide a validator function and
-call `reject` if the pill isn't valid.
 
-The return type for your callback function should be what you want the key to be
-(this is not the displayed value, but rather than important ID of the pill). An
-example of a key vs. a value would be in the case of users. The value
-(human readable) would be the name of the user. We could show their name in the
-pill, but the key would represent their user ID. One could run a function like:
 
 ```js
-pc.onPillCreate(function (value, reject) {
+pills.onPillCreate(function () {
-    var id = users.getIDByFullName(value);
+    update_save_state();
+});
 
-    // user does not exist.
+pills.onPillRemove(function () {
-    if (typeof id === "undefined") {
+    update_save_state();
-        reject();
-    }
-
-    // return the user ID to be the key for retrieval later.
-    return id;
-});
-```
-
-However sometimes, we want to modify the visible text on pill submission, which
-requires changing the value and setting the key. We can use the "object" return
-type in the `onPillCreate` method to return a new key and value.
-
-This could be useful in the case where a user enters a valid user email to send
-to, but we want the pill to display their full name, and the key to be the user ID.
-
-```js
-pc.onPillCreate(function (value, reject) {
-    var user = users.getByEmail(value);
-
-    // user does not exist.
-    if (typeof id === "undefined") {
-        reject();
-    }
-
-    return { key: user.id, value: user.full_name };
 });
 ```

frontend_tests/node_tests/settings_user_groups.js

@@ -1,4 +1,5 @@
 zrequire('dict');
+zrequire('user_pill');
 zrequire('settings_user_groups');
 
 set_global('$', global.make_zjquery());
@@ -21,7 +22,6 @@ set_global('user_groups', {
 set_global('ui_report', {});
 set_global('people', {
     my_current_user_id: noop,
-    get_realm_persons: noop,
 });
 
 (function test_populate_user_groups() {
@@ -47,6 +47,10 @@ set_global('people', {
         full_name: 'Bob',
     };
 
+    people.get_realm_persons = function () {
+        return [iago, alice, bob];
+    };
+
     user_groups.get_realm_user_groups = function () {
         return [realm_user_group];
     };
@@ -81,23 +85,29 @@ set_global('people', {
         get_person_from_user_id_called = true;
     };
 
+    var all_pills = {};
+
     var pill_container_stub = $('.pill-container[data-group-pills="Mobile"]');
-    pills.pill.append = function (name, id) {
+    pills.appendValidatedData = function (item) {
-        if (this.all_pills === undefined) {
+        var id = item.user_id;
-            this.all_pills = {};
+        assert.equal(all_pills[id], undefined);
-        }
+        all_pills[id] = item;
-        assert.equal(this.all_pills[id], undefined);
-        this.all_pills[id] = name;
     };
-    pills.keys = function () {
+    pills.items = function () {
-        return _.map(Object.keys(pills.pill.all_pills),
+        return _.values(all_pills);
-            function (strnum) {
-                return parseInt(strnum, 10);
-            });
     };
 
-    function input_pill_stub(pill_container) {
+    var text_cleared;
-        assert.equal(pill_container, pill_container_stub);
+    pills.clear_text = function () {
+        text_cleared = true;
+    };
+
+    var create_item_handler;
+
+    function input_pill_stub(opts) {
+        assert.equal(opts.container, pill_container_stub);
+        create_item_handler = opts.create_item_from_text;
+        assert(create_item_handler);
         return pills;
     }
     var input_field_stub = $.create('fake-input-field');
@@ -132,6 +142,12 @@ set_global('people', {
             query: 'ali',
         };
 
+        (function test_source() {
+            var result = config.source.call(fake_context, iago);
+            var emails = _.pluck(result, 'email').sort();
+            assert.deepEqual(emails, [alice.email, bob.email]);
+        }());
+
         (function test_matcher() {
             /* Here the query doesn't begin with an '@' because typeahead is triggered
             by the '@' sign and thus removed in the query. */
@@ -160,8 +176,9 @@ set_global('people', {
                 assert.equal(sel, '.save-member-changes');
                 return sibling_context;
             };
+            text_cleared = false;
             config.updater(alice);
-            assert.equal(input_field_stub.text(), '');
+            assert.equal(text_cleared, true);
             assert.equal(pill_container_stub
                 .siblings('.save-member-changes')
                 .css('display'), 'inline-block');
@@ -204,27 +221,25 @@ set_global('people', {
     };
     pills.onPillCreate = function (handler) {
         assert.equal(typeof(handler), 'function');
-        var reject_called = false;
+        handler();
-        function reject() {
+    };
-            reject_called = true;
+
-        }
+    function test_create_item(handler) {
         (function test_rejection_path() {
-            handler(iago.email, reject);
+            var item = handler(iago.email, pills.items());
             assert(get_by_email_called);
-            assert(reject_called);
+            assert.equal(item, undefined);
         }());
 
         (function test_success_path() {
             get_by_email_called = false;
-            reject_called = false;
+            var res = handler(bob.email, pills.items());
-            var res = handler(bob.email, reject);
             assert(get_by_email_called);
-            assert(!reject_called);
             assert.equal(typeof(res), 'object');
-            assert.equal(res.key, bob.user_id);
+            assert.equal(res.user_id, bob.user_id);
-            assert.equal(res.value, bob.full_name);
+            assert.equal(res.display_value, bob.full_name);
         }());
-    };
+    }
 
     pills.onPillRemove = function (handler) {
         realm_user_group.members = Dict.from_array([2, 31]);
@@ -235,13 +250,16 @@ set_global('people', {
         assert(fade_out_called);
     };
 
-    set_global('input_pill', input_pill_stub);
+    set_global('input_pill', {
+        create: input_pill_stub,
+    });
     settings_user_groups.set_up();
     assert(templates_render_called);
     assert(user_groups_list_append_called);
     assert(get_person_from_user_id_called);
     assert(blueslip_warn_called);
     assert(input_typeahead_called);
+    test_create_item(create_item_handler);
 
     // Tests for settings_user_groups.set_up workflow.
     assert.equal(typeof($('.organization').get_on_handler("submit", "form.admin-user-group-form")), 'function');

static/js/input_pill.js

@@ -1,4 +1,8 @@
-var input_pill = function ($parent) {
+var input_pill = (function () {
+
+var exports = {};
+
+exports.create = function (opts) {
     // a dictionary of the key codes that are associated with each key
     // to make if/else more human readable.
     var KEY = {
@@ -9,20 +13,29 @@ var input_pill = function ($parent) {
         COMMA: 188,
     };
 
+    if (!opts.container) {
+        blueslip.error('Pill needs container.');
+        return;
+    }
+
+    if (!opts.create_item_from_text) {
+        blueslip.error('Pill needs create_item_from_text');
+        return;
+    }
+
+    if (!opts.get_text_from_item) {
+        blueslip.error('Pill needs get_text_from_item');
+        return;
+    }
+
     // a stateful object of this `pill_container` instance.
     // all unique instance information is stored in here.
     var store = {
         pills: [],
-        $parent: $parent,
+        $parent: opts.container,
-        $input: $parent.find(".input"),
+        $input: opts.container.find(".input"),
-        copyReturnFunction: function (data) { return data.value; },
+        create_item_from_text: opts.create_item_from_text,
-        getKeyFunction: function () {},
+        get_text_from_item: opts.get_text_from_item,
-        validation: function () {},
-        lastUpdated: null,
-        lastCreated: {
-            keys: null,
-            values: null,
-        },
     };
 
     // a dictionary of internal functions. Some of these are exposed as well,
@@ -39,93 +52,66 @@ var input_pill = function ($parent) {
             input_elem.innerText = "";
         },
 
-        // create the object that will represent the data associated with a pill.
+        clear_text: function () {
-        // each can have a value and an optional key value.
+            store.$input.text("");
-        // the value is a human readable value that is shown, whereas the key
+        },
-        // can be a hidden ID-type value.
-        createPillObject: function (value, optionalKey) {
-            // we need a "global" closure variable that will be flipped if the
-            // key or pill creation was rejected.
-            var rejected = false;
 
-            var reject = function () {
+        create_item: function (text) {
-                rejected = true;
+            var existing_items = funcs.items();
-            };
+            var item = store.create_item_from_text(text, existing_items);
 
-            // the user may provide a function to get a key from a value
+            if (!item || !item.display_value) {
-            // that is entered, so return whatever value is gotten from
-            // this function.
-            // the default function is noop, so the return type is by
-            // default `undefined`.
-            if (typeof optionalKey === "undefined") {
-                optionalKey = store.getKeyFunction(value, reject);
-
-                if (typeof optionalKey === "object" &&
-                    optionalKey.key !== undefined && optionalKey.value !== undefined) {
-                        value = optionalKey.value;
-                        optionalKey = optionalKey.key;
-                    }
-            }
-
-            // now run a separate round of validation, in case they are using
-            // `getKeyFunction` without `reject`, or not using it at all.
-            store.validation(value, optionalKey, reject);
-
-            // if the `rejected` global is now true, it means that the user's
-            // created pill was not accepted, and we should no longer proceed.
-            if (rejected) {
                 store.$input.addClass("shake");
                 return;
             }
 
+            if (typeof store.onPillCreate === "function") {
+                store.onPillCreate();
+            }
+
+            return item;
+        },
+
+        // This is generally called by typeahead logic, where we have all
+        // the data we need (as opposed to, say, just a user-typed email).
+        appendValidatedData: function (item) {
             var id = Math.random().toString(16);
 
+            if (!item.display_value) {
+                blueslip.error('no display_value returned');
+                return;
+            }
+
             var payload = {
                 id: id,
-                value: value,
+                item: item,
-                key: optionalKey,
             };
 
             store.pills.push(payload);
 
-            return payload;
+            payload.$element = $("<div class='pill' data-id='" + payload.id + "' tabindex=0>" + item.display_value + "<div class='exit'>&times;</div></div>");
-        },
+            store.$input.before(payload.$element);
-
-        // the jQuery element representation of the data.
-        createPillElement: function (payload) {
-            store.lastUpdated = new Date();
-            payload.$element = $("<div class='pill' data-id='" + payload.id + "' tabindex=0>" + payload.value + "<div class='exit'>&times;</div></div>");
-            return payload.$element;
         },
 
         // this appends a pill to the end of the container but before the
         // input block.
-        appendPill: function (value, optionalKey) {
+        appendPill: function (value) {
+            if (value.length === 0) {
+              return;
+            }
             if (value.match(",")) {
                 funcs.insertManyPills(value);
                 return false;
             }
-            var payload = this.createPillObject(value, optionalKey);
+
+            var payload = this.create_item(value);
             // if the pill object is undefined, then it means the pill was
             // rejected so we should return out of this.
             if (!payload) {
                 return false;
             }
 
-            var $pill = this.createPillElement(payload);
+            this.appendValidatedData(payload);
-
-            store.$input.before($pill);
-        },
-
-        // this prepends a pill to the beginning of the container.
-        prependPill: function (value, optionalKey) {
-            var payload = this.createPillObject(value, optionalKey);
-            if (!payload) {
-                return false;
-            }
-            var $pill = this.createPillElement(payload);
-
-            store.$parent.prepend($pill);
         },
 
         // this searches given a particlar pill ID for it, removes the node
@@ -142,7 +128,6 @@ var input_pill = function ($parent) {
 
             if (typeof idx === "number") {
                 store.pills[idx].$element.remove();
-                store.lastUpdated = new Date();
                 var pill = store.pills.splice(idx, 1);
                 if (typeof store.removePillFunction === "function") {
                     store.removePillFunction(pill);
@@ -156,7 +141,6 @@ var input_pill = function ($parent) {
         // to the "backspace" key when the value of the input is empty.
         removeLastPill: function () {
             var pill = store.pills.pop();
-            store.lastUpdated = new Date();
 
             if (pill) {
                 pill.$element.remove();
@@ -181,8 +165,6 @@ var input_pill = function ($parent) {
                 });
             }
 
-            store.$input.find(".input");
-
             // this is an array to push all the errored values to, so it's drafts
             // of pills for the user to fix.
             var drafts = [];
@@ -209,58 +191,15 @@ var input_pill = function ($parent) {
             }
         },
 
-        // returns all data of the pills exclusive of their elements.
-        data: function () {
-            return store.pills.map(function (pill) {
-                return {
-                    value: pill.value,
-                    key: pill.key,
-                };
-            });
-        },
-
         getByID: function (id) {
             return _.find(store.pills, function (pill) {
                 return pill.id === id;
             });
         },
 
-        // returns all hidden keys.
+        items: function () {
-        // IMPORTANT: this has a caching mechanism built in to check whether the
+            return _.pluck(store.pills, 'item');
-        // store has changed since the keys/values were last retrieved so that
+        },
-        // if there are many successive pulls, it doesn't have to map and create
-        // an array every time.
-        // this would normally be a micro-optimization, but our codebase's
-        // typeaheads will ask for the keys possibly hundreds or thousands of
-        // times, so this saves a lot of time.
-        keys: (function () {
-            var keys = [];
-            return function () {
-                if (store.lastUpdated >= store.lastCreated.keys) {
-                    keys = store.pills.map(function (pill) {
-                        return pill.key;
-                    });
-                    store.lastCreated.keys = new Date();
-                }
-
-                return keys;
-            };
-        }()),
-
-        // returns all human-readable values.
-        values: (function () {
-            var values = [];
-            return function () {
-                if (store.lastUpdated >= store.lastCreated.values) {
-                    values = store.pills.map(function (pill) {
-                        return pill.value;
-                    });
-                    store.lastCreated.values = new Date();
-                }
-
-                return values;
-            };
-        }()),
     };
 
     (function events() {
@@ -391,47 +330,37 @@ var input_pill = function ($parent) {
         store.$parent.on("copy", ".pill", function (e) {
             var id = store.$parent.find(":focus").data("id");
             var data = funcs.getByID(id);
-            e.originalEvent.clipboardData.setData("text/plain", store.copyReturnFunction(data));
+            e.originalEvent.clipboardData.setData("text/plain", store.get_text_from_item(data.item));
             e.preventDefault();
         });
     }());
 
     // the external, user-accessible prototype.
     var prototype = {
-        pill: {
+        appendValue: funcs.appendPill.bind(funcs),
-            append: funcs.appendPill.bind(funcs),
+        appendValidatedData: funcs.appendValidatedData.bind(funcs),
-            prepend: funcs.prependPill.bind(funcs),
-            remove: funcs.removePill.bind(funcs),
-        },
 
-        data: funcs.data,
+        items: funcs.items,
-        keys: funcs.keys,
-        values: funcs.values,
 
         onPillCreate: function (callback) {
-            store.getKeyFunction = callback;
+            store.onPillCreate = callback;
         },
 
         onPillRemove: function (callback) {
             store.removePillFunction = callback;
         },
 
-        // this is for when a user copies a pill, you can choose in here what
-        // value to return.
-        onCopyReturn: function (callback) {
-            store.copyReturnFunction = callback;
-        },
-
-        validate: function (callback) {
-            store.validation = callback;
-        },
-
         clear: funcs.removeAllPills.bind(funcs),
+        clear_text: funcs.clear_text,
     };
 
     return prototype;
 };
 
+return exports;
+
+}());
+
 if (typeof module !== 'undefined') {
     module.exports = input_pill;
 }

static/js/settings_user_groups.js

@@ -34,19 +34,35 @@ exports.populate_user_groups = function () {
         }));
 
         var pill_container = $('.pill-container[data-group-pills="' + data.name + '"]');
-        var pills = input_pill(pill_container);
+        var pills = input_pill.create({
+            container: pill_container,
+            create_item_from_text: user_pill.create_item_from_email,
+            get_text_from_item: user_pill.get_email_from_item,
+        });
+
+        function get_pill_user_ids() {
+            return user_pill.get_user_ids(pills);
+        }
+
+        function append_user(user) {
+            user_pill.append_person({
+                pill_widget: pills,
+                person: user,
+            });
+        }
 
         data.members.keys().forEach(function (user_id) {
             var user = people.get_person_from_user_id(user_id);
 
             if (user) {
-                pills.pill.append(user.full_name, user_id);
+                append_user(user);
             } else {
                 blueslip.warn('Unknown user ID ' + user_id + ' in members of user group ' + data.name);
             }
         });
 
-        function update_save_state(draft_group) {
+        function update_save_state() {
+            var draft_group = get_pill_user_ids();
             var original_group = user_groups.get_user_group_from_id(data.id).members.keys();
             var same_groups = _.isEqual(_.sortBy(draft_group), _.sortBy(original_group));
             var save_changes = pill_container.siblings('.save-member-changes');
@@ -65,14 +81,13 @@ exports.populate_user_groups = function () {
             items: 5,
             fixed: true,
             dropup: true,
-            source: people.get_realm_persons,
+            source: function () {
+                return user_pill.typeahead_source(pills);
+            },
             highlighter: function (item) {
                 return typeahead_helper.render_person(item);
             },
             matcher: function (item) {
-                if (pills.keys().includes(item.user_id)) {
-                    return false;
-                }
                 var query = this.query.toLowerCase();
                 return (item.email.toLowerCase().indexOf(query) !== -1
                         || item.full_name.toLowerCase().indexOf(query) !== -1);
@@ -82,33 +97,22 @@ exports.populate_user_groups = function () {
                     this.query, matches, "");
             },
             updater: function (user) {
-                pills.pill.append(user.full_name, user.user_id);
+                append_user(user);
-                input.text('');
+                update_save_state();
-                update_save_state(pills.keys());
             },
             stopAdvance: true,
         });
 
-        pills.onPillCreate(function (value, reject) {
+        pills.onPillCreate(function () {
-            var person = people.get_by_email(value);
+            update_save_state();
-            var draft_group = pills.keys();
-
-            if (!person || draft_group.includes(person.user_id)) {
-                return reject();
-            }
-
-            draft_group.push(person.user_id);
-            update_save_state(draft_group);
-
-            return { key: person.user_id, value: person.full_name };
         });
 
         pills.onPillRemove(function () {
-            update_save_state(pills.keys());
+            update_save_state();
         });
 
         $('#user-groups #' + data.id).on('click', '.save-member-changes', function () {
-            var draft_group = pills.keys();
+            var draft_group = get_pill_user_ids();
             var group_data = user_groups.get_user_group_from_id(data.id);
             var original_group = group_data.members.keys();
             var added = _.difference(draft_group, original_group);

static/js/user_pill.js

@@ -0,0 +1,71 @@
+var user_pill = (function () {
+
+var exports = {};
+
+// This will be used for pills for things like composing PMs
+// or adding users to a stream/group.
+
+exports.create_item_from_email = function (email, current_items) {
+    // For normal Zulip use, we need to validate the email for our realm.
+    var user = people.get_by_email(email);
+
+    if (!user) {
+        return;
+    }
+
+    var existing_ids = _.pluck(current_items, 'user_id');
+
+    if (existing_ids.indexOf(user.user_id) >= 0) {
+        return;
+    }
+
+    // We must supply display_value for the widget to work.  Everything
+    // else is for our own use in callbacks.
+    var item = {
+        display_value: user.full_name,
+        user_id: user.user_id,
+        email: user.email,
+    };
+
+    return item;
+};
+
+exports.get_email_from_item = function (item) {
+  return item.email;
+};
+
+exports.append_person = function (opts) {
+    var person = opts.person;
+    var pill_widget = opts.pill_widget;
+
+    pill_widget.appendValidatedData({
+        display_value: person.full_name,
+        user_id: person.user_id,
+        email: person.email,
+    });
+    pill_widget.clear_text();
+};
+
+exports.get_user_ids = function (pill_widget) {
+    var items = pill_widget.items();
+    var user_ids = _.pluck(items, 'user_id');
+    user_ids = _.filter(user_ids); // be defensive about undefined users
+
+    return user_ids;
+};
+
+exports.typeahead_source = function (pill_widget) {
+    var items = people.get_realm_persons();
+    var taken_user_ids = exports.get_user_ids(pill_widget);
+    items = _.filter(items, function (item) {
+        return taken_user_ids.indexOf(item.user_id) === -1;
+    });
+    return items;
+};
+
+return exports;
+}());
+
+if (typeof module !== 'undefined') {
+    module.exports = user_pill;
+}

zproject/settings.py

@@ -1004,6 +1004,7 @@ JS_SPECS = {
             'js/localstorage.js',
             'js/drafts.js',
             'js/input_pill.js',
+            'js/user_pill.js',
             'js/channel.js',
             'js/setup.js',
             'js/unread_ui.js',