1142515 - Add ability to edit conversation context information from the conversation window

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

Part of the work for bug 1115342 - allow editing of the conversation data in the conversation view.

Comment 1

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: add label and placeholder caption for the context edit form

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8584499 [details] [diff] [review]
Patch 1: add label and placeholder caption for the context edit form

>diff --git a/browser/locales/en-US/chrome/browser/loop/loop.properties b/browser/locales/en-US/chrome/browser/loop/loop.properties

>+## LOCALIZATION_NOTE (context_edit_activate_label): {{title}} will be replaced
>+## by the title of the active tab, also known as the title of an HTML document.
>+## The quotes around the title are intentional.
>+context_edit_activate_label=Talk about "{{title}}"

Not related to this patch but I assume we'll need to be careful about handling extra-long or special-character titles.

>+## LOCALIZATION_NOTE (context_edit_url_placeholder): Please don't translate this
>+## as it's an internationally recognized start of a URL.
>+context_edit_url_placeholder=https://

Why have it be a localizable string if it isn't meant to be translated?

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #2)
> >+## LOCALIZATION_NOTE (context_edit_url_placeholder): Please don't translate this
> >+## as it's an internationally recognized start of a URL.
> >+context_edit_url_placeholder=https://
> 
> Why have it be a localizable string if it isn't meant to be translated?

I was kinda unsure about the placeholder value as shown in https://bugzilla.mozilla.org/attachment.cgi?id=8563685, but it being in the properties file here doesn't make it 'configurable'. So I'll just remove it.

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: add label and placeholder caption for the context edit form

Comment 5

[Mike de Boer [:mikedeboer]]

Pushed to fx-team as:

https://hg.mozilla.org/integration/fx-team/rev/1e5d5ed557a7
https://hg.mozilla.org/integration/fx-team/rev/c3ee37eaf917

Comment 6

[Mike de Boer [:mikedeboer]]

Scratch comment 5, it's really just one commit:

https://hg.mozilla.org/integration/fx-team/rev/ed90893af41e

Comment 7

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/mozilla-central/rev/10fcc230fa20

Comment 8

[Francesco Lodolo [:flod]]

As suggested in another bug, adding a link to UI mockup/reference helps a lot, especially when it's unclear if a word is a verb or a noun ('Talk').
https://bug1115342.bugzilla.mozilla.org/attachment.cgi?id=8563685

Comment 9

[Mike de Boer [:mikedeboer]]

Sevaan, I see a 'comments' textarea... where will this piece of text be displayed? I can't see it in https://bug1115342.bugzilla.mozilla.org/attachment.cgi?id=8563677, for example.

Comment 10

[Sevaan Franks [:sevaan]]

Mike, you can see the old design here: http://cl.ly/image/10431Y0u3V3r

Found here: https://docs.google.com/a/mozilla.com/document/d/1BtB605NBrEHsJHYLZxy6HdL9h5XtCwk3O_pRrU7MFQk/edit?usp=sharing

The new design (WIP) may look more like this: http://cl.ly/image/431C3y0P1C3m

Comment 11

[Mike de Boer [:mikedeboer]]

Michael, could I receive a 10x10 SVG icon for the edit button as shown here: http://cl.ly/image/10431Y0u3V3r
? Thanks so much!

Comment 12

[:shell escalante]

Hi Michael - how soon can you get the 10x10 SVG edit icon (tiny pencil next to "let's talk about") so Mike so can resolve this bug - Context work is time sensitive to land.  http://cl.ly/image/10431Y0u3V3r

Comment 13

[Mike de Boer [:mikedeboer]]

Sevaan, I see there's a custom checkbox design you are using in the mockup. Is there a separate spec for that somewhere? Or how does it look when it's checked?

Comment 14

[Sevaan Franks [:sevaan]]

There is a check used in the Chameleon project stuff. Let's put that check mark in this box (but white).

http://people.mozilla.org/~jgruen/chameleon/old/#nav1

Comment 15

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Hello_Edit.svg

Comment 16

[Mike de Boer [:mikedeboer]]

Attachment: WIP: feature complete patch

Mark, if you have the time to glance at this, please do :) It's feature complete, so when you apply it locally it should 'just work'.

I'm off chopping this off into little bite-size chunks.

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: rename the pref contextInConverations to contextInConverSations

Comment 18

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: add a generic custom checkbox widget

Comment 19

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4: add utils to compare simple objects

Comment 20

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5: implement updating a room with changed context information

Comment 21

[Mike de Boer [:mikedeboer]]

Comment on attachment 8596533 [details] [diff] [review]
Patch 5: implement updating a room with changed context information

While working on the tests, I see this is not quite done yet.

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 8596531 [details] [diff] [review]
Patch 3: add a generic custom checkbox widget

