Closed
Bug 1219005
Opened 9 years ago
Closed 9 years ago
Open a new tab with the room context when joining an existing room
Categories
(Hello (Loop) :: Client, defect, P2)
Hello (Loop)
Client
Tracking
(firefox45 fixed)
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: standard8, Assigned: mancas)
References
Details
(Whiteboard: [web sharing])
User Story
Acceptance Criteria: - When a room is opened, if the saved context is not the same as the currently open tab, open a new tab with the saved context. - If there is no context for a room, the tab is not opened.
Attachments
(2 files, 6 obsolete files)
225.71 KB,
video/webm
|
Details | |
5.59 KB,
patch
|
mancas
:
review+
|
Details | Diff | Splinter Review |
As part of the user journey rework, when a user re-enters the room, if there is existing context, it should be re-opened in a bug. See the user story for more detail.
Reporter | ||
Updated•9 years ago
|
Rank: 19
Comment 1•9 years ago
|
||
How "same" is same? The exact url of the context matching the current tab?
Comment 2•9 years ago
|
||
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Updated•9 years ago
|
Assignee: edilee → nobody
Status: ASSIGNED → NEW
Updated•9 years ago
|
Iteration: --- → 45.2 - Nov 30
Comment 3•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → b.mcb
Assignee | ||
Comment 4•9 years ago
|
||
Mark, I've found an issue using Ed's patch. It seems that if we try to open the new tab when |componentDidMount| is called, sometimes when the room is not already initialized, the context url is null as result, no tab is opened. That's the race condition that happens to me, but maybe it's my laptop's fault that spends more than a second when trying to initialize de room and recover the info from the server. To be honest, a more logical approach to me, is opening the tab after the user click the room entry, once it is opened then we launch the conversation window. wdyt?
Attachment #8688063 -
Attachment is obsolete: true
Attachment #8689412 -
Flags: review?(standard8)
Assignee | ||
Comment 5•9 years ago
|
||
Unit tests added.
Attachment #8689412 -
Attachment is obsolete: true
Attachment #8689412 -
Flags: review?(standard8)
Attachment #8689957 -
Flags: review?(standard8)
Reporter | ||
Comment 6•9 years ago
|
||
Comment on attachment 8689957 [details] [diff] [review] Open a new tab with the room context when joining an existing room. Review of attachment 8689957 [details] [diff] [review]: ----------------------------------------------------------------- Spreading review load a bit.
Attachment #8689957 -
Flags: review?(standard8)
Attachment #8689957 -
Flags: review?(mdeboer)
Attachment #8689957 -
Flags: review?(dmose)
Comment 7•9 years ago
|
||
Comment on attachment 8689957 [details] [diff] [review] Open a new tab with the room context when joining an existing room. Review of attachment 8689957 [details] [diff] [review]: ----------------------------------------------------------------- Implementation LGTM, a few things left to do on the tests... ::: browser/components/loop/test/desktop-local/panel_test.js @@ +709,5 @@ > sinon.assert.notCalled(dispatcher.dispatch); > }); > + > + it("should open a new tab with the room context if it is not the same as the currently open tab", function() { > + var stub = sandbox.stub(); `sinon.stub()` is the preferred, more lightweight option. Please use that. @@ +718,5 @@ > + description: "fakeSite" > + }; > + }, > + OpenURL: stub > + }); What if you define this inside the forEach, just before `roomEntry = ` for the 'Copy button' section, wouldn't it make it easier to work this out? Make `stub` a variable in the same scope as `roomEntry` and call it `openURLStub`. I think all the tests will still pass here if you do that. @@ +721,5 @@ > + OpenURL: stub > + }); > + > + roomEntry = mountRoomEntry({ > + deleteRoom: sandbox.stub(), Since you're not testing this, does it need to be a stub?
Attachment #8689957 -
Flags: review?(mdeboer)
Attachment #8689957 -
Flags: review?(dmose)
Attachment #8689957 -
Flags: review-
Assignee | ||
Comment 8•9 years ago
|
||
I've updated the tests accordingly with your comments so it's ready for a second round =) Thanks!
Attachment #8689957 -
Attachment is obsolete: true
Attachment #8690778 -
Flags: review?(mdeboer)
Comment 9•9 years ago
|
||
Comment on attachment 8690778 [details] [diff] [review] Open a new tab with the room context when joining an existing room. Review of attachment 8690778 [details] [diff] [review]: ----------------------------------------------------------------- I'm being a bit strict today ;-) ::: browser/components/loop/test/desktop-local/panel_test.js @@ +736,5 @@ > + description: "fakeSite" > + }; > + }, > + OpenURL: stub > + }); You can remove this stubbing part... @@ +746,5 @@ > + }); > + > + TestUtils.Simulate.click(roomEntry.refs.roomEntry.getDOMNode()); > + > + sinon.assert.notCalled(stub); ...and make the `sinon.assert.notCalled(openRoomStub);`
Attachment #8690778 -
Flags: review?(mdeboer) → review-
Assignee | ||
Comment 10•9 years ago
|
||
Great! I've addressed your comments, please take a look
Attachment #8690778 -
Attachment is obsolete: true
Attachment #8690818 -
Flags: review?(mdeboer)
Comment 11•9 years ago
|
||
Comment on attachment 8690818 [details] [diff] [review] Open a new tab with the room context when joining an existing room. Review of attachment 8690818 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the update! r=me with comments addressed. ::: browser/components/loop/content/js/panel.jsx @@ +440,5 @@ > this.props.dispatcher.dispatch(new sharedActions.OpenRoom({ > roomToken: this.props.room.roomToken > })); > + > + // Open url if needed nit: missing dot. ::: browser/components/loop/test/desktop-local/panel_test.js @@ +726,5 @@ > + sinon.assert.calledWithExactly(openURLStub, "http://testurl.com"); > + }); > + > + it("should not open a new tab if the context is the same as the currently open tab", function() { > + var stub = sandbox.stub(); Please remove this line.
Attachment #8690818 -
Flags: review?(mdeboer) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Comments addressed
Attachment #8690818 -
Attachment is obsolete: true
Attachment #8691271 -
Flags: review+
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8691271 -
Attachment is obsolete: true
Attachment #8691273 -
Flags: review+
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/aaf070bf42d0
Keywords: checkin-needed
Comment 15•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/aaf070bf42d0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•