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)

defect

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Iteration:
45.2 - Nov 30
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)

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.
Rank: 19
How "same" is same? The exact url of the context matching the current tab?
Attached video v1 video
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Assignee: edilee → nobody
Status: ASSIGNED → NEW
Iteration: --- → 45.2 - Nov 30
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → b.mcb
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)
Unit tests added.
Attachment #8689412 - Attachment is obsolete: true
Attachment #8689412 - Flags: review?(standard8)
Attachment #8689957 - Flags: review?(standard8)
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 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-
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 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-
Great! I've addressed your comments, please take a look
Attachment #8690778 - Attachment is obsolete: true
Attachment #8690818 - Flags: review?(mdeboer)
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+
Comments addressed
Attachment #8690818 - Attachment is obsolete: true
Attachment #8691271 - Flags: review+
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aaf070bf42d0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: