Closed Bug 1147375 Opened 5 years ago Closed 5 years ago

Don't fall over if roomName isn't supplied, let users continue to use Loop

Categories

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

defect
Points:
2

Tracking

(firefox38 fixed, firefox39 fixed)

RESOLVED FIXED
mozilla39
Iteration:
39.3 - 30 Mar
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file, 1 obsolete file)

As we move to context for conversations we're encrypting the roomName as part of the context, and the old roomName field will stop being supplied.

Currently, the UI fails if we don't have a roomName - the panel stops listing rooms and you can't get to existing rooms. If you create a new room it doesn't stick around after restart.

We should improve that experience if a user of FF 38 upgraded then did decide to go back.

There's also the fact that writing this as a patch now makes it easier to introduce the context code, as we'll already be treating roomName as optional.
Simple patch to make some of the roomName parameters optional.

I've tested this locally on a store that I've started the context work on. Otherwise testing this is pretty difficult - you'd need to delete roomName from the room object in StandaloneMozLoopRooms.get, LoopRooms.getAll and LoopRooms.get.
Attachment #8583067 - Flags: review?(mdeboer)
Blocks: 1142522
No longer blocks: 1114563
Rank: 7
Flags: firefox-backlog+
Comment on attachment 8583067 [details] [diff] [review]
Don't fall over if roomName isn't supplied, let users continue to use Loop.

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

::: browser/components/loop/MozLoopService.jsm
@@ +1118,5 @@
>        if (window) {
>          window.LoopUI.showNotification({
>            sound: "room-joined",
> +          // Fallback to the brand short name if the roomName isn't available.
> +          title: room.roomName || MozLoopServiceInternal.localizedStrings.get("brandShortname"),

Hmm, why not `clientShortname2`?

::: browser/components/loop/content/shared/js/roomStore.js
@@ +36,5 @@
>     */
>    var roomSchema = {
>      roomToken:    String,
>      roomUrl:      String,
> +    //roomName:     String - Optional.

nit: add a space.
Attachment #8583067 - Flags: review?(mdeboer)
Status: NEW → ASSIGNED
Flags: needinfo?(standard8)
Yeah, I debated between that and just the brand name, but I think we can go with client. Its just a fallback anyway.
Attachment #8583067 - Attachment is obsolete: true
Attachment #8583266 - Flags: review?(mdeboer)
Flags: needinfo?(standard8)
Comment on attachment 8583266 [details] [diff] [review]
Don't fall over if roomName isn't supplied, let users continue to use Loop. v2

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

LGTM, thanks!
Attachment #8583266 - Flags: review?(mdeboer) → review+
https://hg.mozilla.org/integration/fx-team/rev/c2c24e1903f4
Target Milestone: --- → mozilla39
Comment on attachment 8583266 [details] [diff] [review]
Don't fall over if roomName isn't supplied, let users continue to use Loop. v2

Approval Request Comment
[Feature/regressing bug #]: Context in conversations for Loop
[User impact if declined]: Once context in conversations lands in 39, if a user reverts to 38 for some reason (e.g. aurora -> beta, or after merges beta -> release) then the user will not be able to access any of their existing rooms, nor enter new rooms.
[Describe test coverage new/current, TreeHerder]: Landed in fx-team
[Risks and why]: Low, makes a parameter received from the server as optional, rather than required. This at least lets the UI continue working enough that the user can access their rooms.
[String/UUID change made/needed]: None
Attachment #8583266 - Flags: approval-mozilla-aurora?
Attachment #8583266 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.