1110937 - When link clickers visit an expired URL they should see an Expired Link page, rather than the Link Clicker UI

Status: VERIFIED FIXED

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

Description

[Sevaan Franks [:sevaan]]

As a link clicker, when visiting an expired url, I still see the Link Clicker UI with the button asking me to Join the Conversation. Only after clicking the conversation do I see text telling me the link has expired.

Desired result: Upon clicking an expired link I land on a page that right away tells me that the link has expired.

Comment 1

[Maire Reavy [:mreavy]]

Hi Sevaan -- Do you have UX for this? (Or guidance on how you'd like it to look?)

Comment 2

[Sevaan Franks [:sevaan]]

Let's just replace the Join the conversation button (http://cl.ly/image/0L2E0U0x0t3C) with the text saying that the room has expired (http://cl.ly/image/213h1C0J2F1M), so the user see this without having to click.

Comment 3

[Mark Banner (:standard8)]

This most likely depends on bug 1114563 from a development perspective, as that will give us the information we need prior to entering the room.

Comment 4

[Mark Banner (:standard8)]

Sevaan: I think we can do this change quite easily - we already attempt to get the room information, and that should tell us is a room is invalid or not.

A couple of questions - there's going to be a slight delay for getting the response to "join the conversation", on my machine its 200ms, but it could obviously be longer.

Do we want to have a spinner or some other temporary text? or just display nothing until we've got a response?

Additionally, is the existing text good enough if it is displayed up-front? here's the current text:

"Sorry, you cannot join this conversation. The link may be expired or invalid."

Lastly, presumably we wouldn't have the retry button any more?

Comment 5

[Sevaan Franks [:sevaan]]

I think a spinner is probably a good idea. We should have some sort of feedback that information is pending and that would suffice. And yes, no retry button.

Matej, any thoughts on improving the text string, or it's okay as is?: 

"Sorry, you cannot join this conversation. The link may be expired or invalid."

Comment 6

[Matej Novak [:matej]]

(In reply to Sevaan Franks [:sevaan] from comment #5)
> I think a spinner is probably a good idea. We should have some sort of
> feedback that information is pending and that would suffice. And yes, no
> retry button.
> 
> Matej, any thoughts on improving the text string, or it's okay as is?: 
> 
> "Sorry, you cannot join this conversation. The link may be expired or
> invalid."

I'm going back on forth on whether we need the "sorry" or not, but otherwise I think it looks good.

Comment 7

[Sevaan Franks [:sevaan]]

I vote for removing the sorry. It's not our fault and the less words the better.

Comment 8

[Mark Banner (:standard8)]

I'm happy to mentor someone on this bug. Experience with existing Firefox development processes would be useful - this is more complex than a simple good first bug, but should still be fairly easy.

See the user story for the list of things that need doing.

Background to loop development here: https://wiki.mozilla.org/Loop/Development

Note especially the standalone, react and unit test parts of that document.

Comment 10

[Eric Rescorla (:ekr)]

Mark, this isn't exactly the same as 1169763, since a bogus URL should probably show up as different than an expired one.

Comment 11

[Mark Banner (:standard8)]

(In reply to Eric Rescorla (:ekr) from comment #10)
> Mark, this isn't exactly the same as 1169763, since a bogus URL should
> probably show up as different than an expired one.

We don't keep track of expired urls - it is either valid or it isn't, so there isn't a useful distinction that we can make.

Comment 12

[Eric Rescorla (:ekr)]

(In reply to Mark Banner (:standard8) from comment #11)
> (In reply to Eric Rescorla (:ekr) from comment #10)
> > Mark, this isn't exactly the same as 1169763, since a bogus URL should
> > probably show up as different than an expired one.
> 
> We don't keep track of expired urls - it is either valid or it isn't, so
> there isn't a useful distinction that we can make.

I don't know how you're constructing URLs, but it wouldn't be difficult to add
a checksum that would allow you to distinguish these without having to keep
track.

Comment 13

[Mark Banner (:standard8)]

I initially thought this was a good mentored bug, but I investigated a bit during the plane flight for the work week and discovered it was much more complex that I expected. I have a patch in progress so will attach that in a bit.

Comment 14

[Mark Banner (:standard8)]

Attachment: Make Loop's link-clicker show the expired/invalid failure view before the user clicks join.

This makes the store wait for the result of getting the server data before moving onto the READY state. This only affects standalone - as it is in the standalone part of the flow.

As a result of the waiting, we don't display the join button until we're in the READY state. If we hit an error, we go into the failure mode - for expired/invalid urls we'll no longer display the retry button.

I've also added a new action for the retry button, so that its easy to follow the flow, rather than attempting to mutate the JOIN case.

Various manual tests:
- Normal url should load fine & show the context and join
- Invalid url (e.g. change some of the token characters) should display an error message & no retry button.
- If testing with local servers, you can run up the loop-client but not the loop-server. Then when you load the page you get "Something went wrong" with a retry button.
- Also manually tested no media, by changing the if statement for hasDevices in activeRoomStore#joinRoom.

Comment 15

[Mark Banner (:standard8)]

Attachment: Make Loop's link-clicker show the expired/invalid failure view before the user clicks join. v2

Bitrot fix for latest fx-team.

Comment 16

[Mike de Boer [:mikedeboer]]

Comment on attachment 8629900 [details] [diff] [review]
Make Loop's link-clicker show the expired/invalid failure view before the user clicks join. v2

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

Well done! This works just as I'd expect.

::: browser/components/loop/content/shared/js/activeRoomStore.js
@@ +163,5 @@
>  
>      /**
> +     * Attempts to retry getting the room data if there has been a room failure.
> +     */
> +    retryForRoomFailure: function() {

This name bugged me for some reason :)

How about just `retryGetRoomData`? Or `retryOnGetRoomFailure`?

@@ +595,5 @@
>          return;
>        }
>  
> +      var exitState = this._storeState.roomState !== ROOM_STATES.FAILED ?
> +        this._storeState.roomState : this._storeState.failureExitState;

nit: might as well swap this boolean condition around for legibility.

Comment 17

[Pulsebot]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/81993d778728

Comment 19

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

I believe Anthony is working on Graphics now, so assigning to Bogdan who's in charge of testing Hello.

Comment 20

[Bogdan Maris, Desktop QA]

I verified using latest Nightly (2015-07-20) across platforms (Windows 7 64-bit, Mac OS X 10.10 and Ubuntu 14.04 32-bit) and pref 'loop.server' - 'https://loop-dev.stage.mozaws.net/v0' that error 'Sorry, you cannot join this conversation. The link may be expired or invalid.' is properly displayed without a 'Retry' button.