Closed Bug 1142515 Opened 9 years ago Closed 9 years ago

Add ability to edit conversation context information from the conversation window

Categories

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

defect
Points:
8

Tracking

(firefox40 verified)

VERIFIED FIXED
mozilla40
Iteration:
40.2 - 27 Apr
Tracking Status
firefox40 --- verified

People

(Reporter: standard8, Assigned: mikedeboer)

References

()

Details

(Whiteboard: [context])

User Story

https://bugzilla.mozilla.org/attachment.cgi?id=8563685

* The contextual information in the conversation panel can be edited through the edit button:
- Clicking the edit button opens a new panel (slides up from the bottom of the conversation window)   
- The user can edit the URL and the description of the context
- Selecting the tickbox allows changing the context to the context of the currently active tab (gets the URL and title page as a description of the conversation)

Attachments

(7 files, 8 obsolete files)

1.32 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
947 bytes, image/svg+xml
Details
7.88 KB, patch
standard8
: review+
Details | Diff | Splinter Review
13.11 KB, patch
standard8
: review+
Details | Diff | Splinter Review
6.28 KB, patch
standard8
: review+
Details | Diff | Splinter Review
22.71 KB, patch
standard8
: review+
Details | Diff | Splinter Review
61.19 KB, patch
standard8
: review+
Details | Diff | Splinter Review
Part of the work for bug 1115342 - allow editing of the conversation data in the conversation view.
Rank: 7
Flags: qe-verify+
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [context]
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 39.3 - 30 Mar
Points: --- → 5
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?
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] 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.
Attachment #8584499 - Attachment is obsolete: true
Attachment #8584499 - Flags: review?(gavin.sharp)
Attachment #8584634 - Flags: review?(gavin.sharp) → review+
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
Iteration: 39.3 - 30 Mar → 40.1 - 13 Apr
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.
Flags: needinfo?(sfranks)
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
Flags: needinfo?(sfranks)
Rank: 7 → 5
Michael, could I receive a 10x10 SVG icon for the edit button as shown here: http://cl.ly/image/10431Y0u3V3r
? Thanks so much!
Flags: needinfo?(mmaslaney)
Iteration: 40.1 - 13 Apr → 40.2 - 27 Apr
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
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?
Flags: needinfo?(sfranks)
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
Flags: needinfo?(sfranks)
Attached image Hello_Edit.svg
Flags: needinfo?(mmaslaney)
Attached patch WIP: feature complete patch (obsolete) — Splinter Review
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.
Attachment #8596026 - Flags: feedback?(standard8)
Attachment #8596026 - Attachment is obsolete: true
Attachment #8596026 - Flags: feedback?(standard8)
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.
Attachment #8596533 - Flags: review?(standard8)
Attachment #8596530 - Flags: review?(standard8) → review+
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.
Attachment #8596531 - Flags: review?(standard8) → review+
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.
Attachment #8596532 - Flags: review?(standard8) → review+
Attachment #8596533 - Attachment is obsolete: true
Attachment #8598616 - Flags: review?(standard8)
Attached patch Patch 6: tests (obsolete) — Splinter Review
Attachment #8598618 - Flags: review?(standard8)
Comment on attachment 8598618 [details] [diff] [review]
Patch 6: tests

Forgot LoopRooms.jsm test updates.
Attachment #8598618 - Flags: review?(standard8)
Attachment #8598616 - Attachment is obsolete: true
Attachment #8598616 - Flags: review?(standard8)
Attachment #8598654 - Flags: review?(standard8)
Attached patch Patch 6.1: tests (obsolete) — Splinter Review
Attachment #8598655 - Flags: review?(standard8)
Attachment #8598618 - Attachment is obsolete: true
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 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?
Attachment #8598654 - Flags: review?(standard8) → review-
Attachment #8598654 - Attachment is obsolete: true
Attachment #8599789 - Flags: review?(standard8)
Attached patch Patch 6.2: testsSplinter Review
Attachment #8598655 - Attachment is obsolete: true
Attachment #8598655 - Flags: review?(standard8)
Attachment #8599790 - Flags: review?(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.
Attachment #8599789 - Flags: review?(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.
Attachment #8599790 - Flags: review?(standard8) → review+
(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.
Updated per review comments.
Attachment #8599789 - Attachment is obsolete: true
Attachment #8602120 - Flags: review?(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!
Attachment #8602120 - Flags: review?(standard8) → review+
Blocks: 1162435
Blocks: 1162438
Blocks: 1162442
Blocks: 1162444
Points: 5 → 8
(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.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.