Review of attachment 8596531 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/content/shared/js/views.jsx
@@ +613,5 @@
>        );
>      }
>    });
>  
> +  var Checkbox = React.createClass({

I think it would be nice to add an example to the ui-showcase so it could easily be tested for things like rtl.

Or if we don't do that, we should at least have the edit/view context pages shown on the ui-showcase.

@@ +618,5 @@
> +    PropTypes: {
> +      additionalClass: React.PropTypes.string,
> +      checked: React.PropTypes.bool,
> +      disabled: React.PropTypes.bool,
> +      htmlId: React.PropTypes.string,

I'm a little surprised to see the htmlid here, as we don't generally use it.

@@ +621,5 @@
> +      disabled: React.PropTypes.bool,
> +      htmlId: React.PropTypes.string,
> +      label: React.PropTypes.string,
> +      onChange: React.PropTypes.func.isRequired,
> +      value: React.PropTypes.string

I think it would be worth documenting that if value isn't supplied, then it reverts to a true/false setting.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 8596532 [details] [diff] [review]
Patch 4: add utils to compare simple objects

Review of attachment 8596532 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8 with the eslint issue fixed.

::: browser/components/loop/test/shared/utils_test.js
@@ +370,5 @@
> +      obj = {
> +        prop1: null,
> +        prop2: false,
> +        prop3: undefined,
> +        prop4: void(0),

This fails eslint, I think it prefers the "void 0" version.

Comment 24

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5.1: implement updating a room with changed context information

Comment 25

[Mike de Boer [:mikedeboer]]

Attachment: Patch 6: tests

Comment 26

[Mike de Boer [:mikedeboer]]

Comment on attachment 8598618 [details] [diff] [review]
Patch 6: tests

Forgot LoopRooms.jsm test updates.

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5.2: implement updating a room with changed context information

Comment 28

[Mike de Boer [:mikedeboer]]

Attachment: Patch 6.1: tests

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 8598654 [details] [diff] [review]
Patch 5.2: implement updating a room with changed context information

Review of attachment 8598654 [details] [diff] [review]:
-----------------------------------------------------------------

Given this patch/bug has been quite big, I think there's several things we can file as either a single follow-up or multiple, that we may also want to discuss with UX as well.

- When on something like the new tab page, selecting "Add some context" shows 'Talk about ""'. What should we really be doing.
- Do we need a button to get back to viewing context if the user has hidden it.
- The edit context button needs a tooltip, I'd suggest the close/hide context button has one as well.
- Editing existing context doesn't seem to work the way I'd expect it to. I'd sort-of expect to be able to attach a different page, but with the limited UI its kinda strange how it behaves.
- Editing context doesn't update the conversation window when you hide the edit context view.

Here's the comments so far, I still need to look at roomStore.js and conversation.css which I'll look at in the morning.

::: browser/components/loop/LoopRooms.jsm
@@ +690,5 @@
> +      room.decryptedContext.roomName = roomData.roomName || room.roomName;
> +    }
> +    if (roomData.urls && roomData.urls.length) {
> +      // For now we only support adding one URL to the room context.
> +      room.decryptedContext.urls = [roomData.urls[0]];

The way this is set up, means that if we're changing the room name and don't supply the urls, any urls on the room will be removed. I think that's probably fine (as we'll want to be able to remove existing urls), but we should document that in the jsdoc.

::: browser/components/loop/content/js/roomViews.jsx
@@ +225,5 @@
>          return null;
>        }
>  
> +      var canAddContext = this.props.mozLoop.getLoopPref("contextInConversations.enabled")
> +        && !this.props.showContext && !this.state.editMode;

nit: I think we generally prefer the operator on the end of line.

@@ -245,5 @@
> -              <textarea rows="2" type="text" className="input-room-name"
> -                valueLink={this.linkState("newRoomName")}
> -                onBlur={this.handleFormSubmit}
> -                onKeyDown={this.handleTextareaKeyDown}
> -                placeholder={mozL10n.get("rooms_name_this_room_label")} />

rooms_name_this_room_label needs removing from the loop.properties file - please remove it from both the desktop & standalone versions (it shouldn't have been in the standalone anyway, but might as well drop it now).

@@ +288,3 @@
>        // This data is supplied by the activeRoomStore.
>        roomData: React.PropTypes.object.isRequired,
>        show: React.PropTypes.bool.isRequired

I'm think that now we've got editMode, this needs to be slightly more than just "show". Maybe "showContext" or "viewMode" or something?

@@ +316,5 @@
> +           });
> +          }.bind(this));
> +        }
> +      }
> +      if (nextProps.roomData) {

This section needs a comment explaining why we wouldn't update the new state if we've already got values for these (I don't quite understand whilst reading through).

@@ +382,5 @@
> +
> +    handleCheckboxChange: function(state) {
> +      if (state.checked) {
> +        // The checkbox was checked, prefill the fields with the values available
> +        // in `availableContext`

nit: missing full stop at end of line.

@@ +420,5 @@
> +        this.handleFormSubmit(event);
> +      }
> +    },
> +
> +    _getURL: function(roomData) {

nit: please add a jsdoc.

@@ +426,5 @@
> +      return this.props.roomData.roomContextUrls &&
> +        this.props.roomData.roomContextUrls[0];
> +    },
> +
> +    _truncate: function(str, maxLen) {

nit: please add a jsdoc

@@ +442,5 @@
> +      var thumbnail = url && url.thumbnail || "";
> +      var urlDescription = url && url.description || "";
> +      var location = url && url.location || "";
> +      var checkboxLabel = location || (this.state.availableContext ?
> +        this.state.availableContext.url : "")

nit: missing ;

@@ +447,5 @@
> +
> +      var cx = React.addons.classSet;
> +      if (this.state.editMode) {
> +        return (
> +          <div className="room-context">

This feels like it would be better if it was split out into its own edit context view, unless that would increase some complexity I've not noticed.

In any case, I think it could be done as a follow-up bug given where we are.

@@ +467,5 @@
> +                <textarea rows="2" type="text" className="room-context-name"
> +                  onBlur={this.handleFormSubmit}
> +                  onKeyDown={this.handleTextareaKeyDown}
> +                  placeholder={mozL10n.get("context_edit_name_placeholder")}
> +                  valueLink={this.linkState("newRoomName")}/>

nit: space before />

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +339,5 @@
>  ## The quotes around the title are intentional.
>  context_edit_activate_label=Talk about "{{title}}"
>  context_edit_name_placeholder=Conversation Name
>  context_edit_comments_placeholder=Comments
> +context_add_some_label=Add some context!

I don't think we should have the exclamation mark here. That feels a bit too much.

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 8598654 [details] [diff] [review]
Patch 5.2: implement updating a room with changed context information

Review of attachment 8598654 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/content/js/roomStore.js
@@ +475,3 @@
>       */
> +    updateRoomContext: function(actionData) {
> +      this._mozLoop.rooms.get(actionData.roomToken, function(err, room) {

This is something I don't really like about roomStore as it currently exists, it sort-of handles multiple rooms and common functions but doesn't really stand up to its intention when used in the conversation window.

I'm thinking we either need these moving into the "Room" object and/or re-visiting the RoomStore concept, possibly splitting it into two. Maybe activeRoomStore should be derived from "Room".

Anyway, I'm rambling and nothing to be addressed here, just a whine. If you have any suggestions I'd be happy to hear them.

@@ +475,4 @@
>       */
> +    updateRoomContext: function(actionData) {
> +      this._mozLoop.rooms.get(actionData.roomToken, function(err, room) {
> +        var roomData = {};

I know its unlikely, but I think we want to check 'err' just in case someone's done something wrong somewhere.

@@ +483,5 @@
> +          roomData.roomName = newRoomName;
> +        }
> +        var oldRoomURLs = context.urls;
> +        var oldRoomURL = oldRoomURLs && oldRoomURLs[0];
> +        // Since the server won't store falsy (i.e. empty) values for context data,

Not quite true, since the server just gets an encrypted blob of data.

@@ +500,5 @@
> +        if (diff.added.length || diff.updated.length) {
> +          newRoomURL = _.extend(oldRoomURL || {}, newRoomURL);
> +          var isValidURL = false;
> +          try {
> +            isValidURL = new URL(newRoomURL.location);

I think it might be worth doing this validation at the UX level. Maybe spin-off a separate bug and see what Sevaan thinks?

Comment 32

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5.3: implement updating a room with changed context information

Comment 33

[Mike de Boer [:mikedeboer]]

Attachment: Patch 6.2: tests

Comment 34

[Mark Banner (:standard8)]

Comment on attachment 8599789 [details] [diff] [review]
Patch 5.3: implement updating a room with changed context information

Review of attachment 8599789 [details] [diff] [review]:
-----------------------------------------------------------------

This is almost r+, but there's a question on the jsdoc in LoopRooms that needs answering and I'd rather have that clarified first.

::: browser/components/loop/content/js/roomStore.js
@@ +508,5 @@
> +          newRoomURL = _.extend(oldRoomURL || {}, newRoomURL);
> +          var isValidURL = false;
> +          try {
> +            isValidURL = new URL(newRoomURL.location);
> +          } catch(ex){}

nit: missing space after )

::: browser/components/loop/content/js/roomViews.jsx
@@ +447,5 @@
> +     * it'll be suffixed with the unicode ellipsis char, \u2026.
> +     *
> +     * @param  {String} str    The string to truncate, if needed.
> +     * @param  {Number} maxLen Maximum number of characters that the string is
> +     *                         allowed to contain. Optional, defaults to 72. 

nit: trailing space

::: browser/components/loop/modules/LoopRooms.jsm
@@ +671,5 @@
>  
>    /**
> +   * Updates a room.
> +   * IMPORTANT: Room context details in `roomData::decryptedContext` will be
> +   * stored as-is, so any data omitted therein will be gone forever.

I don't quite get this comment at the moment. "roomData" seems to be what would be contained within a room.decryptedContext object, so I wouldn't expect roomData.decryptedContext to exist.

Additionally, there's no room.decryptedContext = roomData.decryptedContext, so that data isn't being stored as-is.

@@ +676,3 @@
>     *
> +   * @param {String} roomToken    The room token
> +   * @param {String} roomData     The updated data for the room

This is now an object not a string.

I think it would be better to specify this as "updated context data for the room". I'm not sure if its also worth specifying the individual fields that we're expecting, since we're not just taking an existing room structure, but just the roomName and urls.

Comment 35

[Mark Banner (:standard8)]

Comment on attachment 8599790 [details] [diff] [review]
Patch 6.2: tests

Review of attachment 8599790 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming no major changes required after any 5.x patch update.

::: browser/components/loop/test/desktop-local/roomStore_test.js
@@ +615,5 @@
>        store = new loop.store.RoomStore(dispatcher, {mozLoop: fakeMozLoop});
>      });
>  
>      it("should rename the room via mozLoop", function() {
> +      fakeMozLoop.rooms.update = sinon.spy();

I think you should be able to define rooms.update as either a dummy function or a sinon.stub() in the definition of fakeMozLoop for this file (note: sinon.stub not .spy). See also the comment below...

@@ +633,2 @@
>        var err = new Error("fake");
> +      sandbox.stub(fakeMozLoop.rooms, "update", function(roomToken, roomData, cb) {

If defined 'globally', then I think you'd be able to do something like:

fakeMozLoop.rooms.update.callsArgWith(3, err);

::: browser/components/loop/test/xpcshell/test_looprooms.js
@@ +633,5 @@
> +  });
> +  Assert.equal(updateData.roomName, "fakeEncrypted", "should have set the new name");
> +});
> +
> +add_task(function* test_updateRoom_unencrypted() {

This has probably bitrotted a little since friday - the test_renameRoom_unencrypted no long exists.

Comment 36

[Mike de Boer [:mikedeboer]]

(In reply to Mark Banner (:standard8) from comment #34)
> I don't quite get this comment at the moment. "roomData" seems to be what
> would be contained within a room.decryptedContext object, so I wouldn't
> expect roomData.decryptedContext to exist.
> 
> Additionally, there's no room.decryptedContext = roomData.decryptedContext,
> so that data isn't being stored as-is.

You're right, this is a poor-mans' attempt at adding the jsDoc comment you requested in comment 29. I'll update the comment as I understand the flow better now.

Comment 37

[Mike de Boer [:mikedeboer]]

Attachment: Patch 5.4: implement updating a room with changed context information

Updated per review comments.

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 8602120 [details] [diff] [review]
Patch 5.4: implement updating a room with changed context information

Review of attachment 8602120 [details] [diff] [review]:
-----------------------------------------------------------------

I checked the interdiff and it looks good. Lets ship it!

Comment 39

[Mike de Boer [:mikedeboer]]

Yay! Pushed to fx-team as:

https://hg.mozilla.org/integration/fx-team/rev/75784d87edf5
https://hg.mozilla.org/integration/fx-team/rev/8fe39788519c
https://hg.mozilla.org/integration/fx-team/rev/991020c5923b
https://hg.mozilla.org/integration/fx-team/rev/de2e38a20cca
https://hg.mozilla.org/integration/fx-team/rev/855bbeb7d41f

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/93cd9d8feeb9

Comment 41

[Mark Banner (:standard8)]

(In reply to Pulsebot from comment #40)
> https://hg.mozilla.org/integration/fx-team/rev/93cd9d8feeb9

This was a bustage fix to change padding-left: auto; to padding-left: 0; as "auto" isn't a valid value.

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/75784d87edf5
https://hg.mozilla.org/mozilla-central/rev/8fe39788519c
https://hg.mozilla.org/mozilla-central/rev/991020c5923b
https://hg.mozilla.org/mozilla-central/rev/de2e38a20cca
https://hg.mozilla.org/mozilla-central/rev/855bbeb7d41f
https://hg.mozilla.org/mozilla-central/rev/93cd9d8feeb9

Comment 43

[Bogdan Maris, Desktop Test Engineering]

Tested using Firefox 40 beta 4 with https://loop-dev.stage.mozaws.net/v0 across platforms (Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit) and I can confirm the implementation is according to user story, found no new issue so I'll mark this as verified fixed.