Currently, when we create a room and there is a failure, there are no errors displayed at all. We should fix that - for this bug, I'm proposing that we use the generic something went wrong string - its fairly obvious what it relates to. In a follow-up, we can add a more specific string and a retry option.

This gives us the minimal message "Something went wrong". This should be enough for now and for the branches where we can't change strings. Bug 1109151 can handle making this better.

Comment on attachment 8533763 [details] [diff] [review] Handle room creation errors properly in Loop Review of attachment 8533763 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, I think we need to ensure that the notifications object is a required dependency though. ::: browser/components/loop/content/shared/js/actions.js @@ +202,5 @@ > CreateRoomError: Action.define("createRoomError", { > + // There's two types of error possible - one thrown by our code (and Error) > + // and the other is an Object about the error codes from the server as > + // returned by the Hawk request. > + error: [Error, Object] Nit: As Error inherits from Object, using Object is probably enough here (but keep the comment so we know why it is that way). ::: browser/components/loop/content/shared/js/roomStore.js @@ +91,5 @@ > if (!options.mozLoop) { > throw new Error("Missing option mozLoop"); > } > this._mozLoop = options.mozLoop; > + this._notifications = options.notifications; This should be a required property.

I think having the conversation view open, show your self view (until we get room info) and then display "something went wrong" in that new conversation window if room creation failed would make sense. I think it would be a lot less cluttered/confusing than trying to display it in the panel (which could have several rooms listed already), but we can ask Sevaan for his thoughts. I also appreciate wanting to do something not too complicated/complex. Shall we do a needinfo to Sevaan?

There is also a related bug where the user gets no feedback as to what's happening if there are network difficulties while the room is being created. I'm about to file a separate bug on this, but there may be overlap to the solutions.

I've just filed bug 1111700 for that case.

This updates the patch for the current code, and handles closing the panel after the room is actually successfully created. If the room failed to be created, then it leaves the panel open and displays the error. I had to reorder a few things in the roomStore as the pendingCreation = false was being set before the actions to update the information had been completed, hence confusing the UI. I was going to add a spinner but it looked a bit more complex than I wanted to handle for this bug - plus we already disable the button, so I think there's a minimum indication there - if we want to add a spinner we can do it in a separate bug.

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #3) > I think having the conversation view open, show your self view (until we get > room info) and then display "something went wrong" in that new conversation > window if room creation failed would make sense. I think it would be a lot > less cluttered/confusing than trying to display it in the panel (which could > have several rooms listed already), but we can ask Sevaan for his thoughts. > I also appreciate wanting to do something not too complicated/complex. I've gone for the simpler leave the panel open until we get the room details back, then display an error or close the panel and open the room automatically.

Comment on attachment 8537915 [details] [diff] [review] Handle room creation errors properly in Loop Review of attachment 8537915 [details] [diff] [review]: ----------------------------------------------------------------- Works and looks good. r+ with comments addressed. ::: browser/components/loop/content/shared/js/roomStore.js @@ +107,5 @@ > if (!options.mozLoop) { > throw new Error("Missing option mozLoop"); > } > this._mozLoop = options.mozLoop; > + this._notifications = options.notifications; I keep feeling this should be a required property, like mozLoop is. @@ +285,5 @@ > /** > + * Executed when a room has been created > + */ > + createdRoom: function(actionData) { > + this.setStoreState({pendingCreation: false}); Nit: Would it make sense to remove previous creation error notifications here as well? ::: browser/components/loop/test/shared/roomStore_test.js @@ +250,5 @@ > cb(null, {roomToken: "fakeToken"}); > }); > > store.createRoom(new sharedActions.CreateRoom(fakeRoomCreationData)); > + }); This test doesn't seem to contain any assertion.

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #8) > > + this._notifications = options.notifications; > > I keep feeling this should be a required property, like mozLoop is. Unfortunately, that would mean specifying a notifications object in the conversation window, which I don't want to do. > > + * Executed when a room has been created > > + */ > > + createdRoom: function(actionData) { > > + this.setStoreState({pendingCreation: false}); > > Nit: Would it make sense to remove previous creation error notifications > here as well? I've set it to null in createRoom - that seemed a slightly better location. > ::: browser/components/loop/test/shared/roomStore_test.js > @@ +250,5 @@ > > cb(null, {roomToken: "fakeToken"}); > > }); > > > > store.createRoom(new sharedActions.CreateRoom(fakeRoomCreationData)); > > + }); > > This test doesn't seem to contain any assertion. Oops, fixed.

I noticed this yesterday when testing Loop connection handling for bug 1100149; HAWKClient uses rest.js to make HTTP requests and that bugger yields nsIException objects inside a HAWK error object. In other words; when the Loop server is down, content does get the error that occurs when creating or deleting a room. This patch fixes that.

Comment on attachment 8537915 [details] [diff] [review] Handle room creation errors properly in Loop Approval Request Comment [Feature/regressing bug #]: If we fail to create a room, we need to display an error message [User impact if declined]: User will not see a message if the server or system has a problem and fails to create a room [Describe test coverage new/current, TBPL]: tbpl [Risks and why]: Minimal risk to Hello, no risk outside of Hello. [String/UUID change made/needed]: no strings

[Tracking Requested - why for this release]: See Comment 13